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

[SeongJae Park <sj@kernel.org>]

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