Re: [PATCH next] mm/mempolicy: Fix error code in sysfs_wi_node_add()

[Rakie Kim <rakie.kim@sk.com>]

On Wed, 23 Apr 2025 12:33:50 -0400 Gregory Price <gourry@gourry.net> wrote:
> On Wed, Apr 23, 2025 at 11:24:58AM +0300, Dan Carpenter wrote:
> > Return -EEXIST if the node already exists.  Don't return success.
> > 
> > Fixes: 1bf270ac1b0a ("mm/mempolicy: support memory hotplug in weighted interleave")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> > Potentially returning success was intentional?  This is from static
> > analysis and I can't be totally sure.
> 
> I think this was intentional to allow hotplug callbacks to continue
> executing.  I will let the SK folks who wrote the patch confirm/deny.
> 
> If it is intentional, then we need to add a comment here to explain.
> 
> ~Gregory
> 
> > 
> >  mm/mempolicy.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> > index f43951668c41..0538a994440a 100644
> > --- a/mm/mempolicy.c
> > +++ b/mm/mempolicy.c
> > @@ -3539,7 +3539,7 @@ static const struct kobj_type wi_ktype = {
> >  
> >  static int sysfs_wi_node_add(int nid)
> >  {
> > -	int ret = 0;
> > +	int ret;
> >  	char *name;
> >  	struct iw_node_attr *new_attr;
> >  
> > @@ -3569,6 +3569,7 @@ static int sysfs_wi_node_add(int nid)
> >  	if (wi_group->nattrs[nid]) {
> >  		mutex_unlock(&wi_group->kobj_lock);
> >  		pr_info("node%d already exists\n", nid);
> > +		ret = -EEXIST;
> >  		goto out;
> >  	}
> >  
> > -- 
> > 2.47.2
> > 
> 

Hi Dan,

Thank you very much for analyzing the issue in this code and for
sharing a detailed patch to address it. Your review is greatly
appreciated.

However, the current behavior of returning success instead of an
-EEXIST or other error code was intentional. I would like to explain
the rationale for this choice and would appreciate your further
thoughts.

The condition:

	if (wi_group->nattrs[nid]) {
		mutex_unlock(&wi_group->kobj_lock);
		pr_info("node%d already exists\n", nid);
		goto out;
	}

is triggered in the following two cases:

1. If `sysfs_wi_node_delete()` fails:
   - This function only performs `sysfs_remove_file()` and frees
     memory, and these operations do not fail in a way that would leave
     the system in an inconsistent state.

2. If `sysfs_wi_node_add()` is invoked multiple times for the same node:
   - While repeated additions for the same node would indicate a
     potential issue in logic, simply skipping the redundant addition
     does not cause a functional problem. The original sysfs entry
     remains valid and continues to work as expected.

Therefore, I chose to return success in this case to avoid interrupting
the flow unnecessarily.

Also, as you pointed out, even if we returned -EEXIST here, it would not
change the runtime behavior. This is because `sysfs_wi_node_add()` is
called from the following memory notifier:

	static int wi_node_notifier(struct notifier_block *nb,
				     unsigned long action, void *data)
	{
		...
		switch (action) {
		case MEM_ONLINE:
			err = sysfs_wi_node_add(nid);
			if (err)
				pr_err("failed to add sysfs for node%d during hotplug: %d\n",
				       nid, err);
			break;
		...
		}
		return NOTIFY_OK;
	}

As discussed in prior reviews (including suggestions by David
Hildenbrand), returning NOTIFY_BAD on failure can interfere with other
notifier chains due to NOTIFY_STOP_MASK behavior. Hence, we always
return NOTIFY_OK to preserve consistent hotplug handling across
subsystems.
I would sincerely appreciate it if you could share any further thoughts
or concerns you may have regarding this decision.

Rakie