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

[Usama Arif <usamaarif642@gmail.com>]

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.

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..