Re: [PATCH 2/4] mm/mempolicy: Enable sysfs support for memory hotplug in weighted interleave

[Joshua Hahn <joshua.hahnjy@gmail.com>]

On Fri,  7 Mar 2025 15:35:31 +0900 Rakie Kim <rakie.kim@sk.com> wrote:

Hi Rakie, thank you for your work on this patch! I think it makes a lot of
sense, given the discussion between Gregory & Honggyu in the weighted
interleave auto-tuning patch.

I have a few small nits and questions that I wanted to raise, but none that
should change the behavior at all : -)

> 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.
> 
> Signed-off-by: Rakie Kim <rakie.kim@sk.com>
> ---
>  mm/mempolicy.c | 51 +++++++++++++++++++++++++++++++-------------------
>  1 file changed, 32 insertions(+), 19 deletions(-)
> 
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 385607179ebd..fc10a9a4be86 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;
> +
>  static ssize_t node_show(struct kobject *kobj, struct kobj_attribute *attr,
>  			 char *buf)
>  {
> @@ -3431,8 +3438,6 @@ 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)
>  {
> @@ -3448,7 +3453,7 @@ 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);
> +		sysfs_wi_node_release(ngrp->nattrs[i], wi_kobj);

Nit: I think it is slightly awkward to have a global struct ngrp, and then have
its members passed individually like this. Of course there's nothing that
we can do for sysfs_wi_release's argument, but I think we can make the
arguments for sysfs_wi_node_release a bit cleaner. An idea would just be to
pass an integer (nid) instead of the nattrs[i] pointer. We also don't need
to pass wi_kobj, since it is accessible from within sysfs_wi_node_release.

Once we make both these changes, patch 3 becomes a little bit cleaner (IMHO),
where we acquire the lock for the ngrp struct, then access its contents,
and we don't have to pass two pointers as arguments when they are already
accessible via the global struct anyways.

>  	kobject_put(wi_kobj);
>  }
>  
> @@ -3486,12 +3491,10 @@ static int add_weight_node(int nid, struct kobject *wi_kobj)
>  		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,10 +3507,10 @@ static int wi_node_notifier(struct notifier_block *nb,
>  
>  	switch(action) {
>  	case MEM_ONLINE:
> -		err = add_weight_node(nid, wi_kobj);
> +		err = add_weight_node(nid, ngrp->wi_kobj);

Same idea here, we probably don't need to pass wi_kobj into add_weight_node.
With that said, I can also see the argument for passing the struct itself,
since it saves a line of variable declaration & definition.

[...snip...]

Please let me know what you think! I hope you have a great day, thank you
again for this patch!
Joshua

Sent using hkml (https://github.com/sjp38/hackermail)