Re: [PATCH v4 1/4] rv/reactors: use LD_WAIT_SPIN as the reactor lockdep wait type
From: Gabriele Monaco <gmonaco@redhat.com>
Date: 2026-08-31 09:48:52
Also in:
lkml
On Fri, 2026-08-28 at 02:22 +0800, wen.yang@linux.dev wrote:
quoted hunk ↗ jump to hunk
From: Wen Yang <redacted>diff --git a/Documentation/trace/rv/monitor_synthesis.rstb/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.
*/
quoted hunk ↗ jump to hunk
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)