Re: (sashiko review) [PATCH] mm/damon/core: disallow non-power of two min_region_sz on damon_start()

[SeongJae Park <sj@kernel.org>]

TL; DR: sashiko found no blocker of this patch.

Forwarding full sashiko review in a reply format with my 
inline comments below, for sharing details of my view and
doing followup discussions via mails if needed.

> # review url: https://sashiko.dev/#/patchset/20260411213638.77768-1-sj@kernel.org
> # start of sashiko.dev inline review
> commit a125056db470cab021e84ad481a0f51ddfc426c2
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/core: disallow non-power of two min_region_sz on damon_start()
> 
> This commit fixes an issue that allows unaligned DAMON region address ranges.
> It adds a check for power-of-two min_region_sz directly in damon_start()
> to catch invalid configurations from the sysfs interface.
> 
> Link: https://lore.kernel.org/20260403155530.64647-1-sj@kernel.org
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 3703f62a876b3..c107d74c77e74 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -1368,6 +1368,11 @@ int damon_start(struct damon_ctx **ctxs, int nr_ctxs, bool exclusive)
> >  	int i;
> >  	int err = 0;
> >  
> > +	for (i = 0; i < nr_ctxs; i++) {
> > +		if (!is_power_of_2(ctxs[i]->min_region_sz))
> > +			return -EINVAL;
> > +	}
> > +
> 
> This isn't a regression introduced by this commit, but modifying
> damon_start() brings up a question about partial failure handling when
> starting multiple contexts.
> 
> If __damon_start() succeeds for the first context but fails for a subsequent
> one (for example if kthread_run() fails), the initialization loop breaks and
> returns an error:
> 
> 	for (i = 0; i < nr_ctxs; i++) {
> 		err = __damon_start(ctxs[i]);
> 		if (err)
> 			break;
> 		nr_running_ctxs++;
> 	}
> 
> Does this leave the previously successful kdamond threads running without
> stopping them?
> 
> Because nr_running_ctxs is incremented for the successful starts and
> running_exclusive_ctxs is set to true, it appears the internal state
> would prevent starting new contexts.
> 
> Would this result in all future calls to damon_start() returning -EBUSY?

Yes, this is not ideal.  But as Sashiko also says it is no blocker of this
patch.  I will revisit this later.

> 
> 
> # end of sashiko.dev inline review
> # review url: https://sashiko.dev/#/patchset/20260411213638.77768-1-sj@kernel.org


Thanks,
SJ

# hkml [1] generated a draft of this mail.  You can regenerate
# this using below command:
#
#     hkml patch sashiko_dev --for_forwarding \
#             20260411213638.77768-1-sj@kernel.org
#
# [1] https://github.com/sjp38/hackermail