[RFC PATCH 0/6] mm/damon: misc cleanup/improvements for core, vaddr, sysfs and tests

[RFC PATCH 0/6] mm/damon: misc cleanup/improvements for core, vaddr, sysfs and tests

[SJ Park]

Refactor code, optimize code speed, and add a missing feature support on
various areas of DAMON.

Consolidate invalid DAMOS quota charge pointers fixes into single place
in core with patch 1.  Optimize the efficiency of vaddr damos_migrate
action under filter failures with patch 2.  Implement sz_filter_passed
feature for damos_migrate action in vaddr, with patch 3.  Drop a
redundant and racy quota goal target value validation in sysfs, in favor
of that in the core layer, with patch 4.  Drop unused variables and
imports from selftests with patch 5 and 6.

SJ Park (6):
  mm/damon/core: handle invalid target in
    damos_fix_stale_charge_pointers()
  mm/damon/vaddr: advance migration attempt in folio level for filter
    out
  mm/damon/vaddr: implement sz_filter_passed for migrate_{hot,cold}
    action
  mm/damon/sysfs-schemes: drop redundant and racy zero target_value
    check
  selftests/damon/damos_apply_interval: remove unused variables
  selftests/damon: remove unused imports of os and sys modules

 mm/damon/core.c                                   | 15 ++++++---------
 mm/damon/sysfs-schemes.c                          |  3 ---
 mm/damon/vaddr.c                                  |  8 ++++++--
 .../selftests/damon/damos_apply_interval.py       |  2 --
 .../selftests/damon/sysfs_no_op_commit_break.py   |  1 -
 .../sysfs_update_schemes_tried_regions_hang.py    |  1 -
 6 files changed, 12 insertions(+), 18 deletions(-)


base-commit: de796f0221d9cfc6a468e83934b9514c43b0a4b3
-- 
2.47.3

[RFC PATCH 1/6] mm/damon/core: handle invalid target in damos_fix_stale_charge_pointers()

[SJ Park]

The fixup of DAMOS quota charge pointers
(damos->charge_{target,addr}_from) for removed targets and regions is
done in damos_fix_stale_charge_pointers().  A similar fixup for invalid
(terminated pid) targets is done in kdamond_apply_schemes().  Having
similar fixups in different places only make it confusing to read the
code.  Consolidate the fixups into damos_fix_stale_charge_pointers().

Signed-off-by: SJ Park <sj@kernel.org>
---
 mm/damon/core.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 73b571b429cb3..ce75db510a9d3 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -3394,7 +3394,11 @@ static void damos_fix_stale_charge_pointers(struct damon_ctx *c,
 		return;
 	damon_for_each_target(t, c) {
 		if (t == quota->charge_target_from) {
-			if (damon_nr_regions(t) > 0)
+			bool valid_target = true;
+
+			if (c->ops.target_valid && !c->ops.target_valid(t))
+				valid_target = false;
+			if (valid_target && damon_nr_regions(t) > 0)
 				return;
 			break;
 		}
@@ -3513,15 +3517,8 @@ static noinline_for_stack void kdamond_apply_schemes(struct damon_ctx *c)
 	max_region_sz = damon_region_sz_limit(c);
 	mutex_lock(&c->walk_control_lock);
 	damon_for_each_target(t, c) {
-		if (c->ops.target_valid && c->ops.target_valid(t) == false) {
-			damon_for_each_scheme(s, c) {
-				if (s->quota.charge_target_from != t)
-					continue;
-				s->quota.charge_target_from = NULL;
-				s->quota.charge_addr_from = 0;
-			}
+		if (c->ops.target_valid && c->ops.target_valid(t) == false)
 			continue;
-		}
 		damos_apply_target(c, t, max_region_sz);
 	}
 
-- 
2.47.3

[RFC PATCH 2/6] mm/damon/vaddr: advance migration attempt in folio level for filter out

[SJ Park]

damos_va_migrate_pmd_entry() finds folios to migrate under the given
pmd, using a loop for the pmd address range.  The loop advances in page
size if a given folio is excluded by DAMOS ops filters.  DAMOS ops
filters work in folio granularity.  Hence it is more efficient and makes
sense to advance in the folio size.

Advance the loop in folio size for the ops filter out decision.  For
that, do the folio size based loop advancement offset update before
applying DAMOS ops filters.

Closes: https://sashiko.dev/#/bug/linux-67c34df7-6dfb-4e18-9ce0-0d26915e5785
Signed-off-by: SJ Park <sj@kernel.org>
---
 mm/damon/vaddr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
index b549496ea8e2c..c271b6d63c027 100644
--- a/mm/damon/vaddr.c
+++ b/mm/damon/vaddr.c
@@ -881,12 +881,12 @@ static int damos_va_migrate_pmd_entry(pmd_t *pmd, unsigned long addr,
 		folio = vm_normal_folio(walk->vma, addr, ptent);
 		if (!folio)
 			continue;
+		page_idx = folio_page_idx(folio, pte_page(ptent));
+		nr = folio_nr_pages(folio) - page_idx;
 		if (damos_va_filter_out(s, folio, walk->vma, addr, pte, NULL))
 			continue;
 		damos_va_migrate_dests_add(folio, walk->vma, addr, dests,
 				migration_lists);
-		page_idx = folio_page_idx(folio, pte_page(ptent));
-		nr = folio_nr_pages(folio) - page_idx;
 	}
 	pte_unmap_unlock(start_pte, ptl);
 	return 0;
-- 
2.47.3

[RFC PATCH 3/6] mm/damon/vaddr: implement sz_filter_passed for migrate_{hot,cold} action

[SJ Park]

Support of DAMOS ops filter and sz_filter_passed support is up to each
operation set.  DAMON_VADDR supports both for only damos_stat action.
It intentionally does not support those for madvise-based actions
because applying filters could make it unnecessarily complicated and
expensive.  The vaddr implementation of migrate_{hot,cold} action does
support DAMOS ops filters but sz_filter_passed.  There is no real reason
to not support sz_filter_passed.  Add the support.

Pass the sz_filter_passed pointer to the filter applying logic and
accumulate the number.

Closes: https://sashiko.dev/#/bug/linux-986e8b64-8e65-4e32-970f-4d3ebdf8206e
Signed-off-by: SJ Park <sj@kernel.org>
---
 mm/damon/vaddr.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
index c271b6d63c027..112095c2d53fd 100644
--- a/mm/damon/vaddr.c
+++ b/mm/damon/vaddr.c
@@ -776,6 +776,7 @@ static bool damos_va_filter_out(struct damos *scheme, struct folio *folio,
 struct damos_va_migrate_private {
 	struct list_head *migration_lists;
 	struct damos *scheme;
+	unsigned long *sz_filter_passed;
 };
 
 /*
@@ -840,6 +841,7 @@ static int damos_va_migrate_pmd_entry(pmd_t *pmd, unsigned long addr,
 	struct damos_va_migrate_private *priv = walk->private;
 	struct list_head *migration_lists = priv->migration_lists;
 	struct damos *s = priv->scheme;
+	unsigned long *sz_filter_passed = priv->sz_filter_passed;
 	struct damos_migrate_dests *dests = &s->migrate_dests;
 	struct folio *folio;
 	spinlock_t *ptl;
@@ -858,6 +860,7 @@ static int damos_va_migrate_pmd_entry(pmd_t *pmd, unsigned long addr,
 			goto huge_out;
 		if (damos_va_filter_out(s, folio, walk->vma, addr, NULL, pmd))
 			goto huge_out;
+		*sz_filter_passed += folio_size(folio);
 		damos_va_migrate_dests_add(folio, walk->vma, addr, dests,
 				migration_lists);
 huge_out:
@@ -885,6 +888,7 @@ static int damos_va_migrate_pmd_entry(pmd_t *pmd, unsigned long addr,
 		nr = folio_nr_pages(folio) - page_idx;
 		if (damos_va_filter_out(s, folio, walk->vma, addr, pte, NULL))
 			continue;
+		*sz_filter_passed += folio_size(folio);
 		damos_va_migrate_dests_add(folio, walk->vma, addr, dests,
 				migration_lists);
 	}
-- 
2.47.3

[RFC PATCH 4/6] mm/damon/sysfs-schemes: drop redundant and racy zero target_value check

[SJ Park]

DAMON sysfs interface disallowed zero target_value because it could
result in divide-by-zero in damos_quota_store().  Because similar checks
were duplicated to other DAMON API callers, a patch series that starts
with commit 610c4438fb66 ("mm/damon/core: error
damos_commit_quota_goal() for zero target_value") added a same check in
the core layer central parameters, and dropped redundant checks in
DAMON_LRU_SORT and DAMON_SAMPLIE_MTIER.  It forgot to drop that in the
DAMON sysfs interface.

Now the zero target_value check in DAMON sysfs interface is only
redundant.  Further, it has a time-of-check to time-of-use (toctou) race
with target_value_store().  It can confuse readers to believe [1] the
division-by-zero can still happen.  Drop the redundant and racy check.

[1] https://lore.kernel.org/20261021005234.94107-1-sj@kernel.org

Signed-off-by: SJ Park <sj@kernel.org>
---
 mm/damon/sysfs-schemes.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
index 637f1bd0c950b..38efdc258ef3f 100644
--- a/mm/damon/sysfs-schemes.c
+++ b/mm/damon/sysfs-schemes.c
@@ -2898,9 +2898,6 @@ static int damos_sysfs_add_quota_score(
 		struct damos_sysfs_quota_goal *sysfs_goal =
 			sysfs_goals->goals_arr[i];
 
-		if (!sysfs_goal->target_value)
-			continue;
-
 		goal = damos_new_quota_goal(sysfs_goal->metric,
 				sysfs_goal->complement,
 				sysfs_goal->target_value);
-- 
2.47.3

[RFC PATCH 5/6] selftests/damon/damos_apply_interval: remove unused variables

[SJ Park]

wss_collected and nr_quota_exceeds in damos_apply_interval.py are
assigned to but never used.  Remove.

The issue is found using flake8.

Signed-off-by: SJ Park <sj@kernel.org>
---
 tools/testing/selftests/damon/damos_apply_interval.py | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/testing/selftests/damon/damos_apply_interval.py b/tools/testing/selftests/damon/damos_apply_interval.py
index 0bf7768b2006a..0ad110be0fb94 100755
--- a/tools/testing/selftests/damon/damos_apply_interval.py
+++ b/tools/testing/selftests/damon/damos_apply_interval.py
@@ -42,8 +42,6 @@ def main():
         print('kdamond start failed: %s' % err)
         exit(1)
 
-    wss_collected = []
-    nr_quota_exceeds = 0
     while proc.poll() == None:
         time.sleep(0.1)
         err = kdamonds.kdamonds[0].update_schemes_stats()
-- 
2.47.3

[RFC PATCH 6/6] selftests/damon: remove unused imports of os and sys modules

[SJ Park]

sysfs_no_op_commit_break.py imports os module but never uses it.
sysfs_update_schemes_tried_regions_hang imports the sys module but never
uses it.  Remove the unnecessary imports.

The issue is found using flake8.

Signed-off-by: SJ Park <sj@kernel.org>
---
 tools/testing/selftests/damon/sysfs_no_op_commit_break.py        | 1 -
 .../selftests/damon/sysfs_update_schemes_tried_regions_hang.py   | 1 -
 2 files changed, 2 deletions(-)

diff --git a/tools/testing/selftests/damon/sysfs_no_op_commit_break.py b/tools/testing/selftests/damon/sysfs_no_op_commit_break.py
index 81774ddc55ac4..5664f75186bac 100755
--- a/tools/testing/selftests/damon/sysfs_no_op_commit_break.py
+++ b/tools/testing/selftests/damon/sysfs_no_op_commit_break.py
@@ -4,7 +4,6 @@
 import json
 import os
 import subprocess
-import sys
 
 import _damon_sysfs
 
diff --git a/tools/testing/selftests/damon/sysfs_update_schemes_tried_regions_hang.py b/tools/testing/selftests/damon/sysfs_update_schemes_tried_regions_hang.py
index 625761c243b58..326a14f040534 100755
--- a/tools/testing/selftests/damon/sysfs_update_schemes_tried_regions_hang.py
+++ b/tools/testing/selftests/damon/sysfs_update_schemes_tried_regions_hang.py
@@ -4,7 +4,6 @@
 import os
 import subprocess
 import sys
-import time
 
 sys.path.append(os.path.dirname(os.path.abspath(__file__)))
 import _damon_sysfs
-- 
2.47.3

Re: [RFC PATCH 6/6] selftests/damon: remove unused imports of os and sys modules

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] selftests/damon: commit message names wrong removed modules
--

commit 55304159b940aad6f9b71d318671a40d9d63ca46
Author: SJ Park <sj@kernel.org>

selftests/damon: remove unused imports of os and sys modules

This commit removes unused Python module imports from two DAMON selftest
scripts.

>     selftests/damon: remove unused imports of os and sys modules
>     
>     sysfs_no_op_commit_break.py imports os module but never uses it.
>     sysfs_update_schemes_tried_regions_hang imports the sys module but never
>     uses it.  Remove the unnecessary imports.
>     
>     The issue is found using flake8.
>     
>     Signed-off-by: SJ Park <sj@kernel.org>

[Severity: Low]
Does the commit message name the wrong removed modules?

In tools/testing/selftests/damon/sysfs_no_op_commit_break.py, the patch
removes import sys rather than import os, and os is still used in
dump_damon_status_dict():

    file_dir = os.path.dirname(os.path.abspath(__file__))
    dump_script = os.path.join(file_dir, 'drgn_dump_damon_status.py')

In tools/testing/selftests/damon/sysfs_update_schemes_tried_regions_hang.py,
the patch removes import time rather than import sys, and both sys and os
are still used at module load time:

    sys.path.append(os.path.dirname(os.path.abspath(__file__)))

Should the commit subject and message be updated to refer to the sys and
time modules instead of os and sys?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031202614.96036-1-sj@kernel.org?part=6

Re: [RFC PATCH 1/6] mm/damon/core: handle invalid target in damos_fix_stale_charge_pointers()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mm/damon/core: stale charge_target_from and skip regression on invalid target
--

commit 3b1cac25b68197d5c2819d4a16dbde02446b2d07
Author: SJ Park <sj@kernel.org>

mm/damon/core: handle invalid target in damos_fix_stale_charge_pointers()

This commit moves the DAMOS quota charge pointer fixup for invalid targets
from kdamond_apply_schemes() into damos_fix_stale_charge_pointers().

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 73b571b429cb3..ce75db510a9d3 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -3394,7 +3394,11 @@ static void damos_fix_stale_charge_pointers(struct damon_ctx *c,
>  		return;
>  	damon_for_each_target(t, c) {
>  		if (t == quota->charge_target_from) {
> -			if (damon_nr_regions(t) > 0)
> +			bool valid_target = true;
> +
> +			if (c->ops.target_valid && !c->ops.target_valid(t))
> +				valid_target = false;
> +			if (valid_target && damon_nr_regions(t) > 0)
>  				return;
>  			break;
>  		}
> @@ -3513,15 +3517,8 @@ static noinline_for_stack void kdamond_apply_schemes(struct damon_ctx *c)
>  	max_region_sz = damon_region_sz_limit(c);
>  	mutex_lock(&c->walk_control_lock);
>  	damon_for_each_target(t, c) {
> -		if (c->ops.target_valid && c->ops.target_valid(t) == false) {
> -			damon_for_each_scheme(s, c) {
> -				if (s->quota.charge_target_from != t)
> -					continue;
> -				s->quota.charge_target_from = NULL;
> -				s->quota.charge_addr_from = 0;
> -			}
> +		if (c->ops.target_valid && c->ops.target_valid(t) == false)
>  			continue;
> -		}

[Severity: Medium]
Can moving the invalid-target reset from kdamond_apply_schemes() into
damos_fix_stale_charge_pointers() cause stale quota->charge_target_from
pointers and alter round-robin target skipping across multiple targets?

First, in kdamond_apply_schemes(), damos_adjust_quota() (which calls
damos_fix_stale_charge_pointers()) runs for all schemes before the
damon_for_each_target() application loop.  For virtual address space
monitoring, c->ops.target_valid(t) is damon_va_target_valid(), which
checks whether the target PID still has a live task_struct.

If the target referenced by s->quota.charge_target_from is valid when
damos_fix_stale_charge_pointers() checks it, but the process exits while
damos_adjust_quota() builds the score histogram or while
damos_apply_target() applies schemes to earlier targets, the target loop
in kdamond_apply_schemes() will see c->ops.target_valid(t) == false and
skip t without clearing s->quota.charge_target_from.  For all remaining
targets after t in c->adaptive_targets, damon_do_apply_schemes() calls
damos_skip_charged_region():

mm/damon/core.c:damos_skip_charged_region() {
	if (quota->charge_target_from) {
		if (t != quota->charge_target_from)
			return true;
...
}

which will skip every subsequent target for that apply interval.

Second, damos_adjust_quota() returns early before calling
damos_fix_stale_charge_pointers() whenever damos_quota_is_set(quota) is
false:

mm/damon/core.c:damos_adjust_quota() {
	if (!damos_quota_is_set(quota))
		return;
	...
	damos_fix_stale_charge_pointers(c, s);
}

whereas damon_do_apply_schemes() calls damos_skip_charged_region()
unconditionally.  If damos_quota_is_set(quota) becomes false while
quota->charge_target_from is still set to a target t (for example, when a
quota is disabled or its goal is removed via online commit in
damos_commit_quota(), which preserves quota->charge_target_from, and
quota->esz becomes 0), and t exits before kdamond_apply_schemes() reaches
t, neither damos_adjust_quota() nor kdamond_apply_schemes() will ever
clear quota->charge_target_from, causing damos_skip_charged_region() to
skip all other targets permanently.

Finally, when monitoring multiple targets [t0, t1, t2] where the previous
quota window stopped at charge_target_from = t1 and t1 has since exited,
kdamond_apply_schemes() previously skipped t0 via
damos_skip_charged_region(), cleared charge_target_from upon reaching t1,
and resumed applying the scheme at t2.  With charge_target_from cleared
upfront in damos_fix_stale_charge_pointers() before the target loop
starts, won't kdamond_apply_schemes() restart charging from t0 instead of
advancing to t2?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031202614.96036-1-sj@kernel.org?part=1

Re: [RFC PATCH 3/6] mm/damon/vaddr: implement sz_filter_passed for migrate_{hot,cold} action

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] mm/damon/vaddr: uninitialized priv.sz_filter_passed in damos_va_migrate()
- [Medium] mm/damon/vaddr: double-counted sz_filter_passed in damos_va_migrate_pmd_entry()
--

commit 872d2945863aba3a6ac672128498f5d65dbc8d40
Author: SJ Park <sj@kernel.org>

mm/damon/vaddr: implement sz_filter_passed for migrate_{hot,cold} action

This commit adds sz_filter_passed accounting to the DAMON virtual address
space implementation of the migrate_hot and migrate_cold DAMOS actions by
accumulating folio sizes in damos_va_migrate_pmd_entry().

> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index c271b6d63c027..112095c2d53fd 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c
> @@ -776,6 +776,7 @@ static bool damos_va_filter_out(struct damos *scheme, struct folio *folio,
>  struct damos_va_migrate_private {
>  	struct list_head *migration_lists;
>  	struct damos *scheme;
> +	unsigned long *sz_filter_passed;
>  };
>  
>  /*
> @@ -840,6 +841,7 @@ static int damos_va_migrate_pmd_entry(pmd_t *pmd, unsigned long addr,
>  	struct damos_va_migrate_private *priv = walk->private;
>  	struct list_head *migration_lists = priv->migration_lists;
>  	struct damos *s = priv->scheme;
> +	unsigned long *sz_filter_passed = priv->sz_filter_passed;

[Severity: Critical]
Is priv.sz_filter_passed ever initialized before the page walk runs?

In damos_va_migrate(), struct damos_va_migrate_private priv is declared on
the stack without an initializer, and only priv.scheme and
priv.migration_lists are set before calling damon_va_walk_page_range():

mm/damon/vaddr.c:damos_va_migrate() {
	...
	struct damos_va_migrate_private priv;
	...
	priv.scheme = s;
	priv.migration_lists = kmalloc_objs(*priv.migration_lists, nr_dests);
	if (!priv.migration_lists)
		return 0;

	for (int i = 0; i < nr_dests; i++)
		INIT_LIST_HEAD(&priv.migration_lists[i]);


	mm = damon_get_mm(target);
	if (!mm)
		goto free_lists;

	damon_va_walk_page_range(mm, r->ar.start, r->ar.end, &walk_ops, &priv);
	...
}

When damon_va_apply_scheme() invokes damos_va_migrate() for DAMOS_MIGRATE_HOT
or DAMOS_MIGRATE_COLD and damos_va_migrate_pmd_entry() encounters a present
folio that passes damos_va_filter_out(), won't *sz_filter_passed +=
folio_size(folio) dereference an uninitialized stack pointer?

>  	struct damos_migrate_dests *dests = &s->migrate_dests;
>  	struct folio *folio;
>  	spinlock_t *ptl;
> @@ -858,6 +860,7 @@ static int damos_va_migrate_pmd_entry(pmd_t *pmd, unsigned long addr,
>  			goto huge_out;
>  		if (damos_va_filter_out(s, folio, walk->vma, addr, NULL, pmd))
>  			goto huge_out;
> +		*sz_filter_passed += folio_size(folio);
>  		damos_va_migrate_dests_add(folio, walk->vma, addr, dests,
>  				migration_lists);
>  huge_out:
> @@ -885,6 +888,7 @@ static int damos_va_migrate_pmd_entry(pmd_t *pmd, unsigned long addr,
>  		nr = folio_nr_pages(folio) - page_idx;
>  		if (damos_va_filter_out(s, folio, walk->vma, addr, pte, NULL))
>  			continue;
> +		*sz_filter_passed += folio_size(folio);

[Severity: Medium]
Can *sz_filter_passed double-count a large folio that spans across multiple
adjacent damon_region ranges?

In damos_va_stat_pmd_entry(), the walk checks damos_va_invalid_folio(folio, s)
and records s->last_applied = folio so that a PMD-mapped THP or PTE-mapped
large folio overlapping more than one damon_region is only counted once per
scheme application pass:

mm/damon/vaddr.c:damos_va_stat_pmd_entry() {
		...
		if (damos_va_invalid_folio(folio, s))
			goto huge_unlock;

		if (!damos_va_filter_out(s, folio, vma, addr, NULL, pmd))
			*sz_filter_passed += folio_size(folio);
		s->last_applied = folio;
		...
}

In damos_va_migrate_pmd_entry(), s->last_applied is neither checked nor
updated on either the THP path or the PTE loop path. If a large folio spans
multiple regions and is not migrated during the first region's walk (for
example, because folio_nid(folio) already matches the target node in
damos_va_migrate_dests_add(), or because isolation or migration fails),
walking the next overlapping damon_region will encounter the same folio and
add the full folio_size(folio) to *sz_filter_passed again.

Should damos_va_migrate_pmd_entry() also check damos_va_invalid_folio(folio,
s) and update s->last_applied = folio?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031202614.96036-1-sj@kernel.org?part=3

Re: [RFC PATCH 1/6] mm/damon/core: handle invalid target in damos_fix_stale_charge_pointers()

[SJ Park]

On Sat, 31 Oct 2026 20:36:25 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] mm/damon/core: stale charge_target_from and skip regression on invalid target
> --
> 
> commit 3b1cac25b68197d5c2819d4a16dbde02446b2d07
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/core: handle invalid target in damos_fix_stale_charge_pointers()
> 
> This commit moves the DAMOS quota charge pointer fixup for invalid targets
> from kdamond_apply_schemes() into damos_fix_stale_charge_pointers().
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 73b571b429cb3..ce75db510a9d3 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -3394,7 +3394,11 @@ static void damos_fix_stale_charge_pointers(struct damon_ctx *c,
> >  		return;
> >  	damon_for_each_target(t, c) {
> >  		if (t == quota->charge_target_from) {
> > -			if (damon_nr_regions(t) > 0)
> > +			bool valid_target = true;
> > +
> > +			if (c->ops.target_valid && !c->ops.target_valid(t))
> > +				valid_target = false;
> > +			if (valid_target && damon_nr_regions(t) > 0)
> >  				return;
> >  			break;
> >  		}
> > @@ -3513,15 +3517,8 @@ static noinline_for_stack void kdamond_apply_schemes(struct damon_ctx *c)
> >  	max_region_sz = damon_region_sz_limit(c);
> >  	mutex_lock(&c->walk_control_lock);
> >  	damon_for_each_target(t, c) {
> > -		if (c->ops.target_valid && c->ops.target_valid(t) == false) {
> > -			damon_for_each_scheme(s, c) {
> > -				if (s->quota.charge_target_from != t)
> > -					continue;
> > -				s->quota.charge_target_from = NULL;
> > -				s->quota.charge_addr_from = 0;
> > -			}
> > +		if (c->ops.target_valid && c->ops.target_valid(t) == false)
> >  			continue;
> > -		}
> 
> [Severity: Medium]
> Can moving the invalid-target reset from kdamond_apply_schemes() into
> damos_fix_stale_charge_pointers() cause stale quota->charge_target_from
> pointers and alter round-robin target skipping across multiple targets?
> 
> First, in kdamond_apply_schemes(), damos_adjust_quota() (which calls
> damos_fix_stale_charge_pointers()) runs for all schemes before the
> damon_for_each_target() application loop.  For virtual address space
> monitoring, c->ops.target_valid(t) is damon_va_target_valid(), which
> checks whether the target PID still has a live task_struct.
> 
> If the target referenced by s->quota.charge_target_from is valid when
> damos_fix_stale_charge_pointers() checks it, but the process exits while
> damos_adjust_quota() builds the score histogram or while
> damos_apply_target() applies schemes to earlier targets, the target loop
> in kdamond_apply_schemes() will see c->ops.target_valid(t) == false and
> skip t without clearing s->quota.charge_target_from.  For all remaining
> targets after t in c->adaptive_targets, damon_do_apply_schemes() calls
> damos_skip_charged_region():
> 
> mm/damon/core.c:damos_skip_charged_region() {
> 	if (quota->charge_target_from) {
> 		if (t != quota->charge_target_from)
> 			return true;
> ...
> }
> 
> which will skip every subsequent target for that apply interval.

This race can happen even in the current code.  That is, the target could be
terminated between the current check and damos_apply_target() call.  The race
can happen more easily, but I don't think this is a regression.

> 
> Second, damos_adjust_quota() returns early before calling
> damos_fix_stale_charge_pointers() whenever damos_quota_is_set(quota) is
> false:
> 
> mm/damon/core.c:damos_adjust_quota() {
> 	if (!damos_quota_is_set(quota))
> 		return;
> 	...
> 	damos_fix_stale_charge_pointers(c, s);
> }
> 
> whereas damon_do_apply_schemes() calls damos_skip_charged_region()
> unconditionally.  If damos_quota_is_set(quota) becomes false while
> quota->charge_target_from is still set to a target t (for example, when a
> quota is disabled or its goal is removed via online commit in
> damos_commit_quota(), which preserves quota->charge_target_from, and
> quota->esz becomes 0), and t exits before kdamond_apply_schemes() reaches
> t, neither damos_adjust_quota() nor kdamond_apply_schemes() will ever
> clear quota->charge_target_from, causing damos_skip_charged_region() to
> skip all other targets permanently.

Good finding.  I will drop this patch from the next version of this series for
now.

> 
> Finally, when monitoring multiple targets [t0, t1, t2] where the previous
> quota window stopped at charge_target_from = t1 and t1 has since exited,
> kdamond_apply_schemes() previously skipped t0 via
> damos_skip_charged_region(), cleared charge_target_from upon reaching t1,
> and resumed applying the scheme at t2.  With charge_target_from cleared
> upfront in damos_fix_stale_charge_pointers() before the target loop
> starts, won't kdamond_apply_schemes() restart charging from t0 instead of
> advancing to t2?

Again, good finding.  This is only a corner case that happen only for one
cycle.  I think it is a reasonable tradeoff for code simplicity.  But since I
decided to drop this patch for now, I will think about this again, too.

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261031202614.96036-1-sj@kernel.org?part=1
> 


Thanks,
SJ

Re: [RFC PATCH 3/6] mm/damon/vaddr: implement sz_filter_passed for migrate_{hot,cold} action

[SJ Park]

On Sat, 31 Oct 2026 20:36:41 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] mm/damon/vaddr: uninitialized priv.sz_filter_passed in damos_va_migrate()
> - [Medium] mm/damon/vaddr: double-counted sz_filter_passed in damos_va_migrate_pmd_entry()
> --
> 
> commit 872d2945863aba3a6ac672128498f5d65dbc8d40
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/vaddr: implement sz_filter_passed for migrate_{hot,cold} action
> 
> This commit adds sz_filter_passed accounting to the DAMON virtual address
> space implementation of the migrate_hot and migrate_cold DAMOS actions by
> accumulating folio sizes in damos_va_migrate_pmd_entry().
> 
> > diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> > index c271b6d63c027..112095c2d53fd 100644
> > --- a/mm/damon/vaddr.c
> > +++ b/mm/damon/vaddr.c
> > @@ -776,6 +776,7 @@ static bool damos_va_filter_out(struct damos *scheme, struct folio *folio,
> >  struct damos_va_migrate_private {
> >  	struct list_head *migration_lists;
> >  	struct damos *scheme;
> > +	unsigned long *sz_filter_passed;
> >  };
> >  
> >  /*
> > @@ -840,6 +841,7 @@ static int damos_va_migrate_pmd_entry(pmd_t *pmd, unsigned long addr,
> >  	struct damos_va_migrate_private *priv = walk->private;
> >  	struct list_head *migration_lists = priv->migration_lists;
> >  	struct damos *s = priv->scheme;
> > +	unsigned long *sz_filter_passed = priv->sz_filter_passed;
> 
> [Severity: Critical]
> Is priv.sz_filter_passed ever initialized before the page walk runs?
> 
> In damos_va_migrate(), struct damos_va_migrate_private priv is declared on
> the stack without an initializer, and only priv.scheme and
> priv.migration_lists are set before calling damon_va_walk_page_range():
> 
> mm/damon/vaddr.c:damos_va_migrate() {
> 	...
> 	struct damos_va_migrate_private priv;
> 	...
> 	priv.scheme = s;
> 	priv.migration_lists = kmalloc_objs(*priv.migration_lists, nr_dests);
> 	if (!priv.migration_lists)
> 		return 0;
> 
> 	for (int i = 0; i < nr_dests; i++)
> 		INIT_LIST_HEAD(&priv.migration_lists[i]);
> 
> 
> 	mm = damon_get_mm(target);
> 	if (!mm)
> 		goto free_lists;
> 
> 	damon_va_walk_page_range(mm, r->ar.start, r->ar.end, &walk_ops, &priv);
> 	...
> }
> 
> When damon_va_apply_scheme() invokes damos_va_migrate() for DAMOS_MIGRATE_HOT
> or DAMOS_MIGRATE_COLD and damos_va_migrate_pmd_entry() encounters a present
> folio that passes damos_va_filter_out(), won't *sz_filter_passed +=
> folio_size(folio) dereference an uninitialized stack pointer?

Nice finding.  I will set the field in damos_va_migrate(), to sz_filter_passed
parameter it received.

> 
> >  	struct damos_migrate_dests *dests = &s->migrate_dests;
> >  	struct folio *folio;
> >  	spinlock_t *ptl;
> > @@ -858,6 +860,7 @@ static int damos_va_migrate_pmd_entry(pmd_t *pmd, unsigned long addr,
> >  			goto huge_out;
> >  		if (damos_va_filter_out(s, folio, walk->vma, addr, NULL, pmd))
> >  			goto huge_out;
> > +		*sz_filter_passed += folio_size(folio);
> >  		damos_va_migrate_dests_add(folio, walk->vma, addr, dests,
> >  				migration_lists);
> >  huge_out:
> > @@ -885,6 +888,7 @@ static int damos_va_migrate_pmd_entry(pmd_t *pmd, unsigned long addr,
> >  		nr = folio_nr_pages(folio) - page_idx;
> >  		if (damos_va_filter_out(s, folio, walk->vma, addr, pte, NULL))
> >  			continue;
> > +		*sz_filter_passed += folio_size(folio);
> 
> [Severity: Medium]
> Can *sz_filter_passed double-count a large folio that spans across multiple
> adjacent damon_region ranges?
> 
> In damos_va_stat_pmd_entry(), the walk checks damos_va_invalid_folio(folio, s)
> and records s->last_applied = folio so that a PMD-mapped THP or PTE-mapped
> large folio overlapping more than one damon_region is only counted once per
> scheme application pass:
> 
> mm/damon/vaddr.c:damos_va_stat_pmd_entry() {
> 		...
> 		if (damos_va_invalid_folio(folio, s))
> 			goto huge_unlock;
> 
> 		if (!damos_va_filter_out(s, folio, vma, addr, NULL, pmd))
> 			*sz_filter_passed += folio_size(folio);
> 		s->last_applied = folio;
> 		...
> }
> 
> In damos_va_migrate_pmd_entry(), s->last_applied is neither checked nor
> updated on either the THP path or the PTE loop path. If a large folio spans
> multiple regions and is not migrated during the first region's walk (for
> example, because folio_nid(folio) already matches the target node in
> damos_va_migrate_dests_add(), or because isolation or migration fails),
> walking the next overlapping damon_region will encounter the same folio and
> add the full folio_size(folio) to *sz_filter_passed again.
> 
> Should damos_va_migrate_pmd_entry() also check damos_va_invalid_folio(folio,
> s) and update s->last_applied = folio?

Good finding.  But this is a pre-existing issue.  I think it is better to be
fixed first, before this patch, though.  I will add the fix to this series, or
post it as a separate one before this series is merged.

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261031202614.96036-1-sj@kernel.org?part=3
> 


Thanks,
SJ

Re: [RFC PATCH 6/6] selftests/damon: remove unused imports of os and sys modules

[SJ Park]

On Sat, 31 Oct 2026 20:29:00 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] selftests/damon: commit message names wrong removed modules
> --
> 
> commit 55304159b940aad6f9b71d318671a40d9d63ca46
> Author: SJ Park <sj@kernel.org>
> 
> selftests/damon: remove unused imports of os and sys modules
> 
> This commit removes unused Python module imports from two DAMON selftest
> scripts.
> 
> >     selftests/damon: remove unused imports of os and sys modules
> >     
> >     sysfs_no_op_commit_break.py imports os module but never uses it.
> >     sysfs_update_schemes_tried_regions_hang imports the sys module but never
> >     uses it.  Remove the unnecessary imports.
> >     
> >     The issue is found using flake8.
> >     
> >     Signed-off-by: SJ Park <sj@kernel.org>
> 
> [Severity: Low]
> Does the commit message name the wrong removed modules?
> 
> In tools/testing/selftests/damon/sysfs_no_op_commit_break.py, the patch
> removes import sys rather than import os, and os is still used in
> dump_damon_status_dict():
> 
>     file_dir = os.path.dirname(os.path.abspath(__file__))
>     dump_script = os.path.join(file_dir, 'drgn_dump_damon_status.py')
> 
> In tools/testing/selftests/damon/sysfs_update_schemes_tried_regions_hang.py,
> the patch removes import time rather than import sys, and both sys and os
> are still used at module load time:
> 
>     sys.path.append(os.path.dirname(os.path.abspath(__file__)))
> 
> Should the commit subject and message be updated to refer to the sys and
> time modules instead of os and sys?

Good catches.  I will fix these in the next revision.

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261031202614.96036-1-sj@kernel.org?part=6
> 


Thanks,
SJ