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

[Wen Yang <wen.yang@linux.dev>]

On 8/3/26 23:39, Gabriele Monaco wrote:
> On Mon, 2026-08-03 at 02:43 +0800, wen.yang@linux.dev wrote:
>> From: Wen Yang <wen.yang@linux.dev>
>>
>> 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?

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

--
Best wishes,
Wen


> 
>> Fixes: 69d8895cb9a9 ("rv: Add explicit lockdep context for reactors")
>> Signed-off-by: Wen Yang <wen.yang@linux.dev>
>> Cc: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
>> ---
>>   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);
>