On Wed, 19 Mar 2025 19:30:16 +0000
Raghavendra K T <raghavendra.kt@xxxxxxx> wrote:
> Add a skeleton to support scanning and migration.
> 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
>
> Separate thread:
> migrate scanned pages to a toptier node based on heuristics
>
> The overall code is heavily influenced by khugepaged design.
>
> Signed-off-by: Raghavendra K T <raghavendra.kt@xxxxxxx>
I'm really bad and reading code and not commenting on the 'small'
stuff. So feel free to ignore this given the RFC status!
This sort of read through helps me get my head around a series.
> ---
> mm/Kconfig | 8 +++
> mm/Makefile | 1 +
> mm/kmmscand.c | 176 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 185 insertions(+)
> create mode 100644 mm/kmmscand.c
>
> diff --git a/mm/Kconfig b/mm/Kconfig
> index 1b501db06417..5a4931633e15 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -783,6 +783,14 @@ config KSM
> until a program has madvised that an area is MADV_MERGEABLE, and
> root has set /sys/kernel/mm/ksm/run to 1 (if CONFIG_SYSFS is set).
>
> +config KMMSCAND
> + bool "Enable PTE A bit scanning and Migration"
> + depends on NUMA_BALANCING
> + help
> + Enable PTE A bit scanning of page. CXL pages accessed are migrated to
Trivial but don't mention CXL. "Other memory tier solutions are available"
> + a regular NUMA node. The option creates a separate kthread for
> + scanning and migration.
> +
> diff --git a/mm/kmmscand.c b/mm/kmmscand.c
> new file mode 100644
> index 000000000000..6c55250b5cfb
> --- /dev/null
> +++ b/mm/kmmscand.c
> +
> +struct kmmscand_scan kmmscand_scan = {
> + .mm_head = LIST_HEAD_INIT(kmmscand_scan.mm_head),
> +};
> +
> +static int kmmscand_has_work(void)
> +{
Unless this is going to get more complex, I'd just put
the implementation inline. Kind of obvious what is doing
so the wrapper doesn't add much.
> + return !list_empty(&kmmscand_scan.mm_head);
> +}
> +
> +static bool kmmscand_should_wakeup(void)
> +{
> + bool wakeup = kthread_should_stop() || need_wakeup ||
bonus space after =
> + time_after_eq(jiffies, kmmscand_sleep_expire);
> + if (need_wakeup)
> + need_wakeup = false;
Why not set it unconditionally? If it is false already, no
harm done and removes need to check.
> +
> + return wakeup;
> +}
> +
> +static void kmmscand_wait_work(void)
> +{
> + const unsigned long scan_sleep_jiffies =
> + msecs_to_jiffies(kmmscand_scan_sleep_ms);
> +
> + if (!scan_sleep_jiffies)
> + return;
> +
> + kmmscand_sleep_expire = jiffies + scan_sleep_jiffies;
> + wait_event_timeout(kmmscand_wait,
> + kmmscand_should_wakeup(),
> + scan_sleep_jiffies);
strange wrap. Maybe add a comment on why we don't care if
this timed out or not.
> + return;
> +}
> +
> +static unsigned long kmmscand_scan_mm_slot(void)
> +{
> + /* placeholder for scanning */
I guess this will make sense later in series!
> + msleep(100);
> + return 0;
> +}
> +
> +static void kmmscand_do_scan(void)
> +{
> + unsigned long iter = 0, mms_to_scan;
> +
unsigned long mms_to_scan = READ_ONCE(kmmscand_mms_to_scan);
> + mms_to_scan = READ_ONCE(kmmscand_mms_to_scan);
> +
> + while (true) {
> + cond_resched();
Odd to do this at start. Maybe at end of loop?
> +
> + if (unlikely(kthread_should_stop()) ||
> + !READ_ONCE(kmmscand_scan_enabled))
> + break;
return; Then we don't need to read on to see if anything else happens.
> +
> + if (kmmscand_has_work())
> + kmmscand_scan_mm_slot();
> +
> + iter++;
> + if (iter >= mms_to_scan)
> + break;
return;
Same argument as above.
> + }
> +}
> +
> +static int kmmscand(void *none)
> +{
> + for (;;) {
while (true) maybe. Feels more natural to me for a loop
with no terminating condition. Obviously same thing in practice.
> + if (unlikely(kthread_should_stop()))
return;
> + break;
> +
> + kmmscand_do_scan();
> +
> + while (!READ_ONCE(kmmscand_scan_enabled)) {
> + cpu_relax();
> + kmmscand_wait_work();
> + }
> +
> + kmmscand_wait_work();
> + }
> + return 0;
> +}
> +
> +static int start_kmmscand(void)
> +{
> + int err = 0;
> +
> + guard(mutex)(&kmmscand_mutex);
> +
> + /* Some one already succeeded in starting daemon */
> + if (kmmscand_thread)
return 0;
> + goto end;
> +
> + kmmscand_thread = kthread_run(kmmscand, NULL, "kmmscand");
> + if (IS_ERR(kmmscand_thread)) {
> + pr_err("kmmscand: kthread_run(kmmscand) failed\n");
> + err = PTR_ERR(kmmscand_thread);
> + kmmscand_thread = NULL;
Use a local variable instead and only assign on success. That
way you don't need to null it out in this path.
> + goto end;
return PTR_ERR(kmmscand_thread_local);
> + } else {
> + pr_info("kmmscand: Successfully started kmmscand");
No need for else give the other path exits.
> + }
> +
> + if (!list_empty(&kmmscand_scan.mm_head))
> + wake_up_interruptible(&kmmscand_wait);
> +
> +end:
> + return err;
> +}
> +
> +static int stop_kmmscand(void)
> +{
> + int err = 0;
No point in err if always 0.
> +
> + guard(mutex)(&kmmscand_mutex);
> +
> + if (kmmscand_thread) {
> + kthread_stop(kmmscand_thread);
> + kmmscand_thread = NULL;
> + }
> +
> + return err;
> +}
> +
> +static int __init kmmscand_init(void)
> +{
> + int err;
> +
> + err = start_kmmscand();
> + if (err)
> + goto err_kmmscand;
start_kmmscand() should be side effect free if it is returning an
error. Not doing that makes for hard to read code.
Superficially looks like it is already side effect free so you
can probably just return here.
> +
> + return 0;
> +
> +err_kmmscand:
> + stop_kmmscand();
> +
> + return err;
> +}
> +subsys_initcall(kmmscand_init);
