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

[Gabriele Monaco <gmonaco@redhat.com>]

On Thu, 2026-08-20 at 01:28 +0800, Wen Yang wrote:
> So I don't think LD_WAIT_SPIN for everything is safe, it's not just 
> "less strict on paper".

Yes it won't be safe, we could have wrong reactors not reported by lockdep, but
so would your conditional check. We'd be catching everything in interrupts, but
if a model never runs in interrupt context, we'd be demoted to LD_WAIT_SPIN
nonetheless.

Your condition is kind of a best-effort to do a bit better than just
LD_WAIT_SPIN. Again, I'm not completely against it, but it isn't a final
solution (not that I can think of a better one) and if most people find it
confusing, we're probably better off without it.

Or am I missing something?

> If a reactor ever does raw_spin_lock() by mistake while running from 
> nrp's path, check_wait_context() takes the wait type straight from the 
> override map:
> 
>      if (unlikely(class->lock_type == LD_LOCK_WAIT_OVERRIDE))
>              curr_inner = prev_inner;        /* SPIN */
>      if (next_outer > curr_inner)
>              return print_lock_invalid_wait_context(...);
> 
> SPIN nested in SPIN is 2 > 2, which is false, so it just passes.
> We'd be silently giving up the one case (hardirq/NMI) where the "no 
> locks" rule for reactors actually matters, which is the same thing that 
> bit you with the signal reactor.

True, that is one, but it isn't the only case where using locks can be an issue,
namely sched_switch is particularly hard to please (interrupts are disabled for
a reason).

> And for what it's worth, checking context to decide the wait type isn't 
> something we'd be introducing -- lockdep does the same thing to get the 
> baseline before any override is applied, in the exact function that 
> produced this splat:
> 
>      static inline short task_wait_context(struct task_struct *curr)
>      {
>              if (lockdep_hardirq_context()) {
>                      if (curr->hardirq_threaded || curr->irq_config)
>                              return LD_WAIT_CONFIG;
>                      return LD_WAIT_SPIN;
>              } else if (curr->softirq_context) {
>                      return LD_WAIT_CONFIG;
>              }
>              return LD_WAIT_MAX;
>      }
> 
> That's four cases. Our in_nmi() || in_hardirq() is a simplification of
> what's already there, not a new habit.

I'm not familiar with the lockdep code, so I may be missing something here, but
isn't this function getting the current context? It isn't used to tune the
lockdep logic for some specific function, it /is/ the lockdep logic itself.

Is this really comparable?

> Thomas, on disabling preemption instead: it does fix the pagefault
> case, and it's a no-op for nrp since preempt_count is already elevated 
> there. But it changes what a reactor is allowed to do, for every 
> reactor, not just the two above.cspin_lock() on RT checks 
> might_resched() before it even looks at thevlock:
> 
>      static __always_inline void __rt_spin_lock(spinlock_t *lock)
>      {
>              rtlock_might_resched();   /* unconditional */
>              rtlock_lock(&lock->lock);
>      }
> 
> so any future reactor using a plain spinlock would hit that every single 
> time, lock contended or not, on top of whatever it's already called for. 
> That seems worse than the thing we're trying to fix.

I'm also against disabling preemption, we may even be able to avoid scheduling
by also disabling interrupts in a convenient order, but in principle I'd avoid
that like plague. RV was born as a tool for real-time and disabling preemption
only to get better lockdep reporting doesn't look right to me.

Sure we could do it only with lockdep and assume lockdep's overhead is already
enough, but we're again complicating things.

> Since neither struct rv_reactor nor rv_react() actually documents what
> context a callback may run in, maybe that's worth spelling out
> separately regardless of what we do here -- something close to what
> printk already does for the same reason (reactor_printk's
> vprintk_deferred() leans on this internally: is_printk_legacy_deferred()
> checks in_nmi() to decide whether it's safe to take console_lock or
> whether it has to go through the lock-free irq_work path instead).

You raise a fair point though, we should document all this somewhere. I believe
this is the best thing we can do at this point.
The peculiarity of reactors and RV in general is that we run from tracepoints,
so we could run virtually from anywhere, that's why there's no one size fits
all..

Overall I'm now leaning towards using only LD_WAIT_SPIN and carefully document
why we had to do this.

What do you all think?

Thanks,
Gabriele