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

SeongJae Park
2026-03-22T20:51:39Z

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

> 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.

TL; DR: there is no deadlock in existing code. I will work on more clean code
or documentation, though.

The scenario that I illustrated above cannot happen, because the second
damos_walk() cannot register its new request before the old request is unset.

The request is unset in three places. damos_walk_complete(),
damos_walk_cancel(), and damos_walk(). damos_walk_complete() and
damos_walk_cancel() are called from same kdamond thread, so no race between
them exists.

damos_walk() unsets the request, only if !damon_is_running(). damos_walk()
seeing !damon_is_running() means the kdamond is stopped. It again means there
can be no concurrent damos_walk_cancel() or damos_walk_complete() that works
for same context and started before the damon_is_running() call.

Unless the same context is restarted, hence, there is no chance to race. Only
DAMON_SYSFS calls damos_walk() and it doesn't restart same context.
DAMON_RECLAIM and DAMON_LRU_SORT do restart same context, but they don't use
damos_walk(). So, there is no deadlock in the existing code (or, no such
deadlock is found so far).

Let's assume there could be damos_walk() call with parallel restart of a DAMON
context, though. In the case, below deadlock is available. Seems this is what
Sashiko was trying to say.

0. A DAMON context is stopped.
1-1. CPU0: calls damos_walk() for the stopped context.
1-2. CPU0: damos_walk(): register a new damos_walk() request to the stopped
context.
1-3. CPU0: damos_walk(): shows !damon_is_running().
2. CPU1: Re-start the DAMON context.
3-1. CPU2: Execute kdamond_fn() -> damos_walk_cancel()
3-2. CPU2: damos_walk_cancel(): complete the walk request that registered on
step 1-2.
4-1. CPU0: damos_walk(): unset the request.
4-2: CPU0: calls damos_walk() again.
4-3: CPU0: damos_walk() 2: register a new damos_walk() request.
4-4: CPU0: damos_walk() 2: wait for the completion.
5-1. CPU2: damos_walk_cancel(): unset the walk request that registered on step
4-3.

Nobody can complete the request that registered on step 4-3. CPU0 infinitely
wait.

In more graphiscal way, this can be illustrated as below:

CPU0 │CPU1 │CPU2
───────────────────────────────┼─────────────────┼────────────────────────────────────────
damos_walk() │ │
│register reqeust │ │
│show !damon_is_running(ctx)│ │
│ │ │
│ │damon_start(ctx) │
│ │ │damos_walk_cancel()
│ │ │ complete first damos_walk() request
│ │ │
│unset request │ │
▼return │ │
│ │
damos_walk() │ │
│register request │ │
│wait completion │ │ unset second request
▼ │ │

As I mentioned abovely, this cannot happen on existing code, since there is no
code that restarts a terminated DAMON context, and calls damos_walk(). In the
future, there might be such use cases or mistakenly made call sequence, though.

I will work on improving this. But, as I mentioned before, it is not a blocker
for this patch.

Thanks,
SJ …