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

Jonathan Cameron
2025-10-03T10:25:46Z

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;