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

On 8/3/26 23:39, Gabriele Monaco wrote:

On Mon, 2026-08-03 at 02:43 +0800, wen.yang@linux.dev wrote:

quoted

From: Wen Yang <redacted>

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?

quoted

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?

Good point, thank you.
We've made the changes in v3 as you suggested.

--
Best wishes,
Wen

quoted

Fixes: 69d8895cb9a9 ("rv: Add explicit lockdep context for reactors")
Signed-off-by: Wen Yang <redacted>
Cc: Thomas Weißschuh <redacted>
---
  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);