Message-ID: <Z9Gy-No6pXFWZAyc@gourry-fedora-PF4VCD3F>
Date: Wed, 12 Mar 2025 12:14:48 -0400
From: Gregory Price <gourry@...rry.net>
To: Rakie Kim <rakie.kim@...com>
Cc: akpm@...ux-foundation.org, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, linux-cxl@...r.kernel.org,
	joshua.hahnjy@...il.com, dan.j.williams@...el.com,
	ying.huang@...ux.alibaba.com, kernel_team@...ynix.com,
	honggyu.kim@...com, yunjeong.mun@...com
Subject: Re: [PATCH v2 3/4] mm/mempolicy: Enable sysfs support for memory
 hotplug in weighted interleave

On Wed, Mar 12, 2025 at 04:56:26PM +0900, Rakie Kim wrote:
> Previously, sysfs entries for weighted interleave were only created during
> initialization, preventing dynamically added memory nodes from being recognized.
> 
> This patch enables sysfs registration for nodes added via memory hotplug,
> allowing weighted interleave settings to be updated as the system memory
> configuration changes.
>

In patch 2 you said:

```
With this enhancement, the weighted interleave policy now properly supports
memory hotplug, ensuring that newly added nodes are recognized and sysfs
entries are created accordingly.
```

By description, this claims to accomplish functionally the same thing
patch 2 claim.

The code below actually does two things:

1) Refactors the sysfs code to break out the weighted_interleave group
   into a global that can be referenced by the hotplug callback.

2) The change the the memory hotplug callback to add/remove nodes.

Move the refactor work out ahead in a separate patch to make it easier
to review the changes individually please.

> Signed-off-by: Rakie Kim <rakie.kim@...com>
> ---
>  mm/mempolicy.c | 78 +++++++++++++++++++++++++++++---------------------
>  1 file changed, 45 insertions(+), 33 deletions(-)
> 
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 94efff89e0be..71aff1276d4d 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -3389,6 +3389,13 @@ struct iw_node_attr {
>  	int nid;
>  };
>  
> +struct iw_node_group {
> +	struct kobject *wi_kobj;
> +	struct iw_node_attr **nattrs;
> +};
> +
> +static struct iw_node_group *ngrp;
> +

for a global, this should have a more descriptive name.

And since this actually represents the weighted_interleave sysfs entry,
it should maybe be `sysfs_wi_group`?  Since it will include more than
just nodes (e.g. the upcoming `auto`)

>  static ssize_t node_show(struct kobject *kobj, struct kobj_attribute *attr,
>  			 char *buf)
>  {
> @@ -3431,24 +3438,22 @@ 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_release(int nid)
>  {
> -	if (!node_attr)
> +	if (!ngrp->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(ngrp->wi_kobj, &ngrp->nattrs[nid]->kobj_attr.attr);
> +	kfree(ngrp->nattrs[nid]->kobj_attr.attr.name);
> +	kfree(ngrp->nattrs[nid]);
>  }
>  
>  static void sysfs_wi_release(struct kobject *wi_kobj)
>  {
> -	int i;
> +	int nid;
>  
> -	for (i = 0; i < nr_node_ids; i++)
> -		sysfs_wi_node_release(node_attrs[i], wi_kobj);
> +	for (nid = 0; nid < nr_node_ids; nid++)
> +		sysfs_wi_node_release(nid);
>  	kobject_put(wi_kobj);
>  }
>  
> @@ -3457,7 +3462,7 @@ static const struct kobj_type wi_ktype = {
>  	.release = sysfs_wi_release,
>  };
>  
> -static int add_weight_node(int nid, struct kobject *wi_kobj)
> +static int sysfs_wi_node_add(int nid)
>  {
>  	struct iw_node_attr *node_attr;
>  	char *name;
> @@ -3479,19 +3484,17 @@ static int add_weight_node(int nid, struct kobject *wi_kobj)
>  	node_attr->kobj_attr.store = node_store;
>  	node_attr->nid = nid;
>  
> -	if (sysfs_create_file(wi_kobj, &node_attr->kobj_attr.attr)) {
> +	if (sysfs_create_file(ngrp->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;
>  	}
>  
> -	node_attrs[nid] = node_attr;
> +	ngrp->nattrs[nid] = node_attr;
>  	return 0;
>  }
>  
> -struct kobject *wi_kobj;
> -
>  static int wi_node_notifier(struct notifier_block *nb,
>  			       unsigned long action, void *data)
>  {
> @@ -3504,15 +3507,15 @@ static int wi_node_notifier(struct notifier_block *nb,
>  
>  	switch(action) {
>  	case MEM_ONLINE:
> -		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);
> -			kobject_put(wi_kobj);
> +			kobject_put(ngrp->wi_kobj);
>  			return NOTIFY_BAD;
>  		}
>  		break;
>  	case MEM_OFFLINE:
> -		sysfs_wi_node_release(node_attrs[nid], wi_kobj);
> +		sysfs_wi_node_release(nid);
>  		break;
>  	}
>  
> @@ -3524,14 +3527,14 @@ static int add_weighted_interleave_group(struct kobject *root_kobj)
>  {
>  	int nid, err;
>  
> -	wi_kobj = kzalloc(sizeof(struct kobject), GFP_KERNEL);
> -	if (!wi_kobj)
> +	ngrp->wi_kobj = kzalloc(sizeof(struct kobject), GFP_KERNEL);
> +	if (!ngrp->wi_kobj)
>  		return -ENOMEM;
>  
> -	err = kobject_init_and_add(wi_kobj, &wi_ktype, root_kobj,
> +	err = kobject_init_and_add(ngrp->wi_kobj, &wi_ktype, root_kobj,
>  				   "weighted_interleave");
>  	if (err) {
> -		kfree(wi_kobj);
> +		kfree(ngrp->wi_kobj);
>  		return err;
>  	}
>  
> @@ -3539,7 +3542,7 @@ static int add_weighted_interleave_group(struct kobject *root_kobj)
>  		if (!node_state(nid, N_MEMORY))
>  			continue;
>  
> -		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);
>  			goto err_out;
> @@ -3550,7 +3553,7 @@ static int add_weighted_interleave_group(struct kobject *root_kobj)
>  	return 0;
>  
>  err_out:
> -	kobject_put(wi_kobj);
> +	kobject_put(ngrp->wi_kobj);
>  	return err;
>  }
>  
> @@ -3565,7 +3568,9 @@ static void mempolicy_kobj_release(struct kobject *kobj)
>  	mutex_unlock(&iw_table_lock);
>  	synchronize_rcu();
>  	kfree(old);
> -	kfree(node_attrs);
> +
> +	kfree(ngrp->nattrs);
> +	kfree(ngrp);
>  	kfree(kobj);
>  }
>  
> @@ -3578,17 +3583,23 @@ static int __init mempolicy_sysfs_init(void)
>  	int err;
>  	static struct kobject *mempolicy_kobj;
>  
> -	node_attrs = kcalloc(nr_node_ids, sizeof(struct iw_node_attr *),
> -			     GFP_KERNEL);
> -	if (!node_attrs) {
> +	ngrp = kzalloc(sizeof(*ngrp), GFP_KERNEL);
> +	if (!ngrp) {
>  		err = -ENOMEM;
>  		goto err_out;
>  	}
>  
> +	ngrp->nattrs = kcalloc(nr_node_ids, sizeof(struct iw_node_attr *),
> +			       GFP_KERNEL);
> +	if (!ngrp->nattrs) {
> +		err = -ENOMEM;
> +		goto ngrp_out;
> +	}
> +
>  	mempolicy_kobj = kzalloc(sizeof(*mempolicy_kobj), GFP_KERNEL);
>  	if (!mempolicy_kobj) {
>  		err = -ENOMEM;
> -		goto node_out;
> +		goto nattr_out;
>  	}
>  
>  	err = kobject_init_and_add(mempolicy_kobj, &mempolicy_ktype, mm_kobj,
> @@ -3606,12 +3617,13 @@ static int __init mempolicy_sysfs_init(void)
>  
>  	return 0;
>  
> -node_out:
> -	kfree(node_attrs);
> +nattr_out:
> +	kfree(ngrp->nattrs);
> +ngrp_out:
> +	kfree(ngrp);
>  err_out:
>  	pr_err("mempolicy sysfs structure failed to initialize\n");
>  	return err;
> -
>  }
>  
>  late_initcall(mempolicy_sysfs_init);
> -- 
> 2.34.1
> 

Powered by blists - more mailing lists