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

[SeongJae Park <sj@kernel.org>]

On Mon, 20 Jan 2025 20:26:02 +0000 Usama Arif <usamaarif642@gmail.com> wrote:

> 
> 
> On 20/01/2025 20:12, SeongJae Park wrote:
> > On Mon, 20 Jan 2025 19:58:24 +0000 Usama Arif <usamaarif642@gmail.com> 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?
> > 
> > For user-space ABI, like DAMOS_FILTER_TYPE_ADDR, let's use
> > 'schemes/<N>/filters/<F>/' directory.  File names 'min' and 'max' look good to
> > me.
> > 
> > For kernel-space API, again, like DAMOS_FILTER_TYPE_ADDR, let's use the union
> > in 'struct damos_filter'.
> > 
> > If you will do the extension together with this patch, I think the default
> > value is not really needed.  It will only add a bit of complexity.  So if you
> > will do so, I'd recommend not having the default value.
> > 
> > Also, if you will revise this patch series for the extension, could you please
> > split and post the first patch of this series as a separate one?  I think the
> > fix is important and has no reason to be tied with this patch.
> > 
> 
> Yeah I think it might be best if we get a version with flexible folio sizes merged
> from the start, especially as it involves creating user ABI. I will try and send
> something tomorrow.

Makes sense, looking forward to the next spin!

> 
> For the first patch, do I need to resend? Or maybe Andrew could merge whats in
> this v3 for patch 1, we discard patch 2 and I just send the hugepage
> filter implementation separately as a new series..

I think the first patch is important but not urgent.  I'd suggest to drop it
from the next revision of the filters extension series, and resend it if Andrew
neither pick it nor ask some action by a few days or weeks.


Thanks,
SJ