On Fri, Mar 19, 2021 at 08:29:29PM +0300, Dmitry Osipenko wrote:

19.03.2021 19:30, Minchan Kim пишет:

quoted

On Fri, Mar 19, 2021 at 07:24:05PM +0300, Dmitry Osipenko wrote:

quoted

19.03.2021 18:50, Greg Kroah-Hartman пишет:

quoted

quoted

Then initialization order won't be a problem.

I don't understand the problem here, what is wrong with the patch as-is?

The cma->stat is NULL at the time when CMA is used on ARM because
cma->stat is initialized at the subsys level. This is the problem,
apparently.

That's true.

quoted

quoted

Also, watch out, if you only make the kobject dynamic, how are you going
to get backwards from the kobject to the values you want to send to
userspace in the show functions?

Still there should be a wrapper around the kobj with a back reference to
the cma entry. If you're suggesting that I should write a patch, then I
may take a look at it later on. Although, I assume that Minchan could
just correct this patch and re-push it to -next.

This is ateempt to address it. Unless any objection, let me send it to
akpm.

From 29a9fb4f300b754ebf55e6182ba84127658ef504 Mon Sep 17 00:00:00 2001
From: Minchan Kim <minchan@kernel.org>
Date: Fri, 22 Jan 2021 12:31:56 -0800
Subject: [PATCH] mm: cma: support sysfs

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/ (local)

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 Documentation/ABI/testing/sysfs-kernel-mm-cma |  25 ++++
 mm/Kconfig                                    |   7 ++
 mm/Makefile                                   |   1 +
 mm/cma.c                                      |   7 +-
 mm/cma.h                                      |  20 ++++
 mm/cma_sysfs.c                                | 107 ++++++++++++++++++
 6 files changed, 165 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..70fd7633fe01 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,13 @@ 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;
+	struct cma_kobject *kobj;
+#endif
 };
 
 extern struct cma cma_areas[MAX_CMA_AREAS];

@@ -26,4 +39,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..ca093e9e9f64
--- /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)

The indentations are still wrong.

CHECK: Alignment should match open parenthesis

Hmm, I didn't know that we have that kinds of rule.
Maybe, my broken checkpatch couldn't catch it up.
Thanks.

$ ./scripts/checkpatch.pl 0001-mm-cma-support-sysfs.patch
Traceback (most recent call last):
  File "scripts/spdxcheck.py", line 6, in <module>
    from ply import lex, yacc


< snip >

quoted

+	if (ZERO_OR_NULL_PTR(cma_kobjs))
+		goto out;
+
+	do {
+		cma = &cma_areas[i];
+		cma->kobj = &cma_kobjs[i];

Does cma really need are pointer to cma_kobj?

Do you mean something like this?

struct cma {
	..
	..
	struct kobject *kobj;
};

Then, how could we we make kobject dynamic model?

We need to get struct cma from kboj like below.

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

So kobj should be not a pointer in the container struct.
Am I missing your point? Otherwise, it would be great if you suggest
better idea.


< snip>

quoted

+			kobject_put(&cma->kobj->kobj);
+			goto out;
+		}
+	} while (++i < cma_area_count);
+
+	return 0;
+out:
+	while (--i >= 0) {
+		cma = &cma_areas[i];
+		kobject_put(&cma->kobj->kobj);

Should the cma->kobj be set to NULL by cma_kobj_release() if cma->kobj is really needed?

True. Then, let's fix cma_kobj_release, too.

quoted

+	}
+
+	kfree(cma_kobjs);
+	kobject_put(cma_kobj_root);
+
+	return -ENOMEM;
+}
+subsys_initcall(cma_sysfs_init);

Tested-by: Dmitry Osipenko <digetx@gmail.com>

Thanks!