| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20251003112540.00002cf7@huawei.com>
Date: Fri, 3 Oct 2025 11:25:40 +0100
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: Raghavendra K T <raghavendra.kt@....com>
CC: <AneeshKumar.KizhakeVeetil@....com>, <Michael.Day@....com>,
<akpm@...ux-foundation.org>, <bharata@....com>, <dave.hansen@...el.com>,
<david@...hat.com>, <dongjoo.linux.dev@...il.com>, <feng.tang@...el.com>,
<gourry@...rry.net>, <hannes@...xchg.org>, <honggyu.kim@...com>,
<hughd@...gle.com>, <jhubbard@...dia.com>, <jon.grimm@....com>,
<k.shutemov@...il.com>, <kbusch@...a.com>, <kmanaouil.dev@...il.com>,
<leesuyeon0506@...il.com>, <leillc@...gle.com>, <liam.howlett@...cle.com>,
<linux-kernel@...r.kernel.org>, <linux-mm@...ck.org>,
<mgorman@...hsingularity.net>, <mingo@...hat.com>, <nadav.amit@...il.com>,
<nphamcs@...il.com>, <peterz@...radead.org>, <riel@...riel.com>,
<rientjes@...gle.com>, <rppt@...nel.org>, <santosh.shukla@....com>,
<shivankg@....com>, <shy828301@...il.com>, <sj@...nel.org>, <vbabka@...e.cz>,
<weixugc@...gle.com>, <willy@...radead.org>, <ying.huang@...ux.alibaba.com>,
<ziy@...dia.com>, <dave@...olabs.net>, <yuanchu@...gle.com>,
<kinseyho@...gle.com>, <hdanton@...a.com>, <harry.yoo@...cle.com>
Subject: Re: [RFC PATCH V3 12/17] sysfs: Add sysfs support to tune scanning
On Thu, 14 Aug 2025 15:33:02 +0000
Raghavendra K T <raghavendra.kt@....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@....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;
Powered by blists - more mailing lists