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

[SeongJae Park <sj@kernel.org>]

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

[...]