| 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: <ddcdd8b9-566c-4f6c-b1f7-861e93a80fbb@redhat.com>
Date: Wed, 4 Jun 2025 14:03:23 +0200
From: David Hildenbrand <david@...hat.com>
To: Oscar Salvador <osalvador@...e.de>,
Andrew Morton <akpm@...ux-foundation.org>
Cc: Vlastimil Babka <vbabka@...e.cz>,
Jonathan Cameron <Jonathan.Cameron@...wei.com>,
Harry Yoo <harry.yoo@...cle.com>, Rakie Kim <rakie.kim@...com>,
Hyeonggon Yoo <42.hyeyoo@...il.com>, linux-mm@...ck.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/3] mm,memory_hotplug: Implement numa node notifier
On 03.06.25 13:08, Oscar Salvador wrote:
> There are at least six consumers of hotplug_memory_notifier that what they
> really are interested in is whether any numa node changed its state, e.g: going
> from being memory aware to becoming memoryless and vice versa.
>
> Implement a specific notifier for numa nodes when their state gets changed,
> and have those consumers that only care about numa node state changes use it.
>
> Signed-off-by: Oscar Salvador <osalvador@...e.de>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> Reviewed-by: Harry Yoo <harry.yoo@...cle.com>
> Reviewed-by: Vlastimil Babka <vbabka@...e.cz>
> ---
> drivers/acpi/numa/hmat.c | 6 +-
> drivers/base/node.c | 21 +++++
> drivers/cxl/core/region.c | 14 ++--
> drivers/cxl/cxl.h | 4 +-
> include/linux/memory.h | 38 ++++++++-
> kernel/cgroup/cpuset.c | 2 +-
> mm/memory-tiers.c | 8 +-
> mm/memory_hotplug.c | 161 +++++++++++++++++---------------------
> mm/mempolicy.c | 8 +-
> mm/slub.c | 13 ++-
> 10 files changed, 155 insertions(+), 120 deletions(-)
>
> diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> index 9d9052258e92..9ac82a767daf 100644
> --- a/drivers/acpi/numa/hmat.c
> +++ b/drivers/acpi/numa/hmat.c
> @@ -962,10 +962,10 @@ static int hmat_callback(struct notifier_block *self,
> unsigned long action, void *arg)
> {
> struct memory_target *target;
> - struct memory_notify *mnb = arg;
> + struct node_notify *mnb = arg;
> int pxm, nid = mnb->status_change_nid;
>
> - if (nid == NUMA_NO_NODE || action != MEM_ONLINE)
> + if (nid == NUMA_NO_NODE || action != NODE_BECAME_MEM_AWARE)
> return NOTIFY_OK;
>
> pxm = node_to_pxm(nid);
> @@ -1118,7 +1118,7 @@ static __init int hmat_init(void)
> hmat_register_targets();
>
> /* Keep the table and structures if the notifier may use them */
> - if (hotplug_memory_notifier(hmat_callback, HMAT_CALLBACK_PRI))
> + if (hotplug_node_notifier(hmat_callback, HMAT_CALLBACK_PRI))
> goto out_put;
>
> if (!hmat_set_default_dram_perf())
> diff --git a/drivers/base/node.c b/drivers/base/node.c
[...]
> diff --git a/include/linux/memory.h b/include/linux/memory.h
> index 5ec4e6d209b9..8c5c88eaffb3 100644
> --- a/include/linux/memory.h
> +++ b/include/linux/memory.h
> @@ -99,6 +99,14 @@ int set_memory_block_size_order(unsigned int order);
> #define MEM_PREPARE_ONLINE (1<<6)
> #define MEM_FINISH_OFFLINE (1<<7)
>
> +/* These states are used for numa node notifiers */
> +#define NODE_BECOMING_MEM_AWARE (1<<0)
> +#define NODE_BECAME_MEM_AWARE (1<<1)
> +#define NODE_BECOMING_MEMORYLESS (1<<2)
> +#define NODE_BECAME_MEMORYLESS (1<<3)
> +#define NODE_CANCEL_MEM_AWARE (1<<4)
> +#define NODE_CANCEL_MEMORYLESS (1<<5)
Very nitpicky: MEM vs. MEMORY inconsistency. Also, I am not sure about
"MEMORYLESS vs. MEMORY AWARE" terminology (opposite of aware is not
less) and "BECOMING" vs. "CANCEL" ...
There must be something better ... but what is it. :)
NODE_ADDING_FIRST_MEMORY
NODE_ADDED_FIRST_MEMORY
NODE_CANCEL_ADDING_FIRST_MEMORY
NODE_REMOVING_LAST_MEMORY
NODE_REMOVED_LAST_MEMORY
NODE_CANCEL_REMOVING_LAST_MEMORY
Maybe something like that? I still don't quite like the "CANCEL" stuff.
NODE_ADDING_FIRST_MEMORY
NODE_ADDED_FIRST_MEMORY
NODE_NOT_ADDED_FIRST_MEMORY
NODE_REMOVING_LAST_MEMORY
NODE_REMOVED_LAST_MEMORY
NODE_NOT_REMOVED_LAST_MEMORY
Hm ...
> +
> struct memory_notify {
> /*
> * The altmap_start_pfn and altmap_nr_pages fields are designated for
> @@ -109,7 +117,10 @@ struct memory_notify {
> unsigned long altmap_nr_pages;
> unsigned long start_pfn;
> unsigned long nr_pages;
> - int status_change_nid_normal;
> + int status_change_nid;
> +};
Could/should that be a separate patch after patch #1 removed the last user?
Also, I think the sequence should be (this patch is getting hard to
review for me due to the size):
#1 existing patch 1
#2 remove status_change_nid_normal
#3 introduce node notifier
#4-#X: convert individual users to node notifier
#X+1: change status_change_nid to always just indicate the nid, renaming
it on the way (incl current patch #3)
> +
> +struct node_notify {
> int status_change_nid;
This should be called "nid" right from the start.
>
> @@ -157,15 +168,34 @@ static inline unsigned long memory_block_advised_max_size(void)
> {
> return 0;
> }
> +
[...]
> * {on,off}lining is constrained to full memory sections (or more
> @@ -1194,11 +1172,22 @@ int online_pages(unsigned long pfn, unsigned long nr_pages,
> /* associate pfn range with the zone */
> move_pfn_range_to_zone(zone, pfn, nr_pages, NULL, MIGRATE_ISOLATE);
>
> - arg.start_pfn = pfn;
> - arg.nr_pages = nr_pages;
> - node_states_check_changes_online(nr_pages, zone, &arg);
> + node_arg.status_change_nid = NUMA_NO_NODE;
> + if (!node_state(nid, N_MEMORY)) {
> + /* Node is becoming memory aware. Notify consumers */
> + cancel_node_notifier_on_err = true;
> + node_arg.status_change_nid = nid;
> + ret = node_notify(NODE_BECOMING_MEM_AWARE, &node_arg);
> + ret = notifier_to_errno(ret);
> + if (ret)
> + goto failed_addition;
> + }
I assume without NUMA, that code would never trigger? I mean, the whole
notifier doesn't make sense without CONFIG_NUMA :)
--
Cheers,
David / dhildenb
Powered by blists - more mailing lists