[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20251002141207.00007b58@huawei.com>
Date: Thu, 2 Oct 2025 14:12:07 +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 01/17] mm: Add kscand kthread for PTE A bit scan
On Thu, 14 Aug 2025 15:32:51 +0000
Raghavendra K T <raghavendra.kt@....com> wrote:
> Also add a config option for the same.
>
> High level design:
>
> While (1):
> Scan the slowtier pages belonging to VMAs of a task.
> Add to migation list.
>
> A separate thread:
> Migrate scanned pages to a toptier node based on heuristics.
>
> The overall code is influenced by khugepaged design.
>
> Signed-off-by: Raghavendra K T <raghavendra.kt@....com>
Hi Raghavendra,
A few comments inline.
Thanks,
Jonathan
> diff --git a/mm/kscand.c b/mm/kscand.c
> new file mode 100644
> index 000000000000..f7bbbc70c86a
> --- /dev/null
> +++ b/mm/kscand.c
> @@ -0,0 +1,163 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/mm.h>
Good to pick an order for includes. Various options, my personal
preference is alphabetical with groups of linux/* asm/* and local headers.
> +#include <linux/mm_types.h>
> +#include <linux/sched.h>
> +#include <linux/sched/mm.h>
> +#include <linux/mmu_notifier.h>
> +#include <linux/swap.h>
> +#include <linux/mm_inline.h>
> +#include <linux/kthread.h>
> +#include <linux/string.h>
> +#include <linux/delay.h>
> +#include <linux/cleanup.h>
> +
> +#include <asm/pgalloc.h>
> +#include "internal.h"
...
> +static void kscand_wait_work(void)
> +{
> + const unsigned long scan_sleep_jiffies =
> + msecs_to_jiffies(kscand_scan_sleep_ms);
> +
> + if (!scan_sleep_jiffies)
> + return;
> +
> + kscand_sleep_expire = jiffies + scan_sleep_jiffies;
> +
> + /* Allows kthread to pause scanning */
> + wait_event_timeout(kscand_wait, kscand_should_wakeup(),
> + scan_sleep_jiffies);
> +}
Trivial but more consistent I think to have a blank line here.
> +static void kscand_do_scan(void)
> +{
> + unsigned long iter = 0, mms_to_scan;
> +
> + mms_to_scan = READ_ONCE(kscand_mms_to_scan);
Slightly nicer perhaps as
unsigned long iter = 0;
unsigned long mms_to_scan = READ_ONCE(kscand_mms_to_scan);
> +
> + while (true) {
> + if (unlikely(kthread_should_stop()) ||
> + !READ_ONCE(kscand_scan_enabled))
> + break;
> +
> + if (kscand_has_work())
> + msleep(100);
> +
> + iter++;
> +
> + if (iter >= mms_to_scan)
> + break;
> + cond_resched();
> + }
> +}
> +
> +
> +static int start_kscand(void)
> +{
> + struct task_struct *kthread;
> +
> + guard(mutex)(&kscand_mutex);
> +
> + if (kscand_thread)
> + return 0;
> +
> + kthread = kthread_run(kscand, NULL, "kscand");
> + if (IS_ERR(kscand_thread)) {
That looks like the wrong variable to check.
> + pr_err("kscand: kthread_run(kscand) failed\n");
> + return PTR_ERR(kthread);
> + }
> +
> + kscand_thread = kthread;
> + pr_info("kscand: Successfully started kscand");
> +
> + if (!list_empty(&kscand_scan.mm_head))
You have kscand_has_work() for this.
> + wake_up_interruptible(&kscand_wait);
> +
> + return 0;
> +}
> +
> +static int stop_kscand(void)
> +{
> + guard(mutex)(&kscand_mutex);
> +
> + if (kscand_thread) {
> + kthread_stop(kscand_thread);
> + kscand_thread = NULL;
> + }
> +
> + return 0;
> +}
> +
> +static int __init kscand_init(void)
> +{
> + int err;
> +
> + err = start_kscand();
> + if (err)
> + goto err_kscand;
This breaks the nice principle of side effect free calls
on error. If start_kscand() has failed, I'd not really expect to have
have stop explicitly. I'm not sure you actually do need to do so.
> +
> + return 0;
> +
> +err_kscand:
> + stop_kscand();
> +
> + return err;
> +}
> +subsys_initcall(kscand_init);
Powered by blists - more mailing lists