| 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: <400b4d5a-8973-4301-a86b-41e1b412a3da@redhat.com>
Date: Mon, 16 Jun 2025 14:35:54 +0200
From: David Hildenbrand <david@...hat.com>
To: Oscar Salvador <osalvador@...e.de>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
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>, Joshua Hahn <joshua.hahnjy@...il.com>,
linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 03/10] mm,memory_hotplug: Implement numa node notifier
On 16.06.25 14:32, Oscar Salvador wrote:
> On Mon, Jun 16, 2025 at 02:21:02PM +0200, David Hildenbrand wrote:
>> Exactly. I recall I checked some of them in the past as well, when I
>> stumbled over this behavior.
>
> Now, about simplying the cancel_{mem,node}_notifier_on_err.
> It would look like this:
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index d6df85452c72..ff887f10b114 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1150,11 +1150,16 @@ void mhp_deinit_memmap_on_memory(unsigned long pfn, unsigned long nr_pages)
> int online_pages(unsigned long pfn, unsigned long nr_pages,
> struct zone *zone, struct memory_group *group)
> {
> - bool cancel_mem_notifier_on_err = false, cancel_node_notifier_on_err = false;
> const int nid = zone_to_nid(zone);
> int need_zonelists_rebuild = 0;
> - struct memory_notify mem_arg;
> - struct node_notify node_arg;
> + struct memory_notify mem_arg = {
> + .start_pfn = pfn,
> + .nr_pages = nr_pages,
> + .status_change_nid = NUMA_NO_NODE,
> + };
> + struct node_notify node_arg = {
> + .nid = NUMA_NO_NODE,
> + };
> unsigned long flags;
> int ret;
>
> @@ -1173,21 +1178,16 @@ 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);
>
> - node_arg.nid = NUMA_NO_NODE;
> if (!node_state(nid, N_MEMORY)) {
> /* Adding memory to the node for the first time */
> - cancel_node_notifier_on_err = true;
> node_arg.nid = nid;
> + mem_arg.status_change_nid = nid;
> ret = node_notify(NODE_ADDING_FIRST_MEMORY, &node_arg);
> ret = notifier_to_errno(ret);
> if (ret)
> goto failed_addition;
> }
>
> - mem_arg.start_pfn = pfn;
> - mem_arg.nr_pages = nr_pages;
> - mem_arg.status_change_nid = node_arg.nid;
> - cancel_mem_notifier_on_err = true;
> ret = memory_notify(MEM_GOING_ONLINE, &mem_arg);
> ret = notifier_to_errno(ret);
> if (ret)
> @@ -1249,9 +1249,8 @@ int online_pages(unsigned long pfn, unsigned long nr_pages,
> pr_debug("online_pages [mem %#010llx-%#010llx] failed\n",
> (unsigned long long) pfn << PAGE_SHIFT,
> (((unsigned long long) pfn + nr_pages) << PAGE_SHIFT) - 1);
> - if (cancel_mem_notifier_on_err)
> - memory_notify(MEM_CANCEL_ONLINE, &mem_arg);
> - if (cancel_node_notifier_on_err)
> + memory_notify(MEM_CANCEL_ONLINE, &mem_arg);
> + if (node_arg.nid != NUMA_NO_NODE)
> node_notify(NODE_CANCEL_ADDING_FIRST_MEMORY, &node_arg);
> remove_pfn_range_from_zone(zone, pfn, nr_pages);
> return ret;
> @@ -1899,13 +1898,18 @@ static int count_system_ram_pages_cb(unsigned long start_pfn,
> int offline_pages(unsigned long start_pfn, unsigned long nr_pages,
> struct zone *zone, struct memory_group *group)
> {
> - bool cancel_mem_notifier_on_err = false, cancel_node_notifier_on_err = false;
> unsigned long pfn, managed_pages, system_ram_pages = 0;
> const unsigned long end_pfn = start_pfn + nr_pages;
> struct pglist_data *pgdat = zone->zone_pgdat;
> const int node = zone_to_nid(zone);
> - struct memory_notify mem_arg;
> - struct node_notify node_arg;
> + struct memory_notify mem_arg = {
> + .start_pfn = pfn,
> + .nr_pages = nr_pages,
> + .status_change_nid = NUMA_NO_NODE,
> + };
> + struct node_notify node_arg = {
> + .nid = NUMA_NO_NODE,
> + };
> unsigned long flags;
> char *reason;
> int ret;
> @@ -1970,20 +1974,15 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages,
> * 'nr_pages' more. If so, we know that the node will become empty, and
> * so we will clear N_MEMORY for it.
> */
> - node_arg.nid = NUMA_NO_NODE;
> if (nr_pages >= pgdat->node_present_pages) {
> node_arg.nid = node;
> - cancel_node_notifier_on_err = true;
> + mem_arg.status_change_nid = node;
> ret = node_notify(NODE_REMOVING_LAST_MEMORY, &node_arg);
> ret = notifier_to_errno(ret);
> if (ret)
> goto failed_removal_isolated;
> }
>
> - mem_arg.start_pfn = start_pfn;
> - mem_arg.nr_pages = nr_pages;
> - mem_arg.status_change_nid = node_arg.nid;
> - cancel_mem_notifier_on_err = true;
> ret = memory_notify(MEM_GOING_OFFLINE, &mem_arg);
> ret = notifier_to_errno(ret);
> if (ret) {
> @@ -2087,9 +2086,8 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages,
> failed_removal_isolated:
> /* pushback to free area */
> undo_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE);
> - if (cancel_mem_notifier_on_err)
> - memory_notify(MEM_CANCEL_OFFLINE, &mem_arg);
> - if (cancel_node_notifier_on_err)
> + memory_notify(MEM_CANCEL_OFFLINE, &mem_arg);
> + if (node_arg.nid != NUMA_NO_NODE)
> node_notify(NODE_CANCEL_REMOVING_LAST_MEMORY, &node_arg);
> failed_removal_pcplists_disabled:
> lru_cache_enable();
>
>
> Not sure if I like keeping the cancel_* stuff.
> Strong opinion here? Feelings? :-)
Looks cleaner to me at least :)
--
Cheers,
David / dhildenb
Powered by blists - more mailing lists