| Message ID | 20250404074623.1179-3-rakie.kim@sk.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enhance sysfs handling for memory hotplug in weighted interleave | expand |
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; > } >
Jonathan Cameron wrote: > 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 [..] > > @@ -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. I had asked for "delete" to pair with "add" and to not get confused with a final kobject_put() callback.
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) { - 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 const struct kobj_type wi_ktype = { @@ -3458,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; @@ -3480,58 +3484,45 @@ 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(&wi_group->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; + wi_group->nattrs[nid] = node_attr; return 0; } -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); + 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; } } - 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); return err; }