| Message ID | 20250307063534.540-3-rakie.kim@sk.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/mempolicy: Add memory hotplug support in weighted interleave | expand |
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)
On Fri, 7 Mar 2025 10:19:59 -0800 Joshua Hahn <joshua.hahnjy@gmail.com> wrote: Hi Joshua Thank you for your response regarding this patch. > 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. > I completely agree with your observations about the use of ngrp and wi_kobj. When I was working on this patch, I aimed to minimize changes to the existing code. This approach led to the creation of similar code being used differently, as you pointed out. I'll make the necessary adjustments and update the patch to version 2. > > 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 I will also update this issue in version 2. > > Sent using hkml (https://github.com/sjp38/hackermail) > Rakie
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); 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); 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; @@ -3521,14 +3524,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; } @@ -3536,7 +3539,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 = add_weight_node(nid, ngrp->wi_kobj); if (err) { pr_err("failed to add sysfs [node%d]\n", nid); goto err_out; @@ -3547,7 +3550,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; } @@ -3562,7 +3565,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); } @@ -3581,13 +3586,19 @@ static int __init mempolicy_sysfs_init(void) goto err_out; } - 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 mempol_out; } + ngrp->nattrs = kcalloc(nr_node_ids, sizeof(struct iw_node_attr *), + GFP_KERNEL); + if (!ngrp->nattrs) { + err = -ENOMEM; + goto ngrp_out; + } + err = kobject_init_and_add(mempolicy_kobj, &mempolicy_ktype, mm_kobj, "mempolicy"); if (err) @@ -3602,7 +3613,9 @@ static int __init mempolicy_sysfs_init(void) return err; node_out: - kfree(node_attrs); + kfree(ngrp->nattrs); +ngrp_out: + kfree(ngrp); mempol_out: kfree(mempolicy_kobj); err_out:
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(-)