2021-03-24 09:48:08

by John Hubbard

Subject: Re: [PATCH v6] mm: cma: support sysfs

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.

thanks,
--
John Hubbard
NVIDIA

2021-03-24 22:35:40

by Minchan Kim

Subject: Re: [PATCH v6] mm: cma: support sysfs

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);
+ kfree(cma->kobj);
+ cma->kobj = NULL;
+ }
+ kobject_put(cma_kobj_root);
+
+ return err;
}
subsys_initcall(cma_sysfs_init);

2021-03-25 03:04:52

by Dmitry Osipenko

Subject: Re: [PATCH v6] mm: cma: support sysfs

24.03.2021 08:44, Minchan Kim пишет:
> 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);
> + kfree(cma->kobj);
> + cma->kobj = NULL;
> + }
> + kobject_put(cma_kobj_root);
> +
> + return err;
> }
> subsys_initcall(cma_sysfs_init);

Since we don't care about the order in which kobjects are put, I'd write it in this way, which I think looks cleaner:

static void cma_sysfs_cleanup(struct kobject *cma_kobj_root)
{
struct cma *cma = cma_areas;
unsigned int i;

for (i = 0; i < cma_area_count; i++, cma++) {
if (!cma->kobj)
break;

kobject_put(&cma->kobj->kobj);
}

kobject_put(cma_kobj_root);
}

static int __init cma_sysfs_init(void)
{
struct kobject *cma_kobj_root;
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++) {
struct cma_kobject *cma_kobj;
struct cma *cma;
int err;

cma_kobj = kzalloc(sizeof(*cma_kobj), GFP_KERNEL);
if (!cma_kobj) {
cma_sysfs_cleanup(cma_kobj_root);
return -ENOMEM;
}

cma = &cma_areas[i];
cma->kobj = cma_kobj;
cma_kobj->cma = cma;
err = kobject_init_and_add(&cma_kobj->kobj, &cma_ktype,
cma_kobj_root, "%s", cma->name);
if (err) {
cma_sysfs_cleanup(cma_kobj_root);
return err;
}
}

return 0;
}
subsys_initcall(cma_sysfs_init);

2021-03-25 03:13:07

by Minchan Kim

Subject: Re: [PATCH v6] mm: cma: support sysfs

On Wed, Mar 24, 2021 at 03:37:02PM +0300, Dmitry Osipenko wrote:
> 24.03.2021 08:44, Minchan Kim пишет:
> > 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);
> > + kfree(cma->kobj);
> > + cma->kobj = NULL;
> > + }
> > + kobject_put(cma_kobj_root);
> > +
> > + return err;
> > }
> > subsys_initcall(cma_sysfs_init);
>
> Since we don't care about the order in which kobjects are put, I'd write it in this way, which I think looks cleaner:
>

Hmm, preference matter. That kinds of goto error handling for unwinding is
familiar in kernel code and simple enough for me. I don't think readbility
is bad enough to need another cleanup function at this moment.

> static void cma_sysfs_cleanup(struct kobject *cma_kobj_root)
> {
> struct cma *cma = cma_areas;
> unsigned int i;
>
> for (i = 0; i < cma_area_count; i++, cma++) {
> if (!cma->kobj)
> break;
>
> kobject_put(&cma->kobj->kobj);
> }
>
> kobject_put(cma_kobj_root);
> }
>
> static int __init cma_sysfs_init(void)
> {
> struct kobject *cma_kobj_root;
> 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++) {
> struct cma_kobject *cma_kobj;
> struct cma *cma;
> int err;
>
> cma_kobj = kzalloc(sizeof(*cma_kobj), GFP_KERNEL);
> if (!cma_kobj) {
> cma_sysfs_cleanup(cma_kobj_root);
> return -ENOMEM;
> }
>
> cma = &cma_areas[i];
> cma->kobj = cma_kobj;
> cma_kobj->cma = cma;
> err = kobject_init_and_add(&cma_kobj->kobj, &cma_ktype,
> cma_kobj_root, "%s", cma->name);
> if (err) {
> cma_sysfs_cleanup(cma_kobj_root);
> return err;
> }
> }
>
> return 0;
> }
> subsys_initcall(cma_sysfs_init);