Re: (sashiko review) [RFC PATCH v4 01/10] mm/damon/core: introduce damon_ctx->paused

SeongJae Park
2026-03-22T17:40:25Z

On Sun, 22 Mar 2026 10:06:59 -0700 SeongJae Park <sj@kernel.org> wrote:

> Forwarding Sashiko review for doing discussions via mails.
>
> # review url: https://sashiko.dev/#/patchset/20260322155728.81434-2-sj@kernel.org
> # start of sashiko.dev inline review
> commit 2a3be2c8b6c327ef72163cd54e6121ef72b9c932
> Author: SeongJae Park <sj@kernel.org>
>
> mm/damon/core: introduce damon_ctx->paused
>
> DAMON supports only start and stop of the execution. When it is
> stopped, its internal data that it self-trained goes away. It will be
> useful if the execution can be paused and resumed with the previous
> self-trained data.
>
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index db6c67e52d2b..0ab2cfa848e6 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
>
> [ ... ]
>
> > @@ -3003,6 +3004,14 @@ static int kdamond_fn(void *data)
> > kdamond_call(ctx, false);
> > if (ctx->maybe_corrupted)
> > break;
> > + while (ctx->pause) {
> > + damos_walk_cancel(ctx);
>
> Is it safe to repeatedly call damos_walk_cancel() here?
>
> In damos_walk(), the walk_control_lock is dropped after setting
> ctx->walk_control to check damon_is_running(), which opens a race window.
>
> Concurrently, kdamond_fn() calls damos_walk_cancel(), which reads
> ctx->walk_control, drops the lock, and completes the old request.
>
> If a new caller invokes damos_walk() and successfully registers a new
> walk_control, could damos_walk_cancel() resume, reacquire the lock,
> and unconditionally set ctx->walk_control = NULL, dropping the new
> request?
>
> This might leave the new caller permanently deadlocked in
> wait_for_completion().

Nice catch. Orthogonal to this patch, though.

The deadlock could happen in below sequence.

CPU0 │CPU1
──────────────────────────────┼────────────────────────
damos_walk() │
│register request │
│wait completion │damos_walk_cancel()
│ │ │complete the request
▼wakeup,return │ │
damos_walk() │ │
│register new request │ │
│ │ │remove the new request
│wait completion │ ▼return
▼ nobody completes it. │

Nonetheless, kdamond_fn() is calling damos_walk() already in several places
including this loop. This issue hence exists regardless of this patch. I will
work on fixing this as a separate hotfix. Below fix may work.

'''
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2321,7 +2321,9 @@ static void damos_walk_cancel(struct damon_ctx *ctx)
control->canceled = true;
complete(&control->completion);
mutex_lock(&ctx->walk_control_lock);
- ctx->walk_control = NULL;
+ /* A new damos_walk() caller could added a new request meanwhile */
+ if (ctx->walk_control == control)
+ ctx->walk_control = NULL;
mutex_unlock(&ctx->walk_control_lock);
}
'''

>
> > + kdamond_usleep(ctx->attrs.sample_interval);
>
> Could this result in a busy loop if sample_interval is 0?
>
> The DAMON API allows attrs.sample_interval to be 0 (for example, when
> tuning is disabled via goal.aggrs == 0).
>
> If sample_interval is 0, this invokes usleep_range_idle(0, 1), which
> sleeps for at most 1 microsecond using an hrtimer.
>
> Since the normally expensive monitoring operations are bypassed while
> paused, would this cause the thread to loop almost instantly and create
> an interrupt storm?

This is an intended behavior for users who really impatient. So this should be
fine.

Thanks,
SJ …