[PATCH v4 0/6] mm/damon: add support for hugepages

[PATCH v4 0/6] mm/damon: add support for hugepages

[Usama Arif]

This includes adding support for larger folios in damon for paddr,
which means largers folios will have their access checked and will
be considered for different DAMOS actions like pageout, prioritization
and migration.

Patches 3-6 add support for DAMOS hugepage filter type. This is to
gather statistics to check if memory regions of specific access
tempratures are backed by hugepages of a size in a specific
range. This filter can help to observe and prove the effectivenes of
different schemes for shrinking/collapsing hugepages.

I have kept patches 1-2 as part of these series as the later patches
are dependent on them.

The corresponding damo PR is at https://github.com/damonitor/damo/pull/20.

v3 -> v4:
- Add support for large folios of all sizes, and not just
  PMD mapped hugepages (David and SJ).
- only get folio while checking access/ applying DAMOS
  scheme if the head page is also part of that region.

v2 -> v3:
- expose hugepage via sysfs even if the kernel is
  built without hugepage support. DAMON will just
  just return 0. (SJ Park)

v1 -> v2:
- Wrap DAMOS_FILTER_TYPE_HUGEPAGE case with
  CONFIG_PGTABLE_HAS_HUGE_LEAVES (SJ Park)

Usama Arif (6):
  mm/damon: have damon_get_folio return folio even for tail pages
  mm/damon/paddr: use damon_get_folio_in_region to obtain folio
  mm/damon/sysfs-schemes: add files for setting damos_filter->folio_size
  mm/damon: introduce DAMOS filter type hugepage
  Docs/ABI/damon: document DAMOS sysfs files to set the min/max
    folio_size
  Docs/admin-guide/mm/damon/usage: Document hugepage filter type

 .../ABI/testing/sysfs-kernel-mm-damon         | 12 +++
 Documentation/admin-guide/mm/damon/usage.rst  | 17 +++--
 include/linux/damon.h                         | 13 ++++
 mm/damon/core.c                               |  3 +
 mm/damon/ops-common.c                         |  2 +-
 mm/damon/paddr.c                              | 75 +++++++++++++++----
 mm/damon/sysfs-schemes.c                      | 54 +++++++++++++
 7 files changed, 151 insertions(+), 25 deletions(-)

-- 
2.43.5

[PATCH v4 1/6] mm/damon: have damon_get_folio return folio even for tail pages

[Usama Arif]

This effectively adds support for large folios in damon for paddr,
as damon_pa_mkold/young won't get a null folio from this function
and won't ignore it, hence access will be checked and reported.
This also means that larger folios will be considered for
different DAMOS actions like pageout, prioritization and migration.
As these DAMOS actions will consider larger folios, iterate through
the region at folio_size and not PAGE_SIZE intervals.
This should not have an affect on vaddr, as damon_young_pmd_entry
considers pmd entries.

Signed-off-by: Usama Arif <usamaarif642@gmail.com>
Reviewed-by: SeongJae Park <sj@kernel.org>
---
 mm/damon/ops-common.c |  2 +-
 mm/damon/paddr.c      | 24 ++++++++++++++++++------
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
index d25d99cb5f2b..d511be201c4c 100644
--- a/mm/damon/ops-common.c
+++ b/mm/damon/ops-common.c
@@ -24,7 +24,7 @@ struct folio *damon_get_folio(unsigned long pfn)
 	struct page *page = pfn_to_online_page(pfn);
 	struct folio *folio;
 
-	if (!page || PageTail(page))
+	if (!page)
 		return NULL;
 
 	folio = page_folio(page);
diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
index 0f9ae14f884d..0fb61f6ddb8d 100644
--- a/mm/damon/paddr.c
+++ b/mm/damon/paddr.c
@@ -266,11 +266,14 @@ static unsigned long damon_pa_pageout(struct damon_region *r, struct damos *s,
 		damos_add_filter(s, filter);
 	}
 
-	for (addr = r->ar.start; addr < r->ar.end; addr += PAGE_SIZE) {
+	addr = r->ar.start;
+	while (addr < r->ar.end) {
 		struct folio *folio = damon_get_folio(PHYS_PFN(addr));
 
-		if (!folio)
+		if (!folio) {
+			addr += PAGE_SIZE;
 			continue;
+		}
 
 		if (damos_pa_filter_out(s, folio))
 			goto put_folio;
@@ -286,6 +289,7 @@ static unsigned long damon_pa_pageout(struct damon_region *r, struct damos *s,
 		else
 			list_add(&folio->lru, &folio_list);
 put_folio:
+		addr += folio_size(folio);
 		folio_put(folio);
 	}
 	if (install_young_filter)
@@ -301,11 +305,14 @@ static inline unsigned long damon_pa_mark_accessed_or_deactivate(
 {
 	unsigned long addr, applied = 0;
 
-	for (addr = r->ar.start; addr < r->ar.end; addr += PAGE_SIZE) {
+	addr = r->ar.start;
+	while (addr < r->ar.end) {
 		struct folio *folio = damon_get_folio(PHYS_PFN(addr));
 
-		if (!folio)
+		if (!folio) {
+			addr += PAGE_SIZE;
 			continue;
+		}
 
 		if (damos_pa_filter_out(s, folio))
 			goto put_folio;
@@ -318,6 +325,7 @@ static inline unsigned long damon_pa_mark_accessed_or_deactivate(
 			folio_deactivate(folio);
 		applied += folio_nr_pages(folio);
 put_folio:
+		addr += folio_size(folio);
 		folio_put(folio);
 	}
 	return applied * PAGE_SIZE;
@@ -464,11 +472,14 @@ static unsigned long damon_pa_migrate(struct damon_region *r, struct damos *s,
 	unsigned long addr, applied;
 	LIST_HEAD(folio_list);
 
-	for (addr = r->ar.start; addr < r->ar.end; addr += PAGE_SIZE) {
+	addr = r->ar.start;
+	while (addr < r->ar.end) {
 		struct folio *folio = damon_get_folio(PHYS_PFN(addr));
 
-		if (!folio)
+		if (!folio) {
+			addr += PAGE_SIZE;
 			continue;
+		}
 
 		if (damos_pa_filter_out(s, folio))
 			goto put_folio;
@@ -479,6 +490,7 @@ static unsigned long damon_pa_migrate(struct damon_region *r, struct damos *s,
 			goto put_folio;
 		list_add(&folio->lru, &folio_list);
 put_folio:
+		addr += folio_size(folio);
 		folio_put(folio);
 	}
 	applied = damon_pa_migrate_pages(&folio_list, s->target_nid);
-- 
2.43.5

[PATCH v4 2/6] mm/damon/paddr: use damon_get_folio_in_region to obtain folio

[Usama Arif]

This is introduced for larger folios. If a large folio has subpages
present in multiple regions, it will be considered multiple times.
This can be when checking access or applying DAMOS schemes. For e.g.
in pa_stat, folios split across N regions will be counted N times,
giving inaccurate results. Hence, only consider a page for access
check/DAMOS scheme only if the head page is part of that region as
well.

Signed-off-by: Usama Arif <usamaarif642@gmail.com>
---
 mm/damon/paddr.c | 44 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 34 insertions(+), 10 deletions(-)

diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
index 0fb61f6ddb8d..3f59a3fdc391 100644
--- a/mm/damon/paddr.c
+++ b/mm/damon/paddr.c
@@ -19,6 +19,30 @@
 #include "../internal.h"
 #include "ops-common.h"
 
+/*
+ * Get an online page for a paddr if it's in the LRU list and if the head page is
+ * before region_start. Otherwise, returns NULL.
+ */
+static struct folio *damon_get_folio_in_region(unsigned long addr, unsigned long region_start)
+{
+	struct page *page = pfn_to_online_page(PHYS_PFN(addr));
+	struct folio *folio;
+
+	if (!page)
+		return NULL;
+
+	folio = page_folio(page);
+	if (addr - folio_page_idx(folio, page) * PAGE_SIZE < region_start)
+		return NULL;
+	if (!folio_test_lru(folio) || !folio_try_get(folio))
+		return NULL;
+	if (unlikely(page_folio(page) != folio || !folio_test_lru(folio))) {
+		folio_put(folio);
+		folio = NULL;
+	}
+	return folio;
+}
+
 static bool damon_folio_mkold_one(struct folio *folio,
 		struct vm_area_struct *vma, unsigned long addr, void *arg)
 {
@@ -58,9 +82,9 @@ static void damon_folio_mkold(struct folio *folio)
 
 }
 
-static void damon_pa_mkold(unsigned long paddr)
+static void damon_pa_mkold(unsigned long paddr, unsigned long region_start)
 {
-	struct folio *folio = damon_get_folio(PHYS_PFN(paddr));
+	struct folio *folio = damon_get_folio_in_region(paddr, region_start);
 
 	if (!folio)
 		return;
@@ -73,7 +97,7 @@ static void __damon_pa_prepare_access_check(struct damon_region *r)
 {
 	r->sampling_addr = damon_rand(r->ar.start, r->ar.end);
 
-	damon_pa_mkold(r->sampling_addr);
+	damon_pa_mkold(r->sampling_addr, r->ar.start);
 }
 
 static void damon_pa_prepare_access_checks(struct damon_ctx *ctx)
@@ -148,9 +172,9 @@ static bool damon_folio_young(struct folio *folio)
 	return accessed;
 }
 
-static bool damon_pa_young(unsigned long paddr, unsigned long *folio_sz)
+static bool damon_pa_young(unsigned long paddr, unsigned long *folio_sz, unsigned long region_start)
 {
-	struct folio *folio = damon_get_folio(PHYS_PFN(paddr));
+	struct folio *folio = damon_get_folio_in_region(paddr, region_start);
 	bool accessed;
 
 	if (!folio)
@@ -176,7 +200,7 @@ static void __damon_pa_check_access(struct damon_region *r,
 		return;
 	}
 
-	last_accessed = damon_pa_young(r->sampling_addr, &last_folio_sz);
+	last_accessed = damon_pa_young(r->sampling_addr, &last_folio_sz, r->ar.start);
 	damon_update_region_access_rate(r, last_accessed, attrs);
 
 	last_addr = r->sampling_addr;
@@ -268,7 +292,7 @@ static unsigned long damon_pa_pageout(struct damon_region *r, struct damos *s,
 
 	addr = r->ar.start;
 	while (addr < r->ar.end) {
-		struct folio *folio = damon_get_folio(PHYS_PFN(addr));
+		struct folio *folio = damon_get_folio_in_region(addr, r->ar.start);
 
 		if (!folio) {
 			addr += PAGE_SIZE;
@@ -307,7 +331,7 @@ static inline unsigned long damon_pa_mark_accessed_or_deactivate(
 
 	addr = r->ar.start;
 	while (addr < r->ar.end) {
-		struct folio *folio = damon_get_folio(PHYS_PFN(addr));
+		struct folio *folio = damon_get_folio_in_region(addr, r->ar.start);
 
 		if (!folio) {
 			addr += PAGE_SIZE;
@@ -474,7 +498,7 @@ static unsigned long damon_pa_migrate(struct damon_region *r, struct damos *s,
 
 	addr = r->ar.start;
 	while (addr < r->ar.end) {
-		struct folio *folio = damon_get_folio(PHYS_PFN(addr));
+		struct folio *folio = damon_get_folio_in_region(addr, r->ar.start);
 
 		if (!folio) {
 			addr += PAGE_SIZE;
@@ -518,7 +542,7 @@ static unsigned long damon_pa_stat(struct damon_region *r, struct damos *s,
 
 	addr = r->ar.start;
 	while (addr < r->ar.end) {
-		struct folio *folio = damon_get_folio(PHYS_PFN(addr));
+		struct folio *folio = damon_get_folio_in_region(addr, r->ar.start);
 
 		if (!folio) {
 			addr += PAGE_SIZE;
-- 
2.43.5

Re: [PATCH v4 2/6] mm/damon/paddr: use damon_get_folio_in_region to obtain folio

[SeongJae Park]

On Mon,  3 Feb 2025 22:55:29 +0000 Usama Arif <usamaarif642@gmail.com> wrote:

> This is introduced for larger folios. If a large folio has subpages
> present in multiple regions, it will be considered multiple times.
> This can be when checking access

I think this behavior makes sense.  If a 2 MiB address range is known to be
accessed or not, treating DAMON regions in the address range as all accessed or
not aligns with the DAMON's region definition and makes sense in general, to
me.  Note that DAMON's adaptive regions adjustment mechanism will make the
DAMON regions in a large folio to be merged in near future.

> or applying DAMOS schemes. For e.g.
> in pa_stat, folios split across N regions will be counted N times,
> giving inaccurate results.

Nice catch!  I agree this could be a problem.

> Hence, only consider a page for access check/DAMOS scheme only if the head
> page is part of that region as well.

For access checking, this seems wrong to me.  This change will unnecessarily
mark some DAMON regions as not accessed.

Even for DAMOS, this seems not very optimum.

1. DAMOS will unnecessarily repeat PAGE_SIZE skippings on second and later
   DAMON regions of a same large folio.
2. If only a small part of the large folio belongs to the first DAMON region,
   and the DAMON region is not eligible to the scheme, while the second region
   is, the scheme action will not applied to the second region.

Also, I think this problem can happen on vaddr case.  Hence, making the change
on core layer makes sense to me.

That is, let damon_ops->apply_scheme() inform the caller (damos_apply_scheme())
how much bytes of the next region should be ignored at applying the action.
Due to the complicated implementation of DAMOS core logic, this might be not
very simple.

I think the additional complexity might outweigh the benefit for following
reasons.  First, adaptive regions adjustment mechanism will make DAMON regions
in same large folios to be merged soon.  So the problem will be not significant
in common case.  Second, any threads could collapse any parts of memory into
large folios while DAMOS is traversing DAMON regions.  So this problem cannot
perfectly be solved unless we make DAMOS' DAMON regions traversal exclusive
with any large folios collapsing.  Which would add more complexity.

And given DAMON's best-effort nature, keeping the issue until we get a simple
and effective solution is ok to me, unless a real significant issue from this
is confirmed.

Usama, what do you think?


Thanks,
SJ

[...]

Re: [PATCH v4 2/6] mm/damon/paddr: use damon_get_folio_in_region to obtain folio

[Usama Arif]

On 04/02/2025 23:06, SeongJae Park wrote:
> On Mon,  3 Feb 2025 22:55:29 +0000 Usama Arif <usamaarif642@gmail.com> wrote:
> 
>> This is introduced for larger folios. If a large folio has subpages
>> present in multiple regions, it will be considered multiple times.
>> This can be when checking access
> 
> I think this behavior makes sense.  If a 2 MiB address range is known to be
> accessed or not, treating DAMON regions in the address range as all accessed or
> not aligns with the DAMON's region definition and makes sense in general, to
> me.  Note that DAMON's adaptive regions adjustment mechanism will make the
> DAMON regions in a large folio to be merged in near future.
> 
>> or applying DAMOS schemes. For e.g.
>> in pa_stat, folios split across N regions will be counted N times,
>> giving inaccurate results.
> 
> Nice catch!  I agree this could be a problem.
> 
>> Hence, only consider a page for access check/DAMOS scheme only if the head
>> page is part of that region as well.
> 
> For access checking, this seems wrong to me.  This change will unnecessarily
> mark some DAMON regions as not accessed.
> 
> Even for DAMOS, this seems not very optimum.
> 
> 1. DAMOS will unnecessarily repeat PAGE_SIZE skippings on second and later
>    DAMON regions of a same large folio.

Hi SJ,

Thanks for the reviews! Just a small point on 1. below, but most is addressed at
the end.

If needed, we could add another arg in damon_get_folio_in_region to return the size
of folio if head page was not part of the region to skip by that size instead of
PAGE_SIZE.

> 2. If only a small part of the large folio belongs to the first DAMON region,
>    and the DAMON region is not eligible to the scheme, while the second region
>    is, the scheme action will not applied to the second region.
>
> Also, I think this problem can happen on vaddr case.  Hence, making the change
> on core layer makes sense to me.
> 
> That is, let damon_ops->apply_scheme() inform the caller (damos_apply_scheme())
> how much bytes of the next region should be ignored at applying the action.
> Due to the complicated implementation of DAMOS core logic, this might be not
> very simple.
> 
> I think the additional complexity might outweigh the benefit for following
> reasons.  First, adaptive regions adjustment mechanism will make DAMON regions
> in same large folios to be merged soon.  So the problem will be not significant
> in common case.  Second, any threads could collapse any parts of memory into
> large folios while DAMOS is traversing DAMON regions.  So this problem cannot
> perfectly be solved unless we make DAMOS' DAMON regions traversal exclusive
> with any large folios collapsing.  Which would add more complexity.
> 
> And given DAMON's best-effort nature, keeping the issue until we get a simple
> and effective solution is ok to me, unless a real significant issue from this
> is confirmed.
> 
> Usama, what do you think?
> 
> 

So the reason I added this patch was when testing out the series on a VM that was only
running a C program continuously accessing 200M hugepages, DAMON was reporting more
than 200M of hugepages everytime, it was usually around 206-208M,
but sometimes quite high. I have added an example at the end showing 266M of hugepages,
which is more than 25% off.
Since this is page level attributes and not sampling based, I believe we should report
accurate results everytime, even at the start.

I think it makes sense when you say that adaptive region adjustment will make the regions
converge, but without this patch even after 20 minutes of running, I couldn't get 200M.
With this patch, I get 200M everytime.

I agree with the points you raised and I thought of the same when trying to come up with
this patch. The reason I did in both access check and DAMOS was I thought its best to have
consistency throughout on how folios in damon regions are treated. I think the solution
for this is probably some compromise between simplicity and accuracy.

I think we have 3 ways forward?
1. Try and improve current patch, where we ignore the folio if the head page is not part of
   the region, for both access check and DAMOS. The way we treat large folios will then be
   consistent for all cases, but there are other issues which you highlighted above.
2. Only do the approach in this patch for DAMOS schemes, i.e. use damon_get_folio_in_region
   only for damon_pa_pageout/mark_accessed_or_deactivate/migrate/stat.
   We dont do it for damon_pa_mkold/young.
3. Only do this approach for pa_stat.

My preference is going forward with option 2, as we wont do pageout/migration/etc repeatedly
to the same folio, but I think option 3 is fine as well, so that we atleast get the right
stats. Let me know what you think is the best way forward?


[root@vm4 src]# cat /proc/meminfo | grep -i anonhuge
AnonHugePages:    204800 kB
[root@vm4 src]# ./run.sh 
heatmap: 44444444888889888887333333332222000000000000000000000000000000000000000000000000
# min/max temperatures: -1,610,000,000, 366,877, column size: 25.586 MiB
# damos filters (df): reject none hugepage
0   addr 1024.000 KiB size 203.875 MiB access 0 %   age 7.500 s       df-passed 0 B
1   addr 204.875 MiB  size 5.949 MiB   access 0 %   age 200 ms        df-passed 0 B
2   addr 210.824 MiB  size 16.062 MiB  access 0 %   age 0 ns          df-passed 0 B
3   addr 226.887 MiB  size 37.480 MiB  access 0 %   age 0 ns          df-passed 18.000 MiB
4   addr 264.367 MiB  size 10.316 MiB  access 15 %  age 0 ns          df-passed 12.000 MiB
5   addr 274.684 MiB  size 4.426 MiB   access 10 %  age 0 ns          df-passed 6.000 MiB
6   addr 279.109 MiB  size 5.688 MiB   access 15 %  age 0 ns          df-passed 6.000 MiB
7   addr 284.797 MiB  size 648.000 KiB access 15 %  age 0 ns          df-passed 2.000 MiB
8   addr 285.430 MiB  size 56.000 KiB  access 10 %  age 100 ms        df-passed 2.000 MiB
9   addr 285.484 MiB  size 504.000 KiB access 10 %  age 100 ms        df-passed 2.000 MiB
10  addr 285.977 MiB  size 844.000 KiB access 5 %   age 0 ns          df-passed 2.000 MiB
11  addr 286.801 MiB  size 3.305 MiB   access 5 %   age 0 ns          df-passed 4.000 MiB
12  addr 290.105 MiB  size 6.281 MiB   access 0 %   age 100 ms        df-passed 8.000 MiB
13  addr 296.387 MiB  size 6.285 MiB   access 0 %   age 100 ms        df-passed 8.000 MiB
14  addr 302.672 MiB  size 1.211 MiB   access 5 %   age 0 ns          df-passed 2.000 MiB
15  addr 303.883 MiB  size 1.211 MiB   access 5 %   age 0 ns          df-passed 2.000 MiB
16  addr 305.094 MiB  size 840.000 KiB access 0 %   age 0 ns          df-passed 2.000 MiB
17  addr 305.914 MiB  size 364.000 KiB access 0 %   age 0 ns          df-passed 2.000 MiB
18  addr 306.270 MiB  size 844.000 KiB access 10 %  age 0 ns          df-passed 2.000 MiB
19  addr 307.094 MiB  size 364.000 KiB access 10 %  age 0 ns          df-passed 2.000 MiB
20  addr 307.449 MiB  size 8.484 MiB   access 5 %   age 0 ns          df-passed 10.000 MiB
21  addr 315.934 MiB  size 8.488 MiB   access 5 %   age 0 ns          df-passed 10.000 MiB
22  addr 324.422 MiB  size 772.000 KiB access 0 %   age 100 ms        df-passed 2.000 MiB
23  addr 325.176 MiB  size 1.133 MiB   access 0 %   age 100 ms        df-passed 2.000 MiB
24  addr 326.309 MiB  size 224.000 KiB access 0 %   age 0 ns          df-passed 2.000 MiB
25  addr 326.527 MiB  size 900.000 KiB access 0 %   age 0 ns          df-passed 2.000 MiB
26  addr 327.406 MiB  size 3.961 MiB   access 0 %   age 0 ns          df-passed 4.000 MiB
27  addr 331.367 MiB  size 5.945 MiB   access 0 %   age 0 ns          df-passed 6.000 MiB
28  addr 337.312 MiB  size 1.867 MiB   access 5 %   age 0 ns          df-passed 2.000 MiB
29  addr 339.180 MiB  size 824.000 KiB access 10 %  age 0 ns          df-passed 2.000 MiB
30  addr 339.984 MiB  size 548.000 KiB access 0 %   age 0 ns          df-passed 2.000 MiB
31  addr 340.520 MiB  size 2.141 MiB   access 0 %   age 0 ns          df-passed 4.000 MiB
32  addr 342.660 MiB  size 2.617 MiB   access 0 %   age 0 ns          df-passed 4.000 MiB
33  addr 345.277 MiB  size 1.125 MiB   access 0 %   age 0 ns          df-passed 2.000 MiB
34  addr 346.402 MiB  size 984.000 KiB access 5 %   age 0 ns          df-passed 2.000 MiB
35  addr 347.363 MiB  size 660.000 KiB access 5 %   age 0 ns          df-passed 2.000 MiB
36  addr 348.008 MiB  size 768.000 KiB access 15 %  age 100 ms        df-passed 2.000 MiB
37  addr 348.758 MiB  size 192.000 KiB access 15 %  age 100 ms        df-passed 2.000 MiB
38  addr 348.945 MiB  size 1.098 MiB   access 5 %   age 0 ns          df-passed 2.000 MiB
39  addr 350.043 MiB  size 1.102 MiB   access 5 %   age 0 ns          df-passed 2.000 MiB
40  addr 351.145 MiB  size 768.000 KiB access 0 %   age 0 ns          df-passed 2.000 MiB
41  addr 351.895 MiB  size 516.000 KiB access 0 %   age 0 ns          df-passed 2.000 MiB
42  addr 352.398 MiB  size 2.258 MiB   access 10 %  age 0 ns          df-passed 4.000 MiB
43  addr 354.656 MiB  size 9.039 MiB   access 10 %  age 0 ns          df-passed 10.000 MiB
44  addr 363.695 MiB  size 38.391 MiB  access 0 %   age 0 ns          df-passed 40.000 MiB
45  addr 402.086 MiB  size 16.457 MiB  access 0 %   age 0 ns          df-passed 18.000 MiB
46  addr 418.543 MiB  size 2.168 MiB   access 5 %   age 0 ns          df-passed 4.000 MiB
47  addr 420.711 MiB  size 3.258 MiB   access 5 %   age 0 ns          df-passed 4.000 MiB
48  addr 423.969 MiB  size 124.000 KiB access 10 %  age 100 ms        df-passed 2.000 MiB
49  addr 424.090 MiB  size 32.000 KiB  access 10 %  age 100 ms        df-passed 2.000 MiB
50  addr 424.121 MiB  size 1.469 MiB   access 5 %   age 0 ns          df-passed 2.000 MiB
51  addr 425.590 MiB  size 380.000 KiB access 5 %   age 0 ns          df-passed 2.000 MiB
52  addr 425.961 MiB  size 752.000 KiB access 10 %  age 0 ns          df-passed 2.000 MiB
53  addr 426.695 MiB  size 1.109 MiB   access 10 %  age 0 ns          df-passed 2.000 MiB
54  addr 427.805 MiB  size 54.031 MiB  access 0 %   age 0 ns          df-passed 22.000 MiB
55  addr 481.836 MiB  size 23.160 MiB  access 0 %   age 0 ns          df-passed 0 B
56  addr 504.996 MiB  size 194.918 MiB access 0 %   age 9 s           df-passed 0 B
57  addr 699.914 MiB  size 118.688 MiB access 0 %   age 11.500 s      df-passed 0 B
58  addr 818.602 MiB  size 199.594 MiB access 0 %   age 15.300 s      df-passed 0 B
59  addr 1018.195 MiB size 1.006 GiB   access 0 %   age 16.100 s      df-passed 0 B
total size: 1.999 GiB  df-passed: 266.000 MiB
[root@vm4 src]# cat /proc/meminfo | grep -i anonhuge
AnonHugePages:    204800 kB

Re: [PATCH v4 2/6] mm/damon/paddr: use damon_get_folio_in_region to obtain folio

[SeongJae Park]

On Wed, 5 Feb 2025 12:46:39 +0000 Usama Arif <usamaarif642@gmail.com> wrote:

> 
> 
> On 04/02/2025 23:06, SeongJae Park wrote:
> > On Mon,  3 Feb 2025 22:55:29 +0000 Usama Arif <usamaarif642@gmail.com> wrote:
> > 
> >> This is introduced for larger folios. If a large folio has subpages
> >> present in multiple regions, it will be considered multiple times.
> >> This can be when checking access
> > 
> > I think this behavior makes sense.  If a 2 MiB address range is known to be
> > accessed or not, treating DAMON regions in the address range as all accessed or
> > not aligns with the DAMON's region definition and makes sense in general, to
> > me.  Note that DAMON's adaptive regions adjustment mechanism will make the
> > DAMON regions in a large folio to be merged in near future.
> > 
> >> or applying DAMOS schemes. For e.g.
> >> in pa_stat, folios split across N regions will be counted N times,
> >> giving inaccurate results.
> > 
> > Nice catch!  I agree this could be a problem.
> > 
> >> Hence, only consider a page for access check/DAMOS scheme only if the head
> >> page is part of that region as well.
> > 
> > For access checking, this seems wrong to me.  This change will unnecessarily
> > mark some DAMON regions as not accessed.
> > 
> > Even for DAMOS, this seems not very optimum.
> > 
> > 1. DAMOS will unnecessarily repeat PAGE_SIZE skippings on second and later
> >    DAMON regions of a same large folio.
> 
> Hi SJ,
> 
> Thanks for the reviews! Just a small point on 1. below, but most is addressed at
> the end.
> 
> If needed, we could add another arg in damon_get_folio_in_region to return the size
> of folio if head page was not part of the region to skip by that size instead of
> PAGE_SIZE.
> 
> > 2. If only a small part of the large folio belongs to the first DAMON region,
> >    and the DAMON region is not eligible to the scheme, while the second region
> >    is, the scheme action will not applied to the second region.
> >
> > Also, I think this problem can happen on vaddr case.  Hence, making the change
> > on core layer makes sense to me.
> > 
> > That is, let damon_ops->apply_scheme() inform the caller (damos_apply_scheme())
> > how much bytes of the next region should be ignored at applying the action.
> > Due to the complicated implementation of DAMOS core logic, this might be not
> > very simple.
> > 
> > I think the additional complexity might outweigh the benefit for following
> > reasons.  First, adaptive regions adjustment mechanism will make DAMON regions
> > in same large folios to be merged soon.  So the problem will be not significant
> > in common case.  Second, any threads could collapse any parts of memory into
> > large folios while DAMOS is traversing DAMON regions.  So this problem cannot
> > perfectly be solved unless we make DAMOS' DAMON regions traversal exclusive
> > with any large folios collapsing.  Which would add more complexity.
> > 
> > And given DAMON's best-effort nature, keeping the issue until we get a simple
> > and effective solution is ok to me, unless a real significant issue from this
> > is confirmed.
> > 
> > Usama, what do you think?
> > 
> > 
> 
> So the reason I added this patch was when testing out the series on a VM that was only
> running a C program continuously accessing 200M hugepages, DAMON was reporting more
> than 200M of hugepages everytime, it was usually around 206-208M,
> but sometimes quite high. I have added an example at the end showing 266M of hugepages,
> which is more than 25% off.
> Since this is page level attributes and not sampling based, I believe we should report
> accurate results everytime, even at the start.

Thank you for kindly sharing the problem you show in detail.  I agree your
points, and now I believe this is definitely better to be more accurate.

> 
> I think it makes sense when you say that adaptive region adjustment will make the regions
> converge, but without this patch even after 20 minutes of running, I couldn't get 200M.

I think that's due to the random regions split mechanism, and it shows we have
rooms to improve the adaptive regions adjustment mechanism.  But as you
mentioned with page level monitoring's expectation, the hugepage handling also
deserves its own enhancement.

> With this patch, I get 200M everytime.

Great!

> 
> I agree with the points you raised and I thought of the same when trying to come up with
> this patch. The reason I did in both access check and DAMOS was I thought its best to have
> consistency throughout on how folios in damon regions are treated.

You're right.  I agree the consistency is important.  Nevertheless, the
monitoring behavior looks sane to me.  The core layer is asking if a two bytes
of a huge page which arises in different DAMON regions are accessed or not.
Regardless of if the two bytes are in same DAMON rgion or not, from 'paddr's
perspective, those are both accessed or unaccessed together.  Saying so makes
no problem to me.

For DAMOS, this is definitely a problem, though.

> I think the solution
> for this is probably some compromise between simplicity and accuracy.
> 
> I think we have 3 ways forward?
> 1. Try and improve current patch, where we ignore the folio if the head page is not part of
>    the region, for both access check and DAMOS. The way we treat large folios will then be
>    consistent for all cases, but there are other issues which you highlighted above.
> 2. Only do the approach in this patch for DAMOS schemes, i.e. use damon_get_folio_in_region
>    only for damon_pa_pageout/mark_accessed_or_deactivate/migrate/stat.
>    We dont do it for damon_pa_mkold/young.
> 3. Only do this approach for pa_stat.
> 
> My preference is going forward with option 2, as we wont do pageout/migration/etc repeatedly
> to the same folio, but I think option 3 is fine as well, so that we atleast get the right
> stats. Let me know what you think is the best way forward?

I also think option 2 is the best among these.  But, I'm not really sure if the
behavior is correct.  I listed two problematic case at applying this approach
for DAMOS.  You suggeested a practical way to mitigate the first case
(unnecessary 4K length skips), but I think the second case is still not solved.

So I think the core-ops collaborative solution I mentioned in the previous
reply makes more sense: "That is, let damon_ops->apply_scheme() inform the
caller (damos_apply_scheme()) how much bytes of the next region should be
ignored at applying the action."

I thought it is too much complexity, but your kind sharing of your case changed
my mind.  If you don't mind, I'd like to make and post that change.  May I?

I think this problem is definitenly need to be better handled, but I still
don't think this is a blocke for the rest of this patch series.  Pleease feel
free to post next spin whenever you want.


Thanks,
SJ

[...]

[PATCH v4 3/6] mm/damon/sysfs-schemes: add files for setting damos_filter->folio_size

[Usama Arif]

Add min and max files for damon filters to let the userspace decide
the min/max folio size to operate on. This will be needed to decide
what folio sizes to give pa_stat for.

Signed-off-by: Usama Arif <usamaarif642@gmail.com>
---
 include/linux/damon.h    | 11 +++++++++
 mm/damon/core.c          |  3 +++
 mm/damon/sysfs-schemes.c | 53 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+)

diff --git a/include/linux/damon.h b/include/linux/damon.h
index af525252b853..6f30ceeff215 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -35,6 +35,16 @@ struct damon_addr_range {
 	unsigned long end;
 };
 
+/**
+ * struct damon_folio_size - Represents size of folio filter on [@min, @max].
+ * @min:	Min size of the folio (inclusive).
+ * @max:	Max size of the folio (inclusive).
+ */
+struct damon_folio_size {
+	unsigned long min;
+	unsigned long max;
+};
+
 /**
  * struct damon_region - Represents a monitoring target region.
  * @ar:			The address range of the region.
@@ -377,6 +387,7 @@ struct damos_filter {
 		struct damon_addr_range addr_range;
 		int target_idx;
 	};
+	struct damon_folio_size folio_size;
 	struct list_head list;
 };
 
diff --git a/mm/damon/core.c b/mm/damon/core.c
index c7b981308862..27323e3a800d 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -776,6 +776,9 @@ static void damos_commit_filter_arg(
 	case DAMOS_FILTER_TYPE_TARGET:
 		dst->target_idx = src->target_idx;
 		break;
+	case DAMOS_FILTER_TYPE_HUGEPAGE:
+		dst->folio_size = src->folio_size;
+		break;
 	default:
 		break;
 	}
diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
index 98f93ae9f59e..bc7ca43ca9c4 100644
--- a/mm/damon/sysfs-schemes.c
+++ b/mm/damon/sysfs-schemes.c
@@ -316,6 +316,7 @@ struct damon_sysfs_scheme_filter {
 	bool allow;
 	char *memcg_path;
 	struct damon_addr_range addr_range;
+	struct damon_folio_size folio_size;
 	int target_idx;
 };
 
@@ -469,6 +470,43 @@ static ssize_t addr_end_store(struct kobject *kobj,
 	struct damon_sysfs_scheme_filter *filter = container_of(kobj,
 			struct damon_sysfs_scheme_filter, kobj);
 	int err = kstrtoul(buf, 0, &filter->addr_range.end);
+	return err ? err : count;
+}
+
+static ssize_t min_show(struct kobject *kobj,
+		struct kobj_attribute *attr, char *buf)
+{
+	struct damon_sysfs_scheme_filter *filter = container_of(kobj,
+			struct damon_sysfs_scheme_filter, kobj);
+
+	return sysfs_emit(buf, "%lu\n", filter->folio_size.min);
+}
+
+static ssize_t min_store(struct kobject *kobj,
+		struct kobj_attribute *attr, const char *buf, size_t count)
+{
+	struct damon_sysfs_scheme_filter *filter = container_of(kobj,
+			struct damon_sysfs_scheme_filter, kobj);
+	int err = kstrtoul(buf, 0, &filter->folio_size.min);
+
+	return err ? err : count;
+}
+
+static ssize_t max_show(struct kobject *kobj,
+		struct kobj_attribute *attr, char *buf)
+{
+	struct damon_sysfs_scheme_filter *filter = container_of(kobj,
+			struct damon_sysfs_scheme_filter, kobj);
+
+	return sysfs_emit(buf, "%lu\n", filter->folio_size.max);
+}
+
+static ssize_t max_store(struct kobject *kobj,
+		struct kobj_attribute *attr, const char *buf, size_t count)
+{
+	struct damon_sysfs_scheme_filter *filter = container_of(kobj,
+			struct damon_sysfs_scheme_filter, kobj);
+	int err = kstrtoul(buf, 0, &filter->folio_size.max);
 
 	return err ? err : count;
 }
@@ -519,6 +557,12 @@ static struct kobj_attribute damon_sysfs_scheme_filter_addr_start_attr =
 static struct kobj_attribute damon_sysfs_scheme_filter_addr_end_attr =
 		__ATTR_RW_MODE(addr_end, 0600);
 
+static struct kobj_attribute damon_sysfs_scheme_filter_min_attr =
+		__ATTR_RW_MODE(min, 0600);
+
+static struct kobj_attribute damon_sysfs_scheme_filter_max_attr =
+		__ATTR_RW_MODE(max, 0600);
+
 static struct kobj_attribute damon_sysfs_scheme_filter_damon_target_idx_attr =
 		__ATTR_RW_MODE(damon_target_idx, 0600);
 
@@ -529,6 +573,8 @@ static struct attribute *damon_sysfs_scheme_filter_attrs[] = {
 	&damon_sysfs_scheme_filter_memcg_path_attr.attr,
 	&damon_sysfs_scheme_filter_addr_start_attr.attr,
 	&damon_sysfs_scheme_filter_addr_end_attr.attr,
+	&damon_sysfs_scheme_filter_min_attr.attr,
+	&damon_sysfs_scheme_filter_max_attr.attr,
 	&damon_sysfs_scheme_filter_damon_target_idx_attr.attr,
 	NULL,
 };
@@ -1953,6 +1999,13 @@ static int damon_sysfs_add_scheme_filters(struct damos *scheme,
 			filter->addr_range = sysfs_filter->addr_range;
 		} else if (filter->type == DAMOS_FILTER_TYPE_TARGET) {
 			filter->target_idx = sysfs_filter->target_idx;
+		} else if (filter->type == DAMOS_FILTER_TYPE_HUGEPAGE) {
+			if (sysfs_filter->folio_size.min >
+					sysfs_filter->folio_size.max) {
+				damos_destroy_filter(filter);
+				return -EINVAL;
+			}
+			filter->folio_size = sysfs_filter->folio_size;
 		}
 
 		damos_add_filter(scheme, filter);
-- 
2.43.5

Re: [PATCH v4 3/6] mm/damon/sysfs-schemes: add files for setting damos_filter->folio_size

[SeongJae Park]

On Mon,  3 Feb 2025 22:55:30 +0000 Usama Arif <usamaarif642@gmail.com> wrote:

> Add min and max files for damon filters to let the userspace decide
> the min/max folio size to operate on. This will be needed to decide
> what folio sizes to give pa_stat for.

I'd prefer implementing the logic with API interface first, and then
implementing sysfs interface on top of the API.

> 
> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
> ---
>  include/linux/damon.h    | 11 +++++++++
>  mm/damon/core.c          |  3 +++
>  mm/damon/sysfs-schemes.c | 53 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 67 insertions(+)
> 
> diff --git a/include/linux/damon.h b/include/linux/damon.h
> index af525252b853..6f30ceeff215 100644
> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
> @@ -35,6 +35,16 @@ struct damon_addr_range {
>  	unsigned long end;
>  };
>  
> +/**
> + * struct damon_folio_size - Represents size of folio filter on [@min, @max].
> + * @min:	Min size of the folio (inclusive).
> + * @max:	Max size of the folio (inclusive).
> + */
> +struct damon_folio_size {
> +	unsigned long min;
> +	unsigned long max;
> +};
> +

I'd suggest giving a more general name, say, damon_size_range, so that we can
reuse this for any possible future size range based filters or whatever.

>  /**
>   * struct damon_region - Represents a monitoring target region.
>   * @ar:			The address range of the region.
> @@ -377,6 +387,7 @@ struct damos_filter {
>  		struct damon_addr_range addr_range;
>  		int target_idx;
>  	};
> +	struct damon_folio_size folio_size;

As this will be used depending on damos_filter->type, let's add this field in
the above union, together with addr_range and target_idx.

Also, I'd prefer calling this hugepage_size, as we discussed on the previous
version of this patch series.

>  	struct list_head list;
>  };
>  
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index c7b981308862..27323e3a800d 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -776,6 +776,9 @@ static void damos_commit_filter_arg(
>  	case DAMOS_FILTER_TYPE_TARGET:
>  		dst->target_idx = src->target_idx;
>  		break;
> +	case DAMOS_FILTER_TYPE_HUGEPAGE:
> +		dst->folio_size = src->folio_size;
> +		break;
>  	default:
>  		break;
>  	}
> diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> index 98f93ae9f59e..bc7ca43ca9c4 100644
> --- a/mm/damon/sysfs-schemes.c
> +++ b/mm/damon/sysfs-schemes.c
> @@ -316,6 +316,7 @@ struct damon_sysfs_scheme_filter {
>  	bool allow;
>  	char *memcg_path;
>  	struct damon_addr_range addr_range;
> +	struct damon_folio_size folio_size;

Again, I'd prefer calling this hugepage_size.

>  	int target_idx;
>  };
>  
> @@ -469,6 +470,43 @@ static ssize_t addr_end_store(struct kobject *kobj,
>  	struct damon_sysfs_scheme_filter *filter = container_of(kobj,
>  			struct damon_sysfs_scheme_filter, kobj);
>  	int err = kstrtoul(buf, 0, &filter->addr_range.end);
> +	return err ? err : count;
> +}

Let's keep the blank line before 'return' so that this is not unnecessarily
marked as a diff.

> +
> +static ssize_t min_show(struct kobject *kobj,
> +		struct kobj_attribute *attr, char *buf)
> +{
> +	struct damon_sysfs_scheme_filter *filter = container_of(kobj,
> +			struct damon_sysfs_scheme_filter, kobj);
> +
> +	return sysfs_emit(buf, "%lu\n", filter->folio_size.min);
> +}
> +
> +static ssize_t min_store(struct kobject *kobj,
> +		struct kobj_attribute *attr, const char *buf, size_t count)
> +{
> +	struct damon_sysfs_scheme_filter *filter = container_of(kobj,
> +			struct damon_sysfs_scheme_filter, kobj);
> +	int err = kstrtoul(buf, 0, &filter->folio_size.min);
> +
> +	return err ? err : count;
> +}
> +
> +static ssize_t max_show(struct kobject *kobj,
> +		struct kobj_attribute *attr, char *buf)
> +{
> +	struct damon_sysfs_scheme_filter *filter = container_of(kobj,
> +			struct damon_sysfs_scheme_filter, kobj);
> +
> +	return sysfs_emit(buf, "%lu\n", filter->folio_size.max);
> +}
> +
> +static ssize_t max_store(struct kobject *kobj,
> +		struct kobj_attribute *attr, const char *buf, size_t count)
> +{
> +	struct damon_sysfs_scheme_filter *filter = container_of(kobj,
> +			struct damon_sysfs_scheme_filter, kobj);
> +	int err = kstrtoul(buf, 0, &filter->folio_size.max);
>  
>  	return err ? err : count;
>  }
> @@ -519,6 +557,12 @@ static struct kobj_attribute damon_sysfs_scheme_filter_addr_start_attr =
>  static struct kobj_attribute damon_sysfs_scheme_filter_addr_end_attr =
>  		__ATTR_RW_MODE(addr_end, 0600);
>  
> +static struct kobj_attribute damon_sysfs_scheme_filter_min_attr =
> +		__ATTR_RW_MODE(min, 0600);
> +
> +static struct kobj_attribute damon_sysfs_scheme_filter_max_attr =
> +		__ATTR_RW_MODE(max, 0600);
> +
>  static struct kobj_attribute damon_sysfs_scheme_filter_damon_target_idx_attr =
>  		__ATTR_RW_MODE(damon_target_idx, 0600);
>  
> @@ -529,6 +573,8 @@ static struct attribute *damon_sysfs_scheme_filter_attrs[] = {
>  	&damon_sysfs_scheme_filter_memcg_path_attr.attr,
>  	&damon_sysfs_scheme_filter_addr_start_attr.attr,
>  	&damon_sysfs_scheme_filter_addr_end_attr.attr,
> +	&damon_sysfs_scheme_filter_min_attr.attr,
> +	&damon_sysfs_scheme_filter_max_attr.attr,
>  	&damon_sysfs_scheme_filter_damon_target_idx_attr.attr,
>  	NULL,
>  };
> @@ -1953,6 +1999,13 @@ static int damon_sysfs_add_scheme_filters(struct damos *scheme,
>  			filter->addr_range = sysfs_filter->addr_range;
>  		} else if (filter->type == DAMOS_FILTER_TYPE_TARGET) {
>  			filter->target_idx = sysfs_filter->target_idx;
> +		} else if (filter->type == DAMOS_FILTER_TYPE_HUGEPAGE) {
> +			if (sysfs_filter->folio_size.min >
> +					sysfs_filter->folio_size.max) {
> +				damos_destroy_filter(filter);
> +				return -EINVAL;
> +			}

I don't think letting users set invalid min/max is a real problem, as long as
the implementation will handle the case (no memory matches the filter).  Let's
just allow it, like addr_range case.  If we really need to disallow this, it
would better to do that from damos_commit_filter() like central point.

> +			filter->folio_size = sysfs_filter->folio_size;
>  		}
>  
>  		damos_add_filter(scheme, filter);
> -- 
> 2.43.5


Thanks,
SJ

Re: [PATCH v4 3/6] mm/damon/sysfs-schemes: add files for setting damos_filter->folio_size

[Usama Arif]

On 04/02/2025 23:10, SeongJae Park wrote:
> On Mon,  3 Feb 2025 22:55:30 +0000 Usama Arif <usamaarif642@gmail.com> wrote:
> 
>> Add min and max files for damon filters to let the userspace decide
>> the min/max folio size to operate on. This will be needed to decide
>> what folio sizes to give pa_stat for.
> 
> I'd prefer implementing the logic with API interface first, and then
> implementing sysfs interface on top of the API.
> 
>>
>> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
>> ---
>>  include/linux/damon.h    | 11 +++++++++
>>  mm/damon/core.c          |  3 +++
>>  mm/damon/sysfs-schemes.c | 53 ++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 67 insertions(+)
>>
>> diff --git a/include/linux/damon.h b/include/linux/damon.h
>> index af525252b853..6f30ceeff215 100644
>> --- a/include/linux/damon.h
>> +++ b/include/linux/damon.h
>> @@ -35,6 +35,16 @@ struct damon_addr_range {
>>  	unsigned long end;
>>  };
>>  
>> +/**
>> + * struct damon_folio_size - Represents size of folio filter on [@min, @max].
>> + * @min:	Min size of the folio (inclusive).
>> + * @max:	Max size of the folio (inclusive).
>> + */
>> +struct damon_folio_size {
>> +	unsigned long min;
>> +	unsigned long max;
>> +};
>> +
> 
> I'd suggest giving a more general name, say, damon_size_range, so that we can
> reuse this for any possible future size range based filters or whatever.
> 
>>  /**
>>   * struct damon_region - Represents a monitoring target region.
>>   * @ar:			The address range of the region.
>> @@ -377,6 +387,7 @@ struct damos_filter {
>>  		struct damon_addr_range addr_range;
>>  		int target_idx;
>>  	};
>> +	struct damon_folio_size folio_size;
> 
> As this will be used depending on damos_filter->type, let's add this field in
> the above union, together with addr_range and target_idx.
> 
> Also, I'd prefer calling this hugepage_size, as we discussed on the previous
> version of this patch series.
> 
>>  	struct list_head list;
>>  };
>>  
>> diff --git a/mm/damon/core.c b/mm/damon/core.c
>> index c7b981308862..27323e3a800d 100644
>> --- a/mm/damon/core.c
>> +++ b/mm/damon/core.c
>> @@ -776,6 +776,9 @@ static void damos_commit_filter_arg(
>>  	case DAMOS_FILTER_TYPE_TARGET:
>>  		dst->target_idx = src->target_idx;
>>  		break;
>> +	case DAMOS_FILTER_TYPE_HUGEPAGE:
>> +		dst->folio_size = src->folio_size;
>> +		break;
>>  	default:
>>  		break;
>>  	}
>> diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
>> index 98f93ae9f59e..bc7ca43ca9c4 100644
>> --- a/mm/damon/sysfs-schemes.c
>> +++ b/mm/damon/sysfs-schemes.c
>> @@ -316,6 +316,7 @@ struct damon_sysfs_scheme_filter {
>>  	bool allow;
>>  	char *memcg_path;
>>  	struct damon_addr_range addr_range;
>> +	struct damon_folio_size folio_size;
> 
> Again, I'd prefer calling this hugepage_size.
> 
>>  	int target_idx;
>>  };
>>  
>> @@ -469,6 +470,43 @@ static ssize_t addr_end_store(struct kobject *kobj,
>>  	struct damon_sysfs_scheme_filter *filter = container_of(kobj,
>>  			struct damon_sysfs_scheme_filter, kobj);
>>  	int err = kstrtoul(buf, 0, &filter->addr_range.end);
>> +	return err ? err : count;
>> +}
> 
> Let's keep the blank line before 'return' so that this is not unnecessarily
> marked as a diff.
> 
>> +
>> +static ssize_t min_show(struct kobject *kobj,
>> +		struct kobj_attribute *attr, char *buf)
>> +{
>> +	struct damon_sysfs_scheme_filter *filter = container_of(kobj,
>> +			struct damon_sysfs_scheme_filter, kobj);
>> +
>> +	return sysfs_emit(buf, "%lu\n", filter->folio_size.min);
>> +}
>> +
>> +static ssize_t min_store(struct kobject *kobj,
>> +		struct kobj_attribute *attr, const char *buf, size_t count)
>> +{
>> +	struct damon_sysfs_scheme_filter *filter = container_of(kobj,
>> +			struct damon_sysfs_scheme_filter, kobj);
>> +	int err = kstrtoul(buf, 0, &filter->folio_size.min);
>> +
>> +	return err ? err : count;
>> +}
>> +
>> +static ssize_t max_show(struct kobject *kobj,
>> +		struct kobj_attribute *attr, char *buf)
>> +{
>> +	struct damon_sysfs_scheme_filter *filter = container_of(kobj,
>> +			struct damon_sysfs_scheme_filter, kobj);
>> +
>> +	return sysfs_emit(buf, "%lu\n", filter->folio_size.max);
>> +}
>> +
>> +static ssize_t max_store(struct kobject *kobj,
>> +		struct kobj_attribute *attr, const char *buf, size_t count)
>> +{
>> +	struct damon_sysfs_scheme_filter *filter = container_of(kobj,
>> +			struct damon_sysfs_scheme_filter, kobj);
>> +	int err = kstrtoul(buf, 0, &filter->folio_size.max);
>>  
>>  	return err ? err : count;
>>  }
>> @@ -519,6 +557,12 @@ static struct kobj_attribute damon_sysfs_scheme_filter_addr_start_attr =
>>  static struct kobj_attribute damon_sysfs_scheme_filter_addr_end_attr =
>>  		__ATTR_RW_MODE(addr_end, 0600);
>>  
>> +static struct kobj_attribute damon_sysfs_scheme_filter_min_attr =
>> +		__ATTR_RW_MODE(min, 0600);
>> +
>> +static struct kobj_attribute damon_sysfs_scheme_filter_max_attr =
>> +		__ATTR_RW_MODE(max, 0600);
>> +
>>  static struct kobj_attribute damon_sysfs_scheme_filter_damon_target_idx_attr =
>>  		__ATTR_RW_MODE(damon_target_idx, 0600);
>>  
>> @@ -529,6 +573,8 @@ static struct attribute *damon_sysfs_scheme_filter_attrs[] = {
>>  	&damon_sysfs_scheme_filter_memcg_path_attr.attr,
>>  	&damon_sysfs_scheme_filter_addr_start_attr.attr,
>>  	&damon_sysfs_scheme_filter_addr_end_attr.attr,
>> +	&damon_sysfs_scheme_filter_min_attr.attr,
>> +	&damon_sysfs_scheme_filter_max_attr.attr,
>>  	&damon_sysfs_scheme_filter_damon_target_idx_attr.attr,
>>  	NULL,
>>  };
>> @@ -1953,6 +1999,13 @@ static int damon_sysfs_add_scheme_filters(struct damos *scheme,
>>  			filter->addr_range = sysfs_filter->addr_range;
>>  		} else if (filter->type == DAMOS_FILTER_TYPE_TARGET) {
>>  			filter->target_idx = sysfs_filter->target_idx;
>> +		} else if (filter->type == DAMOS_FILTER_TYPE_HUGEPAGE) {
>> +			if (sysfs_filter->folio_size.min >
>> +					sysfs_filter->folio_size.max) {
>> +				damos_destroy_filter(filter);
>> +				return -EINVAL;
>> +			}
> 
> I don't think letting users set invalid min/max is a real problem, as long as
> the implementation will handle the case (no memory matches the filter).  Let's
> just allow it, like addr_range case.  If we really need to disallow this, it
> would better to do that from damos_commit_filter() like central point.
> 

hmm, does addr_range allow it? it addr_range.end < addr_range.start it returns an error.
I did it in a similar fashion for hugepage filter if min > max it returns an error.


Full code of damon_sysfs_add_scheme_filters below:

		} else if (filter->type == DAMOS_FILTER_TYPE_ADDR) {
			if (sysfs_filter->addr_range.end <
					sysfs_filter->addr_range.start) {
				damos_destroy_filter(filter);
				return -EINVAL;
			}
			filter->addr_range = sysfs_filter->addr_range;
		} else if (filter->type == DAMOS_FILTER_TYPE_TARGET) {
			filter->target_idx = sysfs_filter->target_idx;
		} else if (filter->type == DAMOS_FILTER_TYPE_HUGEPAGE) {
			if (sysfs_filter->folio_size.min >
					sysfs_filter->folio_size.max) {
				damos_destroy_filter(filter);
				return -EINVAL;
			}
			filter->folio_size = sysfs_filter->folio_size;
		}



>> +			filter->folio_size = sysfs_filter->folio_size;
>>  		}
>>  
>>  		damos_add_filter(scheme, filter);
>> -- 
>> 2.43.5
> 
> 
> Thanks,
> SJ

Re: [PATCH v4 3/6] mm/damon/sysfs-schemes: add files for setting damos_filter->folio_size

[SeongJae Park]

On Wed, 5 Feb 2025 13:57:05 +0000 Usama Arif <usamaarif642@gmail.com> wrote:

> 
> 
> On 04/02/2025 23:10, SeongJae Park wrote:
> > On Mon,  3 Feb 2025 22:55:30 +0000 Usama Arif <usamaarif642@gmail.com> wrote:
> > 
> >> Add min and max files for damon filters to let the userspace decide
> >> the min/max folio size to operate on. This will be needed to decide
> >> what folio sizes to give pa_stat for.
> > 
> > I'd prefer implementing the logic with API interface first, and then
> > implementing sysfs interface on top of the API.
> > 
> >>
> >> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
> >> ---
[...]
> >> @@ -1953,6 +1999,13 @@ static int damon_sysfs_add_scheme_filters(struct damos *scheme,
> >>  			filter->addr_range = sysfs_filter->addr_range;
> >>  		} else if (filter->type == DAMOS_FILTER_TYPE_TARGET) {
> >>  			filter->target_idx = sysfs_filter->target_idx;
> >> +		} else if (filter->type == DAMOS_FILTER_TYPE_HUGEPAGE) {
> >> +			if (sysfs_filter->folio_size.min >
> >> +					sysfs_filter->folio_size.max) {
> >> +				damos_destroy_filter(filter);
> >> +				return -EINVAL;
> >> +			}
> > 
> > I don't think letting users set invalid min/max is a real problem, as long as
> > the implementation will handle the case (no memory matches the filter).  Let's
> > just allow it, like addr_range case.  If we really need to disallow this, it
> > would better to do that from damos_commit_filter() like central point.
> > 
> 
> hmm, does addr_range allow it? it addr_range.end < addr_range.start it returns an error.
> I did it in a similar fashion for hugepage filter if min > max it returns an error.

Oh you're right...  Maybe I was just out of mind, sorry.  Please keep this part
as is your version to be consistent with the address range code.


Thanks,
SJ

> 
> 
> Full code of damon_sysfs_add_scheme_filters below:
> 
> 		} else if (filter->type == DAMOS_FILTER_TYPE_ADDR) {
> 			if (sysfs_filter->addr_range.end <
> 					sysfs_filter->addr_range.start) {
> 				damos_destroy_filter(filter);
> 				return -EINVAL;
> 			}
> 			filter->addr_range = sysfs_filter->addr_range;
> 		} else if (filter->type == DAMOS_FILTER_TYPE_TARGET) {
> 			filter->target_idx = sysfs_filter->target_idx;
> 		} else if (filter->type == DAMOS_FILTER_TYPE_HUGEPAGE) {
> 			if (sysfs_filter->folio_size.min >
> 					sysfs_filter->folio_size.max) {
> 				damos_destroy_filter(filter);
> 				return -EINVAL;
> 			}
> 			filter->folio_size = sysfs_filter->folio_size;
> 		}
> 
> 
> 
> >> +			filter->folio_size = sysfs_filter->folio_size;
> >>  		}
> >>  
> >>  		damos_add_filter(scheme, filter);
> >> -- 
> >> 2.43.5
> > 
> > 
> > Thanks,
> > SJ

[PATCH v4 4/6] mm/damon: introduce DAMOS filter type hugepage

[Usama Arif]

This is to gather statistics to check if memory regions of specific
access tempratures are backed by hugepages of a size in a specific
range
This filter can help to observe and prove the effectivenes of
different schemes for shrinking/collapsing hugepages.

Signed-off-by: Usama Arif <usamaarif642@gmail.com>
---
 include/linux/damon.h    | 2 ++
 mm/damon/paddr.c         | 7 +++++++
 mm/damon/sysfs-schemes.c | 1 +
 3 files changed, 10 insertions(+)

diff --git a/include/linux/damon.h b/include/linux/damon.h
index 6f30ceeff215..5ba6c2114e3f 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -336,6 +336,7 @@ struct damos_stat {
  * @DAMOS_FILTER_TYPE_ANON:	Anonymous pages.
  * @DAMOS_FILTER_TYPE_MEMCG:	Specific memcg's pages.
  * @DAMOS_FILTER_TYPE_YOUNG:	Recently accessed pages.
+ * @DAMOS_FILTER_TYPE_HUGEPAGE:	Page is part of a hugepage.
  * @DAMOS_FILTER_TYPE_ADDR:	Address range.
  * @DAMOS_FILTER_TYPE_TARGET:	Data Access Monitoring target.
  * @NR_DAMOS_FILTER_TYPES:	Number of filter types.
@@ -355,6 +356,7 @@ enum damos_filter_type {
 	DAMOS_FILTER_TYPE_ANON,
 	DAMOS_FILTER_TYPE_MEMCG,
 	DAMOS_FILTER_TYPE_YOUNG,
+	DAMOS_FILTER_TYPE_HUGEPAGE,
 	DAMOS_FILTER_TYPE_ADDR,
 	DAMOS_FILTER_TYPE_TARGET,
 	NR_DAMOS_FILTER_TYPES,
diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
index 3f59a3fdc391..34fe1eb664cc 100644
--- a/mm/damon/paddr.c
+++ b/mm/damon/paddr.c
@@ -227,6 +227,7 @@ static bool damos_pa_filter_match(struct damos_filter *filter,
 {
 	bool matched = false;
 	struct mem_cgroup *memcg;
+	size_t folio_sz;
 
 	switch (filter->type) {
 	case DAMOS_FILTER_TYPE_ANON:
@@ -246,6 +247,12 @@ static bool damos_pa_filter_match(struct damos_filter *filter,
 		if (matched)
 			damon_folio_mkold(folio);
 		break;
+#if defined(CONFIG_PGTABLE_HAS_HUGE_LEAVES)
+	case DAMOS_FILTER_TYPE_HUGEPAGE:
+		folio_sz = folio_size(folio);
+		matched = filter->folio_size.min <= folio_sz && folio_sz <= filter->folio_size.max;
+		break;
+#endif
 	default:
 		break;
 	}
diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
index bc7ca43ca9c4..76aee3ab277e 100644
--- a/mm/damon/sysfs-schemes.c
+++ b/mm/damon/sysfs-schemes.c
@@ -330,6 +330,7 @@ static const char * const damon_sysfs_scheme_filter_type_strs[] = {
 	"anon",
 	"memcg",
 	"young",
+	"hugepage",
 	"addr",
 	"target",
 };
-- 
2.43.5

Re: [PATCH v4 4/6] mm/damon: introduce DAMOS filter type hugepage

[SeongJae Park]

On Mon,  3 Feb 2025 22:55:31 +0000 Usama Arif <usamaarif642@gmail.com> wrote:

> This is to gather statistics to check if memory regions of specific
> access tempratures are backed by hugepages of a size in a specific
> range

nit.  A period is missed?

> This filter can help to observe and prove the effectivenes of
> different schemes for shrinking/collapsing hugepages.
> 
> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
> ---
>  include/linux/damon.h    | 2 ++
>  mm/damon/paddr.c         | 7 +++++++
>  mm/damon/sysfs-schemes.c | 1 +
>  3 files changed, 10 insertions(+)
> 
> diff --git a/include/linux/damon.h b/include/linux/damon.h
> index 6f30ceeff215..5ba6c2114e3f 100644
> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
> @@ -336,6 +336,7 @@ struct damos_stat {
>   * @DAMOS_FILTER_TYPE_ANON:	Anonymous pages.
>   * @DAMOS_FILTER_TYPE_MEMCG:	Specific memcg's pages.
>   * @DAMOS_FILTER_TYPE_YOUNG:	Recently accessed pages.
> + * @DAMOS_FILTER_TYPE_HUGEPAGE:	Page is part of a hugepage.

What about "DAMOS_FILTER_TYPE_HUGEPAGE_SIZE: hugepages of a given size range."?

>   * @DAMOS_FILTER_TYPE_ADDR:	Address range.
>   * @DAMOS_FILTER_TYPE_TARGET:	Data Access Monitoring target.
>   * @NR_DAMOS_FILTER_TYPES:	Number of filter types.
> @@ -355,6 +356,7 @@ enum damos_filter_type {
>  	DAMOS_FILTER_TYPE_ANON,
>  	DAMOS_FILTER_TYPE_MEMCG,
>  	DAMOS_FILTER_TYPE_YOUNG,
> +	DAMOS_FILTER_TYPE_HUGEPAGE,
>  	DAMOS_FILTER_TYPE_ADDR,
>  	DAMOS_FILTER_TYPE_TARGET,
>  	NR_DAMOS_FILTER_TYPES,
> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> index 3f59a3fdc391..34fe1eb664cc 100644
> --- a/mm/damon/paddr.c
> +++ b/mm/damon/paddr.c
> @@ -227,6 +227,7 @@ static bool damos_pa_filter_match(struct damos_filter *filter,
>  {
>  	bool matched = false;
>  	struct mem_cgroup *memcg;
> +	size_t folio_sz;
>  
>  	switch (filter->type) {
>  	case DAMOS_FILTER_TYPE_ANON:
> @@ -246,6 +247,12 @@ static bool damos_pa_filter_match(struct damos_filter *filter,
>  		if (matched)
>  			damon_folio_mkold(folio);
>  		break;
> +#if defined(CONFIG_PGTABLE_HAS_HUGE_LEAVES)

I think we don't really need this macro?

> +	case DAMOS_FILTER_TYPE_HUGEPAGE:
> +		folio_sz = folio_size(folio);
> +		matched = filter->folio_size.min <= folio_sz && folio_sz <= filter->folio_size.max;

We should also return 'false' if the folio is not a large folio (folio_sz ==
PAGE_SIZE), if we agreed to my suggestion on the previous version of this patch
series?

I'd also prefer calling filter->folio_siz as sz_range or size_range, so that we
can reuse it for future filter types of size range.

> +		break;
> +#endif
>  	default:
>  		break;
>  	}
> diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> index bc7ca43ca9c4..76aee3ab277e 100644
> --- a/mm/damon/sysfs-schemes.c
> +++ b/mm/damon/sysfs-schemes.c
> @@ -330,6 +330,7 @@ static const char * const damon_sysfs_scheme_filter_type_strs[] = {
>  	"anon",
>  	"memcg",
>  	"young",
> +	"hugepage",

hugepage_size?

>  	"addr",
>  	"target",
>  };
> -- 
> 2.43.5


Thanks,
SJ

Re: [PATCH v4 4/6] mm/damon: introduce DAMOS filter type hugepage

[Usama Arif]

On 04/02/2025 23:12, SeongJae Park wrote:
> On Mon,  3 Feb 2025 22:55:31 +0000 Usama Arif <usamaarif642@gmail.com> wrote:
> 
>> This is to gather statistics to check if memory regions of specific
>> access tempratures are backed by hugepages of a size in a specific
>> range
> 
> nit.  A period is missed?
> 
>> This filter can help to observe and prove the effectivenes of
>> different schemes for shrinking/collapsing hugepages.
>>
>> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
>> ---
>>  include/linux/damon.h    | 2 ++
>>  mm/damon/paddr.c         | 7 +++++++
>>  mm/damon/sysfs-schemes.c | 1 +
>>  3 files changed, 10 insertions(+)
>>
>> diff --git a/include/linux/damon.h b/include/linux/damon.h
>> index 6f30ceeff215..5ba6c2114e3f 100644
>> --- a/include/linux/damon.h
>> +++ b/include/linux/damon.h
>> @@ -336,6 +336,7 @@ struct damos_stat {
>>   * @DAMOS_FILTER_TYPE_ANON:	Anonymous pages.
>>   * @DAMOS_FILTER_TYPE_MEMCG:	Specific memcg's pages.
>>   * @DAMOS_FILTER_TYPE_YOUNG:	Recently accessed pages.
>> + * @DAMOS_FILTER_TYPE_HUGEPAGE:	Page is part of a hugepage.
> 
> What about "DAMOS_FILTER_TYPE_HUGEPAGE_SIZE: hugepages of a given size range."?
> 
>>   * @DAMOS_FILTER_TYPE_ADDR:	Address range.
>>   * @DAMOS_FILTER_TYPE_TARGET:	Data Access Monitoring target.
>>   * @NR_DAMOS_FILTER_TYPES:	Number of filter types.
>> @@ -355,6 +356,7 @@ enum damos_filter_type {
>>  	DAMOS_FILTER_TYPE_ANON,
>>  	DAMOS_FILTER_TYPE_MEMCG,
>>  	DAMOS_FILTER_TYPE_YOUNG,
>> +	DAMOS_FILTER_TYPE_HUGEPAGE,
>>  	DAMOS_FILTER_TYPE_ADDR,
>>  	DAMOS_FILTER_TYPE_TARGET,
>>  	NR_DAMOS_FILTER_TYPES,
>> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
>> index 3f59a3fdc391..34fe1eb664cc 100644
>> --- a/mm/damon/paddr.c
>> +++ b/mm/damon/paddr.c
>> @@ -227,6 +227,7 @@ static bool damos_pa_filter_match(struct damos_filter *filter,
>>  {
>>  	bool matched = false;
>>  	struct mem_cgroup *memcg;
>> +	size_t folio_sz;
>>  
>>  	switch (filter->type) {
>>  	case DAMOS_FILTER_TYPE_ANON:
>> @@ -246,6 +247,12 @@ static bool damos_pa_filter_match(struct damos_filter *filter,
>>  		if (matched)
>>  			damon_folio_mkold(folio);
>>  		break;
>> +#if defined(CONFIG_PGTABLE_HAS_HUGE_LEAVES)
> 
> I think we don't really need this macro?
> 
>> +	case DAMOS_FILTER_TYPE_HUGEPAGE:
>> +		folio_sz = folio_size(folio);
>> +		matched = filter->folio_size.min <= folio_sz && folio_sz <= filter->folio_size.max;
> 
> We should also return 'false' if the folio is not a large folio (folio_sz ==
> PAGE_SIZE), if we agreed to my suggestion on the previous version of this patch
> series?

I think we could let users get PAGE_SIZE as well incase they wanted it? more on it below..

> 
> I'd also prefer calling filter->folio_siz as sz_range or size_range, so that we
> can reuse it for future filter types of size range.
> 
>> +		break;
>> +#endif
>>  	default:
>>  		break;
>>  	}
>> diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
>> index bc7ca43ca9c4..76aee3ab277e 100644
>> --- a/mm/damon/sysfs-schemes.c
>> +++ b/mm/damon/sysfs-schemes.c
>> @@ -330,6 +330,7 @@ static const char * const damon_sysfs_scheme_filter_type_strs[] = {
>>  	"anon",
>>  	"memcg",
>>  	"young",
>> +	"hugepage",
> 
> hugepage_size?
> 

So from https://lore.kernel.org/all/20250121200510.43645-1-sj@kernel.org/, my understanding
was you didn't have a preference between "hugepage" and "hugepage_size"?

I do prefer hugepage over hugepage_size. This is because this is a type of filter, I would
say hugepage_size is not a type, but hugepage is. For e.g. we have anon as type and not 
anon_number. And the "size" part is clear from the min and max which is documented in patch
5 and 6.

TBH my preference would be "folio". What I think we should do is let users input any value
for min and max, as long as max > min, even if its just 4K page. If the user just wants
to know how much of the region is 4K page, they will get it with this. But as David said,
folio is a kernel concept so might not be best to expose it as sysfs to userspace?

As folio might not be an option, my preference is going with hugepage, but as its not
an implementation detail and just naming, I am ok to change it to hugepage_size if
you have a strong preference for it.


>>  	"addr",
>>  	"target",
>>  };
>> -- 
>> 2.43.5
> 
> 
> Thanks,
> SJ

Re: [PATCH v4 4/6] mm/damon: introduce DAMOS filter type hugepage

[SeongJae Park]

On Wed, 5 Feb 2025 13:52:37 +0000 Usama Arif <usamaarif642@gmail.com> wrote:

> 
> 
> On 04/02/2025 23:12, SeongJae Park wrote:
> > On Mon,  3 Feb 2025 22:55:31 +0000 Usama Arif <usamaarif642@gmail.com> wrote:
> > 
> >> This is to gather statistics to check if memory regions of specific
> >> access tempratures are backed by hugepages of a size in a specific
> >> range
> > 
> > nit.  A period is missed?
> > 
> >> This filter can help to observe and prove the effectivenes of
> >> different schemes for shrinking/collapsing hugepages.
> >>
> >> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
> >> ---
[...]
> >> --- a/mm/damon/sysfs-schemes.c
> >> +++ b/mm/damon/sysfs-schemes.c
> >> @@ -330,6 +330,7 @@ static const char * const damon_sysfs_scheme_filter_type_strs[] = {
> >>  	"anon",
> >>  	"memcg",
> >>  	"young",
> >> +	"hugepage",
> > 
> > hugepage_size?
> > 
> 
> So from https://lore.kernel.org/all/20250121200510.43645-1-sj@kernel.org/, my understanding
> was you didn't have a preference between "hugepage" and "hugepage_size"?

You're right.  And sorry, I changed my mind.

> 
> I do prefer hugepage over hugepage_size. This is because this is a type of filter, I would
> say hugepage_size is not a type, but hugepage is. For e.g. we have anon as type and not 
> anon_number. And the "size" part is clear from the min and max which is documented in patch
> 5 and 6.

I agree your points. The meaning of "min" and "max" is well documented.  But,
not everyone reads the documentation, and human's memory is volatile.  Also we
might want to further extend DAMOS fitler for other hugepage properties, say,
zero-filled hugepage-internal base pages ratio.  So now I prefer
"hugepage_size".

> 
> TBH my preference would be "folio". What I think we should do is let users input any value
> for min and max, as long as max > min, even if its just 4K page. If the user just wants
> to know how much of the region is 4K page, they will get it with this.

We can still do this, since DAMOS filters work for not only matching memory but
also non-matching memory.  It's not very intuitive, but at least makes a sense
in my opinion.

> But as David said,
> folio is a kernel concept so might not be best to expose it as sysfs to userspace?
> 
> As folio might not be an option, my preference is going with hugepage, but as its not
> an implementation detail and just naming, I am ok to change it to hugepage_size if
> you have a strong preference for it.

Yes, I'd suggest calling it "hugepage_size".  I don't find a problem it will
cause, other than the length.  Please let me know if I'm missing something,
though!

Again, sorry for changing my mind after the last discussion.


Thanks,
SJ

[...]

Re: [PATCH v4 4/6] mm/damon: introduce DAMOS filter type hugepage

[Usama Arif]

On 04/02/2025 23:12, SeongJae Park wrote:
> On Mon,  3 Feb 2025 22:55:31 +0000 Usama Arif <usamaarif642@gmail.com> wrote:
> 
> 
>> +	case DAMOS_FILTER_TYPE_HUGEPAGE:
>> +		folio_sz = folio_size(folio);
>> +		matched = filter->folio_size.min <= folio_sz && folio_sz <= filter->folio_size.max;
> 
> We should also return 'false' if the folio is not a large folio (folio_sz ==
> PAGE_SIZE), if we agreed to my suggestion on the previous version of this patch
> series?
> 

Thanks SJ for the reviews.

I will address all of the changes in the next revision. The only thing over here is, I would
like the filter to work for folios of all sizes, even PAGE_SIZE. It would give us a more convenient
way of knowing if hot regions are mainly being backed by 4K pages, and improve observability.
Let me know if thats ok?

Re: [PATCH v4 4/6] mm/damon: introduce DAMOS filter type hugepage

[SeongJae Park]

On Fri, 7 Feb 2025 18:22:04 +0000 Usama Arif <usamaarif642@gmail.com> wrote:

> 
> 
> On 04/02/2025 23:12, SeongJae Park wrote:
> > On Mon,  3 Feb 2025 22:55:31 +0000 Usama Arif <usamaarif642@gmail.com> wrote:
> > 
> > 
> >> +	case DAMOS_FILTER_TYPE_HUGEPAGE:
> >> +		folio_sz = folio_size(folio);
> >> +		matched = filter->folio_size.min <= folio_sz && folio_sz <= filter->folio_size.max;
> > 
> > We should also return 'false' if the folio is not a large folio (folio_sz ==
> > PAGE_SIZE), if we agreed to my suggestion on the previous version of this patch
> > series?
> > 
> 
> Thanks SJ for the reviews.

The pleasure is mine!

> 
> I will address all of the changes in the next revision.

Looking forward to :)

> The only thing over here is, I would
> like the filter to work for folios of all sizes, even PAGE_SIZE. It would give us a more convenient
> way of knowing if hot regions are mainly being backed by 4K pages, and improve observability.
> Let me know if thats ok?

I have no strong opinion here.  Unless others have concerns, I'm ok with your
approach.


Thanks,
SJ

[PATCH v4 5/6] Docs/ABI/damon: document DAMOS sysfs files to set the min/max folio_size

[Usama Arif]

This will be used to decide the min and max folio size to operate on
for pa_stat.

Signed-off-by: Usama Arif <usamaarif642@gmail.com>
---
 Documentation/ABI/testing/sysfs-kernel-mm-damon | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-kernel-mm-damon b/Documentation/ABI/testing/sysfs-kernel-mm-damon
index b057eddefbfc..07daaae3dd16 100644
--- a/Documentation/ABI/testing/sysfs-kernel-mm-damon
+++ b/Documentation/ABI/testing/sysfs-kernel-mm-damon
@@ -345,6 +345,18 @@ Description:	If 'addr' is written to the 'type' file, writing to or reading
 		from this file sets or gets the end address of the address
 		range for the filter.
 
+What:		/sys/kernel/mm/damon/admin/kdamonds/<K>/contexts/<C>/schemes/<S>/filters/<F>/min
+Date:		Jan 2025
+Contact:	SeongJae Park <sj@kernel.org>
+Description:	If 'hugepage' is written to the 'type' file, writing to or reading
+		from this file sets or gets the minimum size of the hugepage for the filter.
+
+What:		/sys/kernel/mm/damon/admin/kdamonds/<K>/contexts/<C>/schemes/<S>/filters/<F>/max
+Date:		Jan 2025
+Contact:	SeongJae Park <sj@kernel.org>
+Description:	If 'hugepage' is written to the 'type' file, writing to or reading
+		from this file sets or gets the maximum size of the hugepage for the filter.
+
 What:		/sys/kernel/mm/damon/admin/kdamonds/<K>/contexts/<C>/schemes/<S>/filters/<F>/target_idx
 Date:		Dec 2022
 Contact:	SeongJae Park <sj@kernel.org>
-- 
2.43.5

Re: [PATCH v4 5/6] Docs/ABI/damon: document DAMOS sysfs files to set the min/max folio_size

[SeongJae Park]

On Mon,  3 Feb 2025 22:55:32 +0000 Usama Arif <usamaarif642@gmail.com> wrote:

> This will be used to decide the min and max folio size to operate on
> for pa_stat.
> 
> Signed-off-by: Usama Arif <usamaarif642@gmail.com>

Other than the trivial comment below, looks good to me.

Reviewed-by: SeongJae Park <sj@kernel.org>

> ---
>  Documentation/ABI/testing/sysfs-kernel-mm-damon | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-kernel-mm-damon b/Documentation/ABI/testing/sysfs-kernel-mm-damon
> index b057eddefbfc..07daaae3dd16 100644
> --- a/Documentation/ABI/testing/sysfs-kernel-mm-damon
> +++ b/Documentation/ABI/testing/sysfs-kernel-mm-damon
> @@ -345,6 +345,18 @@ Description:	If 'addr' is written to the 'type' file, writing to or reading
>  		from this file sets or gets the end address of the address
>  		range for the filter.
>  
> +What:		/sys/kernel/mm/damon/admin/kdamonds/<K>/contexts/<C>/schemes/<S>/filters/<F>/min
> +Date:		Jan 2025

Nit.  It's February now :)

> +Contact:	SeongJae Park <sj@kernel.org>
> +Description:	If 'hugepage' is written to the 'type' file, writing to or reading
> +		from this file sets or gets the minimum size of the hugepage for the filter.
> +
> +What:		/sys/kernel/mm/damon/admin/kdamonds/<K>/contexts/<C>/schemes/<S>/filters/<F>/max
> +Date:		Jan 2025

Ditto.

> +Contact:	SeongJae Park <sj@kernel.org>
> +Description:	If 'hugepage' is written to the 'type' file, writing to or reading
> +		from this file sets or gets the maximum size of the hugepage for the filter.
> +
>  What:		/sys/kernel/mm/damon/admin/kdamonds/<K>/contexts/<C>/schemes/<S>/filters/<F>/target_idx
>  Date:		Dec 2022
>  Contact:	SeongJae Park <sj@kernel.org>
> -- 
> 2.43.5


Thanks,
SJ

[PATCH v4 6/6] Docs/admin-guide/mm/damon/usage: Document hugepage filter type

[Usama Arif]

This includes both the 'hugepage' filter type and the min/max
files used to decide range of sizes to filter on.

Signed-off-by: Usama Arif <usamaarif642@gmail.com>
---
 Documentation/admin-guide/mm/damon/usage.rst | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/Documentation/admin-guide/mm/damon/usage.rst b/Documentation/admin-guide/mm/damon/usage.rst
index 47a44bd348ab..5765896aaa7e 100644
--- a/Documentation/admin-guide/mm/damon/usage.rst
+++ b/Documentation/admin-guide/mm/damon/usage.rst
@@ -83,7 +83,7 @@ comma (",").
     │ │ │ │ │ │ │ │ │ 0/target_metric,target_value,current_value
     │ │ │ │ │ │ │ :ref:`watermarks <sysfs_watermarks>`/metric,interval_us,high,mid,low
     │ │ │ │ │ │ │ :ref:`filters <sysfs_filters>`/nr_filters
-    │ │ │ │ │ │ │ │ 0/type,matching,allow,memcg_path,addr_start,addr_end,target_idx
+    │ │ │ │ │ │ │ │ 0/type,matching,allow,memcg_path,addr_start,addr_end,target_idx,min,max
     │ │ │ │ │ │ │ :ref:`stats <sysfs_schemes_stats>`/nr_tried,sz_tried,nr_applied,sz_applied,sz_ops_filter_passed,qt_exceeds
     │ │ │ │ │ │ │ :ref:`tried_regions <sysfs_schemes_tried_regions>`/total_bytes
     │ │ │ │ │ │ │ │ 0/start,end,nr_accesses,age,sz_filter_passed
@@ -406,13 +406,14 @@ number (``N``) to the file creates the number of child directories named ``0``
 to ``N-1``.  Each directory represents each filter.  The filters are evaluated
 in the numeric order.
 
-Each filter directory contains seven files, namely ``type``, ``matching``,
-``allow``, ``memcg_path``, ``addr_start``, ``addr_end``, and ``target_idx``.
-To ``type`` file, you can write one of five special keywords: ``anon`` for
-anonymous pages, ``memcg`` for specific memory cgroup, ``young`` for young
-pages, ``addr`` for specific address range (an open-ended interval), or
-``target`` for specific DAMON monitoring target filtering.  Meaning of the
-types are same to the description on the :ref:`design doc
+Each filter directory contains nine files, namely ``type``, ``matching``,
+``allow``, ``memcg_path``, ``addr_start``, ``addr_end``, ``min``, ``max``
+and ``target_idx``. To ``type`` file, you can write one of six special
+keywords: ``anon`` for anonymous pages, ``memcg`` for specific memory cgroup,
+``young`` for young pages, ``addr`` for specific address range (an open-ended
+interval), ``hugepage`` for large folios of a specific size range [``min``,
+``max``] or ``target`` for specific DAMON monitoring target filtering. Meaning
+of the types are same to the description on the :ref:`design doc
 <damon_design_damos_filters>`.
 
 In case of the memory cgroup filtering, you can specify the memory cgroup of
-- 
2.43.5

Re: [PATCH v4 6/6] Docs/admin-guide/mm/damon/usage: Document hugepage filter type

[SeongJae Park]

On Mon,  3 Feb 2025 22:55:33 +0000 Usama Arif <usamaarif642@gmail.com> wrote:

> This includes both the 'hugepage' filter type and the min/max
> files used to decide range of sizes to filter on.
> 
> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
> ---
>  Documentation/admin-guide/mm/damon/usage.rst | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/admin-guide/mm/damon/usage.rst b/Documentation/admin-guide/mm/damon/usage.rst
> index 47a44bd348ab..5765896aaa7e 100644
> --- a/Documentation/admin-guide/mm/damon/usage.rst
> +++ b/Documentation/admin-guide/mm/damon/usage.rst
> @@ -83,7 +83,7 @@ comma (",").
>      │ │ │ │ │ │ │ │ │ 0/target_metric,target_value,current_value
>      │ │ │ │ │ │ │ :ref:`watermarks <sysfs_watermarks>`/metric,interval_us,high,mid,low
>      │ │ │ │ │ │ │ :ref:`filters <sysfs_filters>`/nr_filters
> -    │ │ │ │ │ │ │ │ 0/type,matching,allow,memcg_path,addr_start,addr_end,target_idx
> +    │ │ │ │ │ │ │ │ 0/type,matching,allow,memcg_path,addr_start,addr_end,target_idx,min,max
>      │ │ │ │ │ │ │ :ref:`stats <sysfs_schemes_stats>`/nr_tried,sz_tried,nr_applied,sz_applied,sz_ops_filter_passed,qt_exceeds
>      │ │ │ │ │ │ │ :ref:`tried_regions <sysfs_schemes_tried_regions>`/total_bytes
>      │ │ │ │ │ │ │ │ 0/start,end,nr_accesses,age,sz_filter_passed
> @@ -406,13 +406,14 @@ number (``N``) to the file creates the number of child directories named ``0``
>  to ``N-1``.  Each directory represents each filter.  The filters are evaluated
>  in the numeric order.
>  
> -Each filter directory contains seven files, namely ``type``, ``matching``,
> -``allow``, ``memcg_path``, ``addr_start``, ``addr_end``, and ``target_idx``.
> -To ``type`` file, you can write one of five special keywords: ``anon`` for
> -anonymous pages, ``memcg`` for specific memory cgroup, ``young`` for young
> -pages, ``addr`` for specific address range (an open-ended interval), or
> -``target`` for specific DAMON monitoring target filtering.  Meaning of the
> -types are same to the description on the :ref:`design doc
> +Each filter directory contains nine files, namely ``type``, ``matching``,
> +``allow``, ``memcg_path``, ``addr_start``, ``addr_end``, ``min``, ``max``
> +and ``target_idx``. To ``type`` file, you can write one of six special
> +keywords: ``anon`` for anonymous pages, ``memcg`` for specific memory cgroup,
> +``young`` for young pages, ``addr`` for specific address range (an open-ended
> +interval), ``hugepage`` for large folios of a specific size range [``min``,

I'd prefer naming it ``hugepage_size``.

> +``max``] or ``target`` for specific DAMON monitoring target filtering. Meaning
> +of the types are same to the description on the :ref:`design doc
>  <damon_design_damos_filters>`.
>  
>  In case of the memory cgroup filtering, you can specify the memory cgroup of
> -- 
> 2.43.5


Thanks,
SJ

Re: [PATCH v4 0/6] mm/damon: add support for hugepages

[SeongJae Park]

Hi Usama,


Thank you for continuing this great work!

On Mon,  3 Feb 2025 22:55:27 +0000 Usama Arif <usamaarif642@gmail.com> wrote:

> This includes adding support for larger folios in damon for paddr,

nit.  s/larger/large/

> which means largers folios will have their access checked and will
> be considered for different DAMOS actions like pageout, prioritization
> and migration.

Technically speaking, 'paddr' was supporting large folios, but the support was
implemented in a very poor or broken way.  And you're not adding a support that
didn't exist before, but improving or fixing the existing support, right?  Can
we make the sentence more accurate in the way?

> 
> Patches 3-6 add support for DAMOS hugepage filter type. This is to
> gather statistics to check if memory regions of specific access
> tempratures are backed by hugepages of a size in a specific
> range. This filter can help to observe and prove the effectivenes of
> different schemes for shrinking/collapsing hugepages.
> 
> I have kept patches 1-2 as part of these series as the later patches
> are dependent on them.

I understand the two patches will make patches 3-6 more completely work, but
not necessarily a blocker of those.  So please feel free to split those out to
another series if you want.

Also I think the second patch need to be revised, but even in the case, it
might introduce unnecessarily huge complexity for small problem.  Please refer
to the comments on the patch and let me know if I'm missing something.

I'd also ask you to put logic and API implementation before sysfs
implementation.

Added more comments in detail to each patch files except the first one, which
already got my Reviewed-by: and not changed in this version.

> 
> The corresponding damo PR is at https://github.com/damonitor/damo/pull/20.
> 
> v3 -> v4:
> - Add support for large folios of all sizes, and not just
>   PMD mapped hugepages (David and SJ).
> - only get folio while checking access/ applying DAMOS
>   scheme if the head page is also part of that region.
> 
> v2 -> v3:
> - expose hugepage via sysfs even if the kernel is
>   built without hugepage support. DAMON will just
>   just return 0. (SJ Park)
> 
> v1 -> v2:
> - Wrap DAMOS_FILTER_TYPE_HUGEPAGE case with
>   CONFIG_PGTABLE_HAS_HUGE_LEAVES (SJ Park)
> 
> Usama Arif (6):
>   mm/damon: have damon_get_folio return folio even for tail pages
>   mm/damon/paddr: use damon_get_folio_in_region to obtain folio
>   mm/damon/sysfs-schemes: add files for setting damos_filter->folio_size
>   mm/damon: introduce DAMOS filter type hugepage
>   Docs/ABI/damon: document DAMOS sysfs files to set the min/max
>     folio_size
>   Docs/admin-guide/mm/damon/usage: Document hugepage filter type
> 
>  .../ABI/testing/sysfs-kernel-mm-damon         | 12 +++
>  Documentation/admin-guide/mm/damon/usage.rst  | 17 +++--
>  include/linux/damon.h                         | 13 ++++
>  mm/damon/core.c                               |  3 +
>  mm/damon/ops-common.c                         |  2 +-
>  mm/damon/paddr.c                              | 75 +++++++++++++++----
>  mm/damon/sysfs-schemes.c                      | 54 +++++++++++++
>  7 files changed, 151 insertions(+), 25 deletions(-)
> 
> -- 
> 2.43.5

Again, thank you for continuing this great work!


Thanks,
SJ