[PATCH v5] mm: cma: support sysfs

[PATCH v5] mm: cma: support sysfs

[Minchan Kim]

Since CMA is getting used more widely, it's more important to
keep monitoring CMA statistics for system health since it's
directly related to user experience.

This patch introduces sysfs statistics for CMA, in order to provide
some basic monitoring of the CMA allocator.

 * the number of CMA page successful allocations
 * the number of CMA page allocation failures

These two values allow the user to calcuate the allocation
failure rate for each CMA area.

e.g.)
  /sys/kernel/mm/cma/WIFI/alloc_pages_[success|fail]
  /sys/kernel/mm/cma/SENSOR/alloc_pages_[success|fail]
  /sys/kernel/mm/cma/BLUETOOTH/alloc_pages_[success|fail]

The cma_stat was intentionally allocated by dynamic allocation
to harmonize with kobject lifetime management.
https://lore.kernel.org/linux-mm/YCOAmXqt6dZkCQYs@kroah.com/

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
Tested-by: Dmitry Osipenko <digetx@gmail.com>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
From v4 - https://lore.kernel.org/linux-mm/20210309062333.3216138-1-minchan@kernel.org/
 * fix corruption - digetx@

From v3 - https://lore.kernel.org/linux-mm/20210303205053.2906924-1-minchan@kernel.org/
 * fix ZERO_OR_NULL_PTR - kernel test robot
 * remove prefix cma - david@
 * resolve conflict with vmstat cma in mmotm - akpm@
 * rename stat name with success|fail

From v2 - https://lore.kernel.org/linux-mm/20210208180142.2765456-1-minchan@kernel.org/
 * sysfs doc and description modification - jhubbard

From v1 - https://lore.kernel.org/linux-mm/20210203155001.4121868-1-minchan@kernel.org/
 * fix sysfs build and refactoring - willy
 * rename and drop some attributes - jhubbard


 Documentation/ABI/testing/sysfs-kernel-mm-cma |  25 ++++
 mm/Kconfig                                    |   7 ++
 mm/Makefile                                   |   1 +
 mm/cma.c                                      |   7 +-
 mm/cma.h                                      |  21 ++++
 mm/cma_sysfs.c                                | 107 ++++++++++++++++++
 6 files changed, 166 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-kernel-mm-cma
 create mode 100644 mm/cma_sysfs.c

diff --git a/Documentation/ABI/testing/sysfs-kernel-mm-cma b/Documentation/ABI/testing/sysfs-kernel-mm-cma
new file mode 100644
index 000000000000..02b2bb60c296
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-kernel-mm-cma
@@ -0,0 +1,25 @@
+What:		/sys/kernel/mm/cma/
+Date:		Feb 2021
+Contact:	Minchan Kim <minchan@kernel.org>
+Description:
+		/sys/kernel/mm/cma/ contains a subdirectory for each CMA
+		heap name (also sometimes called CMA areas).
+
+		Each CMA heap subdirectory (that is, each
+		/sys/kernel/mm/cma/<cma-heap-name> directory) contains the
+		following items:
+
+			alloc_pages_success
+			alloc_pages_fail
+
+What:		/sys/kernel/mm/cma/<cma-heap-name>/alloc_pages_success
+Date:		Feb 2021
+Contact:	Minchan Kim <minchan@kernel.org>
+Description:
+		the number of pages CMA API succeeded to allocate
+
+What:		/sys/kernel/mm/cma/<cma-heap-name>/alloc_pages_fail
+Date:		Feb 2021
+Contact:	Minchan Kim <minchan@kernel.org>
+Description:
+		the number of pages CMA API failed to allocate
diff --git a/mm/Kconfig b/mm/Kconfig
index 24c045b24b95..febb7e8e24de 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -513,6 +513,13 @@ config CMA_DEBUGFS
 	help
 	  Turns on the DebugFS interface for CMA.
 
+config CMA_SYSFS
+	bool "CMA information through sysfs interface"
+	depends on CMA && SYSFS
+	help
+	  This option exposes some sysfs attributes to get information
+	  from CMA.
+
 config CMA_AREAS
 	int "Maximum count of the CMA areas"
 	depends on CMA
diff --git a/mm/Makefile b/mm/Makefile
index 72227b24a616..56968b23ed7a 100644
--- a/mm/Makefile
+++ b/mm/Makefile
@@ -109,6 +109,7 @@ obj-$(CONFIG_CMA)	+= cma.o
 obj-$(CONFIG_MEMORY_BALLOON) += balloon_compaction.o
 obj-$(CONFIG_PAGE_EXTENSION) += page_ext.o
 obj-$(CONFIG_CMA_DEBUGFS) += cma_debug.o
+obj-$(CONFIG_CMA_SYSFS) += cma_sysfs.o
 obj-$(CONFIG_USERFAULTFD) += userfaultfd.o
 obj-$(CONFIG_IDLE_PAGE_TRACKING) += page_idle.o
 obj-$(CONFIG_DEBUG_PAGE_REF) += debug_page_ref.o
diff --git a/mm/cma.c b/mm/cma.c
index 908f04775686..ac050359faae 100644
--- a/mm/cma.c
+++ b/mm/cma.c
@@ -507,10 +507,13 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
 
 	pr_debug("%s(): returned %p\n", __func__, page);
 out:
-	if (page)
+	if (page) {
 		count_vm_event(CMA_ALLOC_SUCCESS);
-	else
+		cma_sysfs_alloc_pages_count(cma, count);
+	} else {
 		count_vm_event(CMA_ALLOC_FAIL);
+		cma_sysfs_fail_pages_count(cma, count);
+	}
 
 	return page;
 }
diff --git a/mm/cma.h b/mm/cma.h
index 42ae082cb067..f3258791ac58 100644
--- a/mm/cma.h
+++ b/mm/cma.h
@@ -3,6 +3,12 @@
 #define __MM_CMA_H__
 
 #include <linux/debugfs.h>
+#include <linux/kobject.h>
+
+struct cma_kobject {
+	struct cma *cma;
+	struct kobject kobj;
+};
 
 struct cma {
 	unsigned long   base_pfn;
@@ -16,6 +22,14 @@ struct cma {
 	struct debugfs_u32_array dfs_bitmap;
 #endif
 	char name[CMA_MAX_NAME];
+#ifdef CONFIG_CMA_SYSFS
+	/* the number of CMA page successful allocations */
+	atomic64_t nr_pages_succeeded;
+	/* the number of CMA page allocation failures */
+	atomic64_t nr_pages_failed;
+	/* kobject requires dynamic objecjt */
+	struct cma_kobject *kobj;
+#endif
 };
 
 extern struct cma cma_areas[MAX_CMA_AREAS];
@@ -26,4 +40,11 @@ static inline unsigned long cma_bitmap_maxno(struct cma *cma)
 	return cma->count >> cma->order_per_bit;
 }
 
+#ifdef CONFIG_CMA_SYSFS
+void cma_sysfs_alloc_pages_count(struct cma *cma, size_t count);
+void cma_sysfs_fail_pages_count(struct cma *cma, size_t count);
+#else
+static inline void cma_sysfs_alloc_pages_count(struct cma *cma, size_t count) {};
+static inline void cma_sysfs_fail_pages_count(struct cma *cma, size_t count) {};
+#endif
 #endif
diff --git a/mm/cma_sysfs.c b/mm/cma_sysfs.c
new file mode 100644
index 000000000000..6813635cef41
--- /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_alloc_pages_count(struct cma *cma, size_t count)
+{
+	atomic64_add(count, &cma->nr_pages_succeeded);
+}
+
+void cma_sysfs_fail_pages_count(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)
+
+static ssize_t alloc_pages_success_show(struct kobject *kobj,
+					struct kobj_attribute *attr, char *buf)
+{
+	struct cma_kobject *cma_kobj = container_of(kobj,
+						    struct cma_kobject, kobj);
+	struct cma *cma = cma_kobj->cma;
+
+	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 = container_of(kobj,
+						    struct 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 =
+		container_of(kobj, struct 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)
+{
+	int i = 0;
+	struct cma *cma;
+
+	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 *kobj;
+
+		cma = &cma_areas[i];
+		kobj = kzalloc(sizeof(struct cma_kobject), GFP_KERNEL);
+		if (!kobj)
+			goto out;
+
+		kobj->cma = cma;
+		cma->kobj = kobj;
+		if (kobject_init_and_add(&cma->kobj->kobj, &cma_ktype,
+					 cma_kobj_root, "%s", cma->name)) {
+			kobject_put(&cma->kobj->kobj);
+			goto out;
+		}
+	}
+
+	return 0;
+out:
+	kobject_put(cma_kobj_root);
+
+	return -ENOMEM;
+}
+subsys_initcall(cma_sysfs_init);
-- 
2.31.0.291.g576ba9dcdaf-goog

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

[Dmitry Osipenko]

23.03.2021 22:50, Minchan Kim пишет:
> Since CMA is getting used more widely, it's more important to
> keep monitoring CMA statistics for system health since it's
> directly related to user experience.
> 
> This patch introduces sysfs statistics for CMA, in order to provide
> some basic monitoring of the CMA allocator.
> 
>  * the number of CMA page successful allocations
>  * the number of CMA page allocation failures
> 
> These two values allow the user to calcuate the allocation

typo: calculate

>  struct cma {
>  	unsigned long   base_pfn;
> @@ -16,6 +22,14 @@ struct cma {
>  	struct debugfs_u32_array dfs_bitmap;
>  #endif
>  	char name[CMA_MAX_NAME];
> +#ifdef CONFIG_CMA_SYSFS
> +	/* the number of CMA page successful allocations */
> +	atomic64_t nr_pages_succeeded;
> +	/* the number of CMA page allocation failures */
> +	atomic64_t nr_pages_failed;
> +	/* kobject requires dynamic objecjt */

typo: object
...
> +static void cma_kobj_release(struct kobject *kobj)
> +{
> +	struct cma_kobject *cma_kobj =
> +		container_of(kobj, struct cma_kobject, kobj);

I'd add a to_cma_kobject() helper to improve readability.

> +	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)
> +{
> +	int i = 0;

unsigned int, for consistency

There is no need to initialize this variable.

> +	struct cma *cma;
> +
> +	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 *kobj;
> +
> +		cma = &cma_areas[i];
> +		kobj = kzalloc(sizeof(struct cma_kobject), GFP_KERNEL);

Checkpatch should warn that kzalloc(*kobj, ..) is a better variant.

I'd also rename kobj to cma_kobj everywhere, for clarity.

> +		if (!kobj)
> +			goto out;
> +
> +		kobj->cma = cma;
> +		cma->kobj = kobj;
> +		if (kobject_init_and_add(&cma->kobj->kobj, &cma_ktype,
> +					 cma_kobj_root, "%s", cma->name)) {
> +			kobject_put(&cma->kobj->kobj);
> +			goto out;
> +		}
> +	}
> +
> +	return 0;
> +out:
> +	kobject_put(cma_kobj_root);
> +
> +	return -ENOMEM;

kobject_init_and_add returns a error code, it could be different from
ENOMEM. Won't hurt to propagate the proper error code.

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

[Dmitry Osipenko]

24.03.2021 00:19, Dmitry Osipenko пишет:
>> +		if (!kobj)
>> +			goto out;
>> +
>> +		kobj->cma = cma;
>> +		cma->kobj = kobj;
>> +		if (kobject_init_and_add(&cma->kobj->kobj, &cma_ktype,
>> +					 cma_kobj_root, "%s", cma->name)) {
>> +			kobject_put(&cma->kobj->kobj);
>> +			goto out;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +out:
>> +	kobject_put(cma_kobj_root);
>> +
>> +	return -ENOMEM;
> kobject_init_and_add returns a error code, it could be different from
> ENOMEM. Won't hurt to propagate the proper error code.
> 

I think it will be cleaner to write it like this:

		cma_kobj = kzalloc(sizeof(*cma_kobj), GFP_KERNEL);
		if (!cma_kobj) {
			kobject_put(cma_kobj_root);
			return -ENOMEM;
		}

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

	return 0;
}

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

[Dmitry Osipenko]

23.03.2021 22:50, Minchan Kim пишет:
> +#ifdef CONFIG_CMA_SYSFS
> +void cma_sysfs_alloc_pages_count(struct cma *cma, size_t count);
> +void cma_sysfs_fail_pages_count(struct cma *cma, size_t count);

I'd also rename cma_sysfs_alloc_pages_count to
cma_sysfs_account_success_pages and cma_sysfs_fail_pages_count to
cma_sysfs_account_fail_pages.

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

[Minchan Kim]

On Wed, Mar 24, 2021 at 12:44:06AM +0300, Dmitry Osipenko wrote:
> 23.03.2021 22:50, Minchan Kim пишет:
> > +#ifdef CONFIG_CMA_SYSFS
> > +void cma_sysfs_alloc_pages_count(struct cma *cma, size_t count);
> > +void cma_sysfs_fail_pages_count(struct cma *cma, size_t count);
> 
> I'd also rename cma_sysfs_alloc_pages_count to
> cma_sysfs_account_success_pages and cma_sysfs_fail_pages_count to
> cma_sysfs_account_fail_pages.

Every comment makes sense. Will revise and resend.

Thanks for the review.

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

[Matthew Wilcox]

On Tue, Mar 23, 2021 at 12:50:50PM -0700, Minchan Kim wrote:
> +	/* the number of CMA page successful allocations */
> +	atomic64_t nr_pages_succeeded;

> +void cma_sysfs_alloc_pages_count(struct cma *cma, size_t count)
> +{
> +	atomic64_add(count, &cma->nr_pages_succeeded);
> +}

I don't understand.  A size_t is a byte count.  But the variable is called
'nr_pages'.  So which is it, a byte count or a page count?

> +static ssize_t alloc_pages_success_show(struct kobject *kobj,
> +					struct kobj_attribute *attr, char *buf)
> +{
> +	struct cma_kobject *cma_kobj = container_of(kobj,
> +						    struct cma_kobject, kobj);
> +	struct cma *cma = cma_kobj->cma;
> +
> +	return sysfs_emit(buf, "%llu\n",
> +			  atomic64_read(&cma->nr_pages_succeeded));

... if it's in bytes, it should probably be reported in kilobytes
and be suffixed with a 'K' like many other stats.

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

[Minchan Kim]

On Wed, Mar 24, 2021 at 03:02:24AM +0000, Matthew Wilcox wrote:
> On Tue, Mar 23, 2021 at 12:50:50PM -0700, Minchan Kim wrote:
> > +	/* the number of CMA page successful allocations */
> > +	atomic64_t nr_pages_succeeded;
> 
> > +void cma_sysfs_alloc_pages_count(struct cma *cma, size_t count)
> > +{
> > +	atomic64_add(count, &cma->nr_pages_succeeded);
> > +}
> 
> I don't understand.  A size_t is a byte count.  But the variable is called
> 'nr_pages'.  So which is it, a byte count or a page count?

It's page count. I followed the cma_alloc interface since it has
size_t count variable for nr_pages.

Let's go with unsigned long nr_pages:
void cma_sysfs_alloc_pages_count(struct cma *cma, unsigned long
nr_pages)

> 
> > +static ssize_t alloc_pages_success_show(struct kobject *kobj,
> > +					struct kobj_attribute *attr, char *buf)
> > +{
> > +	struct cma_kobject *cma_kobj = container_of(kobj,
> > +						    struct cma_kobject, kobj);
> > +	struct cma *cma = cma_kobj->cma;
> > +
> > +	return sysfs_emit(buf, "%llu\n",
> > +			  atomic64_read(&cma->nr_pages_succeeded));
> 
> ... if it's in bytes, it should probably be reported in kilobytes
> and be suffixed with a 'K' like many other stats.
> 

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

[Matthew Wilcox]

On Tue, Mar 23, 2021 at 08:31:31PM -0700, Minchan Kim wrote:
> On Wed, Mar 24, 2021 at 03:02:24AM +0000, Matthew Wilcox wrote:
> > On Tue, Mar 23, 2021 at 12:50:50PM -0700, Minchan Kim wrote:
> > > +	/* the number of CMA page successful allocations */
> > > +	atomic64_t nr_pages_succeeded;
> > 
> > > +void cma_sysfs_alloc_pages_count(struct cma *cma, size_t count)
> > > +{
> > > +	atomic64_add(count, &cma->nr_pages_succeeded);
> > > +}
> > 
> > I don't understand.  A size_t is a byte count.  But the variable is called
> > 'nr_pages'.  So which is it, a byte count or a page count?
> 
> It's page count. I followed the cma_alloc interface since it has
> size_t count variable for nr_pages.

That's very confusing.  cma_alloc is wrong; if it needs to be an
unsigned long, that's fine.  But it shouldn't be size_t.

7.17 of n1256 defines:

	size_t
which is the unsigned integer type of the result of the sizeof operator

Do you want to submit a patch to fix cma_alloc as well?

> Let's go with unsigned long nr_pages:
> void cma_sysfs_alloc_pages_count(struct cma *cma, unsigned long
> nr_pages)

Works for me!

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

[Minchan Kim]

On Wed, Mar 24, 2021 at 04:34:34AM +0000, Matthew Wilcox wrote:
> On Tue, Mar 23, 2021 at 08:31:31PM -0700, Minchan Kim wrote:
> > On Wed, Mar 24, 2021 at 03:02:24AM +0000, Matthew Wilcox wrote:
> > > On Tue, Mar 23, 2021 at 12:50:50PM -0700, Minchan Kim wrote:
> > > > +	/* the number of CMA page successful allocations */
> > > > +	atomic64_t nr_pages_succeeded;
> > > 
> > > > +void cma_sysfs_alloc_pages_count(struct cma *cma, size_t count)
> > > > +{
> > > > +	atomic64_add(count, &cma->nr_pages_succeeded);
> > > > +}
> > > 
> > > I don't understand.  A size_t is a byte count.  But the variable is called
> > > 'nr_pages'.  So which is it, a byte count or a page count?
> > 
> > It's page count. I followed the cma_alloc interface since it has
> > size_t count variable for nr_pages.
> 
> That's very confusing.  cma_alloc is wrong; if it needs to be an
> unsigned long, that's fine.  But it shouldn't be size_t.
> 
> 7.17 of n1256 defines:
> 
> 	size_t
> which is the unsigned integer type of the result of the sizeof operator
> 
> Do you want to submit a patch to fix cma_alloc as well?

Sure, but it will be separate patch.

> 
> > Let's go with unsigned long nr_pages:
> > void cma_sysfs_alloc_pages_count(struct cma *cma, unsigned long
> > nr_pages)
> 
> Works for me!

Thanks for review!