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

[SeongJae Park <sj@kernel.org>]

On Thu, 16 Jan 2025 14:44:36 +0000 Usama Arif <usamaarif642@gmail.com> wrote:

> This is to gather statistics to check if hotest memory regions

s/hotest/hottest/?

> are backed by hugepages.

This would also be useful for not only hottest memory regions but general
memory regions of specific access temperature.  I like this.

> This includes both THPs and hugetlbfs.
> This filter can help to observe and prove the effectivenes of
> different schemes for shrinking/collapsing hugepages.

Nice extension of DAMON's page level properties based monitoring!

> 
> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
> ---
>  include/linux/damon.h    | 2 ++
>  mm/damon/paddr.c         | 3 +++
>  mm/damon/sysfs-schemes.c | 1 +
>  3 files changed, 6 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..a9e69179d45c 100644
> --- a/mm/damon/paddr.c
> +++ b/mm/damon/paddr.c
> @@ -222,6 +222,9 @@ static bool damos_pa_filter_match(struct damos_filter *filter,
>  		if (matched)
>  			damon_folio_mkold(folio);
>  		break;
> +	case DAMOS_FILTER_TYPE_HUGEPAGE:
> +		matched = folio_size(folio) == HPAGE_PMD_SIZE;
> +		break;

This will make build fails when CONFIG_PGTABLE_HAS_HUGE_LEAVES is not set, like
below:

 /include/linux/compiler_types.h:542:45: error: call to ‘__compiletime_assert_321’ declared with attribute error: BUILD_BUG failed
   542 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
       |                                             ^
[...]
 /include/linux/huge_mm.h:111:35: note: in expansion of macro ‘HPAGE_PMD_SHIFT’
   111 | #define HPAGE_PMD_SIZE  ((1UL) << HPAGE_PMD_SHIFT)
       |                                   ^~~~~~~~~~~~~~~
 /mm/damon/paddr.c:321:48: note: in expansion of macro ‘HPAGE_PMD_SIZE’
   321 |                 matched = folio_size(folio) == HPAGE_PMD_SIZE;
       |                                                ^~~~~~~~~~~~~~


This simple test might help you reproducing it:
https://github.com/damonitor/damon-tests/blob/master/corr/tests/build_i386.sh

What about enclosing the entire case with

    #if defined(CONFIG_TRANSPARENT_HUGEPAGE) || defined(CONFIG_HUGETLB_PAGE) ?

>  	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",
>  };

Other parts look good to me :)


Thanks,
SJ

[...]