On 2026-09-02 15:14:20 [+0100], Puranjay Mohan wrote:
> >
> > But it is wrong to talk about NMI and do this for other reasons not
> > mentioning why this was needed/ made sense.
> > Can this be fixed?
> > Also what is the reasoning for doing it from any IRQ disabled region?
>
> So, the main reason for this patchset is to make call_(s)rcu() work
> from BPF programs, and these BPF programs can run from any context
> like an NMI perf event interrupt or maybe attached to a function
> through ftrace which is in the call chain of call_rcu() itself.
This I expected.
> call_rcu() takes locks with irqs disabled or it modifies lists
> non-atomically with irqs disabled. This means that a BPF program that
> runs from NMI with the lock already taken will deadlock, or if it runs
> between these non-atomic list modifications, it will corrupt the
> lists. A BPF program can also synchronously re-enter call_rcu() if it
> is attached to a function in the call chain of call_rcu() that is
> called with a lock taken. So, all these cases are detected with
> irqs_disabled() and on detection we do stuff that doesn't require
> taking any locks and is safe even if re-entered.
Attaching a BPF program to call_rcu() and re-entering sounds great. But
there is nothing unique about call_rcu(), is there? We do have certain
amount of functions which can be invoked from NMI and their usage is
limited.
Anyway. If this is the sole reason for stunt then it _has_ to be part of
the commit message. Now it looks like an ooopsie where you talk about
NMI and do hardirq check.
> > > > > +static void __rcu_defer_drain(struct rcu_data *rdp)
> > > > > +{
> > > > > + struct llist_node *node, *next;
> > > > > + unsigned long flags;
> > > > > +
> > > > > + if (!IS_ENABLED(CONFIG_RCU_DEFER))
> > > > > + return;
> > > > > +
> > > > > + raw_spin_lock_irqsave(&rdp->defer_lock, flags);
> > > > > + llist_for_each_safe(node, next,
> > > > > llist_del_all(&rdp->defer_head)) {
> > > > > + struct rcu_head *head = (struct rcu_head *)node;
> > > >
> > > > Why do you need the lock. This is still not clear to me despite the
> > > > comment. You can do llist_del_all() towards another list and then feed
> > > > it into rcu_do_enqueue() one by one. And you use the LAZY part.
> > >
> > > Because rcu_barrier() can call this for each cpu and at the same time
> > > rcu_defer_drain() can call it too, so this would cause a race where
> > > the irq work can remove the callbacks (llist_del_all) and before it
> > > can enqueue them, rcu_barrier will see that the list is already empty
> > > and will not wait for these callbacks. We want the llist_del_all() and
> > > rcu_do_enqueue() to happen atomically so rcu_barrier() can work
> > > correctly.
> >
> > So you collect a bunch of callbacks and spent time re-arranging
> > everything with irqs off.
> > What is wrong with keeping it in the llist and consuming it like the
> > regular rcu_segcblist?
>
> I don't understand your last point, all the collection and delayed
> addition into rcu_segcblist is because it is not safe to do it
> directly as I explained above.
That is correct. My point is the more callbacks you add, the more time
you spend to re-add them here. It is not just the one NMI callback
injected from NMI, it is everyone within interrupts disabled region.
After each one you trigger an IRQ. And with LAZY-irq-work or on
architectures which don't have an IRQ-work interrupt, you accumulate
every callback from IRQ disabled region since the last HZ tick.
The question is why not have an additional list which is handled the
same way in terms of grace period like the current one?
> > It does not look like you defer them for long at all. Every callback
> > enqueued in an IRQ-off region will be immediately re-enqueued to the
> > regular list the moment interrupts are enabled again.
> > Except on architectures which don't implement irq-work interrupts where
> > it will be delayed to the next HZ tick. And the next HZ tick does not
> > sound like a long time either.
>
> Yeah, I get your point, and when a CPU goes offline will its LAZY irq
> works be drained or left in the queue? Because we want to make sure
> this irq work is executed before a CPU goes offline.
Hmm. There is smpcfd_dying_cpu() running before rcutree_dying_cpu().
smpcfd_dying_cpu() flushes irq-work near the end. On PREEMPT_RT we have
this thread handling them and this one is parked smpboot_park_threads()
a bit earlier. So here smpcfd_dying_cpu() will wake the parked thread
which then waits until the CPU comes back on.
We can't run the work directly due to the IRQ-off context. We could move
them to another CPU but then the irq-work might expect to be run on the
other CPU…
But we could flush the queue before parking at the very least I suppose…
Sebastian
