[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <7fc8a1db60de959fd22ae898e86683f57fb07be2.camel@gmx.de>
Date: Sat, 06 Sep 2025 04:32:41 +0200
From: Mike Galbraith <efault@....de>
To: John Ogness <john.ogness@...utronix.de>, Breno Leitao
<leitao@...ian.org>, Simon Horman <horms@...nel.org>, kuba@...nel.org,
calvin@...nvd.org
Cc: Pavel Begunkov <asml.silence@...il.com>, Johannes Berg
<johannes@...solutions.net>, paulmck@...nel.org, LKML
<linux-kernel@...r.kernel.org>, netdev@...r.kernel.org, boqun.feng@...il.com
Subject: Re: netconsole: HARDIRQ-safe -> HARDIRQ-unsafe lock order warning
On Fri, 2025-09-05 at 14:54 +0206, John Ogness wrote:
>
> On 2025-08-26, Breno Leitao <leitao@...ian.org> wrote:
> > On Fri, Aug 22, 2025 at 05:54:28AM +0200, Mike Galbraith wrote:
> > > On Thu, 2025-08-21 at 10:35 -0700, Breno Leitao wrote:
> > > > > On Thu, Aug 21, 2025 at 05:51:59AM +0200, Mike Galbraith wrote:
> > >
> > > > > > > --- a/drivers/net/netconsole.c
> > > > > > > +++ b/drivers/net/netconsole.c
> > > > > > > @@ -1952,12 +1952,12 @@ static void netcon_write_thread(struct c
> > > > > > > static void netconsole_device_lock(struct console *con, unsigned long *flags)
> > > > > > > {
> > > > > > > /* protects all the targets at the same time */
> > > > > > > - spin_lock_irqsave(&target_list_lock, *flags);
> > > > > > > + spin_lock(&target_list_lock);
> > > > >
> > > > > I personally think this target_list_lock can be moved to an RCU lock.
> > > > >
> > > > > If that is doable, then we probably make netconsole_device_lock()
> > > > > to a simple `rcu_read_lock()`, which would solve this problem as well.
> > >
> > > The bigger issue for the nbcon patch would seem to be the seemingly
> > > required .write_atomic leading to landing here with disabled IRQs.
>
> Using spin_lock_irqsave()/spin_unlock_irqrestore() within the
> ->device_lock() and ->device->unlock() callbacks is fine. Even with
> PREEMPT_RT this is fine. If you can use RCU to synchronize the target
> list, that is probably a nice optimization, but it is certainly not a
> requirement from the NBCON (and PREEMPT_RT/lockdep) perspective.
I was referring there to !RT+wireless locking meets IRQs disabled, ever
per lockdep, an intolerance shared with RT+spinlock_t.
> > In this case, instead of transmitting through netpoll directly in the
> > .write_atomic context, we could queue the messages for later delivery.
>
> The ->write_atomic() callback is intended to perform immediate
> transmission. It is called with hardware interrupts disabled and is even
> expected to work from NMI context. If you are not able to implement
> these requirements, do not implement ->write_atomic(). Implementing some
> sort of deferrment mechanism is inappropriate. Such a mechanism already
> exists based on ->write_thread().
Truly atomic packet blasting would be a case of happiness, but barring
that, deferment is way better than the nothing that's available to both
RT and !RT+wireless here/now. With a .write_atomic that's really just
.write_thread, both RT and !RT+wireless managed to successfully send a
death rattle with the WIP nbcon patch.. a progression for each of them.
>
-Mike
Powered by blists - more mailing lists