Messages in this thread | | | Date | Fri, 4 Feb 2022 15:01:47 +0100 | From | Sebastian Andrzej Siewior <> | Subject | Re: [PATCH v2] random: remove batched entropy locking |
| |
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@linutronix.de> 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
| |