Re: [RFC PATCH 10/13] mm/kwatch: add anchor thread for global watchpoints

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The anchor thread sleeps in TASK_UNINTERRUPTIBLE, artificially inflating the system load average and risking hung task panics.
- [Medium] The thread's polling loop violates voluntary sleeping rules by checking the exit condition before setting the task state, leading to a lost wakeup.
- [Low] The function `kwatch_auto_stop()` is called in the work handler but is never implemented in the patch, leading to a build failure.
--

--- Patch [10]: [RFC PATCH 10/13] mm/kwatch: add anchor thread for global watchpoints ---
commit e6c643e7bd2882c0ca1793a1b13c96d1e132942e
Author: Jinchao Wang <wangjinchao600@gmail.com>

mm/kwatch: add anchor thread for global watchpoints

Global variables have no function whose execution can bound the
watch window. Provide one: a kernel thread sleeps for the configured
duration inside a dedicated noinline function, kwatch_global_anchor(),
and the probe runtime hooks that function like any other target.

> diff --git a/mm/kwatch/anchor.c b/mm/kwatch/anchor.c
> new file mode 100644
> index 0000000000000..11da6aff94133
> --- /dev/null
> +++ b/mm/kwatch/anchor.c
> @@ -0,0 +1,82 @@

[ ... ]

> +static void kwatch_auto_stop_handler(struct work_struct *work)
> +{
> +	kwatch_auto_stop();

[Severity: Low]
Does this cause a build failure? This function appears to be referenced
here but isn't implemented in this patch, which could lead to an undefined
symbol linker error and break bisectability.

> +}
> +
> +static DECLARE_WORK(kwatch_auto_stop_work, kwatch_auto_stop_handler);
> +
> +noinline void kwatch_global_anchor(unsigned long duration_sec)
> +{
> +	wait_event_timeout(kwatch_anchor_wq, kthread_should_stop(),
> +			   duration_sec * HZ);

[Severity: High]
Does this use of wait_event_timeout risk triggering the hung task
watchdog? Because wait_event_timeout implicitly uses TASK_UNINTERRUPTIBLE,
long watch durations could trigger a panic (if the watchdog is configured)
and permanently inflate the system load average by 1.0 during the session.

> +}
> +
> +static int kwatch_anchor_thread_fn(void *data)
> +{
> +	unsigned long duration = (unsigned long)data;
> +
> +	kwatch_global_anchor(duration);
> +
> +	if (!kthread_should_stop()) {
> +		/* mark before scheduling; cleared under the control mutex */
> +		WRITE_ONCE(kwatch_anchor_expired, true);
> +		schedule_work(&kwatch_auto_stop_work);
> +	}
> +
> +	while (!kthread_should_stop())
> +		schedule_timeout_uninterruptible(HZ);

[Severity: High]
Can this loop also artificially inflate the load average? Sleeping
in TASK_UNINTERRUPTIBLE here will add to the system load while waiting
for teardown.

[Severity: Medium]
Could this sequence result in a lost wakeup? The stop condition is checked
before the task state is set. If kthread_stop() signals the thread
after the while condition is evaluated but before
schedule_timeout_uninterruptible() sets the state to TASK_UNINTERRUPTIBLE,
the thread might sleep for an unnecessary second before observing the stop
flag.

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714182243.10687-1-wangjinchao600@gmail.com?part=10