On Fri, Mar 19, 2021 at 08:29:29PM +0300, Dmitry Osipenko wrote: > 19.03.2021 19:30, Minchan Kim пишет: > > On Fri, Mar 19, 2021 at 07:24:05PM +0300, Dmitry Osipenko wrote: > >> 19.03.2021 18:50, Greg Kroah-Hartman пишет: > >>>> 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. > > > >> > >>> 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@xxxxxxxxxx> > > 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@xxxxxxxxx/ > > > > Signed-off-by: Minchan Kim <minchan@xxxxxxxxxx> > > --- > > 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@xxxxxxxxxx> > > +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@xxxxxxxxxx> > > +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@xxxxxxxxxx> > > +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@xxxxxxxxxx> > > + */ > > + > > +#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 > > > > + 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> > > + 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. > > > + } > > + > > + kfree(cma_kobjs); > > + kobject_put(cma_kobj_root); > > + > > + return -ENOMEM; > > +} > > +subsys_initcall(cma_sysfs_init); > > > > Tested-by: Dmitry Osipenko <digetx@xxxxxxxxx> Thanks!
