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

[SeongJae Park <sj@kernel.org>]

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

[...]