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

[PATCH v3 1/2] 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 bd8cfe10121b..c0ccf4fade24 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 v3 2/2] 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. This includes both THPs
and hugetlbfs.
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>
---
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)
---
 include/linux/damon.h    | 2 ++
 mm/damon/paddr.c         | 5 +++++
 mm/damon/sysfs-schemes.c | 1 +
 3 files changed, 8 insertions(+)

diff --git a/include/linux/damon.h b/include/linux/damon.h
index af525252b853..1d94d7d88b36 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -326,6 +326,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.
@@ -345,6 +346,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 c0ccf4fade24..224308140441 100644
--- a/mm/damon/paddr.c
+++ b/mm/damon/paddr.c
@@ -222,6 +222,11 @@ 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:
+		matched = folio_size(folio) == HPAGE_PMD_SIZE;
+		break;
+#endif
 	default:
 		break;
 	}
diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
index 98f93ae9f59e..de9265f7ccde 100644
--- a/mm/damon/sysfs-schemes.c
+++ b/mm/damon/sysfs-schemes.c
@@ -329,6 +329,7 @@ static const char * const damon_sysfs_scheme_filter_type_strs[] = {
 	"anon",
 	"memcg",
 	"young",
+	"hugepage",
 	"addr",
 	"target",
 };
-- 
2.43.5

Re: [PATCH v3 2/2] mm/damon: introduce DAMOS filter type hugepage

[SeongJae Park]

On Mon, 20 Jan 2025 18:19:59 +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. This includes both THPs
> and hugetlbfs.
> 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>

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

> ---
> 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)

Thank you for writing and patiently revisioning this nice patch!


Thanks,
SJ

[...]

Re: [PATCH v3 2/2] mm/damon: introduce DAMOS filter type hugepage

[David Hildenbrand]

On 20.01.25 19:19, Usama Arif wrote:
> This is to gather statistics to check if memory regions of specific
> access tempratures are backed by hugepages. This includes both THPs
> and hugetlbfs.
> 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>
> ---
> 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)
> ---
>   include/linux/damon.h    | 2 ++
>   mm/damon/paddr.c         | 5 +++++
>   mm/damon/sysfs-schemes.c | 1 +
>   3 files changed, 8 insertions(+)
> 
> diff --git a/include/linux/damon.h b/include/linux/damon.h
> index af525252b853..1d94d7d88b36 100644
> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
> @@ -326,6 +326,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.
> @@ -345,6 +346,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 c0ccf4fade24..224308140441 100644
> --- a/mm/damon/paddr.c
> +++ b/mm/damon/paddr.c
> @@ -222,6 +222,11 @@ 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:
> +		matched = folio_size(folio) == HPAGE_PMD_SIZE;


Can we directly embed in the name and the comments/docs that we are only 
talking about PMD size (both, THP and hugetlb)?

DAMOS_FILTER_TYPE_PMD_HUGEPAGE or sth. like that.


-- 
Cheers,

David / dhildenb

Re: [PATCH v3 2/2] mm/damon: introduce DAMOS filter type hugepage

[SeongJae Park]

On Mon, 20 Jan 2025 19:57:10 +0100 David Hildenbrand <david@redhat.com> wrote:

> On 20.01.25 19:19, Usama Arif wrote:
> > This is to gather statistics to check if memory regions of specific
> > access tempratures are backed by hugepages. This includes both THPs
> > and hugetlbfs.
> > 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>
> > ---
> > 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)
> > ---
> >   include/linux/damon.h    | 2 ++
> >   mm/damon/paddr.c         | 5 +++++
> >   mm/damon/sysfs-schemes.c | 1 +
> >   3 files changed, 8 insertions(+)
> > 
> > diff --git a/include/linux/damon.h b/include/linux/damon.h
> > index af525252b853..1d94d7d88b36 100644
> > --- a/include/linux/damon.h
> > +++ b/include/linux/damon.h
> > @@ -326,6 +326,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.
> > @@ -345,6 +346,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 c0ccf4fade24..224308140441 100644
> > --- a/mm/damon/paddr.c
> > +++ b/mm/damon/paddr.c
> > @@ -222,6 +222,11 @@ 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:
> > +		matched = folio_size(folio) == HPAGE_PMD_SIZE;
> 
> 
> Can we directly embed in the name and the comments/docs that we are only 
> talking about PMD size (both, THP and hugetlb)?
> 
> DAMOS_FILTER_TYPE_PMD_HUGEPAGE or sth. like that.

Nice suggestion, thank you!  And we might later add more filter types for
different size huge pages.  What about extending this to handle more general
case, though?  That is, we can let the filter receives a range of the folio
size to match, like DAMOS_FILTER_TYPE_ADDR does.  Then, the filter could be
used for any size of interest.

I think such extension is not a strong blocker for this patch.  We can do the
extension later without breaking users by having the PAGE_PMD_SIZE as the
default value of the address range input.  I have no strong opinion about this,
though.

What do you think?


Thanks,
SJ

[...]

Re: [PATCH v3 2/2] mm/damon: introduce DAMOS filter type hugepage

[Usama Arif]

On 20/01/2025 18:57, David Hildenbrand wrote:
> On 20.01.25 19:19, Usama Arif wrote:
>> This is to gather statistics to check if memory regions of specific
>> access tempratures are backed by hugepages. This includes both THPs
>> and hugetlbfs.
>> 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>
>> ---
>> 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)
>> ---
>>   include/linux/damon.h    | 2 ++
>>   mm/damon/paddr.c         | 5 +++++
>>   mm/damon/sysfs-schemes.c | 1 +
>>   3 files changed, 8 insertions(+)
>>
>> diff --git a/include/linux/damon.h b/include/linux/damon.h
>> index af525252b853..1d94d7d88b36 100644
>> --- a/include/linux/damon.h
>> +++ b/include/linux/damon.h
>> @@ -326,6 +326,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.
>> @@ -345,6 +346,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 c0ccf4fade24..224308140441 100644
>> --- a/mm/damon/paddr.c
>> +++ b/mm/damon/paddr.c
>> @@ -222,6 +222,11 @@ 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:
>> +        matched = folio_size(folio) == HPAGE_PMD_SIZE;
> 
> 
> Can we directly embed in the name and the comments/docs that we are only talking about PMD size (both, THP and hugetlb)?
> 
> DAMOS_FILTER_TYPE_PMD_HUGEPAGE or sth. like that.
> 
> 

I always think of gigantic page as PUD size, hugepage as PMD size (and not anything smaller), mTHP as smaller than PMD :)
But I don't think thats the official term or standardized across the kernel.  

I can send a v4, or maybe Andrew could apply the diff below?

diff --git a/include/linux/damon.h b/include/linux/damon.h
index 1d94d7d88b36..261c0741dd0f 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -326,7 +326,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_PMD_HUGEPAGE:    Page is part of a hugepage (THP/hugetlb).
  * @DAMOS_FILTER_TYPE_ADDR:    Address range.
  * @DAMOS_FILTER_TYPE_TARGET:  Data Access Monitoring target.
  * @NR_DAMOS_FILTER_TYPES:     Number of filter types.
@@ -346,7 +346,7 @@ enum damos_filter_type {
        DAMOS_FILTER_TYPE_ANON,
        DAMOS_FILTER_TYPE_MEMCG,
        DAMOS_FILTER_TYPE_YOUNG,
-       DAMOS_FILTER_TYPE_HUGEPAGE,
+       DAMOS_FILTER_TYPE_PMD_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 224308140441..e374ea952308 100644
--- a/mm/damon/paddr.c
+++ b/mm/damon/paddr.c
@@ -223,7 +223,7 @@ static bool damos_pa_filter_match(struct damos_filter *filter,
                        damon_folio_mkold(folio);
                break;
 #if defined(CONFIG_PGTABLE_HAS_HUGE_LEAVES)
-       case DAMOS_FILTER_TYPE_HUGEPAGE:
+       case DAMOS_FILTER_TYPE_PMD_HUGEPAGE:
                matched = folio_size(folio) == HPAGE_PMD_SIZE;
                break;
 #endif

Re: [PATCH v3 2/2] mm/damon: introduce DAMOS filter type hugepage

[David Hildenbrand]

On 20.01.25 20:16, SeongJae Park wrote:
> On Mon, 20 Jan 2025 19:57:10 +0100 David Hildenbrand <david@redhat.com> wrote:
> 
>> On 20.01.25 19:19, Usama Arif wrote:
>>> This is to gather statistics to check if memory regions of specific
>>> access tempratures are backed by hugepages. This includes both THPs
>>> and hugetlbfs.
>>> 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>
>>> ---
>>> 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)
>>> ---
>>>    include/linux/damon.h    | 2 ++
>>>    mm/damon/paddr.c         | 5 +++++
>>>    mm/damon/sysfs-schemes.c | 1 +
>>>    3 files changed, 8 insertions(+)
>>>
>>> diff --git a/include/linux/damon.h b/include/linux/damon.h
>>> index af525252b853..1d94d7d88b36 100644
>>> --- a/include/linux/damon.h
>>> +++ b/include/linux/damon.h
>>> @@ -326,6 +326,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.
>>> @@ -345,6 +346,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 c0ccf4fade24..224308140441 100644
>>> --- a/mm/damon/paddr.c
>>> +++ b/mm/damon/paddr.c
>>> @@ -222,6 +222,11 @@ 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:
>>> +		matched = folio_size(folio) == HPAGE_PMD_SIZE;
>>
>>
>> Can we directly embed in the name and the comments/docs that we are only
>> talking about PMD size (both, THP and hugetlb)?
>>
>> DAMOS_FILTER_TYPE_PMD_HUGEPAGE or sth. like that.
> 
> Nice suggestion, thank you!  And we might later add more filter types for
> different size huge pages.  What about extending this to handle more general
> case, though?  That is, we can let the filter receives a range of the folio
> size to match, like DAMOS_FILTER_TYPE_ADDR does.  Then, the filter could be
> used for any size of interest.

That would probably be future proof: either a range or explicitly 
specified sizes (ranges?).

-- 
Cheers,

David / dhildenb

Re: [PATCH v3 2/2] mm/damon: introduce DAMOS filter type hugepage

[David Hildenbrand]

On 20.01.25 20:18, Usama Arif wrote:
> 
> 
> On 20/01/2025 18:57, David Hildenbrand wrote:
>> On 20.01.25 19:19, Usama Arif wrote:
>>> This is to gather statistics to check if memory regions of specific
>>> access tempratures are backed by hugepages. This includes both THPs
>>> and hugetlbfs.
>>> 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>
>>> ---
>>> 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)
>>> ---
>>>    include/linux/damon.h    | 2 ++
>>>    mm/damon/paddr.c         | 5 +++++
>>>    mm/damon/sysfs-schemes.c | 1 +
>>>    3 files changed, 8 insertions(+)
>>>
>>> diff --git a/include/linux/damon.h b/include/linux/damon.h
>>> index af525252b853..1d94d7d88b36 100644
>>> --- a/include/linux/damon.h
>>> +++ b/include/linux/damon.h
>>> @@ -326,6 +326,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.
>>> @@ -345,6 +346,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 c0ccf4fade24..224308140441 100644
>>> --- a/mm/damon/paddr.c
>>> +++ b/mm/damon/paddr.c
>>> @@ -222,6 +222,11 @@ 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:
>>> +        matched = folio_size(folio) == HPAGE_PMD_SIZE;
>>
>>
>> Can we directly embed in the name and the comments/docs that we are only talking about PMD size (both, THP and hugetlb)?
>>
>> DAMOS_FILTER_TYPE_PMD_HUGEPAGE or sth. like that.
>>
>>
> 
> I always think of gigantic page as PUD size, hugepage as PMD size (and not anything smaller), mTHP as smaller than PMD :)

And cont-PMD ... hugetlb, cont-PTE hugetlb ... are ? :)

> But I don't think thats the official term or standardized across the kernel.
> 

We didn't know better when we designed some of the old interfaces, 
that's why I am hoping that we can do better with new interfaces. Either 
spell out the PMD size, or have filters for size lists / size ranges.

> I can send a v4, or maybe Andrew could apply the diff below?
> 
> diff --git a/include/linux/damon.h b/include/linux/damon.h
> index 1d94d7d88b36..261c0741dd0f 100644
> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
> @@ -326,7 +326,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_PMD_HUGEPAGE:    Page is part of a hugepage (THP/hugetlb).
>    * @DAMOS_FILTER_TYPE_ADDR:    Address range.
>    * @DAMOS_FILTER_TYPE_TARGET:  Data Access Monitoring target.
>    * @NR_DAMOS_FILTER_TYPES:     Number of filter types.
> @@ -346,7 +346,7 @@ enum damos_filter_type {
>          DAMOS_FILTER_TYPE_ANON,
>          DAMOS_FILTER_TYPE_MEMCG,
>          DAMOS_FILTER_TYPE_YOUNG,
> -       DAMOS_FILTER_TYPE_HUGEPAGE,
> +       DAMOS_FILTER_TYPE_PMD_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 224308140441..e374ea952308 100644
> --- a/mm/damon/paddr.c
> +++ b/mm/damon/paddr.c
> @@ -223,7 +223,7 @@ static bool damos_pa_filter_match(struct damos_filter *filter,
>                          damon_folio_mkold(folio);
>                  break;
>   #if defined(CONFIG_PGTABLE_HAS_HUGE_LEAVES)
> -       case DAMOS_FILTER_TYPE_HUGEPAGE:
> +       case DAMOS_FILTER_TYPE_PMD_HUGEPAGE:
>                  matched = folio_size(folio) == HPAGE_PMD_SIZE;
>                  break;
>   #endif
> 


-- 
Cheers,

David / dhildenb

Re: [PATCH v3 2/2] mm/damon: introduce DAMOS filter type hugepage

[SeongJae Park]

On Mon, 20 Jan 2025 20:23:20 +0100 David Hildenbrand <david@redhat.com> wrote:

> On 20.01.25 20:16, SeongJae Park wrote:
> > On Mon, 20 Jan 2025 19:57:10 +0100 David Hildenbrand <david@redhat.com> wrote:
> > 
> >> On 20.01.25 19:19, Usama Arif wrote:
[...]
> >>> +#if defined(CONFIG_PGTABLE_HAS_HUGE_LEAVES)
> >>> +	case DAMOS_FILTER_TYPE_HUGEPAGE:
> >>> +		matched = folio_size(folio) == HPAGE_PMD_SIZE;
> >>
> >>
> >> Can we directly embed in the name and the comments/docs that we are only
> >> talking about PMD size (both, THP and hugetlb)?
> >>
> >> DAMOS_FILTER_TYPE_PMD_HUGEPAGE or sth. like that.
> > 
> > Nice suggestion, thank you!  And we might later add more filter types for
> > different size huge pages.  What about extending this to handle more general
> > case, though?  That is, we can let the filter receives a range of the folio
> > size to match, like DAMOS_FILTER_TYPE_ADDR does.  Then, the filter could be
> > used for any size of interest.
> 
> That would probably be future proof: either a range or explicitly 
> specified sizes (ranges?).

DAMON supports installing multiple DAMOS filters.  So multiple DAMOS filters
that each matching single range can be used for the multiple sizes or ranges
use case.


Thanks,
SJ

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

Re: [PATCH v3 2/2] mm/damon: introduce DAMOS filter type hugepage

[Usama Arif]

On 20/01/2025 19:30, SeongJae Park wrote:
> On Mon, 20 Jan 2025 20:23:20 +0100 David Hildenbrand <david@redhat.com> wrote:
> 
>> On 20.01.25 20:16, SeongJae Park wrote:
>>> On Mon, 20 Jan 2025 19:57:10 +0100 David Hildenbrand <david@redhat.com> wrote:
>>>
>>>> On 20.01.25 19:19, Usama Arif wrote:
> [...]
>>>>> +#if defined(CONFIG_PGTABLE_HAS_HUGE_LEAVES)
>>>>> +	case DAMOS_FILTER_TYPE_HUGEPAGE:
>>>>> +		matched = folio_size(folio) == HPAGE_PMD_SIZE;
>>>>
>>>>
>>>> Can we directly embed in the name and the comments/docs that we are only
>>>> talking about PMD size (both, THP and hugetlb)?
>>>>
>>>> DAMOS_FILTER_TYPE_PMD_HUGEPAGE or sth. like that.
>>>
>>> Nice suggestion, thank you!  And we might later add more filter types for
>>> different size huge pages.  What about extending this to handle more general
>>> case, though?  That is, we can let the filter receives a range of the folio
>>> size to match, like DAMOS_FILTER_TYPE_ADDR does.  Then, the filter could be
>>> used for any size of interest.
>>
>> That would probably be future proof: either a range or explicitly 
>> specified sizes (ranges?).
> 
> DAMON supports installing multiple DAMOS filters.  So multiple DAMOS filters
> that each matching single range can be used for the multiple sizes or ranges
> use case.
> 
> 

Does creating something like schemes/<N>/access_pattern/page_size/{min,max}
sound good? with the default value being pmd size?

Re: [PATCH v3 2/2] mm/damon: introduce DAMOS filter type hugepage

[David Hildenbrand]

On 20.01.25 20:58, Usama Arif wrote:
> 
> 
> On 20/01/2025 19:30, SeongJae Park wrote:
>> On Mon, 20 Jan 2025 20:23:20 +0100 David Hildenbrand <david@redhat.com> wrote:
>>
>>> On 20.01.25 20:16, SeongJae Park wrote:
>>>> On Mon, 20 Jan 2025 19:57:10 +0100 David Hildenbrand <david@redhat.com> wrote:
>>>>
>>>>> On 20.01.25 19:19, Usama Arif wrote:
>> [...]
>>>>>> +#if defined(CONFIG_PGTABLE_HAS_HUGE_LEAVES)
>>>>>> +	case DAMOS_FILTER_TYPE_HUGEPAGE:
>>>>>> +		matched = folio_size(folio) == HPAGE_PMD_SIZE;
>>>>>
>>>>>
>>>>> Can we directly embed in the name and the comments/docs that we are only
>>>>> talking about PMD size (both, THP and hugetlb)?
>>>>>
>>>>> DAMOS_FILTER_TYPE_PMD_HUGEPAGE or sth. like that.
>>>>
>>>> Nice suggestion, thank you!  And we might later add more filter types for
>>>> different size huge pages.  What about extending this to handle more general
>>>> case, though?  That is, we can let the filter receives a range of the folio
>>>> size to match, like DAMOS_FILTER_TYPE_ADDR does.  Then, the filter could be
>>>> used for any size of interest.
>>>
>>> That would probably be future proof: either a range or explicitly
>>> specified sizes (ranges?).
>>
>> DAMON supports installing multiple DAMOS filters.  So multiple DAMOS filters
>> that each matching single range can be used for the multiple sizes or ranges
>> use case.
>>
>>
> 
> Does creating something like schemes/<N>/access_pattern/page_size/{min,max}
> sound good? with the default value being pmd size?

"page_size" might be misleading. Not sure if we want to use the word 
"folio_size" here, so far it's more an internal detail that evolved from 
compound pages.

"hugepage_size" would at least match /sys/kernel/mm/hugepages/ and 
/sys/kernel/mm/transparent_hugepage/.

But if you would also support "single page" == e.g., 4K, "hugepage" 
would be wrong.

-- 
Cheers,

David / dhildenb

Re: [PATCH v3 2/2] mm/damon: introduce DAMOS filter type hugepage

[SeongJae Park]

On Mon, 20 Jan 2025 19:58:24 +0000 Usama Arif <usamaarif642@gmail.com> wrote:

> 
> 
> On 20/01/2025 19:30, SeongJae Park wrote:
> > On Mon, 20 Jan 2025 20:23:20 +0100 David Hildenbrand <david@redhat.com> wrote:
> > 
> >> On 20.01.25 20:16, SeongJae Park wrote:
> >>> On Mon, 20 Jan 2025 19:57:10 +0100 David Hildenbrand <david@redhat.com> wrote:
> >>>
> >>>> On 20.01.25 19:19, Usama Arif wrote:
> > [...]
> >>>>> +#if defined(CONFIG_PGTABLE_HAS_HUGE_LEAVES)
> >>>>> +	case DAMOS_FILTER_TYPE_HUGEPAGE:
> >>>>> +		matched = folio_size(folio) == HPAGE_PMD_SIZE;
> >>>>
> >>>>
> >>>> Can we directly embed in the name and the comments/docs that we are only
> >>>> talking about PMD size (both, THP and hugetlb)?
> >>>>
> >>>> DAMOS_FILTER_TYPE_PMD_HUGEPAGE or sth. like that.
> >>>
> >>> Nice suggestion, thank you!  And we might later add more filter types for
> >>> different size huge pages.  What about extending this to handle more general
> >>> case, though?  That is, we can let the filter receives a range of the folio
> >>> size to match, like DAMOS_FILTER_TYPE_ADDR does.  Then, the filter could be
> >>> used for any size of interest.
> >>
> >> That would probably be future proof: either a range or explicitly 
> >> specified sizes (ranges?).
> > 
> > DAMON supports installing multiple DAMOS filters.  So multiple DAMOS filters
> > that each matching single range can be used for the multiple sizes or ranges
> > use case.
> > 
> > 
> 
> Does creating something like schemes/<N>/access_pattern/page_size/{min,max}
> sound good? with the default value being pmd size?

For user-space ABI, like DAMOS_FILTER_TYPE_ADDR, let's use
'schemes/<N>/filters/<F>/' directory.  File names 'min' and 'max' look good to
me.

For kernel-space API, again, like DAMOS_FILTER_TYPE_ADDR, let's use the union
in 'struct damos_filter'.

If you will do the extension together with this patch, I think the default
value is not really needed.  It will only add a bit of complexity.  So if you
will do so, I'd recommend not having the default value.

Also, if you will revise this patch series for the extension, could you please
split and post the first patch of this series as a separate one?  I think the
fix is important and has no reason to be tied with this patch.


Thanks,
SJ

Re: [PATCH v3 2/2] mm/damon: introduce DAMOS filter type hugepage

[Usama Arif]

On 20/01/2025 20:12, SeongJae Park wrote:
> On Mon, 20 Jan 2025 19:58:24 +0000 Usama Arif <usamaarif642@gmail.com> wrote:
> 
>>
>>
>> On 20/01/2025 19:30, SeongJae Park wrote:
>>> On Mon, 20 Jan 2025 20:23:20 +0100 David Hildenbrand <david@redhat.com> wrote:
>>>
>>>> On 20.01.25 20:16, SeongJae Park wrote:
>>>>> On Mon, 20 Jan 2025 19:57:10 +0100 David Hildenbrand <david@redhat.com> wrote:
>>>>>
>>>>>> On 20.01.25 19:19, Usama Arif wrote:
>>> [...]
>>>>>>> +#if defined(CONFIG_PGTABLE_HAS_HUGE_LEAVES)
>>>>>>> +	case DAMOS_FILTER_TYPE_HUGEPAGE:
>>>>>>> +		matched = folio_size(folio) == HPAGE_PMD_SIZE;
>>>>>>
>>>>>>
>>>>>> Can we directly embed in the name and the comments/docs that we are only
>>>>>> talking about PMD size (both, THP and hugetlb)?
>>>>>>
>>>>>> DAMOS_FILTER_TYPE_PMD_HUGEPAGE or sth. like that.
>>>>>
>>>>> Nice suggestion, thank you!  And we might later add more filter types for
>>>>> different size huge pages.  What about extending this to handle more general
>>>>> case, though?  That is, we can let the filter receives a range of the folio
>>>>> size to match, like DAMOS_FILTER_TYPE_ADDR does.  Then, the filter could be
>>>>> used for any size of interest.
>>>>
>>>> That would probably be future proof: either a range or explicitly 
>>>> specified sizes (ranges?).
>>>
>>> DAMON supports installing multiple DAMOS filters.  So multiple DAMOS filters
>>> that each matching single range can be used for the multiple sizes or ranges
>>> use case.
>>>
>>>
>>
>> Does creating something like schemes/<N>/access_pattern/page_size/{min,max}
>> sound good? with the default value being pmd size?
> 
> For user-space ABI, like DAMOS_FILTER_TYPE_ADDR, let's use
> 'schemes/<N>/filters/<F>/' directory.  File names 'min' and 'max' look good to
> me.
> 
> For kernel-space API, again, like DAMOS_FILTER_TYPE_ADDR, let's use the union
> in 'struct damos_filter'.
> 
> If you will do the extension together with this patch, I think the default
> value is not really needed.  It will only add a bit of complexity.  So if you
> will do so, I'd recommend not having the default value.
> 
> Also, if you will revise this patch series for the extension, could you please
> split and post the first patch of this series as a separate one?  I think the
> fix is important and has no reason to be tied with this patch.
> 

Yeah I think it might be best if we get a version with flexible folio sizes merged
from the start, especially as it involves creating user ABI. I will try and send
something tomorrow.

For the first patch, do I need to resend? Or maybe Andrew could merge whats in
this v3 for patch 1, we discard patch 2 and I just send the hugepage
filter implementation separately as a new series..

Re: [PATCH v3 2/2] mm/damon: introduce DAMOS filter type hugepage

[SeongJae Park]

On Mon, 20 Jan 2025 20:26:02 +0000 Usama Arif <usamaarif642@gmail.com> wrote:

> 
> 
> On 20/01/2025 20:12, SeongJae Park wrote:
> > On Mon, 20 Jan 2025 19:58:24 +0000 Usama Arif <usamaarif642@gmail.com> wrote:
> > 
> >>
> >>
> >> On 20/01/2025 19:30, SeongJae Park wrote:
> >>> On Mon, 20 Jan 2025 20:23:20 +0100 David Hildenbrand <david@redhat.com> wrote:
> >>>
> >>>> On 20.01.25 20:16, SeongJae Park wrote:
> >>>>> On Mon, 20 Jan 2025 19:57:10 +0100 David Hildenbrand <david@redhat.com> wrote:
> >>>>>
> >>>>>> On 20.01.25 19:19, Usama Arif wrote:
> >>> [...]
> >>>>>>> +#if defined(CONFIG_PGTABLE_HAS_HUGE_LEAVES)
> >>>>>>> +	case DAMOS_FILTER_TYPE_HUGEPAGE:
> >>>>>>> +		matched = folio_size(folio) == HPAGE_PMD_SIZE;
> >>>>>>
> >>>>>>
> >>>>>> Can we directly embed in the name and the comments/docs that we are only
> >>>>>> talking about PMD size (both, THP and hugetlb)?
> >>>>>>
> >>>>>> DAMOS_FILTER_TYPE_PMD_HUGEPAGE or sth. like that.
> >>>>>
> >>>>> Nice suggestion, thank you!  And we might later add more filter types for
> >>>>> different size huge pages.  What about extending this to handle more general
> >>>>> case, though?  That is, we can let the filter receives a range of the folio
> >>>>> size to match, like DAMOS_FILTER_TYPE_ADDR does.  Then, the filter could be
> >>>>> used for any size of interest.
> >>>>
> >>>> That would probably be future proof: either a range or explicitly 
> >>>> specified sizes (ranges?).
> >>>
> >>> DAMON supports installing multiple DAMOS filters.  So multiple DAMOS filters
> >>> that each matching single range can be used for the multiple sizes or ranges
> >>> use case.
> >>>
> >>>
> >>
> >> Does creating something like schemes/<N>/access_pattern/page_size/{min,max}
> >> sound good? with the default value being pmd size?
> > 
> > For user-space ABI, like DAMOS_FILTER_TYPE_ADDR, let's use
> > 'schemes/<N>/filters/<F>/' directory.  File names 'min' and 'max' look good to
> > me.
> > 
> > For kernel-space API, again, like DAMOS_FILTER_TYPE_ADDR, let's use the union
> > in 'struct damos_filter'.
> > 
> > If you will do the extension together with this patch, I think the default
> > value is not really needed.  It will only add a bit of complexity.  So if you
> > will do so, I'd recommend not having the default value.
> > 
> > Also, if you will revise this patch series for the extension, could you please
> > split and post the first patch of this series as a separate one?  I think the
> > fix is important and has no reason to be tied with this patch.
> > 
> 
> Yeah I think it might be best if we get a version with flexible folio sizes merged
> from the start, especially as it involves creating user ABI. I will try and send
> something tomorrow.

Makes sense, looking forward to the next spin!

> 
> For the first patch, do I need to resend? Or maybe Andrew could merge whats in
> this v3 for patch 1, we discard patch 2 and I just send the hugepage
> filter implementation separately as a new series..

I think the first patch is important but not urgent.  I'd suggest to drop it
from the next revision of the filters extension series, and resend it if Andrew
neither pick it nor ask some action by a few days or weeks.


Thanks,
SJ

Re: [PATCH v3 2/2] mm/damon: introduce DAMOS filter type hugepage

[SeongJae Park]

On Mon, 20 Jan 2025 21:03:05 +0100 David Hildenbrand <david@redhat.com> wrote:

> On 20.01.25 20:58, Usama Arif wrote:
> > 
> > 
> > On 20/01/2025 19:30, SeongJae Park wrote:
> >> On Mon, 20 Jan 2025 20:23:20 +0100 David Hildenbrand <david@redhat.com> wrote:
> >>
> >>> On 20.01.25 20:16, SeongJae Park wrote:
> >>>> On Mon, 20 Jan 2025 19:57:10 +0100 David Hildenbrand <david@redhat.com> wrote:
> >>>>
> >>>>> On 20.01.25 19:19, Usama Arif wrote:
> >> [...]
> >>>>>> +#if defined(CONFIG_PGTABLE_HAS_HUGE_LEAVES)
> >>>>>> +	case DAMOS_FILTER_TYPE_HUGEPAGE:
> >>>>>> +		matched = folio_size(folio) == HPAGE_PMD_SIZE;
> >>>>>
> >>>>>
> >>>>> Can we directly embed in the name and the comments/docs that we are only
> >>>>> talking about PMD size (both, THP and hugetlb)?
> >>>>>
> >>>>> DAMOS_FILTER_TYPE_PMD_HUGEPAGE or sth. like that.
> >>>>
> >>>> Nice suggestion, thank you!  And we might later add more filter types for
> >>>> different size huge pages.  What about extending this to handle more general
> >>>> case, though?  That is, we can let the filter receives a range of the folio
> >>>> size to match, like DAMOS_FILTER_TYPE_ADDR does.  Then, the filter could be
> >>>> used for any size of interest.
> >>>
> >>> That would probably be future proof: either a range or explicitly
> >>> specified sizes (ranges?).
> >>
> >> DAMON supports installing multiple DAMOS filters.  So multiple DAMOS filters
> >> that each matching single range can be used for the multiple sizes or ranges
> >> use case.
> >>
> >>
> > 
> > Does creating something like schemes/<N>/access_pattern/page_size/{min,max}
> > sound good? with the default value being pmd size?
> 
> "page_size" might be misleading.

Good point.  I'm suggesting to add the files on another directory, and
apparently Usama agrees[1].  So the term "page_size" will not be used.

> Not sure if we want to use the word 
> "folio_size" here, so far it's more an internal detail that evolved from 
> compound pages.
> 
> "hugepage_size" would at least match /sys/kernel/mm/hugepages/ and 
> /sys/kernel/mm/transparent_hugepage/.
> 
> But if you would also support "single page" == e.g., 4K, "hugepage" 
> would be wrong.

Again, nice points, thank you for letting us aware of this.  We could error
users if they try to set <=PAGE_SIZE filter range.  FYI, DAMOS filters supports
making the filtering in/out action for not only condition-matching memory, but
also not-matching memory, so it will still be able to be used for filtering
in/out base pages.

That said, I now think "folio_size" might be a better term that allows simple
implementation and flexible usages.  What do you think about changing the
filter type name from "hugepage" to "folio_size", and let users set the range
whatever they want?  I still think "hugepage" type name is ok, but if there's
no objection about the naming, I'd slightly prefer more up to "folio_size".

[1] https://lore.kernel.org/db16fc35-58e7-453d-9fbb-318a88a98cd1@gmail.com


Thanks,
SJ

[...]

Re: [PATCH v3 2/2] mm/damon: introduce DAMOS filter type hugepage

[David Hildenbrand]

On 21.01.25 18:52, SeongJae Park wrote:
> On Mon, 20 Jan 2025 21:03:05 +0100 David Hildenbrand <david@redhat.com> wrote:
> 
>> On 20.01.25 20:58, Usama Arif wrote:
>>>
>>>
>>> On 20/01/2025 19:30, SeongJae Park wrote:
>>>> On Mon, 20 Jan 2025 20:23:20 +0100 David Hildenbrand <david@redhat.com> wrote:
>>>>
>>>>> On 20.01.25 20:16, SeongJae Park wrote:
>>>>>> On Mon, 20 Jan 2025 19:57:10 +0100 David Hildenbrand <david@redhat.com> wrote:
>>>>>>
>>>>>>> On 20.01.25 19:19, Usama Arif wrote:
>>>> [...]
>>>>>>>> +#if defined(CONFIG_PGTABLE_HAS_HUGE_LEAVES)
>>>>>>>> +	case DAMOS_FILTER_TYPE_HUGEPAGE:
>>>>>>>> +		matched = folio_size(folio) == HPAGE_PMD_SIZE;
>>>>>>>
>>>>>>>
>>>>>>> Can we directly embed in the name and the comments/docs that we are only
>>>>>>> talking about PMD size (both, THP and hugetlb)?
>>>>>>>
>>>>>>> DAMOS_FILTER_TYPE_PMD_HUGEPAGE or sth. like that.
>>>>>>
>>>>>> Nice suggestion, thank you!  And we might later add more filter types for
>>>>>> different size huge pages.  What about extending this to handle more general
>>>>>> case, though?  That is, we can let the filter receives a range of the folio
>>>>>> size to match, like DAMOS_FILTER_TYPE_ADDR does.  Then, the filter could be
>>>>>> used for any size of interest.
>>>>>
>>>>> That would probably be future proof: either a range or explicitly
>>>>> specified sizes (ranges?).
>>>>
>>>> DAMON supports installing multiple DAMOS filters.  So multiple DAMOS filters
>>>> that each matching single range can be used for the multiple sizes or ranges
>>>> use case.
>>>>
>>>>
>>>
>>> Does creating something like schemes/<N>/access_pattern/page_size/{min,max}
>>> sound good? with the default value being pmd size?
>>
>> "page_size" might be misleading.
> 
> Good point.  I'm suggesting to add the files on another directory, and
> apparently Usama agrees[1].  So the term "page_size" will not be used.
> 
>> Not sure if we want to use the word
>> "folio_size" here, so far it's more an internal detail that evolved from
>> compound pages.
>>
>> "hugepage_size" would at least match /sys/kernel/mm/hugepages/ and
>> /sys/kernel/mm/transparent_hugepage/.
>>
>> But if you would also support "single page" == e.g., 4K, "hugepage"
>> would be wrong.
> 
> Again, nice points, thank you for letting us aware of this.  We could error
> users if they try to set <=PAGE_SIZE filter range.  FYI, DAMOS filters supports
> making the filtering in/out action for not only condition-matching memory, but
> also not-matching memory, so it will still be able to be used for filtering
> in/out base pages.
> 
> That said, I now think "folio_size" might be a better term that allows simple
> implementation and flexible usages.

The issue with "folio" is that it is mostly a kernel-internal name for 
how we currently manage metadata for all pages and compound pages. In 
the future, some of what we call folio today will no longer be called 
folios ... as soon as we dynamically allocate "struct folio".

-- 
Cheers,

David / dhildenb

Re: [PATCH v3 2/2] mm/damon: introduce DAMOS filter type hugepage

[SeongJae Park]

On Tue, 21 Jan 2025 19:39:01 +0100 David Hildenbrand <david@redhat.com> wrote:

> On 21.01.25 18:52, SeongJae Park wrote:
> > On Mon, 20 Jan 2025 21:03:05 +0100 David Hildenbrand <david@redhat.com> wrote:
> > 
> >> On 20.01.25 20:58, Usama Arif wrote:
> >>>
> >>>
> >>> On 20/01/2025 19:30, SeongJae Park wrote:
> >>>> On Mon, 20 Jan 2025 20:23:20 +0100 David Hildenbrand <david@redhat.com> wrote:
> >>>>
> >>>>> On 20.01.25 20:16, SeongJae Park wrote:
> >>>>>> On Mon, 20 Jan 2025 19:57:10 +0100 David Hildenbrand <david@redhat.com> wrote:
> >>>>>>
> >>>>>>> On 20.01.25 19:19, Usama Arif wrote:
> >>>> [...]
> >>>>>>>> +#if defined(CONFIG_PGTABLE_HAS_HUGE_LEAVES)
> >>>>>>>> +	case DAMOS_FILTER_TYPE_HUGEPAGE:
> >>>>>>>> +		matched = folio_size(folio) == HPAGE_PMD_SIZE;
> >>>>>>>
> >>>>>>>
> >>>>>>> Can we directly embed in the name and the comments/docs that we are only
> >>>>>>> talking about PMD size (both, THP and hugetlb)?
> >>>>>>>
> >>>>>>> DAMOS_FILTER_TYPE_PMD_HUGEPAGE or sth. like that.
> >>>>>>
> >>>>>> Nice suggestion, thank you!  And we might later add more filter types for
> >>>>>> different size huge pages.  What about extending this to handle more general
> >>>>>> case, though?  That is, we can let the filter receives a range of the folio
> >>>>>> size to match, like DAMOS_FILTER_TYPE_ADDR does.  Then, the filter could be
> >>>>>> used for any size of interest.
> >>>>>
> >>>>> That would probably be future proof: either a range or explicitly
> >>>>> specified sizes (ranges?).
> >>>>
> >>>> DAMON supports installing multiple DAMOS filters.  So multiple DAMOS filters
> >>>> that each matching single range can be used for the multiple sizes or ranges
> >>>> use case.
> >>>>
> >>>>
> >>>
> >>> Does creating something like schemes/<N>/access_pattern/page_size/{min,max}
> >>> sound good? with the default value being pmd size?
> >>
> >> "page_size" might be misleading.
> > 
> > Good point.  I'm suggesting to add the files on another directory, and
> > apparently Usama agrees[1].  So the term "page_size" will not be used.
> > 
> >> Not sure if we want to use the word
> >> "folio_size" here, so far it's more an internal detail that evolved from
> >> compound pages.
> >>
> >> "hugepage_size" would at least match /sys/kernel/mm/hugepages/ and
> >> /sys/kernel/mm/transparent_hugepage/.
> >>
> >> But if you would also support "single page" == e.g., 4K, "hugepage"
> >> would be wrong.
> > 
> > Again, nice points, thank you for letting us aware of this.  We could error
> > users if they try to set <=PAGE_SIZE filter range.  FYI, DAMOS filters supports
> > making the filtering in/out action for not only condition-matching memory, but
> > also not-matching memory, so it will still be able to be used for filtering
> > in/out base pages.
> > 
> > That said, I now think "folio_size" might be a better term that allows simple
> > implementation and flexible usages.
> 
> The issue with "folio" is that it is mostly a kernel-internal name for 
> how we currently manage metadata for all pages and compound pages. In 
> the future, some of what we call folio today will no longer be called 
> folios ... as soon as we dynamically allocate "struct folio".

Agreed.  I'd suggest using "hugepage_size" or "hugepage" filter type name,
then.  I have no strong preferrence between the two names.  Also, to have a
flexible usage and a simple logic, let's not error users for wrong input, but
just define the filter type as "match" if the folio is "huge AND the size is in
the given range".  Then we can allow users set <=PAGE_SIZE range but make it
just works as sanely expected, like below.

    static bool damos_pa_filter_match(struct damos_filter *filter,
    [...]
            case DAMOS_FILTER_TYPE_HUGEPAGE_SIZE:
                    size_t sz = folio_size(folio);
    
                    if (sz <= PAGE_SIZE)
                            matched = false;
                    else
                            matched = filter->min <= sz && sz <= filter->max;
                    break;

Thanks,
SJ

[...]