Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: Minchan Kim <minchan.kim@gmail.com>
Date:   Wed, 24 Mar 2021 08:18:14 -0700
From:   Minchan Kim <minchan@kernel.org>
To:     Dmitry Osipenko <digetx@gmail.com>
Cc:     John Hubbard <jhubbard@nvidia.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        linux-mm <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>, gregkh@linuxfoundation.org,
        surenb@google.com, joaodias@google.com, willy@infradead.org
Subject: Re: [PATCH v6] mm: cma: support sysfs
Message-ID: <YFtYNtAQiRDr9+TC@google.com>
References: <20210324010547.4134370-1-minchan@kernel.org>
 <cbe10402-6574-6e46-9fd9-98b503bd26a4@nvidia.com>
 <YFqxm7UQBtWqH6VU@google.com>
 <d224c6bd-f5b1-74f6-2afc-c3d5b0519ba4@nvidia.com>
 <YFrRugjVLlazTNPy@google.com>
 <5b1a9f6c-ea7b-6e90-1c77-a49a10896f08@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <5b1a9f6c-ea7b-6e90-1c77-a49a10896f08@gmail.com>
Precedence: bulk

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