Re: [PATCH v3 2/3] mm,memory_hotplug: Implement numa node notifier

[Honggyu Kim <honggyu.kim@sk.com>]

Hi Gregory and Oscar,

On 5/3/2025 12:28 AM, Gregory Price wrote:
> On Fri, May 02, 2025 at 10:36:23AM +0200, 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@suse.de>
>> Reviewed-by: Harry Yoo <harry.yoo@oracle.com>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>   
>> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
>> index f43951668c41..b3ad63fb3a2b 100644
>> --- a/mm/mempolicy.c
>> +++ b/mm/mempolicy.c
>> @@ -3591,20 +3591,20 @@ static int wi_node_notifier(struct notifier_block *nb,
>>   			       unsigned long action, void *data)
>>   {
>>   	int err;
>> -	struct memory_notify *arg = data;
>> +	struct node_notify *arg = data;
>>   	int nid = arg->status_change_nid;
>>   
>>   	if (nid < 0)
>>   		return NOTIFY_OK;
>>   
>>   	switch (action) {
>> -	case MEM_ONLINE:
>> +	case NODE_BECAME_MEM_AWARE:
>>   		err = sysfs_wi_node_add(nid);
>>   		if (err)
>>   			pr_err("failed to add sysfs for node%d during hotplug: %d\n",
>>   			       nid, err);
>>   		break;
> 
> May I suggest rolling this patch in with this change:
> https://lore.kernel.org/linux-mm/aAij2oUCP1zmcoPv@stanley.mountain/
> 
> seems to fix the underlying problem, and returning an error now makes
> sense given the change.

The 'err' of sysfs_wi_node_add() wasn't propagated to its caller before
this change as discussed with David at the following.
https://lore.kernel.org/198f2cbe-b1cb-4239-833e-9aac33d978fa@redhat.com

But as Gregory mentioned, we can pass 'err' now with this numa node notifier
so for this hunk, shouldn't we add the following change on top of this?

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 3a7717e09506..3073ebd4e7ee 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -3792,7 +3792,7 @@ static int sysfs_wi_node_add(int nid)
  static int wi_node_notifier(struct notifier_block *nb,
                                unsigned long action, void *data)
  {
-       int err;
+       int err = 0;
         struct node_notify *arg = data;
         int nid = arg->status_change_nid;

@@ -3811,7 +3811,7 @@ static int wi_node_notifier(struct notifier_block *nb,
                 break;
         }

-       return NOTIFY_OK;
+       return notifier_from_errno(err);
  }

  static int __init add_weighted_interleave_group(struct kobject *mempolicy_kobj)

> 
> +cc: Honggyu Kim, Dan Carpenter

Thanks for cc-ing me into this thread.

Honggyu

> 
>> -	case MEM_OFFLINE:
>> +	case NODE_BECAME_MEMORYLESS:
>>   		sysfs_wi_node_delete(nid);
>>   		break;
>>   	}
>> @@ -3639,7 +3639,7 @@ static int __init add_weighted_interleave_group(struct kobject *mempolicy_kobj)
>>   		}
>>   	}
>>   
>> -	hotplug_memory_notifier(wi_node_notifier, DEFAULT_CALLBACK_PRI);
>> +	hotplug_node_notifier(wi_node_notifier, DEFAULT_CALLBACK_PRI);
>>   	return 0;
>>   
>>   err_cleanup_kobj: