On 3/23/21 10:44 PM, Minchan Kim wrote:
On Tue, Mar 23, 2021 at 09:47:27PM -0700, John Hubbard wrote:
On 3/23/21 8:27 PM, Minchan Kim wrote:
...
+static int __init cma_sysfs_init(void)
+{
+ unsigned int i;
+
+ cma_kobj_root = kobject_create_and_add("cma", mm_kobj);
+ if (!cma_kobj_root)
+ return -ENOMEM;
+
+ for (i = 0; i < cma_area_count; i++) {
+ int err;
+ struct cma *cma;
+ struct cma_kobject *cma_kobj;
+
+ cma_kobj = kzalloc(sizeof(*cma_kobj), GFP_KERNEL);
+ if (!cma_kobj) {
+ kobject_put(cma_kobj_root);
+ return -ENOMEM;
This leaks little cma_kobj's all over the floor. :)
I thought kobject_put(cma_kobj_root) should deal with it. No?
If this fails when i > 0, there will be cma_kobj instances that
were stashed in the cma_areas[] array. But this code only deletes
the most recently allocated cma_kobj, not anything allocated on
previous iterations of the loop.
Oh, I misunderstood that destroying of root kobject will release
children recursively. Seems not true. Go back to old version.
index 16c81c9cb9b7..418951a3f138 100644
--- a/mm/cma_sysfs.c
+++ b/mm/cma_sysfs.c
@@ -80,20 +80,19 @@ static struct kobj_type cma_ktype = {
static int __init cma_sysfs_init(void)
{
unsigned int i;
+ int err;
+ struct cma *cma;
+ struct cma_kobject *cma_kobj;
cma_kobj_root = kobject_create_and_add("cma", mm_kobj);
if (!cma_kobj_root)
return -ENOMEM;
for (i = 0; i < cma_area_count; i++) {
- int err;
- struct cma *cma;
- struct cma_kobject *cma_kobj;
-
cma_kobj = kzalloc(sizeof(*cma_kobj), GFP_KERNEL);
if (!cma_kobj) {
- kobject_put(cma_kobj_root);
- return -ENOMEM;
+ err = -ENOMEM;
+ goto out;
}
cma = &cma_areas[i];
@@ -103,11 +102,21 @@ static int __init cma_sysfs_init(void)
cma_kobj_root, "%s", cma->name);
if (err) {
kobject_put(&cma_kobj->kobj);
- kobject_put(cma_kobj_root);
- return err;
+ goto out;
}
}
return 0;
+out:
+ while (--i >= 0) {
+ cma = &cma_areas[i];
+
+ kobject_put(&cma->kobj->kobj);
OK. As long as you are spinning a new version, let's fix up the naming to
be a little better, too. In this case, with a mildly dizzying mix of cma's
and kobjects, it actually makes a real difference. I wouldn't have asked,
but the above cma->kobj->kobj chain really made it obvious for me just now.
So instead of this (in cma.h):
struct cma_kobject {
struct cma *cma;
struct kobject kobj;
};
struct cma {
...
struct cma_kobject *kobj;
};
, how about approximately this:
struct cma_kobject_wrapper {
struct cma *parent;
struct kobject kobj;
};
struct cma {
...
struct cma_kobject_wrapper *cma_kobj_wrapper;
};
...thus allowing readers of cma_sysfs.c to read that file more easily.
+ kfree(cma->kobj);
+ cma->kobj = NULL;
+ }
+ kobject_put(cma_kobj_root);
+
+ return err;
}
subsys_initcall(cma_sysfs_init);
thanks,
--
John Hubbard
NVIDIA
