Re: [PATCH v2 1/4] rv/reactors: use context-sensitive lockdep wait type in rv_react()

[Gabriele Monaco]

On Mon, 2026-08-03 at 02:43 +0800, wen.yang@xxxxxxxxx wrote:
> From: Wen Yang <wen.yang@xxxxxxxxx>
> 
> The single DEFINE_WAIT_OVERRIDE_MAP(rv_react_map, LD_WAIT_FREE) in
> rv_react() declares wait_type_inner = LD_WAIT_FREE for every execution
> context.  In a preemptible context (e.g. CONFIG_PREEMPT_RT or a KUnit
> test running on a task), a timer interrupt can fire during a reactor

We are obviously not doing this for KUnit tests, but aren't tracepoint handlers
also running with preemption enabled on non-PREEMPT_RT kernels now?
So technically this is a problem with any configuration if events don't run with
preemption disabled for other reasons.

Or is the issue with spinlocks only popping out on PREEMPT_RT because they
become sleeping locks?
Is lockdep really happy to allow an interrupt/schedule taking spinlocks under
LD_WAIT_FREE on non-PREEMPT_RT?

> callback; the interrupt exit path then schedules and acquires rq->__lock
> (LD_WAIT_SPIN) while the override map is still held.  Since the map
> declares the context to be wait-free, lockdep reports a spurious
> "Invalid wait context" warning:
> 
>     [ BUG: Invalid wait context ]
>     context-{5:5}
>     1 lock held by kunit_try_catch/209:
>      #0: (rv_react_map-wait-type-override){+.+.}-{1:1}
>     kunit_try_catch/209 is trying to lock:
>     ffff8a743ed3e8a0 (&rq->__lock){-...}-{2:2}
> 
> Use two lockdep override maps, selected by execution context:
> 
>   - Preemptible context (task, softirq, PREEMPT_RT irq thread): the
>     scheduler may preempt, so use LD_WAIT_SPIN, the tightest wait type
>     the scheduler itself uses, to suppress the spurious warning.
> 
>   - NMI/hardirq context: preemption is disabled and the scheduler cannot
>     run, so the false positive cannot arise.  Keep LD_WAIT_FREE here to
>     preserve the original constraint that reactors must not take raw
>     spinlocks in atomic context.

So here you're describing at length the solution but not really why you're doing
that. A reader that didn't follow the discussion might think the requirement is
indeed context-dependant, it isn't.

I'd write very bluntly something like:

  "Reactors are not supposed to explicitly take locks, reactor code must comply
with LD_WAIT_FREE. However reactors may run with interrupts and preemption
enabled, so the interrupting code may not satisfy this constraint. Relax it if
we are running from a context that cannot be interrupted to avoid false
positives."

I would write something like that also in the comment, to make clear that
reactors really should be LD_WAIT_FREE, but we are asserting that as best
effort.

What do you think?
Thanks,
Gabriele

> Fixes: 69d8895cb9a9 ("rv: Add explicit lockdep context for reactors")
> Signed-off-by: Wen Yang <wen.yang@xxxxxxxxx>
> Cc: Thomas Weißschuh <thomas.weissschuh@xxxxxxxxxxxxx>
> ---
>  kernel/trace/rv/rv_reactors.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/trace/rv/rv_reactors.c b/kernel/trace/rv/rv_reactors.c
> index 2f5fc8d18dea..cd571b1649f5 100644
> --- a/kernel/trace/rv/rv_reactors.c
> +++ b/kernel/trace/rv/rv_reactors.c
> @@ -465,18 +465,25 @@ int init_rv_reactors(struct dentry *root_dir)
>  
>  void rv_react(struct rv_monitor *monitor, const char *msg, ...)
>  {
> -	static DEFINE_WAIT_OVERRIDE_MAP(rv_react_map, LD_WAIT_FREE);
> +	/*
> +	 * A reactor callback can be preempted; the scheduler then takes
> +	 * rq->__lock (LD_WAIT_SPIN).  Advertise that in preemptible contexts
> +	 * to avoid a spurious lockdep report, and keep LD_WAIT_FREE in
> atomic
> +	 * ones where the scheduler cannot run.
> +	 */
> +	static DEFINE_WAIT_OVERRIDE_MAP(rv_react_map,        LD_WAIT_SPIN);
> +	static DEFINE_WAIT_OVERRIDE_MAP(rv_react_map_atomic, LD_WAIT_FREE);
> +	struct lockdep_map * __maybe_unused map;
>  	va_list args;
>  
>  	if (!rv_reacting_on() || !monitor->react)
>  		return;
>  
> +	map = (in_nmi() || in_hardirq()) ? &rv_react_map_atomic :
> &rv_react_map;
>  	va_start(args, msg);
> -
> -	lock_map_acquire_try(&rv_react_map);
> +	lock_map_acquire_try(map);
>  	monitor->react(msg, args);
> -	lock_map_release(&rv_react_map);
> -
> +	lock_map_release(map);
>  	va_end(args);
>  }
>  EXPORT_SYMBOL_GPL(rv_react);