Re: [PATCH v4 1/4] rv/reactors: use LD_WAIT_SPIN as the reactor lockdep wait type

[Gabriele Monaco]

On Fri, 2026-08-28 at 02:22 +0800, wen.yang@xxxxxxxxx wrote:
> From: Wen Yang <wen.yang@xxxxxxxxx>
> 
> diff --git a/Documentation/trace/rv/monitor_synthesis.rst
> b/Documentation/trace/rv/monitor_synthesis.rst
> index 2c1b5a0ae154..aab4b0342d5f 100644
> --- a/Documentation/trace/rv/monitor_synthesis.rst
> +++ b/Documentation/trace/rv/monitor_synthesis.rst
> @@ -365,6 +365,26 @@ but higher overhead. The timer wheel (``HA_TIMER_WHEEL``)
> is a good alternative
>  for monitors with several instances (e.g. per-task) that achieves lower
>  overhead with increased latency, yet without compromising precision.
>  
> +Reactors
> +--------
> +
> +A reactor is a callback triggered by a monitor when a violation is
> +detected. Reactors are registered via ``/sys/kernel/tracing/rv/reactors/``
> +and enabled per monitor.

This is not correct, they stay in
/sys/kernel/tracing/rv/monitors/MONITOR/reactors and it is already explained in
Documentation/trace/rv/runtime-verification.rst .

You don't need to add any further section here (you're more than welcome to
update the existing one obviously, but not as part of this patch/series).

monitor_synthesis.rst is not about reactors.
At the moment we don't really have a good spot under Documentation to describe
the /implementation/ of reactors.

I'd say for the purpose of this patch, in-code documentation (i.e. a comment) is
sufficient, we can revise this later if needed.

> +
> +Reactor Locking Rules
> ++++++++++++++++++++++
> +
> +A reactor callback may be invoked from various contexts (process,
> +softirq, hardirq, NMI) depending on the tracepoint to which its
> +monitor is attached.
> +
> +Lockdep uses a fixed wait type: ``LD_WAIT_SPIN``. This allows
> +``raw_spinlock_t`` but disallows sleepable locks. ``LD_WAIT_FREE`` is
> +not viable in preemptible contexts because scheduler preemption takes
> +``rq->__lock`` (``LD_WAIT_SPIN``), which would cause false-positive
> +warnings.
> +

The code looks fine but this documentation is missing the point. We don't need
to explain what lockdep does but why we use a specific class and what we
actually wanted to use. Let's readapt the comment you added in v3

	/*
	 * Reactors must not explicitly take locks, so they should be
	 * LD_WAIT_FREE.  However, reactor callbacks can run with preemption
	 * enabled, meaning the preempting code (e.g. the scheduler taking
	 * rq->__lock at LD_WAIT_SPIN) may violate that constraint.  Use
	 * LD_WAIT_SPIN to avoid false-positive lockdep reports.
         * But you should still NOT be using locks in reactors.
	 */

>  Final remarks
>  -------------
>  
> diff --git a/kernel/trace/rv/rv_reactors.c b/kernel/trace/rv/rv_reactors.c
> index 2f5fc8d18dea..afc97d097109 100644
> --- a/kernel/trace/rv/rv_reactors.c
> +++ b/kernel/trace/rv/rv_reactors.c
> @@ -465,7 +465,11 @@ 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);
> +	/*
> +	 * Use LD_WAIT_SPIN uniformly for deterministic lockdep checking.
> +	 * See Documentation/trace/rv/runtime-verification.rst.
> +	 */

See above, I would only add an explicit comment here of /why/ we do LD_WAIT_SPIN
although we would like LD_WAIT_FREE.

Thanks,
Gabriele

> +	static DEFINE_WAIT_OVERRIDE_MAP(rv_react_map, LD_WAIT_SPIN);
>  	va_list args;
>  
>  	if (!rv_reacting_on() || !monitor->react)