On Tue, Jul 08, 2025 at 03:40:05PM -0400, Mathieu Desnoyers wrote:
> On 2025-07-07 17:56, Paul E. McKenney wrote:
> > On Mon, Jun 23, 2025 at 11:13:03AM -0700, Paul E. McKenney wrote:
> > > On Mon, Jun 23, 2025 at 12:49:41PM +0200, Sebastian Andrzej Siewior wrote:
> > > > On 2025-06-20 04:23:49 [-0700], Paul E. McKenney wrote:
> > > > > > I hope not because it is not any different from
> > > > > >
> > > > > > CPU 2 CPU 3
> > > > > > ===== =====
> > > > > > NMI
> > > > > > rcu_read_lock();
> > > > > > synchronize_rcu();
> > > > > > // need all CPUs report a QS.
> > > > > > rcu_read_unlock();
> > > > > > // no rcu_read_unlock_special() due to in_nmi().
> > > > > >
> > > > > > If the NMI happens while the CPU is in userland (say a perf event)
> > > > > > then
> > > > > > the NMI returns directly to userland.
> > > > > > After the tracing event completes (in this case) the CPU should run
> > > > > > into
> > > > > > another RCU section on its way out via context switch or the tick
> > > > > > interrupt.
> > > > > > I assume the tick interrupt is what makes the NMI case work.
> > > > >
> > > > > Are you promising that interrupts will be always be disabled across
> > > > > the whole rcu_read_lock_notrace() read-side critical section? If so,
> > > > > could we please have a lockdep_assert_irqs_disabled() call to check
> > > > > that?
> > > >
> > > > No, that should stay preemptible because bpf can attach itself to
> > > > tracepoints and this is the root cause of the exercise. Now if you say
> > > > it has to be run with disabled interrupts to match the NMI case then it
> > > > makes sense (since NMIs have interrupts off) but I do not understand why
> > > > it matters here (since the CPU returns to userland without passing the
> > > > kernel).
> > >
> > > Given your patch, if you don't disable interrupts in a preemptible kernel
> > > across your rcu_read_lock_notrace()/rcu_read_unlock_notrace() pair, then a
> > > concurrent expedited grace period might send its IPI in the middle of that
> > > critical section. That IPI handler would set up state so that the next
> > > rcu_preempt_deferred_qs_irqrestore() would report the quiescent state.
> > > Except that without the call to rcu_read_unlock_special(), there might
> > > not be any subsequent call to rcu_preempt_deferred_qs_irqrestore().
> > >
> > > This is even more painful if this is a CONFIG_PREEMPT_RT kernel.
> > > Then if that critical section was preempted and then priority-boosted,
> > > the unboosting also won't happen until the next call to that same
> > > rcu_preempt_deferred_qs_irqrestore() function, which again might not
> > > happen. Or might be significantly delayed.
> > >
> > > Or am I missing some trick that fixes all of this?
> > >
> > > > I'm not sure how much can be done here due to the notrace part. Assuming
> > > > rcu_read_unlock_special() is not doable, would forcing a context switch
> > > > (via setting need-resched and irq_work, as the IRQ-off case) do the
> > > > trick?
> > > > Looking through rcu_preempt_deferred_qs_irqrestore() it does not look to
> > > > be "usable from the scheduler (with rq lock held)" due to RCU-boosting
> > > > or the wake of expedited_wq (which is one of the requirement).
> > >
> > > But if rq_lock is held, then interrupts are disabled, which will
> > > cause the unboosting to be deferred.
> > >
> > > Or are the various deferral mechanisms also unusable in this context?
> >
> > OK, looking back through this thread, it appears that you need both
> > an rcu_read_lock_notrace() and an rcu_read_unlock_notrace() that are
> > covered by Mathieu's list of requirements [1]:
>
> I'm just jumping into this email thread, so I'll try to clarify
> what I may.
Thank you for jumping in!
> > | - NMI-safe
> > This is covered by the existing rcu_read_lock() and
> > rcu_read_unlock().
>
> OK
>
> > | - notrace
> > I am guessing that by "notrace", you mean the "notrace" CPP
> > macro attribute defined in include/linux/compiler_types.h.
> > This has no fewer than four different definitions, so I will
> > need some help understanding what the restrictions are.
>
> When I listed notrace in my desiderata, I had in mind the
> preempt_{disable,enable}_notrace macros, which ensure that
> those macros don't call instrumentation, and therefore can be
> used from the implementation of tracepoint instrumentation or
> from a tracer callback.
OK, that makes sense. I have some questions:
Are interrupts disabled at the calls to rcu_read_unlock_notrace()?
If interrupts are instead enabled, is it OK for an IRQ-work handler that
did an immediate self-interrupt and a bunch of non-notrace work?
If interrupts are to be enabled, but an immediate IRQ-work handler is
ruled out, what mechanism should I be using to defer the work, given
that it cannot be deferred for very long without messing up real-time
latencies? As in a delay of nanoseconds or maybe a microsecond, but
not multiple microseconds let alone milliseconds?
(I could imagine short-timeout hrtimer, but I am not seeing notrace
variants of these guys, either.)
> > | - usable from the scheduler (with rq lock held)
> > This is covered by the existing rcu_read_lock() and
> > rcu_read_unlock().
>
> OK
>
> > | - usable to trace the RCU implementation
> > This one I don't understand. Can I put tracepoints on
> > rcu_read_lock_notrace() and rcu_read_unlock_notrace() or can't I?
> > I was assuming that tracepoints would be forbidden. Until I
> > reached this requirement, that is.
>
> At a high level, a rcu_read_{lock,unlock}_notrace should be something
> that is safe to call from the tracepoint implementation or from a
> tracer callback.
>
> My expectation is that the "normal" (not notrace) RCU APIs would
> be instrumented with tracepoints, and tracepoints and tracer callbacks
> are allowed to use the _notrace RCU APIs.
>
> This provides instrumentation coverage of RCU with the exception of the
> _notrace users, which is pretty much the best we can hope for without
> having a circular dependency.
OK, got it, and that does make sense. I am not sure how I would have
intuited that from the description, but what is life without a challenge?
> > One possible path forward is to ensure that rcu_read_unlock_special()
> > calls only functions that are compatible with the notrace/trace
> > requirements. The ones that look like they might need some help are
> > raise_softirq_irqoff() and irq_work_queue_on(). Note that although
> > rcu_preempt_deferred_qs_irqrestore() would also need help, it is easy to
> > avoid its being invoked, for example, by disabing interrupts across the
> > call to rcu_read_unlock_notrace(). Or by making rcu_read_unlock_notrace()
> > do the disabling.
> >
> > However, I could easily be missing something, especially given my being
> > confused by the juxtaposition of "notrace" and "usable to trace the
> > RCU implementation". These appear to me to be contradicting each other.
> >
> > Help?
>
> You indeed need to ensure that everything that is called from
> rcu_{lock,unlock]_notrace don't end up executing instrumentation
> to prevent a circular dependency. You hinted at a few ways to achieve
> this. Other possible approaches:
>
> - Add a "trace" bool parameter to rcu_read_unlock_special(),
> - Duplicate rcu_read_unlock_special() and introduce a notrace symbol.
OK, both of these are reasonable alternatives for the API, but it will
still be necessary to figure out how to make the notrace-incompatible
work happen.
> - Keep some nesting count in the task struct to prevent calling the
> instrumentation when nested in notrace,
OK, for this one, is the idea to invoke some TBD RCU API the tracing
exits the notrace region? I could see that working. But there would
need to be a guarantee that if the notrace API was invoked, a call to
this TBD RCU API would follow in short order. And I suspect that
preemption (and probably also interrupts) would need to be disabled
across this region.
> There are probably other possible approaches I am missing, each with
> their respective trade offs.
I am pretty sure that we also have some ways to go before we have the
requirements fully laid out, for that matter. ;-)
Could you please tell me where in the current tracing code these
rcu_read_lock_notrace()/rcu_read_unlock_notrace() calls would be placed?
Thanx, Paul
