Re: [PATCH v4 3/6] mm/damon/sysfs-schemes: add files for setting damos_filter->folio_size

[SeongJae Park <sj@kernel.org>]

On Wed, 5 Feb 2025 13:57:05 +0000 Usama Arif <usamaarif642@gmail.com> wrote:

> 
> 
> On 04/02/2025 23:10, SeongJae Park wrote:
> > On Mon,  3 Feb 2025 22:55:30 +0000 Usama Arif <usamaarif642@gmail.com> 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@gmail.com>
> >> ---
[...]
> >> @@ -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