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:   Tue, 23 Mar 2021 23:57:25 -0700
From:   Minchan Kim <minchan@kernel.org>
To:     John Hubbard <jhubbard@nvidia.com>
Cc:     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,
        digetx@gmail.com
Subject: Re: [PATCH v6] mm: cma: support sysfs
Message-ID: <YFri1Qn1YorOPvsM@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>
 <71b7914d-d9ff-2200-d6c9-6eab28499887@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <71b7914d-d9ff-2200-d6c9-6eab28499887@nvidia.com>
Precedence: bulk

On Tue, Mar 23, 2021 at 11:26:08PM -0700, John Hubbard wrote:
> 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.

I agree cma->kobj->kobj is awkward but personally, I don't like the
naming: cma_kobject_wrapper parent pointer. cma_kobject is alredy
wrapper so it sounds me redundant and it's not a parent in same
hierarchy.

Since the kobj->kobj is just one line in the code(I don't imagine
it could grow up in cma_sysfs in future), I don't think it would
be a problem. If we really want to make it more clear, maybe?

   cma->cma_kobj->kobj

It would be consistent with other variables in cma_sysfs_init.