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

[Usama Arif <usamaarif642@gmail.com>]

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