Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 4 Feb 2022 15:01:47 +0100
From:   Sebastian Andrzej Siewior <bigeasy@linutronix.de>
To:     "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc:     Andy Lutomirski <luto@amacapital.net>,
        Boqun Feng <boqun.feng@gmail.com>,
        Will Deacon <will@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Waiman Long <longman@redhat.com>,
        Sultan Alsawaf <sultan@kerneltoast.com>,
        Theodore Ts'o <tytso@mit.edu>,
        Andy Lutomirski <luto@kernel.org>,
        Jonathan =?utf-8?Q?Neusch=C3=A4fer?= <j.neuschaefer@gmx.net>,
        LKML <linux-kernel@vger.kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH v2] random: remove batched entropy locking
Message-ID: <Yf0xy4kZ2Mn65yp8@linutronix.de>
References: <CAHmME9pe2BEJV4WiZNHmDmH_XK621Qqr1JCBdgTNZmr4JGBA4w@mail.gmail.com>
 <20220128223548.97807-1-Jason@zx2c4.com>
 <CAHmME9qtjZX2kVNSQqUsTrZv1cdR8y6n3yZS-RnpVCCzX9okcA@mail.gmail.com>
 <Yf0JlXf3ARsBpL9K@linutronix.de>
 <CAHmME9r0XxX3LqNLpVeqAjDQ_OVskPf15QOwxtZYy0tb_x_7HQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CAHmME9r0XxX3LqNLpVeqAjDQ_OVskPf15QOwxtZYy0tb_x_7HQ@mail.gmail.com>
Precedence: bulk

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