Re: [PATCH v2] random: remove batched entropy locking

[Sebastian Andrzej Siewior]

On 2022-02-04 14:42:03 [+0100], Jason A. Donenfeld wrote:
> Hi Sebastian,
Hi Jason,

> Please calm down a bit: this patch doesn't minimize the importance of
> working out a real solution for PREEMPT_RT, and I'm not under the
> illusion that this one here is the silver bullet. It does, however,
> have other merits, which may or may not have anything to do with
> PREEMPT_RT. To reiterate: I am taking your PREEMPT_RT concerns
> seriously, and I want to come up with a solution to that, which we're
> working toward more broadly in that other thread.
> 
> Per your feedback on v1, this is no longer marked for stable and no
> longer purports to fix the PREEMPT_RT issues entirely. Actually, a
> large motivation for this includes the reason why Andy's original
> patch was laying around in the first place: we're trying to make this
> code faster.

The commit in tree you cited is b43db859a36cb553102c9c80431fc44618703bda.
It does not mention anything regarding faster nor the performance
improvement and conditions (hoth path, etc). It still has a stable tag.

> I can improve the commit message a bit though.
> 
> On Fri, Feb 4, 2022 at 12:10 PM Sebastian Andrzej Siewior
> <bigeasy@xxxxxxxxxxxxx> wrote:
> > - This splat only occurs with CONFIG_PROVE_RAW_LOCK_NESTING enabled.
> 
> Right, the commit message for v2 mentions that.
> 
> > - The problem identified by the splat affects only PREEMPT_RT. Non-RT is
> >   not affected by this.
> 
> Right.
> 
> >
> > - This patch disables interrupts and invokes extract_crng() which leads
> >   to other problems.
> 
> The existing code, which uses a spinlock, also disables interrupts,
> right? So this isn't actually regressing in that regard. It just
> doesn't fix your PREEMPT_RT issue, right?

The existing code uses spin_lock_irqsave() which do not disable on
PREEMPT_RT. The local_irq_save() on the hand does.

> Or is the issue you see that spinlock_t is a mutex on PREEMPT_RT, so
> we're disabling interrupts here in a way that we _weren't_ originally,
> in a PREEMPT_RT context? If that's the case, then I think I see your
> objection.

Exactly.
 
> I wonder if it'd be enough here to disable preemption instead? But
> then we run into trouble if this is called from an interrupt.

Disabling preemption does not allow to acquire sleeping locks so no win.

> Maybe it'd be best to retain the spinlock_t, which will amount to
> disabling interrupts on !PREEMPT_RT, since it'll never be contended,
> but will turn into a mutex on PREEMPT_RT, where it'll do the right
> thing from an exclusivity perspective. Would this be reasonable?

what does retain the spinlock_t mean since we already have a spinlock_t?

> Andy? Any suggestions?
> 
> Jason

Sebastian