Re: [RFC PATCH V3 12/17] sysfs: Add sysfs support to tune scanning

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Thu, 14 Aug 2025 15:33:02 +0000
Raghavendra K T <raghavendra.kt@amd.com> wrote:

> Support below tunables:
> scan_enable: turn on or turn off mm_struct scanning
> scan_period: initial scan_period (default: 2sec)
> scan_sleep_ms: sleep time between two successive round of scanning and
> migration.
> mms_to_scan: total mm_struct to scan before taking a pause.
> target_node: default regular node to which migration of accessed pages
> is done (this is only fall back mechnism, otherwise target_node
> heuristic is used).
> 
> Signed-off-by: Raghavendra K T <raghavendra.kt@amd.com>

I'd suggest writing
Documentation/ABI/testing/sysfs-...
doc for this as you'll need it in the end and it tends to be easier to review
a doc for this stuff than the code + figuring out where the entrees end up.

Various comments inline.

J
> ---
>  mm/kscand.c | 205 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 205 insertions(+)
> 
> diff --git a/mm/kscand.c b/mm/kscand.c
> index 41321d373be7..a73606f7ca3c 100644
> --- a/mm/kscand.c
> +++ b/mm/kscand.c
> @@ -21,6 +21,7 @@
>  #include <linux/delay.h>
>  #include <linux/cleanup.h>
>  #include <linux/minmax.h>
> +#include <trace/events/kmem.h>
>  
>  #include <asm/pgalloc.h>
>  #include "internal.h"
> @@ -173,6 +174,171 @@ static bool kscand_eligible_srcnid(int nid)
>  	return  !node_is_toptier(nid);
>  }
>  
> +#ifdef CONFIG_SYSFS

See below. Should not be necessary - the compiler should be able to
see these are unused after it squashes the stubs for sysfs calls
in and hence remove this for us.


> +
> +static struct kobj_attribute scan_sleep_ms_attr =
> +	__ATTR_RW(scan_sleep_ms);

Fits on one line under 80 chars.

> +
> +static ssize_t mm_scan_period_ms_show(struct kobject *kobj,
> +					 struct kobj_attribute *attr,
> +					 char *buf)
> +{
> +	return sysfs_emit(buf, "%u\n", kscand_mm_scan_period_ms);
> +}


> +
> +static struct kobj_attribute mms_to_scan_attr =
> +	__ATTR_RW(mms_to_scan);

Fits on one line.

> +
> +static ssize_t scan_enabled_show(struct kobject *kobj,
> +					 struct kobj_attribute *attr,
> +					 char *buf)

Odd indent.

> +{
> +	return sysfs_emit(buf, "%u\n", kscand_scan_enabled ? 1 : 0);
> +}
> +
> +static ssize_t scan_enabled_store(struct kobject *kobj,
> +					  struct kobj_attribute *attr,
> +					  const char *buf, size_t count)

Another odd looking indent.

> +{
> +	unsigned int val;
> +	int err;
> +
> +	err = kstrtouint(buf, 10, &val);

Maybe use kstrtobool

> +	if (err || val > 1)
> +		return -EINVAL;
> +
> +	if (val) {
> +		kscand_scan_enabled = true;
> +		need_wakeup = true;
> +	} else
> +		kscand_scan_enabled = false;
> +
> +	kscand_sleep_expire = 0;
> +	wake_up_interruptible(&kscand_wait);
> +
> +	return count;
> +}
> +
> +static struct kobj_attribute scan_enabled_attr =
> +	__ATTR_RW(scan_enabled);
One line.

> +

> +}
> +static struct kobj_attribute target_node_attr =
> +	__ATTR_RW(target_node);
One line.
> +
> +static struct attribute *kscand_attr[] = {
> +	&scan_sleep_ms_attr.attr,
> +	&mm_scan_period_ms_attr.attr,
> +	&mms_to_scan_attr.attr,
> +	&scan_enabled_attr.attr,
> +	&target_node_attr.attr,
> +	NULL,

No comma for terminating entries as we don't want to add anything
after this.

> +};
> +
> +struct attribute_group kscand_attr_group = {
> +	.attrs = kscand_attr,
> +	.name = "kscand",
> +};
> +#endif
> +
>  static inline int kscand_has_work(void)
>  {
>  	return !list_empty(&kscand_scan.mm_head);
> @@ -1231,11 +1397,45 @@ static int kscand(void *none)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_SYSFS

The functions used have stubs in sysfs.h so no need
to make these conditional on CONFIG_SYSFS.

Let the compilers dead code removal get rid of the structures
above etc as well.

> +extern struct kobject *mm_kobj;
> +static int __init kscand_init_sysfs(struct kobject **kobj)
> +{
> +	int err;
> +
> +	err = sysfs_create_group(*kobj, &kscand_attr_group);
> +	if (err) {
> +		pr_err("failed to register kscand group\n");
> +		goto err_kscand_attr;
> +	}
> +
> +	return 0;
> +
> +err_kscand_attr:
If create group failed, you shouldn't have anything to remove.



> +	sysfs_remove_group(*kobj, &kscand_attr_group);
> +	return err;
> +}
> +
> +static void __init kscand_exit_sysfs(struct kobject *kobj)
> +{
> +		sysfs_remove_group(kobj, &kscand_attr_group);

Odd indent.

> +}
> +#else
> +static inline int __init kscand_init_sysfs(struct kobject **kobj)
> +{
> +	return 0;
> +}
> +static inline void __init kscand_exit_sysfs(struct kobject *kobj)
> +{
> +}
> +#endif
> +
>  static inline void kscand_destroy(void)
>  {
>  	kmem_cache_destroy(kscand_slot_cache);
>  	/* XXX: move below to kmigrated thread */
>  	kmem_cache_destroy(kmigrated_slot_cache);
> +	kscand_exit_sysfs(mm_kobj);

As it's separate setup step, I'd expect to see this called directly
in error paths in kscand_init() rather than via this helper function.
That way it is easy to see it is only called in paths where it is
appropriate.


>  }
>  
>  void __kscand_enter(struct mm_struct *mm)
> @@ -1421,6 +1621,10 @@ static int __init kscand_init(void)
>  		return -ENOMEM;
>  	}
>  
> +	err = kscand_init_sysfs(&mm_kobj);
> +	if (err)
> +		goto err_init_sysfs;
> +
>  	init_list();
>  	err = start_kscand();
>  	if (err)
> @@ -1437,6 +1641,7 @@ static int __init kscand_init(void)
>  
>  err_kscand:
>  	stop_kscand();
> +err_init_sysfs:
>  	kscand_destroy();
>  
>  	return err;