Re: [PATCH] arm64/sve: Make kernel FPU protection RT friendly

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
To: Dave Martin <Dave.Martin@arm.com>
Cc: linux-arm-kernel@lists.infradead.org,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH] arm64/sve: Make kernel FPU protection RT friendly
Date: Thu, 29 Jul 2021 19:11:19 +0200	[thread overview]
Message-ID: <20210729171119.5iu532i65hgmgypi@linutronix.de> (raw)
In-Reply-To: <20210729163227.GR1724@arm.com>

On 2021-07-29 17:32:27 [+0100], Dave Martin wrote:
> On Thu, Jul 29, 2021 at 06:07:56PM +0200, Sebastian Andrzej Siewior wrote:
> > On 2021-07-29 18:00:31 [+0200], To Dave Martin wrote:
> > > 
> > > But I get what you mean. I'm just not sure regarding the naming since
> > > the code behaves the same on x86 and arm64 here.
> > 
> > Assuming that everyone follows this pattern what about
> >   fpregs_lock()
> >   fpregs_unlock()
> 
> I'm not sure about the "fpregs".  This is really about CPU-local
> resources that are contended between softirq and task context.
> 
> Some arches might not to use fp in softirq context and then their fp
> regs wouldn't need this; others might have other resources that aren't
> "fp" regs, but are contended in the same way.

if FP / SIMD is used in crypto then it is likely that they will aim for
softirq for ipsec reasons. Wireguard, dm-crypt perform crypto in process
context if I remember correctly.

> My "local_bh" was meaning purely softirqs running on this CPU.  This was
> the original meaning of "local" in this API IIUC.  This is one reason
> why they must disable preemption: "local" is meaningless if preemption
> is enabled, since otherwise we might randomly migrate between CPUs.

You assume that BH also disable preemption which is an implementation
detail. On RT local_bh_disable() disables BH on the current CPU. The
task won't migrate to another CPU while preempted and another task, which
invokes local_bh_disable() on the same CPU, will wait until the previous
local_bh_disable() section completes.
So all semantics which are expected by local_bh_disable() are preserved
in RT - except for the part where preemption is disabled.

> I guess the "local" was preserved in the naming on PREEMPT_RT to reduce
> the amount of noise that would have resulted from a treewide rename, but
> this word seems confusing if there is no CPU-localness involved.

As I explained above, the BH on the local/current CPU is disabled as in
no softirq is currently running on this CPU. The context however remains
preemptible so there is no need for a rename.

> In this particular case, we really do want to bind ourselves onto the
> current CPU and disable softirqs on this CPU -- if they continue to run
> elsewhere, that's just fine.
> 
> What do you think about:
> 
> get_bh_cpu_context()
> put_bh_cpu_context()
> 
> or something along those lines?

So you are not buying the fpregs_lock()? Then I don't need to reach for
the simd_lock() or such from the shelf?
The problem I have with _bh_ is that on RT the BH/softirq context is not
forced to complete if preempted as it would be the case with
local_bh_disable(). So that is misleading.
It is just that it happens not to matter in this case and it is
sufficient on RT to disable preemption. It would be wrong to disable BH
and preemption on RT (but it might be okay / needed in other cases).

powerpc for instance has
	arch/powerpc/crypto/crc32c-vpmsum_glue.c
doing doing crc32c with ALTIVEC (simd). They only disable preemption and
their crypto_simd_usable() (the generic version of it) forbids an
invocation in the softirq context.
So matter how I look at it, it really comes down to the specific SIMD
usage on an architecture.

> Cheers
> ---Dave

Sebastian

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