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

On Mon, 2026-08-03 at 02:43 +0800, [email protected] wrote:
> From: Wen Yang <[email protected]>
>=20
> The single DEFINE_WAIT_OVERRIDE_MAP(rv_react_map, LD_WAIT_FREE) in
> rv_react() declares wait_type_inner =3D LD_WAIT_FREE for every execution
> context.=C2=A0 In a preemptible context (e.g. CONFIG_PREEMPT_RT or a KUni=
t
> 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 hand=
lers
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 und=
er
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.=C2=A0 Since the map
> declares the context to be wait-free, lockdep reports a spurious
> "Invalid wait context" warning:
>=20
> =C2=A0=C2=A0=C2=A0 [ BUG: Invalid wait context ]
> =C2=A0=C2=A0=C2=A0 context-{5:5}
> =C2=A0=C2=A0=C2=A0 1 lock held by kunit_try_catch/209:
> =C2=A0=C2=A0=C2=A0=C2=A0 #0: (rv_react_map-wait-type-override){+.+.}-{1:1=
}
> =C2=A0=C2=A0=C2=A0 kunit_try_catch/209 is trying to lock:
> =C2=A0=C2=A0=C2=A0 ffff8a743ed3e8a0 (&rq->__lock){-...}-{2:2}
>=20
> Use two lockdep override maps, selected by execution context:
>=20
> =C2=A0 - Preemptible context (task, softirq, PREEMPT_RT irq thread): the
> =C2=A0=C2=A0=C2=A0 scheduler may preempt, so use LD_WAIT_SPIN, the tighte=
st wait type
> =C2=A0=C2=A0=C2=A0 the scheduler itself uses, to suppress the spurious wa=
rning.
>=20
> =C2=A0 - NMI/hardirq context: preemption is disabled and the scheduler ca=
nnot
> =C2=A0=C2=A0=C2=A0 run, so the false positive cannot arise.=C2=A0 Keep LD=
_WAIT_FREE here to
> =C2=A0=C2=A0=C2=A0 preserve the original constraint that reactors must no=
t take raw
> =C2=A0=C2=A0=C2=A0 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 requiremen=
t 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 co=
mply
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 <[email protected]>
> Cc: Thomas Wei=C3=9Fschuh <[email protected]>
> ---
> =C2=A0kernel/trace/rv/rv_reactors.c | 17 ++++++++++++-----
> =C2=A01 file changed, 12 insertions(+), 5 deletions(-)
>=20
> 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)
> =C2=A0
> =C2=A0void rv_react(struct rv_monitor *monitor, const char *msg, ...)
> =C2=A0{
> -=09static DEFINE_WAIT_OVERRIDE_MAP(rv_react_map, LD_WAIT_FREE);
> +=09/*
> +=09 * A reactor callback can be preempted; the scheduler then takes
> +=09 * rq->__lock (LD_WAIT_SPIN).=C2=A0 Advertise that in preemptible con=
texts
> +=09 * to avoid a spurious lockdep report, and keep LD_WAIT_FREE in
> atomic
> +=09 * ones where the scheduler cannot run.
> +=09 */
> +=09static DEFINE_WAIT_OVERRIDE_MAP(rv_react_map,=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 LD_WAIT_SPIN);
> +=09static DEFINE_WAIT_OVERRIDE_MAP(rv_react_map_atomic, LD_WAIT_FREE);
> +=09struct lockdep_map * __maybe_unused map;
> =C2=A0=09va_list args;
> =C2=A0
> =C2=A0=09if (!rv_reacting_on() || !monitor->react)
> =C2=A0=09=09return;
> =C2=A0
> +=09map =3D (in_nmi() || in_hardirq()) ? &rv_react_map_atomic :
> &rv_react_map;
> =C2=A0=09va_start(args, msg);
> -
> -=09lock_map_acquire_try(&rv_react_map);
> +=09lock_map_acquire_try(map);
> =C2=A0=09monitor->react(msg, args);
> -=09lock_map_release(&rv_react_map);
> -
> +=09lock_map_release(map);
> =C2=A0=09va_end(args);
> =C2=A0}
> =C2=A0EXPORT_SYMBOL_GPL(rv_react);