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

[Minchan Kim <minchan@kernel.org>]

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);