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

[Gregory Price <gourry@gourry.net>]

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?

> +
> +	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.
                          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.

> +			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