On Wed, 23 Apr 2025 12:33:50 -0400 Gregory Price <gourry@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
