Message-ID: <20250313063416.703-1-rakie.kim@sk.com>
Date: Thu, 13 Mar 2025 15:34:10 +0900
From: Rakie Kim <rakie.kim@...com>
To: Gregory Price <gourry@...rry.net>
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,
	Rakie Kim <rakie.kim@...com>
Subject: Re: [PATCH v2 3/4] mm/mempolicy: Enable sysfs support for memory hotplug in weighted interleave

On Wed, 12 Mar 2025 12:14:48 -0400 Gregory Price <gourry@...rry.net> wrote:
> 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.

This change primarily addresses 1) from your feedback. The
modification to the memory hotplug callback was necessary to adapt
to the new `struct iw_node_group`.

Given that this adjustment is part of integrating the refactored
structure, I believe this patch does not need to be split into two.
However, I would appreciate any further input you may have on this.

> 
> > 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`)

Regarding your naming suggestion, I agree that `sysfs_wi_group` is
more descriptive and aligns well with its role in managing the
weighted_interleave sysfs entry. I will update the patch accordingly.

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