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>
>> ---
>> include/linux/damon.h | 11 +++++++++
>> mm/damon/core.c | 3 +++
>> mm/damon/sysfs-schemes.c | 53 ++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 67 insertions(+)
>>
>> diff --git a/include/linux/damon.h b/include/linux/damon.h
>> index af525252b853..6f30ceeff215 100644
>> --- a/include/linux/damon.h
>> +++ b/include/linux/damon.h
>> @@ -35,6 +35,16 @@ struct damon_addr_range {
>> unsigned long end;
>> };
>>
>> +/**
>> + * struct damon_folio_size - Represents size of folio filter on [@min, @max].
>> + * @min: Min size of the folio (inclusive).
>> + * @max: Max size of the folio (inclusive).
>> + */
>> +struct damon_folio_size {
>> + unsigned long min;
>> + unsigned long max;
>> +};
>> +
>
> I'd suggest giving a more general name, say, damon_size_range, so that we can
> reuse this for any possible future size range based filters or whatever.
>
>> /**
>> * struct damon_region - Represents a monitoring target region.
>> * @ar: The address range of the region.
>> @@ -377,6 +387,7 @@ struct damos_filter {
>> struct damon_addr_range addr_range;
>> int target_idx;
>> };
>> + struct damon_folio_size folio_size;
>
> As this will be used depending on damos_filter->type, let's add this field in
> the above union, together with addr_range and target_idx.
>
> Also, I'd prefer calling this hugepage_size, as we discussed on the previous
> version of this patch series.
>
>> struct list_head list;
>> };
>>
>> diff --git a/mm/damon/core.c b/mm/damon/core.c
>> index c7b981308862..27323e3a800d 100644
>> --- a/mm/damon/core.c
>> +++ b/mm/damon/core.c
>> @@ -776,6 +776,9 @@ static void damos_commit_filter_arg(
>> case DAMOS_FILTER_TYPE_TARGET:
>> dst->target_idx = src->target_idx;
>> break;
>> + case DAMOS_FILTER_TYPE_HUGEPAGE:
>> + dst->folio_size = src->folio_size;
>> + break;
>> default:
>> break;
>> }
>> diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
>> index 98f93ae9f59e..bc7ca43ca9c4 100644
>> --- a/mm/damon/sysfs-schemes.c
>> +++ b/mm/damon/sysfs-schemes.c
>> @@ -316,6 +316,7 @@ struct damon_sysfs_scheme_filter {
>> bool allow;
>> char *memcg_path;
>> struct damon_addr_range addr_range;
>> + struct damon_folio_size folio_size;
>
> Again, I'd prefer calling this hugepage_size.
>
>> int target_idx;
>> };
>>
>> @@ -469,6 +470,43 @@ static ssize_t addr_end_store(struct kobject *kobj,
>> struct damon_sysfs_scheme_filter *filter = container_of(kobj,
>> struct damon_sysfs_scheme_filter, kobj);
>> int err = kstrtoul(buf, 0, &filter->addr_range.end);
>> + return err ? err : count;
>> +}
>
> Let's keep the blank line before 'return' so that this is not unnecessarily
> marked as a diff.
>
>> +
>> +static ssize_t min_show(struct kobject *kobj,
>> + struct kobj_attribute *attr, char *buf)
>> +{
>> + struct damon_sysfs_scheme_filter *filter = container_of(kobj,
>> + struct damon_sysfs_scheme_filter, kobj);
>> +
>> + return sysfs_emit(buf, "%lu\n", filter->folio_size.min);
>> +}
>> +
>> +static ssize_t min_store(struct kobject *kobj,
>> + struct kobj_attribute *attr, const char *buf, size_t count)
>> +{
>> + struct damon_sysfs_scheme_filter *filter = container_of(kobj,
>> + struct damon_sysfs_scheme_filter, kobj);
>> + int err = kstrtoul(buf, 0, &filter->folio_size.min);
>> +
>> + return err ? err : count;
>> +}
>> +
>> +static ssize_t max_show(struct kobject *kobj,
>> + struct kobj_attribute *attr, char *buf)
>> +{
>> + struct damon_sysfs_scheme_filter *filter = container_of(kobj,
>> + struct damon_sysfs_scheme_filter, kobj);
>> +
>> + return sysfs_emit(buf, "%lu\n", filter->folio_size.max);
>> +}
>> +
>> +static ssize_t max_store(struct kobject *kobj,
>> + struct kobj_attribute *attr, const char *buf, size_t count)
>> +{
>> + struct damon_sysfs_scheme_filter *filter = container_of(kobj,
>> + struct damon_sysfs_scheme_filter, kobj);
>> + int err = kstrtoul(buf, 0, &filter->folio_size.max);
>>
>> return err ? err : count;
>> }
>> @@ -519,6 +557,12 @@ static struct kobj_attribute damon_sysfs_scheme_filter_addr_start_attr =
>> static struct kobj_attribute damon_sysfs_scheme_filter_addr_end_attr =
>> __ATTR_RW_MODE(addr_end, 0600);
>>
>> +static struct kobj_attribute damon_sysfs_scheme_filter_min_attr =
>> + __ATTR_RW_MODE(min, 0600);
>> +
>> +static struct kobj_attribute damon_sysfs_scheme_filter_max_attr =
>> + __ATTR_RW_MODE(max, 0600);
>> +
>> static struct kobj_attribute damon_sysfs_scheme_filter_damon_target_idx_attr =
>> __ATTR_RW_MODE(damon_target_idx, 0600);
>>
>> @@ -529,6 +573,8 @@ static struct attribute *damon_sysfs_scheme_filter_attrs[] = {
>> &damon_sysfs_scheme_filter_memcg_path_attr.attr,
>> &damon_sysfs_scheme_filter_addr_start_attr.attr,
>> &damon_sysfs_scheme_filter_addr_end_attr.attr,
>> + &damon_sysfs_scheme_filter_min_attr.attr,
>> + &damon_sysfs_scheme_filter_max_attr.attr,
>> &damon_sysfs_scheme_filter_damon_target_idx_attr.attr,
>> NULL,
>> };
>> @@ -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.
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
