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;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.112.34 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.112.34; helo=mail.nvidia.com;
Subject: Re: [PATCH v6] mm: cma: support sysfs
To:     Minchan Kim <minchan@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>
CC:     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>
References: <20210324010547.4134370-1-minchan@kernel.org>
From:   John Hubbard <jhubbard@nvidia.com>
Message-ID: <cbe10402-6574-6e46-9fd9-98b503bd26a4@nvidia.com>
Date:   Tue, 23 Mar 2021 19:34:12 -0700
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: <20210324010547.4134370-1-minchan@kernel.org>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Mar 2021 02:34:13.6997
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: b7f00d70-b699-4a88-2701-08d8ee6d503f
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.112.34];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: CO1NAM11FT056.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR12MB4336
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 3/23/21 6:05 PM, Minchan Kim wrote:
...> diff --git a/mm/cma_sysfs.c b/mm/cma_sysfs.c
> new file mode 100644
> index 000000000000..c3791a032dc5
> --- /dev/null
> +++ b/mm/cma_sysfs.c
> @@ -0,0 +1,107 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CMA SysFS Interface
> + *
> + * Copyright (c) 2021 Minchan Kim <minchan@kernel.org>
> + */
> +
> +#include <linux/cma.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +
> +#include "cma.h"
> +
> +void cma_sysfs_account_success_pages(struct cma *cma, size_t count)
> +{
> +	atomic64_add(count, &cma->nr_pages_succeeded);
> +}
> +
> +void cma_sysfs_account_fail_pages(struct cma *cma, size_t count)
> +{
> +	atomic64_add(count, &cma->nr_pages_failed);
> +}
> +
> +#define CMA_ATTR_RO(_name) \
> +	static struct kobj_attribute _name##_attr = __ATTR_RO(_name)
> +
> +#define to_cma_kobject(x) container_of(x, struct cma_kobject, kobj)

I really don't think that helps. container_of() is so widely used and
understood that it is not a good move make people read one more wrapper
for it. Instead, see below...

> +
> +static ssize_t alloc_pages_success_show(struct kobject *kobj,
> +					struct kobj_attribute *attr, char *buf)
> +{
> +	struct cma_kobject *cma_kobj = to_cma_kobject(kobj);
> +	struct cma *cma = cma_kobj->cma;

...if you're looking to get rid of the real code duplication, then you
could put *both* of those lines into a wrapper function, instead, like this:

static inline struct cma* cma_from_kobj(struct kobject *kobj)
{
	struct cma_kobject *cma_kobj = container_of(kobj, struct cma_kobject,
						    kobj);
	struct cma *cma = cma_kobj->cma;

	return cma;
}

static ssize_t alloc_pages_success_show(struct kobject *kobj,
					struct kobj_attribute *attr, char *buf)
{
	struct cma *cma = cma_from_kobj(kobj);

	return sysfs_emit(buf, "%llu\n",
			  atomic64_read(&cma->nr_pages_succeeded));
}
CMA_ATTR_RO(alloc_pages_success);

static ssize_t alloc_pages_fail_show(struct kobject *kobj,
				     struct kobj_attribute *attr, char *buf)
{
	struct cma *cma = cma_from_kobj(kobj);

	return sysfs_emit(buf, "%llu\n", atomic64_read(&cma->nr_pages_failed));
}
CMA_ATTR_RO(alloc_pages_fail);

static void cma_kobj_release(struct kobject *kobj)
{
	struct cma_kobject *cma_kobj = container_of(kobj, struct cma_kobject,
						    kobj);
	struct cma *cma = cma_kobj->cma;

	kfree(cma_kobj);
	cma->kobj = NULL;
}

...isn't that nicer? Saves a little code, gets rid of a macro.

> +
> +	return sysfs_emit(buf, "%llu\n",
> +			  atomic64_read(&cma->nr_pages_succeeded));
> +}
> +CMA_ATTR_RO(alloc_pages_success);
> +
> +static ssize_t alloc_pages_fail_show(struct kobject *kobj,
> +				     struct kobj_attribute *attr, char *buf)
> +{
> +	struct cma_kobject *cma_kobj = to_cma_kobject(kobj);
> +	struct cma *cma = cma_kobj->cma;
> +
> +	return sysfs_emit(buf, "%llu\n", atomic64_read(&cma->nr_pages_failed));
> +}
> +CMA_ATTR_RO(alloc_pages_fail);
> +
> +static void cma_kobj_release(struct kobject *kobj)
> +{
> +	struct cma_kobject *cma_kobj = to_cma_kobject(kobj);
> +	struct cma *cma = cma_kobj->cma;
> +
> +	kfree(cma_kobj);
> +	cma->kobj = NULL;
> +}
> +
> +static struct attribute *cma_attrs[] = {
> +	&alloc_pages_success_attr.attr,
> +	&alloc_pages_fail_attr.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(cma);
> +
> +static struct kobject *cma_kobj_root;
> +
> +static struct kobj_type cma_ktype = {
> +	.release = cma_kobj_release,
> +	.sysfs_ops = &kobj_sysfs_ops,
> +	.default_groups = cma_groups
> +};
> +
> +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. :)

What you might want here is a separate routine to clean up, because
it has to loop through and free whatever was allocated on previous
iterations of this loop here.

> +		}
> +
> +		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) {
> +			kobject_put(&cma_kobj->kobj);
> +			kobject_put(cma_kobj_root);
> +			return err;

Hopefully this little bit of logic could also go into the cleanup
routine.

> +		}
> +	}
> +
> +	return 0;
> +}
> +subsys_initcall(cma_sysfs_init);
> 

thanks,
-- 
John Hubbard
NVIDIA