Re: [PATCH v6 2/3] mm/mempolicy: Prepare weighted interleave sysfs for memory hotplug

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Fri, 4 Apr 2025 16:46:20 +0900
Rakie Kim <rakie.kim@sk.com> wrote:

> Previously, the weighted interleave sysfs structure was statically
> managed during initialization. This prevented new nodes from being
> recognized when memory hotplug events occurred, limiting the ability
> to update or extend sysfs entries dynamically at runtime.
> 
> To address this, this patch refactors the sysfs infrastructure and
> encapsulates it within a new structure, `sysfs_wi_group`, which holds
> both the kobject and an array of node attribute pointers.
> 
> By allocating this group structure globally, the per-node sysfs
> attributes can be managed beyond initialization time, enabling
> external modules to insert or remove node entries in response to
> events such as memory hotplug or node online/offline transitions.
> 
> Instead of allocating all per-node sysfs attributes at once, the
> initialization path now uses the existing sysfs_wi_node_add() and
> sysfs_wi_node_delete() helpers. This refactoring makes it possible
> to modularly manage per-node sysfs entries and ensures the
> infrastructure is ready for runtime extension.
> 
> Signed-off-by: Rakie Kim <rakie.kim@sk.com>
> Signed-off-by: Honggyu Kim <honggyu.kim@sk.com>
> Signed-off-by: Yunjeong Mun <yunjeong.mun@sk.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
Hi Rakie,

Some things I was requesting in patch 1 are done here.
Mostly I think what is wanted is moving some of that
refactoring back to that patch rather than here.

Some of the label and function naming needs another look.

Jonathan

> ---
>  mm/mempolicy.c | 73 ++++++++++++++++++++++----------------------------
>  1 file changed, 32 insertions(+), 41 deletions(-)
> 
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index af3753925573..73a9405ff352 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -3388,6 +3388,13 @@ struct iw_node_attr {
>  	int nid;
>  };
>  
> +struct sysfs_wi_group {
> +	struct kobject wi_kobj;
> +	struct iw_node_attr *nattrs[];
> +};
> +
> +static struct sysfs_wi_group *wi_group;
> +
>  static ssize_t node_show(struct kobject *kobj, struct kobj_attribute *attr,
>  			 char *buf)
>  {
> @@ -3430,27 +3437,24 @@ static ssize_t node_store(struct kobject *kobj, struct kobj_attribute *attr,
>  	return count;
>  }
>  
> -static struct iw_node_attr **node_attrs;
> -
> -static void sysfs_wi_node_release(struct iw_node_attr *node_attr,
> -				  struct kobject *parent)
> +static void sysfs_wi_node_delete(int nid)

Maybe stick to release naming to match the sysfs_wi_release()
below? I don't really care about this.

>  {
> -	if (!node_attr)
> +	if (!wi_group->nattrs[nid])
>  		return;
> -	sysfs_remove_file(parent, &node_attr->kobj_attr.attr);
> -	kfree(node_attr->kobj_attr.attr.name);
> -	kfree(node_attr);
> +
> +	sysfs_remove_file(&wi_group->wi_kobj,
> +			  &wi_group->nattrs[nid]->kobj_attr.attr);
> +	kfree(wi_group->nattrs[nid]->kobj_attr.attr.name);
> +	kfree(wi_group->nattrs[nid]);
>  }
>  
>  static void sysfs_wi_release(struct kobject *wi_kobj)
>  {
> -	int i;
> -
> -	for (i = 0; i < nr_node_ids; i++)
> -		sysfs_wi_node_release(node_attrs[i], wi_kobj);
> +	int nid;
>  
> -	kfree(node_attrs);
> -	kfree(wi_kobj);
> +	for (nid = 0; nid < nr_node_ids; nid++)
> +		sysfs_wi_node_delete(nid);
> +	kfree(wi_group);
>  }

> -static int add_weighted_interleave_group(struct kobject *root_kobj)
> +static int add_weighted_interleave_group(struct kobject *mempolicy_kobj)
>  {
> -	struct kobject *wi_kobj;
>  	int nid, err;
>  
> -	node_attrs = kcalloc(nr_node_ids, sizeof(struct iw_node_attr *),
> -			     GFP_KERNEL);
> -	if (!node_attrs)
> +	wi_group = kzalloc(struct_size(wi_group, nattrs, nr_node_ids),
> +			GFP_KERNEL);

Align to after the ( 
I think it's a couple of spaces short of that.


> +	if (!wi_group)
>  		return -ENOMEM;
>  
> -	wi_kobj = kzalloc(sizeof(struct kobject), GFP_KERNEL);
> -	if (!wi_kobj) {
> -		err = -ENOMEM;
> -		goto node_out;
> -	}
> -
> -	err = kobject_init_and_add(wi_kobj, &wi_ktype, root_kobj,
> +	err = kobject_init_and_add(&wi_group->wi_kobj, &wi_ktype, mempolicy_kobj,
>  				   "weighted_interleave");
> -	if (err) {
> -		kobject_put(wi_kobj);
> +	if (err)
>  		goto err_out;
> -	}
>  
>  	for_each_node_state(nid, N_POSSIBLE) {
> -		err = add_weight_node(nid, wi_kobj);
> +		err = sysfs_wi_node_add(nid);
>  		if (err) {
>  			pr_err("failed to add sysfs [node%d]\n", nid);
> -			break;
> +			goto err_del;
Ah!  This is what I was looking for in patch 1, but it's down here.
Move it back to there.
>  		}
>  	}
> -	if (err) {
> -		kobject_del(wi_kobj);
> -		kobject_put(wi_kobj);
> -		goto err_out;
> -	}
>  
>  	return 0;
>  
> -node_out:
> -	kfree(node_attrs);
> +err_del:
> +	kobject_del(&wi_group->wi_kobj);
>  err_out:
> +	kobject_put(&wi_group->wi_kobj);
Same issue as previous patch on naming of the label.
Moving to this single error block is fine but belongs in patch 1.

>  	return err;
>  }
>