Linux-mm Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mm/damon/core: reject empty access pattern intervals on scheme commit
@ 2026-11-03 10:54 Xiaolei Wang (Kaihong)
  2026-11-03 11:29 ` sashiko-bot
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Xiaolei Wang (Kaihong) @ 2026-11-03 10:54 UTC (permalink / raw)
  To: sj, akpm, david, ljs, error27, xiaoleiwangxiaolei
  Cc: damon, linux-mm, linux-kernel, stable, Liam R. Howlett,
	Michal Hocko, Mike Rapoport, Suren Baghdasaryan, Vlastimil Babka

DAMOS schemes accept access pattern bounds with min > max (e.g.
min_sz_region > max_sz_region).  No input path validates the bounds:
damon_new_scheme() stores the pattern as is, and the sysfs ul_range
stores are plain kstrtoul().

At runtime __damos_valid_target() evaluates "min <= x <= max" for each
of the three pattern dimensions.  A reversed interval is an empty set,
so every region fails the check and the scheme silently never matches
anything: nr_tried stays zero forever while the kdamond keeps running
normally.

With such a reversed interval, damo starts normally (rc=0, kdamond
state=on), but nr_tried and nr_applied stay zero forever and the target
RSS does not drop.

The same holds for the watermarks thresholds, which must be monotonic
(low <= mid <= high).

Validate the access pattern and watermarks at the beginning of
damos_commit(), through which both new schemes (damo start) and online
updates (damo tune) pass.

Fixes: 1f366e421c8f ("mm/damon/core: implement DAMON-based Operation Schemes (DAMOS)")
Cc: stable@vger.kernel.org
Cc: Liam R. Howlett <liam@infradead.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Mike Rapoport <rppt@kernel.org>
Cc: Suren Baghdasaryan <surenb@google.com>
Cc: Vlastimil Babka <vbabka@kernel.org>
Signed-off-by: Xiaolei Wang (Kaihong) <xiaoleiwangxiaolei@gmail.com>
---
 mm/damon/core.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index eb1fcdabc007..865627bda240 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -1566,10 +1566,29 @@ static struct damos *damon_nth_scheme(int n, struct damon_ctx *ctx)
 	return NULL;
 }
 
+static int damos_validate_scheme(struct damos *s)
+{
+	struct damos_access_pattern *p = &s->pattern;
+	struct damos_watermarks *wmarks = &s->wmarks;
+
+	if (p->min_sz_region > p->max_sz_region ||
+	    p->min_nr_accesses > p->max_nr_accesses ||
+	    p->min_age_region > p->max_age_region)
+		return -EINVAL;
+	if (wmarks->metric != DAMOS_WMARK_NONE &&
+	    (wmarks->low > wmarks->mid || wmarks->mid > wmarks->high))
+		return -EINVAL;
+	return 0;
+}
+
 static int damos_commit(struct damos *dst, struct damos *src)
 {
 	int err;
 
+	err = damos_validate_scheme(src);
+	if (err)
+		return err;
+
 	dst->pattern = src->pattern;
 	dst->action = src->action;
 	dst->apply_interval_us = src->apply_interval_us;
-- 
2.53.0



^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] mm/damon/core: reject empty access pattern intervals on scheme commit
  2026-11-03 10:54 [PATCH] mm/damon/core: reject empty access pattern intervals on scheme commit Xiaolei Wang (Kaihong)
@ 2026-11-03 11:29 ` sashiko-bot
  2026-11-03 11:44 ` Aveline Noir
  2026-11-03 14:53 ` SJ Park
  2 siblings, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-11-03 11:29 UTC (permalink / raw)
  To: Xiaolei Wang (Kaihong), sj, akpm, david, ljs, error27
  Cc: damon, linux-mm, Liam R. Howlett, Michal Hocko, Mike Rapoport,
	Suren Baghdasaryan, Vlastimil Babka

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mm/damon: damos_validate_scheme() breaks DAMON sysfs.py selftest
--

commit f1effb4d8401c0508f7cb04b14b9b55d0d6abd17
Author: Xiaolei Wang (Kaihong) <xiaoleiwangxiaolei@gmail.com>

mm/damon/core: reject empty access pattern intervals on scheme commit

This commit introduces damos_validate_scheme() and calls it at the start of
damos_commit() to reject DAMOS schemes with reversed access pattern bounds
(min > max) or non-monotonic watermark thresholds with -EINVAL.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index d36823f4d0928..9b2b2a63a2576 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -1561,10 +1561,29 @@ static struct damos *damon_nth_scheme(int n, struct damon_ctx *ctx)
>  	return NULL;
>  }
>  
> +static int damos_validate_scheme(struct damos *s)
> +{
> +	struct damos_access_pattern *p = &s->pattern;
> +	struct damos_watermarks *wmarks = &s->wmarks;
> +
> +	if (p->min_sz_region > p->max_sz_region ||
> +	    p->min_nr_accesses > p->max_nr_accesses ||
> +	    p->min_age_region > p->max_age_region)
> +		return -EINVAL;

[Severity: Medium]
Does this validation break the existing DAMON sysfs kselftest in
tools/testing/selftests/damon/sysfs.py?

In tools/testing/selftests/damon/sysfs.py, main() constructs a non-default
scheme for testing online parameter commitment with min_sz_region = 4096 and
max_sz_region = 1024:

tools/testing/selftests/damon/sysfs.py:main()
            schemes=[_damon_sysfs.Damos(
                action='pageout',
                access_pattern=_damon_sysfs.DamosAccessPattern(
                    size=[4096, 2**10],
                    nr_accesses=[3, 317],
                    age=[5,71]),

When main() subsequently calls kdamonds.kdamonds[0].commit() and
assert_ctxs_committed(kdamonds), writing commit to the kdamond state sysfs
file invokes:

state_store()
  -> damon_sysfs_handle_cmd()
    -> damon_sysfs_commit_input()
      -> damon_commit_ctx()
        -> __damon_commit_ctx()
          -> damon_commit_schemes()
            -> damos_commit()
              -> damos_validate_scheme()

Because p->min_sz_region (4096) > p->max_sz_region (1024),
damos_validate_scheme() returns -EINVAL, causing kd.commit() in
assert_ctxs_committed() to fail and exit with status 1:

tools/testing/selftests/damon/sysfs.py:assert_ctxs_committed()
        if kd in kdamonds_paused_for_dump:
            err = kd.commit()
            if err is not None:
                print('pause fail (%s)' % err)
                kdamonds.stop()
                exit(1)

Should tools/testing/selftests/damon/sysfs.py also be updated to use a
non-empty size range (for example, size=[4096, 2**20]) so the selftest
continues to pass?

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] mm/damon/core: reject empty access pattern intervals on scheme commit
  2026-11-03 10:54 [PATCH] mm/damon/core: reject empty access pattern intervals on scheme commit Xiaolei Wang (Kaihong)
  2026-11-03 11:29 ` sashiko-bot
@ 2026-11-03 11:44 ` Aveline Noir
  2026-11-03 14:53 ` SJ Park
  2 siblings, 0 replies; 4+ messages in thread
From: Aveline Noir @ 2026-11-03 11:44 UTC (permalink / raw)
  To: Xiaolei Wang (Kaihong)
  Cc: akpm, damon, david, error27, Liam R. Howlett, linux-kernel,
	linux-mm, ljs, Michal Hocko, Mike Rapoport, sashiko-bot,
	sashiko-reviews, sj, stable, Suren Baghdasaryan, Vlastimil Babka

Hi Xiaolei,

On Tue, 03 Nov 2026 18:54:18 +0800 Xiaolei Wang (Kaihong) wrote:

> At runtime __damos_valid_target() evaluates "min <= x <= max" for each
> of the three pattern dimensions.  A reversed interval is an empty set,
> so every region fails the check and the scheme silently never matches
> anything: nr_tried stays zero forever while the kdamond keeps running
> normally.

My understanding is that DAMON just does what the user asked for here,
so this looks more like an input validation improvement than a bug fix.
Since previously accepted input would now fail with -EINVAL, maybe drop
the Fixes: and Cc: stable tags, and mention the behavior change in the
changelog?

> The same holds for the watermarks thresholds, which must be monotonic
> (low <= mid <= high).

Not for all cases, I think.  With mid > high (and low <= high), the
scheme still works; mid just has no effect.  The subject would also
need to mention watermarks.

> +	if (p->min_sz_region > p->max_sz_region ||
> +	    p->min_nr_accesses > p->max_nr_accesses ||
> +	    p->min_age_region > p->max_age_region)
> +		return -EINVAL;

As Sashiko pointed out, this would break the DAMON sysfs.py selftest,
which uses size=[4096, 2**10].  Maybe update it together, and also add
a kunit case and a note in the DAMON usage document?

Thanks,
Aveline Noir


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] mm/damon/core: reject empty access pattern intervals on scheme commit
  2026-11-03 10:54 [PATCH] mm/damon/core: reject empty access pattern intervals on scheme commit Xiaolei Wang (Kaihong)
  2026-11-03 11:29 ` sashiko-bot
  2026-11-03 11:44 ` Aveline Noir
@ 2026-11-03 14:53 ` SJ Park
  2 siblings, 0 replies; 4+ messages in thread
From: SJ Park @ 2026-11-03 14:53 UTC (permalink / raw)
  To: Xiaolei Wang (Kaihong)
  Cc: SJ Park, akpm, david, ljs, error27, damon, linux-mm, linux-kernel,
	stable, Liam R. Howlett, Michal Hocko, Mike Rapoport,
	Suren Baghdasaryan, Vlastimil Babka

On Tue,  3 Nov 2026 18:54:18 +0800 "Xiaolei Wang (Kaihong)" <xiaoleiwangxiaolei@gmail.com> wrote:

> DAMOS schemes accept access pattern bounds with min > max (e.g.
> min_sz_region > max_sz_region).  No input path validates the bounds:
> damon_new_scheme() stores the pattern as is, and the sysfs ul_range
> stores are plain kstrtoul().
> 
> At runtime __damos_valid_target() evaluates "min <= x <= max" for each
> of the three pattern dimensions.  A reversed interval is an empty set,
> so every region fails the check and the scheme silently never matches
> anything: nr_tried stays zero forever while the kdamond keeps running
> normally.
> 
> With such a reversed interval, damo starts normally (rc=0, kdamond
> state=on), but nr_tried and nr_applied stay zero forever and the target
> RSS does not drop.
> 
> The same holds for the watermarks thresholds, which must be monotonic
> (low <= mid <= high).
> 
> Validate the access pattern and watermarks at the beginning of
> damos_commit(), through which both new schemes (damo start) and online
> updates (damo tune) pass.

The documentation is not explicitly mentioning about the inputs (min > max and
non-monotonic watermarks).  But I was thinking it is not really difficult to
infer this behavior based on the existing document.

If my understanding of the documentation is not wrong, I find no problem at
accepting the inputs and just behave as documented.  It might look silly, but
who knows the users have a creative reason to do that?  As long as it is not
causing a harm, just supporting it is better for both users and code
maintainability, in my humble opinion.

Pleae let me know if I am missing something.


Thanks,
SJ

[...]


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-11-03 14:53 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-11-03 10:54 [PATCH] mm/damon/core: reject empty access pattern intervals on scheme commit Xiaolei Wang (Kaihong)
2026-11-03 11:29 ` sashiko-bot
2026-11-03 11:44 ` Aveline Noir
2026-11-03 14:53 ` SJ Park

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox