Message-ID: <20250314060053.743-1-rakie.kim@sk.com>
Date: Fri, 14 Mar 2025 15:00:45 +0900
From: Rakie Kim <rakie.kim@...com>
To: David Hildenbrand <david@...hat.com>
Cc: akpm@...ux-foundation.org,
	linux-mm@...ck.org,
	linux-kernel@...r.kernel.org,
	linux-cxl@...r.kernel.org,
	joshua.hahnjy@...il.com,
	dan.j.williams@...el.com,
	ying.huang@...ux.alibaba.com,
	kernel_team@...ynix.com,
	honggyu.kim@...com,
	yunjeong.mun@...com,
	Gregory Price <gourry@...rry.net>,
	Rakie Kim <rakie.kim@...com>
Subject: Re: [PATCH v2 2/4] mm/mempolicy: Support memory hotplug in weighted interleave

On Thu, 13 Mar 2025 23:36:47 +0100 David Hildenbrand <david@...hat.com> wrote:
> On 13.03.25 17:23, Gregory Price wrote:
> > On Thu, Mar 13, 2025 at 03:33:37PM +0900, Rakie Kim wrote:
> >>> I'm fairly certain this logic is wrong.  If I add two memory blocks and
> >>> then remove one, would this logic not remove the sysfs entries despite
> >>> there being a block remaining?
> >>
> >> Regarding the assumption about node configuration:
> >> Are you assuming that a node has two memory blocks and that
> >> MEM_OFFLINE is triggered when one of them is offlined? If so, then
> >> you are correct that this logic would need modification.
> >>
> >> I performed a simple test by offlining a single memory block:
> >> # echo 0 > /sys/devices/system/node/node2/memory100/online
> >>
> >> In this case, MEM_OFFLINE was not triggered. However, I need to
> >> conduct further analysis to confirm this behavior under different
> >> conditions. I will review this in more detail and share my
> >> findings, including the test methodology and results.
> >>
> > 
> > +David - might have a quick answer to this.  I would have expected a
> > single memory block going offline to cause a notification.
> 
> Yes. Unless offlining failed, or the block was already offline :)
> 
> If it doesn't happen for an actual online memory block that is offline 
> after the call, we would have a bug.
> 
> 
> -- 
> Cheers,
> 
> David / dhildenb
>

Hi David,

I am currently working on adding memory hotplug-related functionality
to the weighted interleave feature. While discussing this with Gregory,
a question came up. If you happen to know the answer to the following,
I would greatly appreciate your input.

I have added the following logic to call add_weight_node when a node
transitions to the N_MEMORY state to create a sysfs entry. Conversely,
when all memory blocks of a node go offline (!N_MEMORY),
I call sysfs_wi_node_release to remove the corresponding sysfs entry.

+static int wi_node_notifier(struct notifier_block *nb,
+                              unsigned long action, void *data)
+{
+       int err;
+       struct memory_notify *arg = data;
+       int nid = arg->status_change_nid;
+
+       if (nid < 0)
+               goto notifier_end;
+
+       switch(action) {
+       case MEM_ONLINE:
+               err = add_weight_node(nid, wi_kobj);
+               if (err) {
+                       pr_err("failed to add sysfs [node%d]\n", nid);
+                       kobject_put(wi_kobj);
+                       return NOTIFY_BAD;
+               }
+               break;
+       case MEM_OFFLINE:
+               sysfs_wi_node_release(node_attrs[nid], wi_kobj);
+               break;
+       }
+
+notifier_end:
+       return NOTIFY_OK;
+}

One question I have is whether the MEM_OFFLINE action in the code
below will be triggered when a node that consists of multiple memory
blocks has only one of its memory blocks transitioning to the offline state.

+       int nid = arg->status_change_nid;
+
+       if (nid < 0)
+               goto notifier_end;

Based on my analysis, wi_node_notifier should function as expected
because arg->status_change_nid only holds a non-negative value
under the following conditions:

1) !N_MEMORY -> N_MEMORY
   When the first memory block of a node transitions to the online state,
   it holds a non-negative value.
   In all other cases, it remains -1 (NUMA_NO_NODE).

2) N_MEMORY -> !N_MEMORY
   When all memory blocks of a node transition to the offline state,
   it holds a non-negative value.
   In all other cases, it remains -1 (NUMA_NO_NODE).

I would truly appreciate it if you could confirm whether this analysis is correct.
Below is a more detailed explanation of my findings.

<memory block online>
- The callback function registered in hotplug_memory_notifier
  receives the MEM_ONLINE action in online_pages.
int online_pages(unsigned long pfn, unsigned long nr_pages,
		       struct zone *zone, struct memory_group *group)
{
	struct memory_notify arg;
...
	node_states_check_changes_online(nr_pages, zone, &arg);
	ret = memory_notify(MEM_GOING_ONLINE, &arg);
...
	node_states_set_node(nid, &arg);
...
	memory_notify(MEM_ONLINE, &arg);
}

- If the node is in the !N_MEMORY state,
  arg->status_change_nid is set to the node ID.
static void node_states_check_changes_online(unsigned long nr_pages,
	struct zone *zone, struct memory_notify *arg)
{
	int nid = zone_to_nid(zone);

	arg->status_change_nid = NUMA_NO_NODE;
	arg->status_change_nid_normal = NUMA_NO_NODE;

	if (!node_state(nid, N_MEMORY))
		arg->status_change_nid = nid;
...
}
- If arg->status_change_nid >= 0, the node transitions to the N_MEMORY state.
static void node_states_set_node(int node, struct memory_notify *arg)
{
...
	if (arg->status_change_nid >= 0)
		node_set_state(node, N_MEMORY);
}


<memory block offline>
- The callback function registered in hotplug_memory_notifier
  receives the MEM_OFFLINE action in offline_pages.
int offline_pages(unsigned long start_pfn, unsigned long nr_pages,
			struct zone *zone, struct memory_group *group)
{
	struct memory_notify arg;
...
	node_states_check_changes_offline(nr_pages, zone, &arg);
	ret = memory_notify(MEM_GOING_OFFLINE, &arg);
...
	node_states_clear_node(node, &arg);
...
	memory_notify(MEM_OFFLINE, &arg);
}

- If the node becomes empty,
  arg->status_change_nid is set to the node ID.
static void node_states_check_changes_offline(unsigned long nr_pages,
		struct zone *zone, struct memory_notify *arg)
{
...
	/*
	 * We have accounted the pages from [0..ZONE_NORMAL); ZONE_HIGHMEM
	 * does not apply as we don't support 32bit.
	 * Here we count the possible pages from ZONE_MOVABLE.
	 * If after having accounted all the pages, we see that the nr_pages
	 * to be offlined is over or equal to the accounted pages,
	 * we know that the node will become empty, and so, we can clear
	 * it for N_MEMORY as well.
	 */
	present_pages += pgdat->node_zones[ZONE_MOVABLE].present_pages;

	if (nr_pages >= present_pages)
		arg->status_change_nid = zone_to_nid(zone);
}

- If arg->status_change_nid >= 0,
  the node transitions to the !N_MEMORY state.
static void node_states_clear_node(int node, struct memory_notify *arg)
{
...
	if (arg->status_change_nid >= 0)
		node_clear_state(node, N_MEMORY);
}

Thank you very much for taking the time to read this lengthy email.

Rakie

Powered by blists - more mailing lists