Re: [patch 3/3] x86/fpu: Make FPU protection more robust

Re: [patch 3/3] x86/fpu: Make FPU protection more robust

[Thomas Gleixner]

On Tue, May 03 2022 at 11:06, Peter Zijlstra wrote:
> On Mon, May 02, 2022 at 05:58:40PM +0200, Thomas Gleixner wrote:
>> Right, though currently it's guaranteed that softirq processing context
>> can use the FPU. Quite some of the network crypto work runs in softirq
>> context, so this might cause a regression. If so, then this needs to be
>> an explicit commit on top which is easy to revert. Let me stare at it
>> some more.
>
> Right, so with the:
>
> 	preempt_disable();
> 	this_cpu_write(fpu_in_use, true);
> 	barrier();
>
> sequence it is safe against both softirq and hardirq fpu usage. The only
> concern is performance not correctness when dropping that
> local_bh_disable() thing.
>
> So what Thomas proposes makes sense to me.

Now I was looking at it the other way round too; i.e. to use
local_bh_disable() for both fpregs_lock() and kernel_fpu_begin().

Using local_bh_disable() for both fpregs_lock() and kernel_fpu_begin()
is not possible with the current constraints, because kernel_fpu_begin()
can be called from hard interrupt context.

But the only use case which utilizes FPU from hard interrupt context is
the random generator via add_randomness_...().

I did a benchmark of these functions, which invoke blake2s_update()
three times in a row, on a SKL-X and a ZEN3. The generic code and the
FPU accelerated code are pretty much on par vs. execution time of the
algorithm itself plus/minus noise.

But in case that the interrupt hits a userspace task the FPU needs to be
saved and if the interrupt does not result in rescheduling then the
return to user space has to restore it. That's _expensive_ and the
actual cost depends on the FPU state, but 200-300 cycles for save and
200-700 cycles for restore are due.

Even if we ignore the save/restore part and assume that it averages out
vs. schedule() having to save FPU state anyway, then there is another
aspect to this: power consumption which affects also thermal budget and
capacity.

Though that made me more curious and I did the same comparison for crc32
which is heavily used by ext4. crc32c_pcl_intel_update() already
contains a switch to software when the buffer length is less than 512
bytes. But even on larger buffers, typically ~4k, FPU is not necessarily
a win. It's consistently slower by a factor of ~1.4x. And that's not due
to xsave/rstor overhead because these computations run on a worker
thread which does not do that dance at all.

IOW, using the FPU blindly for this kind of computations is not
necessarily a good plan. I have no idea how these things are analyzed
and evaluated if at all. Maybe the crypto people can shed some light on
this.

Thanks,

        tglx

Re: [patch 3/3] x86/fpu: Make FPU protection more robust

[Jason A. Donenfeld]

Hi Thomas,

On Wed, May 04, 2022 at 05:36:38PM +0200, Thomas Gleixner wrote:
> But the only use case which utilizes FPU from hard interrupt context is
> the random generator via add_randomness_...().
> 
> I did a benchmark of these functions, which invoke blake2s_update()
> three times in a row, on a SKL-X and a ZEN3. The generic code and the
> FPU accelerated code are pretty much on par vs. execution time of the
> algorithm itself plus/minus noise.
>
> IOW, using the FPU blindly for this kind of computations is not
> necessarily a good plan. I have no idea how these things are analyzed
> and evaluated if at all. Maybe the crypto people can shed some light on
> this.

drivers/net/wireguard/{noise,cookie}.c makes pretty heavy use of BLAKE2s
in hot paths where the FPU is already being used for other algorithms,
and so there the save/restore is worth it (assuming restore finally
works lazily). In benchmarks, the SIMD code made a real difference.

But this presumably regards mix_pool_bytes() in the RNG. If it turns out
that supporting the FPU in hard IRQ context is a major PITA, and the RNG
is the only thing making use of it, then sure, drop hard IRQ context
support for it. However... This may be unearthing a larger bug.
Sebastian and I put in a decent amount of work during 5.18 to remove all
calls to mix_pool_bytes() (and hence to blake2s_compress()) from
add_interrupt_randomness(). Have a look:

https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git/tree/drivers/char/random.c#n1289

It now accumulates in some per-CPU buffer, and then every 64 interrupts
a worker runs that does the actual mix_pool_bytes() from kthread
context.

So the question is: what is still hitting mix_pool_bytes() from hard IRQ
context? I'll investigate a bit and see.

Jason

Re: [patch 3/3] x86/fpu: Make FPU protection more robust

[Thomas Gleixner]

Jason,

On Wed, May 04 2022 at 17:55, Jason A. Donenfeld wrote:
> On Wed, May 04, 2022 at 05:36:38PM +0200, Thomas Gleixner wrote:
>> But the only use case which utilizes FPU from hard interrupt context is
>> the random generator via add_randomness_...().
>> 
>> I did a benchmark of these functions, which invoke blake2s_update()
>> three times in a row, on a SKL-X and a ZEN3. The generic code and the
>> FPU accelerated code are pretty much on par vs. execution time of the
>> algorithm itself plus/minus noise.
>>
>> IOW, using the FPU blindly for this kind of computations is not
>> necessarily a good plan. I have no idea how these things are analyzed
>> and evaluated if at all. Maybe the crypto people can shed some light on
>> this.
>
> drivers/net/wireguard/{noise,cookie}.c makes pretty heavy use of BLAKE2s
> in hot paths where the FPU is already being used for other algorithms,
> and so there the save/restore is worth it (assuming restore finally
> works lazily). In benchmarks, the SIMD code made a real difference.

I'm sure there are very valid use cases, but just the two things I
looked at turned out to be questionable at least.

> But this presumably regards mix_pool_bytes() in the RNG. If it turns out
> that supporting the FPU in hard IRQ context is a major PITA, and the
> RNG

Supporting FPU in hard interrupt context is possible if required and the
preexisting bug which survived 10+ years has been fixed.
x
I just started to look into this because of that bug and due to the
inconsistency between the FPU protections we have. The inconsistency
comes from the hardirq requirement.

> is the only thing making use of it, then sure, drop hard IRQ context
> support for it. However... This may be unearthing a larger bug.
> Sebastian and I put in a decent amount of work during 5.18 to remove all
> calls to mix_pool_bytes() (and hence to blake2s_compress()) from
> add_interrupt_randomness(). Have a look:

I know.

> It now accumulates in some per-CPU buffer, and then every 64 interrupts
> a worker runs that does the actual mix_pool_bytes() from kthread
> context.

That's add_interrupt_randomness() and not affected by this.

> So the question is: what is still hitting mix_pool_bytes() from hard IRQ
> context? I'll investigate a bit and see.

add_disk_randomness() on !RT kernels. That's what made me look into this
in the first place as it unearthed the long standing FPU protection
bug. See the first patch in this thread.

Possibly add_device_randomness() too, but I haven't seen evidence so far.

Thanks,

        tglx

Re: [patch 3/3] x86/fpu: Make FPU protection more robust

[Jason A. Donenfeld]

Hey Thomas,

On Wed, May 04, 2022 at 06:45:45PM +0200, Thomas Gleixner wrote:
> add_disk_randomness() on !RT kernels. That's what made me look into this
> in the first place as it unearthed the long standing FPU protection
> bug. See the first patch in this thread.
> 
> Possibly add_device_randomness() too, but I haven't seen evidence so far.

It looks like it's being hit from add_input_randomness() via
input_handle_event() too. There are two positions we could take toward
this:

One stance to take would be that if an event happens in an interrupt,
add_interrupt_randomness() should in theory already be siphashing in a
cycle counter so additional calls to add_{input,disk}_randomness() don't
contribute substantially (unless you assume the num field has some
entropic value). If that's compelling, then the next thing to do would
be adding a `if (in_interrupt()) return;` early on in some function, and
then we forever after impose a rule, "never mix into the input pool
directly from an irq".

The other stance is that these input/disk events are relatively rare --
compared to, say, a storm of interrupts from a NIC -- so mixing into the
input pool from there isn't actually a problem, and we benefit from the
quasi domain-specific accounting and the superior mixing function,
there, so keep it around. And the non-raw spinlock on the input pool
won't negatively affect RT from this context, because all its callers on
RT should be threaded.

The second stance seems easier and more conservative from a certain
perspective -- we don't need to change anything -- so I'm more inclined
toward it. And given that you've fixed the bug now, it sounds like
that's fine with you too. But if you're thinking about it differently in
fact, let me know.

Jason

Re: [patch 3/3] x86/fpu: Make FPU protection more robust

[Thomas Gleixner]

Jason,

On Wed, May 04 2022 at 21:05, Jason A. Donenfeld wrote:
> The other stance is that these input/disk events are relatively rare --
> compared to, say, a storm of interrupts from a NIC -- so mixing into the
> input pool from there isn't actually a problem, and we benefit from the
> quasi domain-specific accounting and the superior mixing function,
> there, so keep it around. And the non-raw spinlock on the input pool
> won't negatively affect RT from this context, because all its callers on
> RT should be threaded.

I'm not worried about RT here.

> The second stance seems easier and more conservative from a certain
> perspective -- we don't need to change anything -- so I'm more inclined
> toward it.

That's not conservative, that's lazy and lame. Staying with the status
quo and piling more stuff on top because we can is just increasing
technical debt. Works for a while by some definition of works.

> And given that you've fixed the bug now, it sounds like that's fine
> with you too. But if you're thinking about it differently in fact, let
> me know.

That still does not address my observation that using the FPU for this
mixing, which is handling a couple of bytes per invocation, is not
really benefitial.

Which in turn bears the question, why we have to maintain an asymmetric
FPU protection mechanism in order to support hard interrupt FPU usage
for no or questionable benefit.

The current implementation, courtesy to hard interrupt support, has the
following downside:

  Any FPU usage in task context where soft interrupts are enabled will
  prevent FPU usage in soft interrupt processing when the interrupt hits
  into the FPU usage region. That means the softirq processing has to
  fall back to the generic implementations.

Sure, the protection could be context dependent, but that's generally
frowned upon. If we go there, then there has to be a really convincing
technical argument.

Thanks,

        tglx

Re: [patch 3/3] x86/fpu: Make FPU protection more robust

[Jason A. Donenfeld]

Hi Thomas,

On Wed, May 04, 2022 at 11:04:12PM +0200, Thomas Gleixner wrote:

> > The second stance seems easier and more conservative from a certain
> > perspective -- we don't need to change anything -- so I'm more inclined
> > toward it.
> 
> That's not conservative, that's lazy and lame. Staying with the status
> quo and piling more stuff on top because we can is just increasing
> technical debt. Works for a while by some definition of works.

I actually find this minorly upsetting :(. Considering the amount of
technical debt I've been tirelessly cleaning up over the last 5 months,
"lazy" certainly can't be correct here. None of this has anything to do
with laziness, but rather how the entropy collection logic works out.
It'd be lazy to just wave my hands around and say, "oh well it's handled
in add_interrupt_randomness() anyway, so let's torch the other thing,"
without having taken the time to do the analysis to see if that's
actually true or not. One way or another, it _will_ require real
analysis. Which obviously I'm volunteering to do here (it's an
interesting question to me) and have already started poking around with
it.

> > And given that you've fixed the bug now, it sounds like that's fine
> > with you too. But if you're thinking about it differently in fact, let
> > me know.
> 
> That still does not address my observation that using the FPU for this
> mixing, which is handling a couple of bytes per invocation, is not
> really benefitial.
> 
> Which in turn bears the question, why we have to maintain an asymmetric
> FPU protection mechanism in order to support hard interrupt FPU usage
> for no or questionable benefit.
> 
> The current implementation, courtesy to hard interrupt support, has the
> following downside:
> 
>   Any FPU usage in task context where soft interrupts are enabled will
>   prevent FPU usage in soft interrupt processing when the interrupt hits
>   into the FPU usage region. That means the softirq processing has to
>   fall back to the generic implementations.
> 
> Sure, the protection could be context dependent, but that's generally
> frowned upon. If we go there, then there has to be a really convincing
> technical argument.

That's curious about the softirq; I hadn't realized that. Indeed it
sounds like the technical burden of supporting it may not be worth it.
From the perspective of high-speed crypto, I know two areas pretty well:
pacrypt/padata and wireguard's queueing. Both of these run in workqueues
pinned to a CPU with queue_work_on(). In some cases, I believe locks may
be held during various crypto operations though. Also, I suspect some
paths of xfrm and mac80211 may process during softirq. Anyway, none of
those cases is hardirq. I haven't looked at dmcrypt, but I'd be
surprised if that was doing anything in hardirqs either.

So if truly the only user of this is random.c as of 5.18 (is it? I'm
assuming from a not very thorough survey...), and if the performance
boost doesn't even exist, then yeah, I think it'd make sense to just get
rid of it, and have kernel_fpu_usable() return false in those cases.

I'll run some benchmarks on a little bit more hardware in representative
cases and see.

Jason

Re: [patch 3/3] x86/fpu: Make FPU protection more robust

[Thomas Gleixner]

Jason,

On Thu, May 05 2022 at 01:52, Jason A. Donenfeld wrote:
> On Wed, May 04, 2022 at 11:04:12PM +0200, Thomas Gleixner wrote:
>
>> > The second stance seems easier and more conservative from a certain
>> > perspective -- we don't need to change anything -- so I'm more inclined
>> > toward it.
>> 
>> That's not conservative, that's lazy and lame. Staying with the status
>> quo and piling more stuff on top because we can is just increasing
>> technical debt. Works for a while by some definition of works.
>
> I actually find this minorly upsetting :(.

I'm glad it's only minorly. This was not meant offensive, but I have
heard the "let's be more conservative -- we don't need to change
anything -- " song so many times that it became of of those buttons...

> Considering the amount of technical debt I've been tirelessly cleaning
> up over the last 5 months, "lazy" certainly can't be correct here.

I'm aware of this and I'm thankful that you are tackling these things.

> So if truly the only user of this is random.c as of 5.18 (is it? I'm
> assuming from a not very thorough survey...), and if the performance
> boost doesn't even exist, then yeah, I think it'd make sense to just get
> rid of it, and have kernel_fpu_usable() return false in those cases.
>
> I'll run some benchmarks on a little bit more hardware in representative
> cases and see.

Find below a combo patch which makes use of strict softirq serialization
for the price of not supporting the hardirq FPU usage. 

Thanks,

        tglx
---
diff --git a/arch/um/include/asm/fpu/api.h b/arch/um/include/asm/fpu/api.h
index 71bfd9ef3938..e3deb051b596 100644
--- a/arch/um/include/asm/fpu/api.h
+++ b/arch/um/include/asm/fpu/api.h
@@ -11,7 +11,7 @@
 #define kernel_fpu_begin() (void)0
 #define kernel_fpu_end() (void)0
 
-static inline bool irq_fpu_usable(void)
+static inline bool kernel_fpu_usable(void)
 {
 	return true;
 }
diff --git a/arch/x86/include/asm/fpu/api.h b/arch/x86/include/asm/fpu/api.h
index c83b3020350a..21b7ef7e0cfb 100644
--- a/arch/x86/include/asm/fpu/api.h
+++ b/arch/x86/include/asm/fpu/api.h
@@ -19,7 +19,7 @@
  * disables preemption so be careful if you intend to use it for long periods
  * of time.
  * If you intend to use the FPU in irq/softirq you need to check first with
- * irq_fpu_usable() if it is possible.
+ * kernel_fpu_usable() if it is possible.
  */
 
 /* Kernel FPU states to initialize in kernel_fpu_begin_mask() */
@@ -28,7 +28,7 @@
 
 extern void kernel_fpu_begin_mask(unsigned int kfpu_mask);
 extern void kernel_fpu_end(void);
-extern bool irq_fpu_usable(void);
+extern bool kernel_fpu_usable(void);
 extern void fpregs_mark_activate(void);
 
 /* Code that is unaware of kernel_fpu_begin_mask() can use this */
@@ -66,21 +66,8 @@ static inline void kernel_fpu_begin(void)
  *
  * Disabling preemption also serializes against kernel_fpu_begin().
  */
-static inline void fpregs_lock(void)
-{
-	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
-		local_bh_disable();
-	else
-		preempt_disable();
-}
-
-static inline void fpregs_unlock(void)
-{
-	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
-		local_bh_enable();
-	else
-		preempt_enable();
-}
+extern void fpregs_lock(void);
+extern void fpregs_unlock(void);
 
 #ifdef CONFIG_X86_DEBUG_FPU
 extern void fpregs_assert_state_consistent(void);
diff --git a/arch/x86/include/asm/simd.h b/arch/x86/include/asm/simd.h
index a341c878e977..bdc0629bd987 100644
--- a/arch/x86/include/asm/simd.h
+++ b/arch/x86/include/asm/simd.h
@@ -8,5 +8,5 @@
  */
 static __must_check inline bool may_use_simd(void)
 {
-	return irq_fpu_usable();
+	return kernel_fpu_usable();
 }
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index c049561f373a..7361583a5cfb 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -41,62 +41,127 @@ struct fpu_state_config fpu_user_cfg __ro_after_init;
  */
 struct fpstate init_fpstate __ro_after_init;
 
+/* Track in-kernel FPU usage */
+static DEFINE_PER_CPU(bool, fpu_in_use);
+
 /*
- * Track whether the kernel is using the FPU state
- * currently.
- *
- * This flag is used:
+ * This protects against preemption, soft interrupts and in-kernel FPU
+ * usage on both !RT and RT enabled kernels.
  *
- *   - by IRQ context code to potentially use the FPU
- *     if it's unused.
+ * !RT kernels use local_bh_disable() to prevent soft interrupt processing
+ * and preemption.
  *
- *   - to debug kernel_fpu_begin()/end() correctness
+ * On RT kernels local_bh_disable() is not sufficient because it only
+ * serializes soft interrupt related sections via a local lock, but stays
+ * preemptible. Disabling preemption is the right choice here as bottom
+ * half processing is always in thread context on RT kernels so disabling
+ * preemption implicitly prevents bottom half processing.
  */
-static DEFINE_PER_CPU(bool, in_kernel_fpu);
+static inline void fpu_in_use_begin(void)
+{
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+		local_bh_disable();
+	else
+		preempt_disable();
+
+	WARN_ON_ONCE(__this_cpu_read(fpu_in_use));
+	__this_cpu_write(fpu_in_use, true);
+}
+
+static inline void fpu_in_use_end(void)
+{
+	WARN_ON_ONCE(!__this_cpu_read(fpu_in_use));
+	__this_cpu_write(fpu_in_use, false);
+
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+		local_bh_enable();
+	else
+		preempt_enable();
+}
 
 /*
  * Track which context is using the FPU on the CPU:
  */
 DEFINE_PER_CPU(struct fpu *, fpu_fpregs_owner_ctx);
 
-static bool kernel_fpu_disabled(void)
+/**
+ * fpregs_lock - Lock FPU state for current's FPU state maintenance operations
+ *
+ * For FPU internal usage to initialize, safe, restore FPU state of
+ * the current task. Not for in-kernel FPU usage.
+ */
+void fpregs_lock(void)
 {
-	return this_cpu_read(in_kernel_fpu);
+	fpu_in_use_begin();
 }
+EXPORT_SYMBOL_GPL(fpregs_lock);
 
-static bool interrupted_kernel_fpu_idle(void)
+/**
+ * fpregs_unlock - Unlock FPU state after maintenance operations
+ *
+ * Counterpart to fpregs_lock().
+ */
+void fpregs_unlock(void)
 {
-	return !kernel_fpu_disabled();
+	fpu_in_use_end();
 }
+EXPORT_SYMBOL_GPL(fpregs_unlock);
 
-/*
- * Were we in user mode (or vm86 mode) when we were
- * interrupted?
+/**
+ * kernel_fpu_usable - Check whether kernel FPU usage is possible
  *
- * Doing kernel_fpu_begin/end() is ok if we are running
- * in an interrupt context from user mode - we'll just
- * save the FPU state as required.
+ * Must be checked before calling kernel_fpu_begin().
  */
-static bool interrupted_user_mode(void)
+bool kernel_fpu_usable(void)
 {
-	struct pt_regs *regs = get_irq_regs();
-	return regs && user_mode(regs);
+	if (WARN_ON_ONCE(in_nmi()))
+		return false;
+
+	return !in_hardirq() && !this_cpu_read(fpu_in_use);
 }
+EXPORT_SYMBOL(kernel_fpu_usable);
 
-/*
- * Can we use the FPU in kernel mode with the
- * whole "kernel_fpu_begin/end()" sequence?
+/**
+ * kernel_fpu_begin_mask - Start a in-kernel FPU usage section
+ * @kfpu_mask:		Bitmask to indicate initialization requirements
  *
- * It's always ok in process context (ie "not interrupt")
- * but it is sometimes ok even from an irq.
+ * Has to be invoked before in-kernel FPU usage. This saves the current
+ * tasks FPU register state if necessary and initializes MXCSR and/or the
+ * legacy FPU depending on @kfpu_mask.
+ *
+ * The function returns with softirqs disabled, so the call site has to
+ * ensure that the FPU usage is limited in terms of runtime.
  */
-bool irq_fpu_usable(void)
+void kernel_fpu_begin_mask(unsigned int kfpu_mask)
 {
-	return !in_interrupt() ||
-		interrupted_user_mode() ||
-		interrupted_kernel_fpu_idle();
+	fpu_in_use_begin();
+
+	if (!(current->flags & PF_KTHREAD) &&
+	    !test_thread_flag(TIF_NEED_FPU_LOAD)) {
+		set_thread_flag(TIF_NEED_FPU_LOAD);
+		save_fpregs_to_fpstate(&current->thread.fpu);
+	}
+	__cpu_invalidate_fpregs_state();
+
+	/* Put sane initial values into the control registers. */
+	if (likely(kfpu_mask & KFPU_MXCSR) && boot_cpu_has(X86_FEATURE_XMM))
+		ldmxcsr(MXCSR_DEFAULT);
+
+	if (unlikely(kfpu_mask & KFPU_387) && boot_cpu_has(X86_FEATURE_FPU))
+		asm volatile ("fninit");
 }
-EXPORT_SYMBOL(irq_fpu_usable);
+EXPORT_SYMBOL_GPL(kernel_fpu_begin_mask);
+
+/**
+ * kernel_fpu_end - End a in-kernel FPU usage section
+ *
+ * Counterpart to kernel_fpu_begin_mask().
+ */
+void kernel_fpu_end(void)
+{
+	fpu_in_use_end();
+}
+EXPORT_SYMBOL_GPL(kernel_fpu_end);
 
 /*
  * Track AVX512 state use because it is known to slow the max clock
@@ -420,40 +485,6 @@ int fpu_copy_uabi_to_guest_fpstate(struct fpu_guest *gfpu, const void *buf,
 EXPORT_SYMBOL_GPL(fpu_copy_uabi_to_guest_fpstate);
 #endif /* CONFIG_KVM */
 
-void kernel_fpu_begin_mask(unsigned int kfpu_mask)
-{
-	preempt_disable();
-
-	WARN_ON_FPU(!irq_fpu_usable());
-	WARN_ON_FPU(this_cpu_read(in_kernel_fpu));
-
-	this_cpu_write(in_kernel_fpu, true);
-
-	if (!(current->flags & PF_KTHREAD) &&
-	    !test_thread_flag(TIF_NEED_FPU_LOAD)) {
-		set_thread_flag(TIF_NEED_FPU_LOAD);
-		save_fpregs_to_fpstate(&current->thread.fpu);
-	}
-	__cpu_invalidate_fpregs_state();
-
-	/* Put sane initial values into the control registers. */
-	if (likely(kfpu_mask & KFPU_MXCSR) && boot_cpu_has(X86_FEATURE_XMM))
-		ldmxcsr(MXCSR_DEFAULT);
-
-	if (unlikely(kfpu_mask & KFPU_387) && boot_cpu_has(X86_FEATURE_FPU))
-		asm volatile ("fninit");
-}
-EXPORT_SYMBOL_GPL(kernel_fpu_begin_mask);
-
-void kernel_fpu_end(void)
-{
-	WARN_ON_FPU(!this_cpu_read(in_kernel_fpu));
-
-	this_cpu_write(in_kernel_fpu, false);
-	preempt_enable();
-}
-EXPORT_SYMBOL_GPL(kernel_fpu_end);
-
 /*
  * Sync the FPU register state to current's memory register state when the
  * current task owns the FPU. The hardware register state is preserved.
diff --git a/net/netfilter/nft_set_pipapo_avx2.c b/net/netfilter/nft_set_pipapo_avx2.c
index 52e0d026d30a..dde1d2260f51 100644
--- a/net/netfilter/nft_set_pipapo_avx2.c
+++ b/net/netfilter/nft_set_pipapo_avx2.c
@@ -1128,7 +1128,7 @@ bool nft_pipapo_avx2_lookup(const struct net *net, const struct nft_set *set,
 	bool map_index;
 	int i, ret = 0;
 
-	if (unlikely(!irq_fpu_usable()))
+	if (unlikely(!kernel_fpu_usable()))
 		return nft_pipapo_lookup(net, set, key, ext);
 
 	m = rcu_dereference(priv->match);

Re: [patch 3/3] x86/fpu: Make FPU protection more robust

[Jason A. Donenfeld]

Hi Thomas,

On Thu, May 05, 2022 at 02:55:58AM +0200, Thomas Gleixner wrote:
> > So if truly the only user of this is random.c as of 5.18 (is it? I'm
> > assuming from a not very thorough survey...), and if the performance
> > boost doesn't even exist, then yeah, I think it'd make sense to just get
> > rid of it, and have kernel_fpu_usable() return false in those cases.
> >
> > I'll run some benchmarks on a little bit more hardware in representative
> > cases and see.
> 
> Find below a combo patch which makes use of strict softirq serialization
> for the price of not supporting the hardirq FPU usage. 

Thanks, I'll give it a shot in the morning (3am) when trying to do a
more realistic benchmark. But just as a synthetic thing, I ran the
numbers in kBench900 and am getting:

     generic:    430 cycles per call
       ssse3:    315 cycles per call
      avx512:    277 cycles per call

for a single call to the compression function, which is the most any of
those mix_pool_bytes() calls do from add_{input,disk}_randomness(), on
Tiger Lake, using RDPMC from kernel space.

This _doesn't_ take into account the price of calling kernel_fpu_begin().
That's a little hard to bench synthetically by running it in a loop and
taking medians because of the lazy restoration. But that's an indication
anyway that I should be looking at the cost of the actual function as
its running in random.c, rather than the synthetic test. Will keep this
thread updated.

Jason

Re: [patch 3/3] x86/fpu: Make FPU protection more robust

[Thomas Gleixner]

Jason,

On Thu, May 05 2022 at 03:11, Jason A. Donenfeld wrote:
> On Thu, May 05, 2022 at 02:55:58AM +0200, Thomas Gleixner wrote:
>> > So if truly the only user of this is random.c as of 5.18 (is it? I'm
>> > assuming from a not very thorough survey...), and if the performance
>> > boost doesn't even exist, then yeah, I think it'd make sense to just get
>> > rid of it, and have kernel_fpu_usable() return false in those cases.
>> >
>> > I'll run some benchmarks on a little bit more hardware in representative
>> > cases and see.
>> 
>> Find below a combo patch which makes use of strict softirq serialization
>> for the price of not supporting the hardirq FPU usage. 
>
> Thanks, I'll give it a shot in the morning (3am) when trying to do a
> more realistic benchmark. But just as a synthetic thing, I ran the
> numbers in kBench900 and am getting:
>
>      generic:    430 cycles per call
>        ssse3:    315 cycles per call
>       avx512:    277 cycles per call
>
> for a single call to the compression function, which is the most any of
> those mix_pool_bytes() calls do from add_{input,disk}_randomness(), on
> Tiger Lake, using RDPMC from kernel space.

I'm well aware of the difference between synthetic benchmarks and real
world scenarios and with the more in depth instrumentation of these
things I'm even more concerned that the difference is underestimated.

> This _doesn't_ take into account the price of calling kernel_fpu_begin().
> That's a little hard to bench synthetically by running it in a loop and
> taking medians because of the lazy restoration. But that's an indication
> anyway that I should be looking at the cost of the actual function as
> its running in random.c, rather than the synthetic test. Will keep this
> thread updated.

Appreciated.

Thanks,

        tglx

Re: [patch 3/3] x86/fpu: Make FPU protection more robust

[Jason A. Donenfeld]

Hey again,

On Thu, May 05, 2022 at 03:21:43AM +0200, Thomas Gleixner wrote:
> > Thanks, I'll give it a shot in the morning (3am) when trying to do a
> > more realistic benchmark. But just as a synthetic thing, I ran the
> > numbers in kBench900 and am getting:
> >
> >      generic:    430 cycles per call
> >        ssse3:    315 cycles per call
> >       avx512:    277 cycles per call
> >
> > for a single call to the compression function, which is the most any of
> > those mix_pool_bytes() calls do from add_{input,disk}_randomness(), on
> > Tiger Lake, using RDPMC from kernel space.
> 
> I'm well aware of the difference between synthetic benchmarks and real
> world scenarios and with the more in depth instrumentation of these
> things I'm even more concerned that the difference is underestimated.
> 
> > This _doesn't_ take into account the price of calling kernel_fpu_begin().
> > That's a little hard to bench synthetically by running it in a loop and
> > taking medians because of the lazy restoration. But that's an indication
> > anyway that I should be looking at the cost of the actual function as
> > its running in random.c, rather than the synthetic test. Will keep this
> > thread updated.
> 
> Appreciated.

Interestingly, disabling the simd paths makes things around 283 cycles
slower on my Tiger Lake laptop, just doing ordinary things. I'm actually
slightly surprised, so I'll probably keep playing with this. My patch
for this is attached. Let me know if you have a different methodology in
mind...

Jason

diff --git a/drivers/char/random.c b/drivers/char/random.c
index bd292927654c..7a70a186c26b 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -53,6 +53,7 @@
 #include <linux/uuid.h>
 #include <linux/uaccess.h>
 #include <linux/suspend.h>
+#include <linux/sort.h>
 #include <crypto/chacha.h>
 #include <crypto/blake2s.h>
 #include <asm/processor.h>
@@ -755,9 +756,48 @@ static struct {
 	.lock = __SPIN_LOCK_UNLOCKED(input_pool.lock),
 };

+struct {
+	u32 durations[1 << 20];
+	u32 pos, len;
+} irqbench;
+
 static void _mix_pool_bytes(const void *in, size_t nbytes)
 {
+	u32 ctr = input_pool.hash.t[0];
+	cycles_t end, start = get_cycles();
 	blake2s_update(&input_pool.hash, in, nbytes);
+	end = get_cycles();
+
+	if (ctr == input_pool.hash.t[0] || !in_hardirq())
+		return;
+
+	irqbench.durations[irqbench.pos++ % ARRAY_SIZE(irqbench.durations)] = end - start;
+	irqbench.len = min_t(u32, irqbench.len + 1, ARRAY_SIZE(irqbench.durations));
+}
+
+static int cmp_u32(const void *a, const void *b)
+{
+	return *(const u32 *)a - *(const u32 *)b;
+}
+
+static int proc_do_irqbench_median(struct ctl_table *table, int write, void *buffer,
+				   size_t *lenp, loff_t *ppos)
+{
+	u32 len = READ_ONCE(irqbench.len), median, *sorted;
+	struct ctl_table fake_table = {
+		.data = &median,
+		.maxlen = sizeof(median)
+	};
+	if (!len)
+		return -ENODATA;
+	sorted = kmalloc_array(len, sizeof(*sorted), GFP_KERNEL);
+	if (!sorted)
+		return -ENOMEM;
+	memcpy(sorted, irqbench.durations, len * sizeof(*sorted));
+	sort(sorted, len, sizeof(*sorted), cmp_u32, NULL);
+	median = sorted[len / 2];
+	kfree(sorted);
+	return write ? 0 : proc_douintvec(&fake_table, 0, buffer, lenp, ppos);
 }

 /*
@@ -1709,6 +1749,18 @@ static struct ctl_table random_table[] = {
 		.mode		= 0444,
 		.proc_handler	= proc_do_uuid,
 	},
+	{
+		.procname	= "irqbench_median",
+		.mode		= 0444,
+		.proc_handler	= proc_do_irqbench_median,
+	},
+	{
+		.procname	= "irqbench_count",
+		.data		= &irqbench.len,
+		.maxlen		= sizeof(irqbench.len),
+		.mode		= 0444,
+		.proc_handler	= proc_douintvec,
+	},
 	{ }
 };

RE: [patch 3/3] x86/fpu: Make FPU protection more robust

[David Laight]

...
> +	cycles_t end, start = get_cycles();
>  	blake2s_update(&input_pool.hash, in, nbytes);
> +	end = get_cycles();

If get_cycles() is rdtsc then that gives meaningless numbers.
The cpu clock frequency will change on you.

You can use one of the performance counters to get an actual
cycle count - although that is only stable for 'hot cache'
as any memory accesses are clock speed dependant.

OTOH the entropy mixing is very likely to be 'cold cache'
and all the unrolling in blakes7 will completely kill
performance.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [patch 3/3] x86/fpu: Make FPU protection more robust

[Jason A. Donenfeld]

On Thu, May 5, 2022 at 1:34 PM David Laight <David.Laight@aculab.com> wrote:
>
> ...
> > +     cycles_t end, start = get_cycles();
> >       blake2s_update(&input_pool.hash, in, nbytes);
> > +     end = get_cycles();
>
> If get_cycles() is rdtsc then that gives meaningless numbers.
> The cpu clock frequency will change on you.
>
> You can use one of the performance counters to get an actual

Indeed. In the process of wiring up rdpmc now.

Jason

RE: [patch 3/3] x86/fpu: Make FPU protection more robust

[David Laight]

> -----Original Message-----
> From: Jason A. Donenfeld <Jason@zx2c4.com>
> Sent: 05 May 2022 12:36
> To: David Laight <David.Laight@ACULAB.COM>
> Cc: Thomas Gleixner <tglx@linutronix.de>; Peter Zijlstra <peterz@infradead.org>; Borislav Petkov
> <bp@alien8.de>; LKML <linux-kernel@vger.kernel.org>; x86@kernel.org; Filipe Manana
> <fdmanana@suse.com>; linux-crypto@vger.kernel.org
> Subject: Re: [patch 3/3] x86/fpu: Make FPU protection more robust
> 
> On Thu, May 5, 2022 at 1:34 PM David Laight <David.Laight@aculab.com> wrote:
> >
> > ...
> > > +     cycles_t end, start = get_cycles();
> > >       blake2s_update(&input_pool.hash, in, nbytes);
> > > +     end = get_cycles();
> >
> > If get_cycles() is rdtsc then that gives meaningless numbers.
> > The cpu clock frequency will change on you.
> >
> > You can use one of the performance counters to get an actual
> 
> Indeed. In the process of wiring up rdpmc now.

I've used this before now.
But the loop getting the pmc value in non-deterministic.
So I sometimes remove it.
Also you need to add a serialising instruction - otherwise
the pmc get read before the code you are measuring
actually finishes.

I've used similar code to measure iterations on the ip checksum code.
Can show how many bytes/clock that achieves in its inner loop.
Which can match what you might expect the instructions to generate.

	David

static inline unsigned int rdpmc(unsigned int counter)
{
        unsigned int low, high;

        asm volatile("rdpmc" : "=a" (low), "=d" (high) : "c" (counter));

        // return low bits, counter might to 32 or 40 bits wide.
        return low;
}

static inline unsigned int rdtsc(void)
{
        unsigned int low, high;

        asm volatile("rdtsc" : "=a" (low), "=d" (high));

        return low;
}

unsigned int read_cpu_cycles(void)
{
        static struct perf_event_attr perf_attr = {
                .type = PERF_TYPE_HARDWARE,
                .config = PERF_COUNT_HW_CPU_CYCLES,
                .pinned = 1,
        };
        static __thread struct perf_event_mmap_page *pc;
        static int pmc_failed;

        unsigned int seq, idx, count;

        if (pmc_failed)
                return rdtsc();

        if (!pc) {
                int perf_fd;
                perf_fd = syscall(__NR_perf_event_open, &perf_attr, 0, -1, -1, 0);
                if (perf_fd < 0) {
                        pmc_failed = 1;
                        return rdtsc();
                }
                pc = mmap(NULL, sysconf(_SC_PAGESIZE), PROT_READ, MAP_SHARED, perf_fd, 0);
                close(perf_fd);
                if (pc == MAP_FAILED) {
                        pmc_failed = 1;
                        return rdtsc();
                }
        }

        do {
                seq = pc->lock;
                asm volatile("":::"memory");
                idx = pc->index;
                if (!idx) //  || !pc->cap_user_rdpmc)
                        return 0;
                count = pc->offset + rdpmc(idx - 1);
                asm volatile("":::"memory");
        } while (pc->lock != seq);

        return count;
}

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [patch 3/3] x86/fpu: Make FPU protection more robust

[Jason A. Donenfeld]

Hi David,

On Thu, May 05, 2022 at 11:34:40AM +0000, David Laight wrote:
> OTOH the entropy mixing is very likely to be 'cold cache'
> and all the unrolling in blakes7 will completely kill
> performance.

I've seen you mention the BLAKE2s unrolling in like 8 different threads
now, and I'm not convinced that you're entirely wrong, nor am I
convinced that you're entirely right. My response to you is the same as
always: please send a patch with some measurements! I'd love to get this
worked out in a real way.

The last time I went benching these, the unrolled code was ~100 cycles
faster, if I recall correctly, than the rolled code, when used from
WireGuard's hot path. I don't doubt that a cold path would be more
fraught, though, as that's a decent amount of code. So the question is
how to re-roll the rounds without sacrificing those 100 cycles.

In order to begin to figure that out, we have to look at why the
re-rolled loop is slow and the unrolled loop fast. It's not because of
complicated pipeline things. It's because the BLAKE2s permutation is
actually 10 different permutations, one for each round. Take a look at
the core function, G, and its uses of the round number, r:

    #define G(r, i, a, b, c, d) do { \
        a += b + m[blake2s_sigma[r][2 * i + 0]]; \
        d = ror32(d ^ a, 16); \
        c += d; \
        b = ror32(b ^ c, 12); \
        a += b + m[blake2s_sigma[r][2 * i + 1]]; \
        d = ror32(d ^ a, 8); \
        c += d; \
        b = ror32(b ^ c, 7); \
    } while (0)

The blake2s_sigma array is a `static const u8 blake2s_sigma[10][16]`,
with a row for every one of the 10 rounds. What this is actually doing
is reading the message words in a different order each round, so that
the whole permutation is different.

When the loop is unrolled, blake2s_sigma gets inlined, and then there
are no memory accesses. When it's re-rolled, every round accesses
blake2s_sigma 16 times, which hinders performance.

You'll notice, on the other hand, that the SIMD hand coded assembly
implementations do not unroll. The trick is to hide the cost of the
blake2s_sigma indirection in the data dependencies, so that performance
isn't affected. Naively re-rolling the generic code does not inspire the
compiler to do that. But maybe you can figure something out?

Anyway, that's about where my thinking is on this, but I'd love to see
some patches from you at some point if you're interested.

Jason

RE: [patch 3/3] x86/fpu: Make FPU protection more robust

[David Laight]

From: Jason A. Donenfeld
> Sent: 06 May 2022 23:34
> 
> Hi David,
> 
> On Thu, May 05, 2022 at 11:34:40AM +0000, David Laight wrote:
> > OTOH the entropy mixing is very likely to be 'cold cache'
> > and all the unrolling in blakes7 will completely kill
> > performance.
> 
> I've seen you mention the BLAKE2s unrolling in like 8 different threads
> now, and I'm not convinced that you're entirely wrong, nor am I
> convinced that you're entirely right. My response to you is the same as
> always: please send a patch with some measurements! I'd love to get this
> worked out in a real way.
> 
> The last time I went benching these, the unrolled code was ~100 cycles
> faster, if I recall correctly, than the rolled code, when used from
> WireGuard's hot path. I don't doubt that a cold path would be more
> fraught, though, as that's a decent amount of code. So the question is
> how to re-roll the rounds without sacrificing those 100 cycles.
> 
> In order to begin to figure that out, we have to look at why the
> re-rolled loop is slow and the unrolled loop fast. It's not because of
> complicated pipeline things. It's because the BLAKE2s permutation is
> actually 10 different permutations, one for each round. Take a look at
> the core function, G, and its uses of the round number, r:
> 
>     #define G(r, i, a, b, c, d) do { \
>         a += b + m[blake2s_sigma[r][2 * i + 0]]; \
>         d = ror32(d ^ a, 16); \
>         c += d; \
>         b = ror32(b ^ c, 12); \
>         a += b + m[blake2s_sigma[r][2 * i + 1]]; \
>         d = ror32(d ^ a, 8); \
>         c += d; \
>         b = ror32(b ^ c, 7); \
>     } while (0)

Each of those lines is a couple of instructions and they are
all dependant on the preceding value.
I count 14 - excluding the m[] accesses.
There are 80 copies of G() - total 1120, or 17.5/byte.
To get any faster than that you need to get the compiler
to interleave the generated code for multiple expansions of G().

> The blake2s_sigma array is a `static const u8 blake2s_sigma[10][16]`,
> with a row for every one of the 10 rounds. What this is actually doing
> is reading the message words in a different order each round, so that
> the whole permutation is different.
> 
> When the loop is unrolled, blake2s_sigma gets inlined, and then there
> are no memory accesses. When it's re-rolled, every round accesses
> blake2s_sigma 16 times, which hinders performance.

It shouldn't really make much difference.
There are only two memory reads for each 14 arithmetic ops.
So unless you manage to maintain 4 instructions/clock there
are spare clocks for the extra memory cycles.
Any that is assuming one read/clock, modern x86 can do 2
(with a following wind!)
On x86 the array index is free.

> You'll notice, on the other hand, that the SIMD hand coded assembly
> implementations do not unroll. The trick is to hide the cost of the
> blake2s_sigma indirection in the data dependencies, so that performance
> isn't affected. Naively re-rolling the generic code does not inspire the
> compiler to do that. But maybe you can figure something out?

I've not looked at that version.
I have written AVX/SSE code - hard work finding the asm mnumonics!

> Anyway, that's about where my thinking is on this, but I'd love to see
> some patches from you at some point if you're interested.

Ok I just ran some tests looping over the ROUND() without updating v[].
These are using rdpmc to time single calls - not averaging over
a large number of iterations.

On my i7-7700 the unrolled loop is about 6.2 clocks/byte.
The 'cold cache' for a single 64 byte block is about 20 clocks/byte.

If I use:
	for (u32 i = 0; i < 10; i++) ROUND(i);
it drops to 7.8 clocks/byte but the single block is about 15.
Part of the problem is extra register spills to stack.

So we play some games:
Remove the blake2s_sigma[] out of G() into ROUND()
so the 'normal' code uses ROUND(blake2s_sigma[0]) (etc).
Then we can use the loop:
	for (const u8 *bs = &blake2s_sigma[0][0]; bs < end; bs += 16)
		ROUND(bs);
(with some bits to make it compile.)
Annoyingly that makes the compiler spill all the bs[] to stack.
A few carefully placed barrier() would help.
But simpler is 'volatile const u8 *bs'.
This has the desired effect of interleaving the 'sigma' reads
with the buffer ones.
This gave me 7.2 clocks/byte, single block maybe 14.

So about 1 clock/byte (maybe 15%) slower.
OTOH in many cases this isn't critical.

The cold cache cutoff for the unrolled loop is around 256 bytes.

The other big effect the unrolled loop has for small buffers
is that it displaces a lot of code from the i-cache.
That will make the unrolled code worse for many real situations.

I suspect that the compiler ends up spilling another register (or 2)
to the stack.
Hand assembly could avoid that.

I've not looked at what gets spilled, gcc can make horrid choices.
It might be worth making (say) a and c volatile (by splitting v[])
and using a temporary in G(), eg:
	c = t = c + d;
	b = ror32(b ^ t, 12);

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [patch 3/3] x86/fpu: Make FPU protection more robust

[Jason A. Donenfeld]

Hey again Thomas,

On Thu, May 05, 2022 at 01:02:02PM +0200, Jason A. Donenfeld wrote:
> Interestingly, disabling the simd paths makes things around 283 cycles
> slower on my Tiger Lake laptop, just doing ordinary things. I'm actually
> slightly surprised, so I'll probably keep playing with this. My patch
> for this is attached. Let me know if you have a different methodology in
> mind...

Using RDPMC/perf, the performance is shown to be even closer for real
world cases, with the simd code only ~80 cycles faster. Bench code
follows below. If the observation on this hardware holds for other
hardware, we can probably improve the performance of the generic code a
bit, and then the difference really won't matter. Any thoughts about
this and the test code?

Jason

diff --git a/drivers/char/random.c b/drivers/char/random.c
index bd292927654c..6577e9f2f3b7 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -53,6 +53,7 @@
 #include <linux/uuid.h>
 #include <linux/uaccess.h>
 #include <linux/suspend.h>
+#include <linux/sort.h>
 #include <crypto/chacha.h>
 #include <crypto/blake2s.h>
 #include <asm/processor.h>
@@ -755,9 +756,54 @@ static struct {
 	.lock = __SPIN_LOCK_UNLOCKED(input_pool.lock),
 };
 
+static DEFINE_PER_CPU(int, pmc_index) = -1;
+static struct {
+	u32 durations[1 << 20];
+	u32 pos, len;
+} irqbench;
+
 static void _mix_pool_bytes(const void *in, size_t nbytes)
 {
+	int idx = *this_cpu_ptr(&pmc_index);
+	u32 ctr = input_pool.hash.t[0], reg = 0;
+	cycles_t end, start;
+
+
+	native_cpuid(&reg, &reg, &reg, &reg);
+	start = idx == -1 ? 0 : native_read_pmc(idx);
 	blake2s_update(&input_pool.hash, in, nbytes);
+	end = idx == -1 ? 0 : native_read_pmc(idx);
+
+	if (ctr == input_pool.hash.t[0] || !in_hardirq() || idx == -1)
+		return;
+
+	irqbench.durations[irqbench.pos++ % ARRAY_SIZE(irqbench.durations)] = end - start;
+	irqbench.len = min_t(u32, irqbench.len + 1, ARRAY_SIZE(irqbench.durations));
+}
+
+static int cmp_u32(const void *a, const void *b)
+{
+	return *(const u32 *)a - *(const u32 *)b;
+}
+
+static int proc_do_irqbench_median(struct ctl_table *table, int write, void *buffer,
+				   size_t *lenp, loff_t *ppos)
+{
+	u32 len = READ_ONCE(irqbench.len), median, *sorted;
+	struct ctl_table fake_table = {
+		.data = &median,
+		.maxlen = sizeof(median)
+	};
+	if (!len)
+		return -ENODATA;
+	sorted = kmalloc_array(len, sizeof(*sorted), GFP_KERNEL);
+	if (!sorted)
+		return -ENOMEM;
+	memcpy(sorted, irqbench.durations, len * sizeof(*sorted));
+	sort(sorted, len, sizeof(*sorted), cmp_u32, NULL);
+	median = sorted[len / 2];
+	kfree(sorted);
+	return write ? 0 : proc_douintvec(&fake_table, 0, buffer, lenp, ppos);
 }
 
 /*
@@ -1709,6 +1755,18 @@ static struct ctl_table random_table[] = {
 		.mode		= 0444,
 		.proc_handler	= proc_do_uuid,
 	},
+	{
+		.procname	= "irqbench_median",
+		.mode		= 0444,
+		.proc_handler	= proc_do_irqbench_median,
+	},
+	{
+		.procname	= "irqbench_count",
+		.data		= &irqbench.len,
+		.maxlen		= sizeof(irqbench.len),
+		.mode		= 0444,
+		.proc_handler	= proc_douintvec,
+	},
 	{ }
 };
 
@@ -1718,6 +1776,21 @@ static struct ctl_table random_table[] = {
  */
 static int __init random_sysctls_init(void)
 {
+	int i;
+	struct perf_event *cycles_event;
+	struct perf_event_attr perf_cycles_attr = {
+		.type = PERF_TYPE_HARDWARE,
+		.config = PERF_COUNT_HW_CPU_CYCLES,
+		.size = sizeof(struct perf_event_attr),
+		.pinned = true
+	};
+	for_each_possible_cpu(i) {
+		cycles_event = perf_event_create_kernel_counter(&perf_cycles_attr, i, NULL, NULL, NULL);
+		if (IS_ERR(cycles_event))
+			pr_err("unable to create perf counter on cpu %d: %ld\n", i, PTR_ERR(cycles_event));
+		else
+			*per_cpu_ptr(&pmc_index, i) = cycles_event->hw.event_base_rdpmc;
+	}
 	register_sysctl_init("kernel/random", random_table);
 	return 0;
 }
 

Re: [patch 3/3] x86/fpu: Make FPU protection more robust

[Jason A. Donenfeld]

Hi Thomas,

On Wed, May 04, 2022 at 09:05:01PM +0200, Jason A. Donenfeld wrote:
> Hey Thomas,
> 
> On Wed, May 04, 2022 at 06:45:45PM +0200, Thomas Gleixner wrote:
> > add_disk_randomness() on !RT kernels. That's what made me look into this
> > in the first place as it unearthed the long standing FPU protection
> > bug. See the first patch in this thread.
> > 
> > Possibly add_device_randomness() too, but I haven't seen evidence so far.
> 
> It looks like it's being hit from add_input_randomness() via
> input_handle_event() too. There are two positions we could take toward
> this:
> 
> One stance to take would be that if an event happens in an interrupt,
> add_interrupt_randomness() should in theory already be siphashing in a
> cycle counter so additional calls to add_{input,disk}_randomness() don't
> contribute substantially (unless you assume the num field has some
> entropic value). If that's compelling, then the next thing to do would
> be adding a `if (in_interrupt()) return;` early on in some function, and
> then we forever after impose a rule, "never mix into the input pool
> directly from an irq".
> 
> The other stance is that these input/disk events are relatively rare --
> compared to, say, a storm of interrupts from a NIC -- so mixing into the
> input pool from there isn't actually a problem, and we benefit from the
> quasi domain-specific accounting and the superior mixing function,
> there, so keep it around. And the non-raw spinlock on the input pool
> won't negatively affect RT from this context, because all its callers on
> RT should be threaded.

It turned out to be more complicated than this. Sometimes a given
interrupt handler would have multiple calls to add_disk_randomness(),
followed by a single call to add_interrupt_randomness(). Since those
multiple calls all represent different things being measured, the second
option of discarding them didn't seem like a good idea, but the first
option seemed pretty bad too, since the status quo is way too much
hashing from an irq handler. Further complicating things is the fact
that add_{input,disk}_randomness() is never just called from irq or from
elsewhere, as different drivers do different things. Luckily the way the
entropy estimator currently works allows for a pretty straight forward
compromise, which I posted here:

https://lore.kernel.org/lkml/20220506215454.1671-2-Jason@zx2c4.com/

The upshot for this discussion is that it means we can probably get rid
of hard irq FPU support. (This is in addition to the performance
argument, in which tentatively it seemed like the difference wasn't
/that/ much to justify the complexity.)

I intend to mull this over a bit more, though, so if my patch is to make
it in, it'll be for 5.19 and certainly not 5.18. So for 5.18, I think
it's probably a good idea to apply the patches in this thread. And then
for 5.19 we can take the next step.

Jason