On Wed, 5 Feb 2025 13:57:05 +0000 Usama Arif <usamaarif642@xxxxxxxxx> wrote:
>
>
> On 04/02/2025 23:10, SeongJae Park wrote:
> > On Mon, 3 Feb 2025 22:55:30 +0000 Usama Arif <usamaarif642@xxxxxxxxx> wrote:
> >
> >> Add min and max files for damon filters to let the userspace decide
> >> the min/max folio size to operate on. This will be needed to decide
> >> what folio sizes to give pa_stat for.
> >
> > I'd prefer implementing the logic with API interface first, and then
> > implementing sysfs interface on top of the API.
> >
> >>
> >> Signed-off-by: Usama Arif <usamaarif642@xxxxxxxxx>
> >> ---
[...]
> >> @@ -1953,6 +1999,13 @@ static int damon_sysfs_add_scheme_filters(struct damos *scheme,
> >> filter->addr_range = sysfs_filter->addr_range;
> >> } else if (filter->type == DAMOS_FILTER_TYPE_TARGET) {
> >> filter->target_idx = sysfs_filter->target_idx;
> >> + } else if (filter->type == DAMOS_FILTER_TYPE_HUGEPAGE) {
> >> + if (sysfs_filter->folio_size.min >
> >> + sysfs_filter->folio_size.max) {
> >> + damos_destroy_filter(filter);
> >> + return -EINVAL;
> >> + }
> >
> > I don't think letting users set invalid min/max is a real problem, as long as
> > the implementation will handle the case (no memory matches the filter). Let's
> > just allow it, like addr_range case. If we really need to disallow this, it
> > would better to do that from damos_commit_filter() like central point.
> >
>
> hmm, does addr_range allow it? it addr_range.end < addr_range.start it returns an error.
> I did it in a similar fashion for hugepage filter if min > max it returns an error.
Oh you're right... Maybe I was just out of mind, sorry. Please keep this part
as is your version to be consistent with the address range code.
Thanks,
SJ
>
>
> Full code of damon_sysfs_add_scheme_filters below:
>
> } else if (filter->type == DAMOS_FILTER_TYPE_ADDR) {
> if (sysfs_filter->addr_range.end <
> sysfs_filter->addr_range.start) {
> damos_destroy_filter(filter);
> return -EINVAL;
> }
> filter->addr_range = sysfs_filter->addr_range;
> } else if (filter->type == DAMOS_FILTER_TYPE_TARGET) {
> filter->target_idx = sysfs_filter->target_idx;
> } else if (filter->type == DAMOS_FILTER_TYPE_HUGEPAGE) {
> if (sysfs_filter->folio_size.min >
> sysfs_filter->folio_size.max) {
> damos_destroy_filter(filter);
> return -EINVAL;
> }
> filter->folio_size = sysfs_filter->folio_size;
> }
>
>
>
> >> + filter->folio_size = sysfs_filter->folio_size;
> >> }
> >>
> >> damos_add_filter(scheme, filter);
> >> --
> >> 2.43.5
> >
> >
> > Thanks,
> > SJ
