Re: [PATCH v4 3/3] mm/mempolicy: Support memory hotplug in weighted interleave

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

On Tue, 1 Apr 2025 16:32:42 -0400 Gregory Price <gourry@gourry.net> wrote:
> On Tue, Apr 01, 2025 at 06:08:59PM +0900, Rakie Kim wrote:
> >  static void sysfs_wi_release(struct kobject *wi_kobj)
> > @@ -3464,35 +3477,84 @@ static const struct kobj_type wi_ktype = {
> >  
> >  static int sysfs_wi_node_add(int nid)
> >  {
> ... snip ..
> > +	mutex_lock(&wi_group->kobj_lock);
> > +	if (wi_group->nattrs[nid]) {
> > +		mutex_unlock(&wi_group->kobj_lock);
> > +		pr_info("Node [%d] already exists\n", nid);
> > +		kfree(new_attr);
> > +		kfree(name);
> > +		return 0;
> > +	}
> >  
> > -	if (sysfs_create_file(&wi_group->wi_kobj, &node_attr->kobj_attr.attr)) {
> > -		kfree(node_attr->kobj_attr.attr.name);
> > -		kfree(node_attr);
> > -		pr_err("failed to add attribute to weighted_interleave\n");
> > -		return -ENOMEM;
> > +	wi_group->nattrs[nid] = new_attr;
> > +	mutex_unlock(&wi_group->kobj_lock);
> > +
> 
> Shouldn't all of this
> vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
> > +	sysfs_attr_init(&wi_group->nattrs[nid]->kobj_attr.attr);
> > +	wi_group->nattrs[nid]->kobj_attr.attr.name = name;
> > +	wi_group->nattrs[nid]->kobj_attr.attr.mode = 0644;
> > +	wi_group->nattrs[nid]->kobj_attr.show = node_show;
> > +	wi_group->nattrs[nid]->kobj_attr.store = node_store;
> > +	wi_group->nattrs[nid]->nid = nid;
> > +
> > +	ret = sysfs_create_file(&wi_group->wi_kobj,
> > +				&wi_group->nattrs[nid]->kobj_attr.attr);
> > +	if (ret) {
> > +		kfree(wi_group->nattrs[nid]->kobj_attr.attr.name);
> > +		kfree(wi_group->nattrs[nid]);
> > +		wi_group->nattrs[nid] = NULL;
> > +		pr_err("Failed to add attribute to weighted_interleave: %d\n", ret);
> >  	}
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Be happening inside the lock as well?

I agree that applying your suggestion would make the code more robust.
I will update the patch to follow your recommendation.

> 
> > +
> > +	switch(action) {
> > +	case MEM_ONLINE:
> > +		if (node_state(nid, N_MEMORY)) {
> 
> Hm, I see the issue here, ok, this node_state check isn't needed, as it
> will always be true.  So this function needs to handle duplicates still.

Yes, you're right. The `node_state(nid, N_MEMORY)` check I added here is
redundant because it will always be true in this context. I will remove it
to avoid unnecessary duplication.

>                           vvv 
> > +			err = sysfs_wi_node_add(nid);
> > +			if (err) {
> > +				pr_err("failed to add sysfs [node%d]\n", nid);
> > +				return NOTIFY_BAD;
> > +			}
> > +		}
> > +		break;
> > +	case MEM_OFFLINE:
> > +		if (!node_state(nid, N_MEMORY))
> 
> This check is good for the time being.

This check looks appropriate for now and I'll keep it as-is.

> 
> > +			sysfs_wi_node_release(nid);
> > +		break;
> > +	}
> > +
> > +notifier_end:
> > +	return NOTIFY_OK;
> >  }
> >  
> > 
> 
> But really I think we probably just want to change to build on top of this:
> https://lore.kernel.org/all/20250401092716.537512-2-osalvador@suse.de/
> 
> And use register_node_notifier with NODE_BECAME_MEMORYLESS and NODE_BECAME_MEM_AWARE
> 
> ~Gregory

Thank you for sharing the link regarding `node_notify`. I agree that the
mechanism you pointed out would be a better fit for this patch.

By using `register_node_notifier` with `NODE_BECAME_MEMORYLESS` and
`NODE_BECAME_MEM_AWARE`, we can avoid unnecessary callbacks and implement
this functionality more efficiently.

However, I think it would be better to apply the current patch first and
then update it to use `node_notify` once that support is finalized and
available upstream.

Rakie