| Message ID | 20250320041749.881-2-rakie.kim@sk.com |
|---|---|
| State | New |
| Headers | show |
| Series | Enhance sysfs handling for memory hotplug in weighted interleave | expand |
On Thu, 20 Mar 2025 13:17:46 +0900 Rakie Kim <rakie.kim@sk.com> wrote: Hi Gregory I initially planned to separate this patch from the hotplug-related patch series as an independent update. However, after reviewing the code following Jonathan's suggestion to consolidate `kobject` and `node_attrs` into a single struct, I realized that most of the intended functionality for Patch 2 was already incorporated. As a result, Patch 1 now only contains the `kobject_put` fix, while the struct consolidation work has been included in Patch 2. Given the current design, it seems more natural to keep Patch 1 and Patch 2 together as part of the same patch series rather than separating them. Rakie > Memory leaks occurred when removing sysfs attributes for weighted > interleave. Improper kobject deallocation led to unreleased memory > when initialization failed or when nodes were removed. > > This patch resolves the issue by replacing unnecessary `kfree()` > calls with `kobject_put()`, ensuring proper cleanup and preventing > memory leaks. > > By correctly using `kobject_put()`, the release function now > properly deallocates memory without causing resource leaks, > thereby improving system stability. > > Fixes: dce41f5ae253 ("mm/mempolicy: implement the sysfs-based weighted_interleave interface") > Signed-off-by: Rakie Kim <rakie.kim@sk.com> > --- > mm/mempolicy.c | 61 +++++++++++++++++++++++++------------------------- > 1 file changed, 31 insertions(+), 30 deletions(-) > > diff --git a/mm/mempolicy.c b/mm/mempolicy.c > index bbaadbeeb291..5950d5d5b85e 100644 > --- a/mm/mempolicy.c > +++ b/mm/mempolicy.c > @@ -3448,7 +3448,9 @@ static void sysfs_wi_release(struct kobject *wi_kobj) > > for (i = 0; i < nr_node_ids; i++) > sysfs_wi_node_release(node_attrs[i], wi_kobj); > - kobject_put(wi_kobj); > + > + kfree(node_attrs); > + kfree(wi_kobj); > } > > static const struct kobj_type wi_ktype = { > @@ -3494,15 +3496,22 @@ static int add_weighted_interleave_group(struct kobject *root_kobj) > struct kobject *wi_kobj; > int nid, err; > > - wi_kobj = kzalloc(sizeof(struct kobject), GFP_KERNEL); > - if (!wi_kobj) > + node_attrs = kcalloc(nr_node_ids, sizeof(struct iw_node_attr *), > + GFP_KERNEL); > + if (!node_attrs) > 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, > "weighted_interleave"); > if (err) { > - kfree(wi_kobj); > - return err; > + kobject_put(wi_kobj); > + goto err_out; > } > > for_each_node_state(nid, N_POSSIBLE) { > @@ -3512,9 +3521,17 @@ static int add_weighted_interleave_group(struct kobject *root_kobj) > break; > } > } > - if (err) > + if (err) { > kobject_put(wi_kobj); > + goto err_out; > + } > + > return 0; > + > +node_out: > + kfree(node_attrs); > +err_out: > + return err; > } > > static void mempolicy_kobj_release(struct kobject *kobj) > @@ -3528,7 +3545,6 @@ static void mempolicy_kobj_release(struct kobject *kobj) > mutex_unlock(&iw_table_lock); > synchronize_rcu(); > kfree(old); > - kfree(node_attrs); > kfree(kobj); > } > > @@ -3542,37 +3558,22 @@ static int __init mempolicy_sysfs_init(void) > static struct kobject *mempolicy_kobj; > > mempolicy_kobj = kzalloc(sizeof(*mempolicy_kobj), GFP_KERNEL); > - if (!mempolicy_kobj) { > - err = -ENOMEM; > - goto err_out; > - } > - > - node_attrs = kcalloc(nr_node_ids, sizeof(struct iw_node_attr *), > - GFP_KERNEL); > - if (!node_attrs) { > - err = -ENOMEM; > - goto mempol_out; > - } > + if (!mempolicy_kobj) > + return -ENOMEM; > > err = kobject_init_and_add(mempolicy_kobj, &mempolicy_ktype, mm_kobj, > "mempolicy"); > if (err) > - goto node_out; > + goto err_out; > > err = add_weighted_interleave_group(mempolicy_kobj); > - if (err) { > - pr_err("mempolicy sysfs structure failed to initialize\n"); > - kobject_put(mempolicy_kobj); > - return err; > - } > + if (err) > + goto err_out; > + > + return 0; > > - return err; > -node_out: > - kfree(node_attrs); > -mempol_out: > - kfree(mempolicy_kobj); > err_out: > - pr_err("failed to add mempolicy kobject to the system\n"); > + kobject_put(mempolicy_kobj); > return err; > } > > > base-commit: 4701f33a10702d5fc577c32434eb62adde0a1ae1 > -- > 2.34.1 >
Hi Rakie, thank you for the new version! I have just a few questions / nits about this patch. On Thu, 20 Mar 2025 13:17:46 +0900 Rakie Kim <rakie.kim@sk.com> wrote: > Memory leaks occurred when removing sysfs attributes for weighted > interleave. Improper kobject deallocation led to unreleased memory > when initialization failed or when nodes were removed. > > This patch resolves the issue by replacing unnecessary `kfree()` > calls with `kobject_put()`, ensuring proper cleanup and preventing > memory leaks. > > By correctly using `kobject_put()`, the release function now > properly deallocates memory without causing resource leaks, > thereby improving system stability. > > Fixes: dce41f5ae253 ("mm/mempolicy: implement the sysfs-based weighted_interleave interface") > Signed-off-by: Rakie Kim <rakie.kim@sk.com> > --- > mm/mempolicy.c | 61 +++++++++++++++++++++++++------------------------- > 1 file changed, 31 insertions(+), 30 deletions(-) > > diff --git a/mm/mempolicy.c b/mm/mempolicy.c > index bbaadbeeb291..5950d5d5b85e 100644 > --- a/mm/mempolicy.c > +++ b/mm/mempolicy.c > @@ -3448,7 +3448,9 @@ static void sysfs_wi_release(struct kobject *wi_kobj) > > for (i = 0; i < nr_node_ids; i++) > sysfs_wi_node_release(node_attrs[i], wi_kobj); > - kobject_put(wi_kobj); > + > + kfree(node_attrs); > + kfree(wi_kobj); > } I think the intent here is to make mempolicy_sysfs_init call kobject_put, which will then call sysfs_wi_release when the refcount is 0. So I think replacing kobject_put with kfree makes a lot of sense here. However, I think it is a bit confusing based on the commit message, which states that you are doing the opposite (replacing kfree with kobject_put). Perhaps it makes more sense to say that you are moving kfree() from sysfs_init to the release function, so that the struct and the node_attrs struct is freed together by the last reference holder. > static const struct kobj_type wi_ktype = { > @@ -3494,15 +3496,22 @@ static int add_weighted_interleave_group(struct kobject *root_kobj) > struct kobject *wi_kobj; > int nid, err; > > - wi_kobj = kzalloc(sizeof(struct kobject), GFP_KERNEL); > - if (!wi_kobj) > + node_attrs = kcalloc(nr_node_ids, sizeof(struct iw_node_attr *), > + GFP_KERNEL); > + if (!node_attrs) > return -ENOMEM; It's also not obvious to me why the allocation for node_attrs was moved to add_weighted_interleave_group. Maybe this refactoring belongs in patch 2, whose described intent is to consolidate the two objects into one (I expand on this idea below) > + 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, > "weighted_interleave"); > if (err) { > - kfree(wi_kobj); > - return err; > + kobject_put(wi_kobj); > + goto err_out; > } > > for_each_node_state(nid, N_POSSIBLE) { > @@ -3512,9 +3521,17 @@ static int add_weighted_interleave_group(struct kobject *root_kobj) > break; > } > } > - if (err) > + if (err) { > kobject_put(wi_kobj); > + goto err_out; > + } > + > return 0; > + > +node_out: > + kfree(node_attrs); > +err_out: NIT: Is there a reason why we have a single line goto statement? Maybe it is more readable to replace all `goto err_out` with `return err` and save a few jumps : -) > + return err; > } > > static void mempolicy_kobj_release(struct kobject *kobj) > @@ -3528,7 +3545,6 @@ static void mempolicy_kobj_release(struct kobject *kobj) > mutex_unlock(&iw_table_lock); > synchronize_rcu(); > kfree(old); > - kfree(node_attrs); I think the intent of this patch is slightly confusing. Viewing this patch alone, it is not entirely obvious why the kfree for node_attrs is now being moved from the release of mempolicy_kobj to wi_kobj. Of course, we know that it is actually because this patch serves a secondary purpose of moving the allocations / freeing of nattrs and wi_kobj together, so that in the next patch they can be combined into a single struct. I think one way to make this patch more readable and maintainable is to separate it into (1) fixes, (as the Fixes: tag in your commit message suggests) and (2) refactoring that prepares for the next patch. Please let me know what you think -- these were just some thoughts that I had while I was reading the patch. Thank you again for this new version! Have a great day : -) Joshua Sent using hkml (https://github.com/sjp38/hackermail)
On Thu, Mar 20, 2025 at 02:40:01PM +0900, Rakie Kim wrote: > On Thu, 20 Mar 2025 13:17:46 +0900 Rakie Kim <rakie.kim@sk.com> wrote: > > Hi Gregory > > I initially planned to separate this patch from the hotplug-related patch > series as an independent update. However, after reviewing the code following > Jonathan's suggestion to consolidate `kobject` and `node_attrs` into a single > struct, I realized that most of the intended functionality for Patch 2 was > already incorporated. > > As a result, Patch 1 now only contains the `kobject_put` fix, while the > struct consolidation work has been included in Patch 2. Given the current > design, it seems more natural to keep Patch 1 and Patch 2 together as part > of the same patch series rather than separating them. > > Rakie > The point of submitting separately was to backport this to LTS via -stable. We probably still want this since it ostensibly solves a memory leak - even if the design is to support this work. ~Gregory
diff --git a/mm/mempolicy.c b/mm/mempolicy.c index bbaadbeeb291..5950d5d5b85e 100644 --- a/mm/mempolicy.c +++ b/mm/mempolicy.c @@ -3448,7 +3448,9 @@ static void sysfs_wi_release(struct kobject *wi_kobj) for (i = 0; i < nr_node_ids; i++) sysfs_wi_node_release(node_attrs[i], wi_kobj); - kobject_put(wi_kobj); + + kfree(node_attrs); + kfree(wi_kobj); } static const struct kobj_type wi_ktype = { @@ -3494,15 +3496,22 @@ static int add_weighted_interleave_group(struct kobject *root_kobj) struct kobject *wi_kobj; int nid, err; - wi_kobj = kzalloc(sizeof(struct kobject), GFP_KERNEL); - if (!wi_kobj) + node_attrs = kcalloc(nr_node_ids, sizeof(struct iw_node_attr *), + GFP_KERNEL); + if (!node_attrs) 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, "weighted_interleave"); if (err) { - kfree(wi_kobj); - return err; + kobject_put(wi_kobj); + goto err_out; } for_each_node_state(nid, N_POSSIBLE) { @@ -3512,9 +3521,17 @@ static int add_weighted_interleave_group(struct kobject *root_kobj) break; } } - if (err) + if (err) { kobject_put(wi_kobj); + goto err_out; + } + return 0; + +node_out: + kfree(node_attrs); +err_out: + return err; } static void mempolicy_kobj_release(struct kobject *kobj) @@ -3528,7 +3545,6 @@ static void mempolicy_kobj_release(struct kobject *kobj) mutex_unlock(&iw_table_lock); synchronize_rcu(); kfree(old); - kfree(node_attrs); kfree(kobj); } @@ -3542,37 +3558,22 @@ static int __init mempolicy_sysfs_init(void) static struct kobject *mempolicy_kobj; mempolicy_kobj = kzalloc(sizeof(*mempolicy_kobj), GFP_KERNEL); - if (!mempolicy_kobj) { - err = -ENOMEM; - goto err_out; - } - - node_attrs = kcalloc(nr_node_ids, sizeof(struct iw_node_attr *), - GFP_KERNEL); - if (!node_attrs) { - err = -ENOMEM; - goto mempol_out; - } + if (!mempolicy_kobj) + return -ENOMEM; err = kobject_init_and_add(mempolicy_kobj, &mempolicy_ktype, mm_kobj, "mempolicy"); if (err) - goto node_out; + goto err_out; err = add_weighted_interleave_group(mempolicy_kobj); - if (err) { - pr_err("mempolicy sysfs structure failed to initialize\n"); - kobject_put(mempolicy_kobj); - return err; - } + if (err) + goto err_out; + + return 0; - return err; -node_out: - kfree(node_attrs); -mempol_out: - kfree(mempolicy_kobj); err_out: - pr_err("failed to add mempolicy kobject to the system\n"); + kobject_put(mempolicy_kobj); return err; }
Memory leaks occurred when removing sysfs attributes for weighted interleave. Improper kobject deallocation led to unreleased memory when initialization failed or when nodes were removed. This patch resolves the issue by replacing unnecessary `kfree()` calls with `kobject_put()`, ensuring proper cleanup and preventing memory leaks. By correctly using `kobject_put()`, the release function now properly deallocates memory without causing resource leaks, thereby improving system stability. Fixes: dce41f5ae253 ("mm/mempolicy: implement the sysfs-based weighted_interleave interface") Signed-off-by: Rakie Kim <rakie.kim@sk.com> --- mm/mempolicy.c | 61 +++++++++++++++++++++++++------------------------- 1 file changed, 31 insertions(+), 30 deletions(-) base-commit: 4701f33a10702d5fc577c32434eb62adde0a1ae1