On Mon, 20 Jan 2025 19:58:24 +0000 Usama Arif <usamaarif642@xxxxxxxxx> wrote:
>
>
> On 20/01/2025 19:30, SeongJae Park wrote:
> > On Mon, 20 Jan 2025 20:23:20 +0100 David Hildenbrand <david@xxxxxxxxxx> wrote:
> >
> >> On 20.01.25 20:16, SeongJae Park wrote:
> >>> On Mon, 20 Jan 2025 19:57:10 +0100 David Hildenbrand <david@xxxxxxxxxx> 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.
Thanks,
SJ
