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;
Subject: Re: [PATCH v6] mm: cma: support sysfs
To:     Minchan Kim <minchan@kernel.org>,
        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
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>
From:   Dmitry Osipenko <digetx@gmail.com>
Message-ID: <5b1a9f6c-ea7b-6e90-1c77-a49a10896f08@gmail.com>
Date:   Wed, 24 Mar 2021 15:37:02 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.1
MIME-Version: 1.0
In-Reply-To: <YFrRugjVLlazTNPy@google.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

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