On 2/22/21 5:58 PM, Catalin Marinas wrote:

That's because cpu_hotplug_lock is not a spinlock but a semaphore which
implies sleeping. I don't think avoiding taking this semaphore
altogether (as in the *_cpuslocked functions) is the correct workaround.

Thinking at it a second time I agree, it is not a good idea avoiding to take the
semaphore in this case.

The mte_enable_kernel_async() function is called on each secondary CPU
but we don't really need to attempt to toggle the static key on each of
them as they all have the same configuration. Maybe do something like:

	if (!static_branch_unlikely(&mte_async_mode)))
		static_branch_enable(&mte_async_mode);

so that it's only set on the boot CPU.

This should work, maybe with a comment that if we plan to introduce runtime
switching in between async and sync in future we need to revisit our strategy.

The alternative is to use a per-CPU mask/variable instead of static
branches but it's probably too expensive for those functions that were
meant to improve performance.

I would not go for this approach because the reason why we introduced static
branches instead of having a normal variable saving the state was performances.

We'll still have an issue with dynamically switching the async/sync mode
at run-time. Luckily kasan doesn't do this now. The problem is that
until the last CPU have been switched from async to sync, we can't
toggle the static label. When switching from sync to async, we need
to do it on the first CPU being switched.

I totally agree on this point. In the case of runtime switching we might need
the rethink completely the strategy and depends a lot on what we want to allow
and what not. For the kernel I imagine we will need to expose something in sysfs
that affects all the cores and then maybe stop_machine() to propagate it to all
the cores. Do you think having some of the cores running in sync mode and some
in async is a viable solution?

Probably it is worth to discuss it further once we cross that bridge.

So, I think currently we can set the mte_async_mode label to true in
mte_enable_kernel_async(), with the 'if' check above. For
mte_enable_kernel_sync(), don't bother with setting the key to false but
place a WARN_ONCE if the mte_async_mode is true. We can revisit it if
kasan ever gains this run-time switch mode.

Indeed, this should work for now.

-- 
Regards,
Vincenzo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel