[PATCH v1 0/2] mm/damon: Add damos_stat support for vaddr

[PATCH v1 0/2] mm/damon: Add damos_stat support for vaddr

[Yueyang Pan]

From: PanJason <pyyjason@gmail.com>

Previously damos_stat only supoort paddr. This patch set adds support 
for damos_stat for vaddr. Also all different types of filters are 
supported. 

Functionality Test
==================
I wrote a small test program which allocates 10GB of DRAM, use 
madvise(MADV_HUGEPAGE) to convert the base pages to 2MB huge pages
Then my program does the following things in order:
1. Write sequentially to the whole 10GB region
2. Read the first 5GB region sequentially for 10 times
3. Sleep 5s
4. Read the second 5GB region sequentially for 10 times

With a proper damon setting, we are expected to see df-passed to be 10GB
and hot region move around with the read

$ # Start damon record
$sudo ./damo/damo record "./my_test/test" --monitoring_intervals 100000\
1000000 2000000

$ # damon report
$sudo ./damo/damo report access --snapshot_damos_filter reject none \
hugepage_size 2MiB 2MiB
heatmap:
# min/max temperatures: -900,000,000, 100,002,000, column size: 136.564 MiB
intervals: sample 100 ms aggr 1 s (max access hz 10)
# damos filters (df): reject none hugepage_size [2.000 MiB, 2.000 MiB]
df-pass:
# min/max temperatures: -663,075,528, 100,002,000, column size: 128.037 MiB
0   addr 86.082 TiB   size 682.039 MiB access 0 hz   age 9 s           df-passed 0 B
1   addr 127.225 TiB  size 466.039 MiB access 1.000 hz age 0 ns          df-passed 468.000 MiB
2   addr 127.225 TiB  size 51.785 MiB  access 2.000 hz age 0 ns          df-passed 50.000 MiB
3   addr 127.225 TiB  size 785.219 MiB access 2.000 hz age 1 s           df-passed 786.000 MiB
4   addr 127.226 TiB  size 87.250 MiB  access 1.000 hz age 1 s           df-passed 88.000 MiB
5   addr 127.226 TiB  size 982.719 MiB access 1.000 hz age 0 ns          df-passed 982.000 MiB
6   addr 127.227 TiB  size 109.191 MiB access 0 hz   age 0 ns          df-passed 110.000 MiB
7   addr 127.227 TiB  size 103.945 MiB access 0 hz   age 0 ns          df-passed 104.000 MiB
8   addr 127.227 TiB  size 935.539 MiB access 0 hz   age 0 ns          df-passed 934.000 MiB
9   addr 127.228 TiB  size 386.547 MiB access 0 hz   age 0 ns          df-passed 388.000 MiB
10  addr 127.228 TiB  size 579.824 MiB access 0 hz   age 0 ns          df-passed 580.000 MiB
11  addr 127.229 TiB  size 168.676 MiB access 1.000 hz age 1 s           df-passed 168.000 MiB
12  addr 127.229 TiB  size 42.172 MiB  access 1.000 hz age 1 s           df-passed 42.000 MiB
13  addr 127.229 TiB  size 63.254 MiB  access 0 hz   age 0 ns          df-passed 64.000 MiB
14  addr 127.229 TiB  size 147.598 MiB access 0 hz   age 0 ns          df-passed 146.000 MiB
15  addr 127.229 TiB  size 112.453 MiB access 1.000 hz age 0 ns          df-passed 114.000 MiB
16  addr 127.229 TiB  size 168.684 MiB access 1.000 hz age 0 ns          df-passed 168.000 MiB
17  addr 127.229 TiB  size 810.512 MiB access 0 hz   age 6 s           df-passed 810.000 MiB
18  addr 127.230 TiB  size 202.629 MiB access 0 hz   age 6 s           df-passed 204.000 MiB
19  addr 127.230 TiB  size 206.859 MiB access 0 hz   age 6 s           df-passed 206.000 MiB
20  addr 127.231 TiB  size 827.449 MiB access 0 hz   age 6 s           df-passed 828.000 MiB
21  addr 127.231 TiB  size 326.004 MiB access 0 hz   age 5 s           df-passed 326.000 MiB
22  addr 127.232 TiB  size 760.680 MiB access 0 hz   age 5 s           df-passed 760.000 MiB
23  addr 127.232 TiB  size 148.238 MiB access 0 hz   age 5 s           df-passed 148.000 MiB
24  addr 127.233 TiB  size 592.965 MiB access 0 hz   age 5 s           df-passed 594.000 MiB
25  addr 127.233 TiB  size 321.695 MiB access 0 hz   age 5 s           df-passed 320.000 MiB
26  addr 127.233 TiB  size 750.629 MiB access 0 hz   age 5 s           df-passed 752.000 MiB
27  addr 127.234 TiB  size 73.078 MiB  access 0 hz   age 7 s           df-passed 72.000 MiB
28  addr 127.234 TiB  size 31.320 MiB  access 0 hz   age 7 s           df-passed 28.000 MiB
29  addr 127.997 TiB  size 132.000 KiB access 0 hz   age 9 s           df-passed 0 B
memory bw estimate: 3.615 GiB per second  df-passed: 3.615 GiB per second
total size: 10.669 GiB  df-passed 10.000 GiB
record DAMON intervals: sample 100 ms, aggr 1 s

$ # damon report again
$sudo ./damo/damo report access --snapshot_damos_filter reject none \
hugepage_size 2MiB 2MiB
heatmap:
# min/max temperatures: -1,100,000,000, 300,001,000, column size: 136.564 MiB
intervals: sample 100 ms aggr 1 s (max access hz 10)
# damos filters (df): reject none hugepage_size [2.000 MiB, 2.000 MiB]
df-pass:
# min/max temperatures: -800,000,000, 300,001,000, column size: 128.037 MiB
0   addr 86.082 TiB   size 682.039 MiB access 0 hz   age 11 s          df-passed 0 B
1   addr 127.225 TiB  size 10.355 MiB  access 1.000 hz age 0 ns          df-passed 12.000 MiB
2   addr 127.225 TiB  size 93.207 MiB  access 1.000 hz age 0 ns          df-passed 92.000 MiB
3   addr 127.225 TiB  size 414.262 MiB access 1.000 hz age 0 ns          df-passed 414.000 MiB
4   addr 127.225 TiB  size 706.695 MiB access 1.000 hz age 3 s           df-passed 708.000 MiB
5   addr 127.226 TiB  size 78.523 MiB  access 1.000 hz age 3 s           df-passed 78.000 MiB
6   addr 127.226 TiB  size 87.250 MiB  access 1.000 hz age 3 s           df-passed 88.000 MiB
7   addr 127.226 TiB  size 384.469 MiB access 1.000 hz age 0 ns          df-passed 384.000 MiB
8   addr 127.226 TiB  size 256.312 MiB access 1.000 hz age 0 ns          df-passed 256.000 MiB
9   addr 127.226 TiB  size 427.191 MiB access 1.000 hz age 0 ns          df-passed 428.000 MiB
10  addr 127.227 TiB  size 319.023 MiB access 1.000 hz age 0 ns          df-passed 318.000 MiB
11  addr 127.227 TiB  size 212.688 MiB access 0 hz   age 0 ns          df-passed 212.000 MiB
12  addr 127.227 TiB  size 531.711 MiB access 0 hz   age 0 ns          df-passed 532.000 MiB
13  addr 127.228 TiB  size 541.164 MiB access 0 hz   age 0 ns          df-passed 542.000 MiB
14  addr 127.228 TiB  size 135.293 MiB access 0 hz   age 0 ns          df-passed 136.000 MiB
15  addr 127.229 TiB  size 289.914 MiB access 0 hz   age 0 ns          df-passed 290.000 MiB
16  addr 127.229 TiB  size 29.516 MiB  access 1.000 hz age 0 ns          df-passed 28.000 MiB
17  addr 127.229 TiB  size 68.879 MiB  access 1.000 hz age 0 ns          df-passed 70.000 MiB
18  addr 127.229 TiB  size 42.172 MiB  access 1.000 hz age 0 ns          df-passed 42.000 MiB
19  addr 127.229 TiB  size 134.941 MiB access 2.000 hz age 0 ns          df-passed 134.000 MiB
20  addr 127.229 TiB  size 314.871 MiB access 2.000 hz age 0 ns          df-passed 316.000 MiB
21  addr 127.229 TiB  size 112.457 MiB access 2.000 hz age 0 ns          df-passed 112.000 MiB
22  addr 127.229 TiB  size 50.656 MiB  access 0 hz   age 8 s           df-passed 50.000 MiB
23  addr 127.230 TiB  size 50.656 MiB  access 0 hz   age 8 s           df-passed 52.000 MiB
24  addr 127.230 TiB  size 911.828 MiB access 0 hz   age 8 s           df-passed 912.000 MiB
25  addr 127.230 TiB  size 413.719 MiB access 0 hz   age 8 s           df-passed 412.000 MiB
26  addr 127.231 TiB  size 103.434 MiB access 0 hz   age 8 s           df-passed 104.000 MiB
27  addr 127.231 TiB  size 517.156 MiB access 0 hz   age 8 s           df-passed 518.000 MiB
28  addr 127.231 TiB  size 880.207 MiB access 0 hz   age 7 s           df-passed 880.000 MiB
29  addr 127.232 TiB  size 97.805 MiB  access 0 hz   age 7 s           df-passed 98.000 MiB
30  addr 127.232 TiB  size 108.672 MiB access 0 hz   age 7 s           df-passed 108.000 MiB
31  addr 127.232 TiB  size 212.578 MiB access 0 hz   age 7 s           df-passed 212.000 MiB
32  addr 127.233 TiB  size 212.578 MiB access 0 hz   age 7 s           df-passed 214.000 MiB
33  addr 127.233 TiB  size 637.742 MiB access 0 hz   age 7 s           df-passed 636.000 MiB
34  addr 127.233 TiB  size 102.602 MiB access 0 hz   age 7 s           df-passed 104.000 MiB
35  addr 127.234 TiB  size 239.406 MiB access 0 hz   age 7 s           df-passed 238.000 MiB
36  addr 127.234 TiB  size 513.020 MiB access 0 hz   age 7 s           df-passed 510.000 MiB
37  addr 127.997 TiB  size 132.000 KiB access 0 hz   age 11 s          df-passed 0 B
memory bw estimate: 3.948 GiB per second  df-passed: 3.947 GiB per second
total size: 10.669 GiB  df-passed 10.000 GiB
record DAMON intervals: sample 100 ms, aggr 1 s

As you can see the total df-passed region is 10GiB and the hot region
moves as the seq read keeps going

PanJason (2):
  mm/damon: Move invalid folio and has filter to ops-common
  mm/damon: Add damos_stat support for vaddr

 mm/damon/ops-common.c |  19 +++++++
 mm/damon/ops-common.h |   3 ++
 mm/damon/paddr.c      |  29 ++---------
 mm/damon/vaddr.c      | 113 +++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 139 insertions(+), 25 deletions(-)

-- 
2.47.3

[PATCH v1 1/2] mm/damon: Move invalid folio and has filter to ops-common

[Yueyang Pan]

From: PanJason <pyyjason@gmail.com>

This patch moves the damon_pa_invalid_damos_folio and
damon_pa_scheme_has_filter to ops-common. renaming them
to damon_invalid_damos_folio and damon_scheme_has_filter.
Doing so allows us to reuse their logic in the vaddr version
of DAMOS_STAT
---
 mm/damon/ops-common.c | 19 +++++++++++++++++++
 mm/damon/ops-common.h |  3 +++
 mm/damon/paddr.c      | 29 +++++------------------------
 3 files changed, 27 insertions(+), 24 deletions(-)

diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
index 99321ff5cb92..7d3b48cc0f86 100644
--- a/mm/damon/ops-common.c
+++ b/mm/damon/ops-common.c
@@ -412,3 +412,22 @@ unsigned long damon_migrate_pages(struct list_head *folio_list, int target_nid)
 
 	return nr_migrated;
 }
+
+bool damon_scheme_has_filter(struct damos *s)
+{
+	struct damos_filter *f;
+	damos_for_each_ops_filter(f, s)
+		return true;
+	return false;
+}
+
+bool damon_invalid_damos_folio(struct folio *folio, struct damos *s)
+{
+	if (!folio)
+		return true;
+	if (folio == s->last_applied) {
+		folio_put(folio);
+		return true;
+	}
+	return false;
+}
diff --git a/mm/damon/ops-common.h b/mm/damon/ops-common.h
index 61ad54aaf256..4e905477fdce 100644
--- a/mm/damon/ops-common.h
+++ b/mm/damon/ops-common.h
@@ -21,3 +21,6 @@ int damon_hot_score(struct damon_ctx *c, struct damon_region *r,
 
 bool damos_folio_filter_match(struct damos_filter *filter, struct folio *folio);
 unsigned long damon_migrate_pages(struct list_head *folio_list, int target_nid);
+
+bool damon_scheme_has_filter(struct damos *s);
+bool damon_invalid_damos_folio(struct folio *folio, struct damos *s);
diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
index 53a55c5114fb..a8b7048e871e 100644
--- a/mm/damon/paddr.c
+++ b/mm/damon/paddr.c
@@ -114,16 +114,6 @@ static bool damos_pa_filter_out(struct damos *scheme, struct folio *folio)
 	return scheme->ops_filters_default_reject;
 }
 
-static bool damon_pa_invalid_damos_folio(struct folio *folio, struct damos *s)
-{
-	if (!folio)
-		return true;
-	if (folio == s->last_applied) {
-		folio_put(folio);
-		return true;
-	}
-	return false;
-}
 
 static unsigned long damon_pa_pageout(struct damon_region *r, struct damos *s,
 		unsigned long *sz_filter_passed)
@@ -152,7 +142,7 @@ static unsigned long damon_pa_pageout(struct damon_region *r, struct damos *s,
 	addr = r->ar.start;
 	while (addr < r->ar.end) {
 		folio = damon_get_folio(PHYS_PFN(addr));
-		if (damon_pa_invalid_damos_folio(folio, s)) {
+		if (damon_invalid_damos_folio(folio, s)) {
 			addr += PAGE_SIZE;
 			continue;
 		}
@@ -192,7 +182,7 @@ static inline unsigned long damon_pa_mark_accessed_or_deactivate(
 	addr = r->ar.start;
 	while (addr < r->ar.end) {
 		folio = damon_get_folio(PHYS_PFN(addr));
-		if (damon_pa_invalid_damos_folio(folio, s)) {
+		if (damon_invalid_damos_folio(folio, s)) {
 			addr += PAGE_SIZE;
 			continue;
 		}
@@ -239,7 +229,7 @@ static unsigned long damon_pa_migrate(struct damon_region *r, struct damos *s,
 	addr = r->ar.start;
 	while (addr < r->ar.end) {
 		folio = damon_get_folio(PHYS_PFN(addr));
-		if (damon_pa_invalid_damos_folio(folio, s)) {
+		if (damon_invalid_damos_folio(folio, s)) {
 			addr += PAGE_SIZE;
 			continue;
 		}
@@ -262,28 +252,19 @@ static unsigned long damon_pa_migrate(struct damon_region *r, struct damos *s,
 	return applied * PAGE_SIZE;
 }
 
-static bool damon_pa_scheme_has_filter(struct damos *s)
-{
-	struct damos_filter *f;
-
-	damos_for_each_ops_filter(f, s)
-		return true;
-	return false;
-}
-
 static unsigned long damon_pa_stat(struct damon_region *r, struct damos *s,
 		unsigned long *sz_filter_passed)
 {
 	unsigned long addr;
 	struct folio *folio;
 
-	if (!damon_pa_scheme_has_filter(s))
+	if (!damon_scheme_has_filter(s))
 		return 0;
 
 	addr = r->ar.start;
 	while (addr < r->ar.end) {
 		folio = damon_get_folio(PHYS_PFN(addr));
-		if (damon_pa_invalid_damos_folio(folio, s)) {
+		if (damon_invalid_damos_folio(folio, s)) {
 			addr += PAGE_SIZE;
 			continue;
 		}
-- 
2.47.3

[PATCH v1 2/2] mm/damon: Add damos_stat support for vaddr

[Yueyang Pan]

From: PanJason <pyyjason@gmail.com>

This patch adds support for damos_stat in virtual address space.
It leverages the walk_page_range to walk the page table and gets
the folio from page table. The last folio scanned is stored in
damos->last_applied to prevent double counting.
---
 mm/damon/vaddr.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 112 insertions(+), 1 deletion(-)

diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
index 87e825349bdf..3e319b51cfd4 100644
--- a/mm/damon/vaddr.c
+++ b/mm/damon/vaddr.c
@@ -890,6 +890,117 @@ static unsigned long damos_va_migrate(struct damon_target *target,
 	return applied * PAGE_SIZE;
 }
 
+struct damos_va_stat_private {
+	struct damos *scheme;
+	unsigned long *sz_filter_passed;
+};
+
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
+static int damos_va_stat_pmd_entry(pmd_t *pmd, unsigned long addr,
+		unsigned long next, struct mm_walk *walk)
+{
+	struct damos_va_stat_private *priv = walk->private;
+	struct damos *s = priv->scheme;
+	unsigned long *sz_filter_passed = priv->sz_filter_passed;
+	struct folio *folio;
+	spinlock_t *ptl;
+	pmd_t pmde;
+
+	ptl = pmd_lock(walk->mm, pmd);
+	pmde = pmdp_get(pmd);
+
+	if (!pmd_present(pmde) || !pmd_trans_huge(pmde))
+		goto unlock;
+
+	/* Tell page walk code to not split the PMD */
+	walk->action = ACTION_CONTINUE;
+
+	folio = damon_get_folio(pmd_pfn(pmde));
+	if (!folio)
+		goto unlock;
+
+	if (damon_invalid_damos_folio(folio, s))
+		goto update_last_applied;
+
+	if (!damos_va_filter_out(s, folio, walk->vma, addr, NULL, pmd)){
+		*sz_filter_passed += folio_size(folio);
+	}
+
+	folio_put(folio);
+update_last_applied:
+	s->last_applied = folio;
+unlock:
+	spin_unlock(ptl);
+	return 0;
+}
+#else
+#define damon_va_stat_pmd_entry NULL
+#endif
+
+static int damos_va_stat_pte_entry(pte_t *pte, unsigned long addr,
+		unsigned long next, struct mm_walk *walk)
+{
+	struct damos_va_stat_private *priv = walk->private;
+	struct damos *s = priv->scheme;
+	unsigned long *sz_filter_passed = priv->sz_filter_passed;
+	struct folio *folio;
+	pte_t ptent;
+
+	ptent = ptep_get(pte);
+	if (pte_none(ptent) || !pte_present(ptent))
+		return 0;
+
+	folio = damon_get_folio(pte_pfn(ptent));
+	if (!folio)
+		return 0;
+
+	if (damon_invalid_damos_folio(folio, s))
+		goto update_last_applied;
+
+	if (!damos_va_filter_out(s, folio, walk->vma, addr, pte, NULL)){
+		*sz_filter_passed += folio_size(folio);
+	}
+
+	folio_put(folio);
+
+update_last_applied:
+	s->last_applied = folio;
+	return 0;
+}
+
+static unsigned long damos_va_stat(struct damon_target *target,
+		struct damon_region *r, struct damos *s,
+		unsigned long *sz_filter_passed)
+{
+
+	struct damos_va_stat_private priv;
+	struct mm_struct *mm;
+	struct mm_walk_ops walk_ops = {
+		.pmd_entry = damos_va_stat_pmd_entry,
+		.pte_entry = damos_va_stat_pte_entry,
+		.walk_lock = PGWALK_RDLOCK,
+	};
+
+	priv.scheme = s;
+	priv.sz_filter_passed = sz_filter_passed;
+
+	if (!damon_scheme_has_filter(s)){
+		return 0;
+	}
+
+	mm = damon_get_mm(target);
+	if (!mm)
+		return 0;
+
+	mmap_read_lock(mm);
+	walk_page_range(mm, r->ar.start, r->ar.end, &walk_ops, &priv);
+	mmap_read_unlock(mm);
+	mmput(mm);
+	pr_debug("Call va_stat: %lu\n", *sz_filter_passed);
+	return 0;
+
+}
+
 static unsigned long damon_va_apply_scheme(struct damon_ctx *ctx,
 		struct damon_target *t, struct damon_region *r,
 		struct damos *scheme, unsigned long *sz_filter_passed)
@@ -916,7 +1027,7 @@ static unsigned long damon_va_apply_scheme(struct damon_ctx *ctx,
 	case DAMOS_MIGRATE_COLD:
 		return damos_va_migrate(t, r, scheme, sz_filter_passed);
 	case DAMOS_STAT:
-		return 0;
+		return damos_va_stat(t, r, scheme, sz_filter_passed);
 	default:
 		/*
 		 * DAMOS actions that are not yet supported by 'vaddr'.
-- 
2.47.3

Re: [PATCH v1 2/2] mm/damon: Add damos_stat support for vaddr

[David Hildenbrand]

On 29.07.25 15:53, Yueyang Pan wrote:
> From: PanJason <pyyjason@gmail.com>
> 
> This patch adds support for damos_stat in virtual address space.
> It leverages the walk_page_range to walk the page table and gets
> the folio from page table. The last folio scanned is stored in
> damos->last_applied to prevent double counting.
> ---
>   mm/damon/vaddr.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 112 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index 87e825349bdf..3e319b51cfd4 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c
> @@ -890,6 +890,117 @@ static unsigned long damos_va_migrate(struct damon_target *target,
>   	return applied * PAGE_SIZE;
>   }
>   
> +struct damos_va_stat_private {
> +	struct damos *scheme;
> +	unsigned long *sz_filter_passed;
> +};
> +
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +static int damos_va_stat_pmd_entry(pmd_t *pmd, unsigned long addr,
> +		unsigned long next, struct mm_walk *walk)
> +{
> +	struct damos_va_stat_private *priv = walk->private;
> +	struct damos *s = priv->scheme;
> +	unsigned long *sz_filter_passed = priv->sz_filter_passed;
> +	struct folio *folio;
> +	spinlock_t *ptl;
> +	pmd_t pmde;
> +
> +	ptl = pmd_lock(walk->mm, pmd);
> +	pmde = pmdp_get(pmd);
> +
> +	if (!pmd_present(pmde) || !pmd_trans_huge(pmde))
> +		goto unlock;
> +
> +	/* Tell page walk code to not split the PMD */
> +	walk->action = ACTION_CONTINUE;
> +
> +	folio = damon_get_folio(pmd_pfn(pmde));
> +	if (!folio)
> +		goto unlock;
> +
> +	if (damon_invalid_damos_folio(folio, s))
> +		goto update_last_applied;
> +
> +	if (!damos_va_filter_out(s, folio, walk->vma, addr, NULL, pmd)){
> +		*sz_filter_passed += folio_size(folio);

See my comment below regarding vm_normal_page and folio references.

But this split into two handlers is fairly odd. Usually we only have a 
pmd_entry callback (see madvise_cold_or_pageout_pte_range as an 
example), and handle !CONFIG_TRANSPARENT_HUGEPAGE in there.

Then, there is also no need to mess with ACTION_CONTINUE

> +	}
> +
> +	folio_put(folio);
> +update_last_applied:
> +	s->last_applied = folio;
> +unlock:
> +	spin_unlock(ptl);
> +	return 0;
> +}
> +#else
> +#define damon_va_stat_pmd_entry NULL
> +#endif
> +
> +static int damos_va_stat_pte_entry(pte_t *pte, unsigned long addr,
> +		unsigned long next, struct mm_walk *walk)
> +{
> +	struct damos_va_stat_private *priv = walk->private;
> +	struct damos *s = priv->scheme;
> +	unsigned long *sz_filter_passed = priv->sz_filter_passed;
> +	struct folio *folio;
> +	pte_t ptent;
> +
> +	ptent = ptep_get(pte);
> +	if (pte_none(ptent) || !pte_present(ptent))
> +		return 0;
> +
> +	folio = damon_get_folio(pte_pfn(ptent));
> +	if (!folio)
> +		return 0;

We have vm_normal_folio() and friends for a reason -- so you don't have 
to do pte_pfn() manually.

... and now I am confused. We are holding the PTL, so why would you have 
to grab+put a folio reference here *at all*.

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 2/2] mm/damon: Add damos_stat support for vaddr

[SeongJae Park]

Hi Pan and David, thank you for this patch and comments!

On Tue, 29 Jul 2025 16:11:32 +0200 David Hildenbrand <david@redhat.com> wrote:

> On 29.07.25 15:53, Yueyang Pan wrote:
> > From: PanJason <pyyjason@gmail.com>
> > 
> > This patch adds support for damos_stat in virtual address space.
> > It leverages the walk_page_range to walk the page table and gets
> > the folio from page table. The last folio scanned is stored in
> > damos->last_applied to prevent double counting.
> > ---
> >   mm/damon/vaddr.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 112 insertions(+), 1 deletion(-)
> > 
> > diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> > index 87e825349bdf..3e319b51cfd4 100644
> > --- a/mm/damon/vaddr.c
> > +++ b/mm/damon/vaddr.c
> > @@ -890,6 +890,117 @@ static unsigned long damos_va_migrate(struct damon_target *target,
> >   	return applied * PAGE_SIZE;
> >   }
> >   
> > +struct damos_va_stat_private {
> > +	struct damos *scheme;
> > +	unsigned long *sz_filter_passed;
> > +};
> > +
> > +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> > +static int damos_va_stat_pmd_entry(pmd_t *pmd, unsigned long addr,
> > +		unsigned long next, struct mm_walk *walk)
> > +{
> > +	struct damos_va_stat_private *priv = walk->private;
> > +	struct damos *s = priv->scheme;
> > +	unsigned long *sz_filter_passed = priv->sz_filter_passed;
> > +	struct folio *folio;
> > +	spinlock_t *ptl;
> > +	pmd_t pmde;
> > +
> > +	ptl = pmd_lock(walk->mm, pmd);
> > +	pmde = pmdp_get(pmd);
> > +
> > +	if (!pmd_present(pmde) || !pmd_trans_huge(pmde))
> > +		goto unlock;
> > +
> > +	/* Tell page walk code to not split the PMD */
> > +	walk->action = ACTION_CONTINUE;
> > +
> > +	folio = damon_get_folio(pmd_pfn(pmde));
> > +	if (!folio)
> > +		goto unlock;
> > +
> > +	if (damon_invalid_damos_folio(folio, s))
> > +		goto update_last_applied;
> > +
> > +	if (!damos_va_filter_out(s, folio, walk->vma, addr, NULL, pmd)){
> > +		*sz_filter_passed += folio_size(folio);
> 
> See my comment below regarding vm_normal_page and folio references.
> 
> But this split into two handlers is fairly odd. Usually we only have a 
> pmd_entry callback (see madvise_cold_or_pageout_pte_range as an 
> example), and handle !CONFIG_TRANSPARENT_HUGEPAGE in there.
> 
> Then, there is also no need to mess with ACTION_CONTINUE

I don't really mind this, but I agree keeping the consisteency would be good.
Pan, could you please unify the handlers into one?

> 
> > +	}
> > +
> > +	folio_put(folio);
> > +update_last_applied:
> > +	s->last_applied = folio;
> > +unlock:
> > +	spin_unlock(ptl);
> > +	return 0;
> > +}
> > +#else
> > +#define damon_va_stat_pmd_entry NULL
> > +#endif
> > +
> > +static int damos_va_stat_pte_entry(pte_t *pte, unsigned long addr,
> > +		unsigned long next, struct mm_walk *walk)
> > +{
> > +	struct damos_va_stat_private *priv = walk->private;
> > +	struct damos *s = priv->scheme;
> > +	unsigned long *sz_filter_passed = priv->sz_filter_passed;
> > +	struct folio *folio;
> > +	pte_t ptent;
> > +
> > +	ptent = ptep_get(pte);
> > +	if (pte_none(ptent) || !pte_present(ptent))
> > +		return 0;
> > +
> > +	folio = damon_get_folio(pte_pfn(ptent));
> > +	if (!folio)
> > +		return 0;
> 
> We have vm_normal_folio() and friends for a reason -- so you don't have 
> to do pte_pfn() manually.
> 
> ... and now I am confused. We are holding the PTL, so why would you have 
> to grab+put a folio reference here *at all*.

We don't have to.  I think Pan does so because other similar existing code in
this file is also doing so.  I was doing so because I wanted to use the handy
damon_get_folio() and those are not making real problems.

But, yes, unnecessary things are unnecessary things.  Pan, could you please use
vm_normal_folio() instead of damon_get_folio() and remove the related
folio_put() call?

I will also work on cleanup of existing unnecessary folio reference
manipulations, regardless of this patch series.


Thanks,
SJ

[...]

Re: [PATCH v1 2/2] mm/damon: Add damos_stat support for vaddr

[SeongJae Park]

On Tue, 29 Jul 2025 06:53:30 -0700 Yueyang Pan <pyyjason@gmail.com> wrote:

> From: PanJason <pyyjason@gmail.com>
> 
> This patch adds support for damos_stat in virtual address space.
> It leverages the walk_page_range to walk the page table and gets
> the folio from page table. The last folio scanned is stored in
> damos->last_applied to prevent double counting.

Thank you for this patch, Pan!  I left a few comments below.  I think those are
mostly insignificant change requests, though.

> ---
>  mm/damon/vaddr.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 112 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index 87e825349bdf..3e319b51cfd4 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c
> @@ -890,6 +890,117 @@ static unsigned long damos_va_migrate(struct damon_target *target,
>  	return applied * PAGE_SIZE;
>  }
>  
> +struct damos_va_stat_private {
> +	struct damos *scheme;
> +	unsigned long *sz_filter_passed;
> +};
> +
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +static int damos_va_stat_pmd_entry(pmd_t *pmd, unsigned long addr,
> +		unsigned long next, struct mm_walk *walk)
> +{
> +	struct damos_va_stat_private *priv = walk->private;
> +	struct damos *s = priv->scheme;
> +	unsigned long *sz_filter_passed = priv->sz_filter_passed;
> +	struct folio *folio;
> +	spinlock_t *ptl;
> +	pmd_t pmde;
> +
> +	ptl = pmd_lock(walk->mm, pmd);
> +	pmde = pmdp_get(pmd);
> +
> +	if (!pmd_present(pmde) || !pmd_trans_huge(pmde))
> +		goto unlock;
> +
> +	/* Tell page walk code to not split the PMD */
> +	walk->action = ACTION_CONTINUE;

As David suggested, let's unify this with pte handler following the pattern of
madvise_cold_or_pageout_pte_range() and drop above ACTION_CONTINUE code, unless
you have different opinions.

> +
> +	folio = damon_get_folio(pmd_pfn(pmde));

As also David suggested, let's use vm_normal_folio_pmd() instead, and drop
unnecessary folio_put().

> +	if (!folio)
> +		goto unlock;

damon_invalid_damos_folio() returns true if folio is NULL, so I think above
check is unnecessary.

> +
> +	if (damon_invalid_damos_folio(folio, s))
> +		goto update_last_applied;

Because we didn't really apply the DAMOS action, I think it is more proper to
goto 'unlock' directly.

Oh, and I now realize damon_invalid_damos_folio() puts the folio for none-NULL
invalid folio...

Because the code is simple, let's implement and use 'va' version
invalid_damos_folio(), say, damon_va_invalid_damos_folio(), which doesn't put
the folio.

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

Let's remove braces for single statement, as suggested[1] by the coding style.

> +
> +	folio_put(folio);
> +update_last_applied:
> +	s->last_applied = folio;
> +unlock:
> +	spin_unlock(ptl);
> +	return 0;
> +}
> +#else
> +#define damon_va_stat_pmd_entry NULL
> +#endif
> +
> +static int damos_va_stat_pte_entry(pte_t *pte, unsigned long addr,
> +		unsigned long next, struct mm_walk *walk)
> +{
> +	struct damos_va_stat_private *priv = walk->private;
> +	struct damos *s = priv->scheme;
> +	unsigned long *sz_filter_passed = priv->sz_filter_passed;
> +	struct folio *folio;
> +	pte_t ptent;
> +
> +	ptent = ptep_get(pte);
> +	if (pte_none(ptent) || !pte_present(ptent))
> +		return 0;
> +
> +	folio = damon_get_folio(pte_pfn(ptent));

As David suggested, let's use vm_normal_folio() here, and remove below
folio_put().

> +	if (!folio)
> +		return 0;

As also mentioned above, let's drop above NULL case check, in favor of that in
damon_va_invalid_damos_folio().

> +
> +	if (damon_invalid_damos_folio(folio, s))
> +		goto update_last_applied;

Again, I don't think we need to update s->last_applied in this case.  Let's
do only necessary cleanups and return.

> +
> +	if (!damos_va_filter_out(s, folio, walk->vma, addr, pte, NULL)){
> +		*sz_filter_passed += folio_size(folio);
> +	}

Let's drop braces for single statement[1].

> +
> +	folio_put(folio);
> +
> +update_last_applied:
> +	s->last_applied = folio;
> +	return 0;
> +}
> +
> +static unsigned long damos_va_stat(struct damon_target *target,
> +		struct damon_region *r, struct damos *s,
> +		unsigned long *sz_filter_passed)
> +{
> +

Let's remove this unnecessary blank line.

> +	struct damos_va_stat_private priv;
> +	struct mm_struct *mm;
> +	struct mm_walk_ops walk_ops = {
> +		.pmd_entry = damos_va_stat_pmd_entry,
> +		.pte_entry = damos_va_stat_pte_entry,
> +		.walk_lock = PGWALK_RDLOCK,
> +	};
> +
> +	priv.scheme = s;
> +	priv.sz_filter_passed = sz_filter_passed;
> +
> +	if (!damon_scheme_has_filter(s)){
> +		return 0;
> +	}

Let's remove braces for single statement[1].

> +
> +	mm = damon_get_mm(target);
> +	if (!mm)
> +		return 0;
> +
> +	mmap_read_lock(mm);
> +	walk_page_range(mm, r->ar.start, r->ar.end, &walk_ops, &priv);
> +	mmap_read_unlock(mm);
> +	mmput(mm);
> +	pr_debug("Call va_stat: %lu\n", *sz_filter_passed);

I don't think we really need this debug log.  Can we remove?

> +	return 0;
> +

Yet another unnecessary blank line.  Let's remove.

> +}
> +
>  static unsigned long damon_va_apply_scheme(struct damon_ctx *ctx,
>  		struct damon_target *t, struct damon_region *r,
>  		struct damos *scheme, unsigned long *sz_filter_passed)
> @@ -916,7 +1027,7 @@ static unsigned long damon_va_apply_scheme(struct damon_ctx *ctx,
>  	case DAMOS_MIGRATE_COLD:
>  		return damos_va_migrate(t, r, scheme, sz_filter_passed);
>  	case DAMOS_STAT:
> -		return 0;
> +		return damos_va_stat(t, r, scheme, sz_filter_passed);
>  	default:
>  		/*
>  		 * DAMOS actions that are not yet supported by 'vaddr'.
> -- 
> 2.47.3

[1] https://docs.kernel.org/process/coding-style.html


Thanks,
SJ

Re: [PATCH v1 1/2] mm/damon: Move invalid folio and has filter to ops-common

[SeongJae Park]

On Tue, 29 Jul 2025 06:53:29 -0700 Yueyang Pan <pyyjason@gmail.com> wrote:

> From: PanJason <pyyjason@gmail.com>
> 
> This patch moves the damon_pa_invalid_damos_folio and
> damon_pa_scheme_has_filter to ops-common. renaming them
> to damon_invalid_damos_folio and damon_scheme_has_filter.
> Doing so allows us to reuse their logic in the vaddr version
> of DAMOS_STAT

You forgot adding your Signed-off-by:

> ---
>  mm/damon/ops-common.c | 19 +++++++++++++++++++
>  mm/damon/ops-common.h |  3 +++
>  mm/damon/paddr.c      | 29 +++++------------------------
>  3 files changed, 27 insertions(+), 24 deletions(-)
> 
> diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
> index 99321ff5cb92..7d3b48cc0f86 100644
> --- a/mm/damon/ops-common.c
> +++ b/mm/damon/ops-common.c
> @@ -412,3 +412,22 @@ unsigned long damon_migrate_pages(struct list_head *folio_list, int target_nid)
>  
>  	return nr_migrated;
>  }
> +
> +bool damon_scheme_has_filter(struct damos *s)
> +{
> +	struct damos_filter *f;
> +	damos_for_each_ops_filter(f, s)
> +		return true;
> +	return false;
> +}
> +
> +bool damon_invalid_damos_folio(struct folio *folio, struct damos *s)
> +{
> +	if (!folio)
> +		return true;
> +	if (folio == s->last_applied) {
> +		folio_put(folio);
> +		return true;
> +	}
> +	return false;
> +}

Unless you have different opinions about this function I mentioned on the reply
to the next patch, let's not move this function.

> diff --git a/mm/damon/ops-common.h b/mm/damon/ops-common.h
> index 61ad54aaf256..4e905477fdce 100644
> --- a/mm/damon/ops-common.h
> +++ b/mm/damon/ops-common.h
> @@ -21,3 +21,6 @@ int damon_hot_score(struct damon_ctx *c, struct damon_region *r,
>  
>  bool damos_folio_filter_match(struct damos_filter *filter, struct folio *folio);
>  unsigned long damon_migrate_pages(struct list_head *folio_list, int target_nid);
> +
> +bool damon_scheme_has_filter(struct damos *s);
> +bool damon_invalid_damos_folio(struct folio *folio, struct damos *s);
> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> index 53a55c5114fb..a8b7048e871e 100644
> --- a/mm/damon/paddr.c
> +++ b/mm/damon/paddr.c
> @@ -114,16 +114,6 @@ static bool damos_pa_filter_out(struct damos *scheme, struct folio *folio)
>  	return scheme->ops_filters_default_reject;
>  }
>  
> -static bool damon_pa_invalid_damos_folio(struct folio *folio, struct damos *s)
> -{
> -	if (!folio)
> -		return true;
> -	if (folio == s->last_applied) {
> -		folio_put(folio);
> -		return true;
> -	}
> -	return false;
> -}
>  
>  static unsigned long damon_pa_pageout(struct damon_region *r, struct damos *s,
>  		unsigned long *sz_filter_passed)
> @@ -152,7 +142,7 @@ static unsigned long damon_pa_pageout(struct damon_region *r, struct damos *s,
>  	addr = r->ar.start;
>  	while (addr < r->ar.end) {
>  		folio = damon_get_folio(PHYS_PFN(addr));
> -		if (damon_pa_invalid_damos_folio(folio, s)) {
> +		if (damon_invalid_damos_folio(folio, s)) {
>  			addr += PAGE_SIZE;
>  			continue;
>  		}
> @@ -192,7 +182,7 @@ static inline unsigned long damon_pa_mark_accessed_or_deactivate(
>  	addr = r->ar.start;
>  	while (addr < r->ar.end) {
>  		folio = damon_get_folio(PHYS_PFN(addr));
> -		if (damon_pa_invalid_damos_folio(folio, s)) {
> +		if (damon_invalid_damos_folio(folio, s)) {
>  			addr += PAGE_SIZE;
>  			continue;
>  		}
> @@ -239,7 +229,7 @@ static unsigned long damon_pa_migrate(struct damon_region *r, struct damos *s,
>  	addr = r->ar.start;
>  	while (addr < r->ar.end) {
>  		folio = damon_get_folio(PHYS_PFN(addr));
> -		if (damon_pa_invalid_damos_folio(folio, s)) {
> +		if (damon_invalid_damos_folio(folio, s)) {
>  			addr += PAGE_SIZE;
>  			continue;
>  		}
> @@ -262,28 +252,19 @@ static unsigned long damon_pa_migrate(struct damon_region *r, struct damos *s,
>  	return applied * PAGE_SIZE;
>  }
>  
> -static bool damon_pa_scheme_has_filter(struct damos *s)
> -{
> -	struct damos_filter *f;
> -
> -	damos_for_each_ops_filter(f, s)
> -		return true;
> -	return false;
> -}
> -
>  static unsigned long damon_pa_stat(struct damon_region *r, struct damos *s,
>  		unsigned long *sz_filter_passed)
>  {
>  	unsigned long addr;
>  	struct folio *folio;
>  
> -	if (!damon_pa_scheme_has_filter(s))
> +	if (!damon_scheme_has_filter(s))
>  		return 0;
>  
>  	addr = r->ar.start;
>  	while (addr < r->ar.end) {
>  		folio = damon_get_folio(PHYS_PFN(addr));
> -		if (damon_pa_invalid_damos_folio(folio, s)) {
> +		if (damon_invalid_damos_folio(folio, s)) {
>  			addr += PAGE_SIZE;
>  			continue;
>  		}
> -- 
> 2.47.3

Other than above, looks good to me.


Thanks,
SJ

Re: [PATCH v1 2/2] mm/damon: Add damos_stat support for vaddr

[SeongJae Park]

On Tue, 29 Jul 2025 06:53:30 -0700 Yueyang Pan <pyyjason@gmail.com> wrote:

> From: PanJason <pyyjason@gmail.com>
> 
> This patch adds support for damos_stat in virtual address space.
> It leverages the walk_page_range to walk the page table and gets
> the folio from page table. The last folio scanned is stored in
> damos->last_applied to prevent double counting.

Please add your Singed-off-by: tag[1] here.

[1] https://docs.kernel.org/process/submitting-patches.html#developer-s-certificate-of-origin-1-1


Thanks,
SJ

[...]

Re: [PATCH v1 0/2] mm/damon: Add damos_stat support for vaddr

[SeongJae Park]

Hello Pan (Or, should I call you Yueyang or Jason?  Please let me know your
preferrence if you have),

On Tue, 29 Jul 2025 06:53:28 -0700 Yueyang Pan <pyyjason@gmail.com> wrote:

> From: PanJason <pyyjason@gmail.com>
> 
> Previously damos_stat only supoort paddr. This patch set adds support 
> for damos_stat for vaddr. Also all different types of filters are 
> supported. 
> 
> Functionality Test
> ==================
> I wrote a small test program which allocates 10GB of DRAM, use 
> madvise(MADV_HUGEPAGE) to convert the base pages to 2MB huge pages
> Then my program does the following things in order:
> 1. Write sequentially to the whole 10GB region
> 2. Read the first 5GB region sequentially for 10 times
> 3. Sleep 5s
> 4. Read the second 5GB region sequentially for 10 times
> 
> With a proper damon setting, we are expected to see df-passed to be 10GB
> and hot region move around with the read
> 
> $ # Start damon record
> $sudo ./damo/damo record "./my_test/test" --monitoring_intervals 100000\
> 1000000 2000000

You can use 'start' instead of 'record' for this test purpose.

--monitoring_intervals receive more human-friendly format, so you can do
'--monitoring_intervals 100ms 1s 2s'.

> 
> $ # damon report
> $sudo ./damo/damo report access --snapshot_damos_filter reject none \
> hugepage_size 2MiB 2MiB

The --snapshot_damos_filter option means you want to make folios that not
having size 2 MiB not passed by the filter.  You can ask same thing in more
intuitive way, like below.

    --snapshot_damos_filter allow hugepage_size 2MiB 2MiB

> heatmap:
> # min/max temperatures: -900,000,000, 100,002,000, column size: 136.564 MiB

checkpatch.pl gives me below warning:

    WARNING: Commit log lines starting with '#' are dropped by git as comments

I'd suggest adding four spaces prefix to quoted command outputs like this.

> intervals: sample 100 ms aggr 1 s (max access hz 10)
> # damos filters (df): reject none hugepage_size [2.000 MiB, 2.000 MiB]
> df-pass:
> # min/max temperatures: -663,075,528, 100,002,000, column size: 128.037 MiB
> 0   addr 86.082 TiB   size 682.039 MiB access 0 hz   age 9 s           df-passed 0 B
> 1   addr 127.225 TiB  size 466.039 MiB access 1.000 hz age 0 ns          df-passed 468.000 MiB
[...]
> memory bw estimate: 3.615 GiB per second  df-passed: 3.615 GiB per second
> total size: 10.669 GiB  df-passed 10.000 GiB
> record DAMON intervals: sample 100 ms, aggr 1 s
> 
> $ # damon report again
> $sudo ./damo/damo report access --snapshot_damos_filter reject none \
> hugepage_size 2MiB 2MiB
> heatmap:
> # min/max temperatures: -1,100,000,000, 300,001,000, column size: 136.564 MiB
> intervals: sample 100 ms aggr 1 s (max access hz 10)
> # damos filters (df): reject none hugepage_size [2.000 MiB, 2.000 MiB]
> df-pass:
> # min/max temperatures: -800,000,000, 300,001,000, column size: 128.037 MiB
> 0   addr 86.082 TiB   size 682.039 MiB access 0 hz   age 11 s          df-passed 0 B
> 1   addr 127.225 TiB  size 10.355 MiB  access 1.000 hz age 0 ns          df-passed 12.000 MiB
> 2   addr 127.225 TiB  size 93.207 MiB  access 1.000 hz age 0 ns          df-passed 92.000 MiB
> 3   addr 127.225 TiB  size 414.262 MiB access 1.000 hz age 0 ns          df-passed 414.000 MiB
> 4   addr 127.225 TiB  size 706.695 MiB access 1.000 hz age 3 s           df-passed 708.000 MiB
> 5   addr 127.226 TiB  size 78.523 MiB  access 1.000 hz age 3 s           df-passed 78.000 MiB
[...]
> total size: 10.669 GiB  df-passed 10.000 GiB
> record DAMON intervals: sample 100 ms, aggr 1 s
> 
> As you can see the total df-passed region is 10GiB and the hot region
> moves as the seq read keeps going
> 
> PanJason (2):
>   mm/damon: Move invalid folio and has filter to ops-common
>   mm/damon: Add damos_stat support for vaddr

The changes look good overall, though I left a few comments for formatting and
unnecessary folio references that inherited from my original sin.  Looking
forward to more discussions and next version of this great patch series!


Thanks,
SJ

[...]

Re: [PATCH v1 2/2] mm/damon: Add damos_stat support for vaddr

[Yueyang Pan]

On Tue, Jul 29, 2025 at 04:11:32PM +0200, David Hildenbrand wrote:
> On 29.07.25 15:53, Yueyang Pan wrote:
> > From: PanJason <pyyjason@gmail.com>
> > 
> > This patch adds support for damos_stat in virtual address space.
> > It leverages the walk_page_range to walk the page table and gets
> > the folio from page table. The last folio scanned is stored in
> > damos->last_applied to prevent double counting.
> > ---
> >   mm/damon/vaddr.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 112 insertions(+), 1 deletion(-)
> > 
> > diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> > index 87e825349bdf..3e319b51cfd4 100644
> > --- a/mm/damon/vaddr.c
> > +++ b/mm/damon/vaddr.c
> > @@ -890,6 +890,117 @@ static unsigned long damos_va_migrate(struct damon_target *target,
> >   	return applied * PAGE_SIZE;
> >   }
> > +struct damos_va_stat_private {
> > +	struct damos *scheme;
> > +	unsigned long *sz_filter_passed;
> > +};
> > +
> > +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> > +static int damos_va_stat_pmd_entry(pmd_t *pmd, unsigned long addr,
> > +		unsigned long next, struct mm_walk *walk)
> > +{
> > +	struct damos_va_stat_private *priv = walk->private;
> > +	struct damos *s = priv->scheme;
> > +	unsigned long *sz_filter_passed = priv->sz_filter_passed;
> > +	struct folio *folio;
> > +	spinlock_t *ptl;
> > +	pmd_t pmde;
> > +
> > +	ptl = pmd_lock(walk->mm, pmd);
> > +	pmde = pmdp_get(pmd);
> > +
> > +	if (!pmd_present(pmde) || !pmd_trans_huge(pmde))
> > +		goto unlock;
> > +
> > +	/* Tell page walk code to not split the PMD */
> > +	walk->action = ACTION_CONTINUE;
> > +
> > +	folio = damon_get_folio(pmd_pfn(pmde));
> > +	if (!folio)
> > +		goto unlock;
> > +
> > +	if (damon_invalid_damos_folio(folio, s))
> > +		goto update_last_applied;
> > +
> > +	if (!damos_va_filter_out(s, folio, walk->vma, addr, NULL, pmd)){
> > +		*sz_filter_passed += folio_size(folio);
> 
> See my comment below regarding vm_normal_page and folio references.
> 
> But this split into two handlers is fairly odd. Usually we only have a
> pmd_entry callback (see madvise_cold_or_pageout_pte_range as an example),
> and handle !CONFIG_TRANSPARENT_HUGEPAGE in there.
> 
> Then, there is also no need to mess with ACTION_CONTINUE
> 

Hi David. Thanks for your comment. I was not aware of the convention so I
followed the existing code. I had a discussion with SJ today and in the
next version I will change this by combining *pmd_entry() and *pte_entry().
We will also format the existing code in future patches.

> > +	}
> > +
> > +	folio_put(folio);
> > +update_last_applied:
> > +	s->last_applied = folio;
> > +unlock:
> > +	spin_unlock(ptl);
> > +	return 0;
> > +}
> > +#else
> > +#define damon_va_stat_pmd_entry NULL
> > +#endif
> > +
> > +static int damos_va_stat_pte_entry(pte_t *pte, unsigned long addr,
> > +		unsigned long next, struct mm_walk *walk)
> > +{
> > +	struct damos_va_stat_private *priv = walk->private;
> > +	struct damos *s = priv->scheme;
> > +	unsigned long *sz_filter_passed = priv->sz_filter_passed;
> > +	struct folio *folio;
> > +	pte_t ptent;
> > +
> > +	ptent = ptep_get(pte);
> > +	if (pte_none(ptent) || !pte_present(ptent))
> > +		return 0;
> > +
> > +	folio = damon_get_folio(pte_pfn(ptent));
> > +	if (!folio)
> > +		return 0;
> 
> We have vm_normal_folio() and friends for a reason -- so you don't have to
> do pte_pfn() manually.
> 
> ... and now I am confused. We are holding the PTL, so why would you have to
> grab+put a folio reference here *at all*.
> 

Thanks for pointing out. I thought someone could still change the folio and
realized it could not happen with PTL. I will use vm_normal_folio* in the
next version.

> -- 
> Cheers,
> 
> David / dhildenb
>