On Mon, Feb 08, 2021 at 01:34:06PM -0800, John Hubbard wrote: > On 2/8/21 10:01 AM, Minchan Kim wrote: > > 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 for the CMA and exposes stats below > > to keep monitor for telemetric in the system. > > Or: > > This patch introduces sysfs statistics for CMA, in order to provide > some basic monitoring of the CMA allocator. Yub, take it. > > > > > * the number of CMA page allocation attempts > > * the number of CMA page allocation failures > > > > With those per-CMA statistics, we could know how CMA allocadtion > > failure rate for each usecases. > > Maybe: > > These two values allow the user to calcuate the allocation > failure rate for each CMA area. Good to me. > > > > > e.g.) > > /sys/kernel/mm/cma/WIFI/cma_alloc_pages_[attempt|fail] > > /sys/kernel/mm/cma/SENSOR/cma_alloc_pages_[attempt|fail] > > /sys/kernel/mm/cma/BLUETOOTH/cma_alloc_pages_[attempt|fail] > > > > Signed-off-by: Minchan Kim <minchan@xxxxxxxxxx> > > --- > > > > From v1 - https://lore.kernel.org/linux-mm/20210203155001.4121868-1-minchan@xxxxxxxxxx/ > > * 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 | 6 +- > > mm/cma.h | 18 +++ > > mm/cma_sysfs.c | 114 ++++++++++++++++++ > > 6 files changed, 170 insertions(+), 1 deletion(-) > > 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..68bdcc8c7681 > > --- /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 number of subdirectories by > > + cma-heap name. The subdirectory contains a number of files > > + to represent cma allocation statistics. > > + > > + There are number of files under > > + /sys/kernel/mm/cma/<cma-heap-name> directory > > + > > + - cma_alloc_pages_attempt > > + - cma_alloc_pages_fail > > How about this instead: > 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: > > cma_alloc_pages_attempt > cma_alloc_pages_fail > Yub. > > > + > > +What: /sys/kernel/mm/cma/<cma-heap-name>/cma_alloc_pages_attempt > > Actually, shall we change that from "attempt" to "attempts"? Otherwise, the > language is a little odd there. Sure. > > > +Date: Feb 2021 > > +Contact: Minchan Kim <minchan@xxxxxxxxxx> > > +Description: > > + the number of pages CMA API tried to allocate > > + > > +What: /sys/kernel/mm/cma/<cma-heap-name>/cma_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 ec35bf406439..ad7e9c065657 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 b2a564eec27f..0ae764e5b1a8 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 > > Remove the unnecessary tab there, none of the other nearby lines have one. Oops. > > > 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 23d4a97c834a..0611202d6e7d 100644 > > --- a/mm/cma.c > > +++ b/mm/cma.c > > @@ -447,9 +447,10 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align, > > offset = cma_bitmap_aligned_offset(cma, align); > > bitmap_maxno = cma_bitmap_maxno(cma); > > bitmap_count = cma_bitmap_pages_to_bits(cma, count); > > + cma_sysfs_alloc_count(cma, count); > > if (bitmap_count > bitmap_maxno) > > - return NULL; > > + goto out; > > for (;;) { > > mutex_lock(&cma->lock); > > @@ -504,6 +505,9 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align, > > __func__, count, ret); > > cma_debug_show_areas(cma); > > } > > +out: > > + if (!page) > > + cma_sysfs_fail(cma, count); > > pr_debug("%s(): returned %p\n", __func__, page); > > return page; > > diff --git a/mm/cma.h b/mm/cma.h > > index 42ae082cb067..49a8ceddd9e8 100644 > > --- a/mm/cma.h > > +++ b/mm/cma.h > > @@ -3,6 +3,14 @@ > > #define __MM_CMA_H__ > > #include <linux/debugfs.h> > > +#include <linux/kobject.h> > > + > > +struct cma_stat { > > + spinlock_t lock; > > + unsigned long pages_attempt; /* the number of CMA page allocation attempts */ > > + unsigned long pages_fail; /* the number of CMA page allocation failures */ > > + struct kobject kobj; > > +}; > > struct cma { > > unsigned long base_pfn; > > @@ -16,6 +24,9 @@ struct cma { > > struct debugfs_u32_array dfs_bitmap; > > #endif > > char name[CMA_MAX_NAME]; > > +#ifdef CONFIG_CMA_SYSFS > > + struct cma_stat *stat; > > This should not be a pointer. By making it a pointer, you've added a bunch of pointless > extra code to the implementation. Originally, I went with the object lifetime with struct cma as you suggested to make code simple. However, Greg KH wanted to have release for kobj_type since it is consistent with other kboject handling. > > Here's a diff to implement the non-pointer way, and also to fix a build error in this > patch (missing semicolon): > > diff --git a/Documentation/ABI/testing/sysfs-kernel-mm-cma > b/Documentation/ABI/testing/sysfs-kernel-mm-cma > index 68bdcc8c7681..f3769b4e1a3c 100644 > --- a/Documentation/ABI/testing/sysfs-kernel-mm-cma > +++ b/Documentation/ABI/testing/sysfs-kernel-mm-cma > @@ -2,15 +2,15 @@ What: /sys/kernel/mm/cma/ > Date: Feb 2021 > Contact: Minchan Kim <minchan@xxxxxxxxxx> > Description: > - /sys/kernel/mm/cma/ contains a number of subdirectories by > - cma-heap name. The subdirectory contains a number of files > - to represent cma allocation statistics. > + /sys/kernel/mm/cma/ contains a subdirectory for each CMA heap name (also > + sometimes called CMA areas). > > - There are number of files under > - /sys/kernel/mm/cma/<cma-heap-name> directory > + Each CMA heap subdirectory (that is, each > + /sys/kernel/mm/cma/<cma-heap-name> directory) contains the following > + items: > > - - cma_alloc_pages_attempt > - - cma_alloc_pages_fail > + cma_alloc_pages_attempt > + cma_alloc_pages_fail > > What: /sys/kernel/mm/cma/<cma-heap-name>/cma_alloc_pages_attempt > Date: Feb 2021 > diff --git a/mm/cma.h b/mm/cma.h > index 49a8ceddd9e8..1e109830f553 100644 > --- a/mm/cma.h > +++ b/mm/cma.h > @@ -25,7 +25,7 @@ struct cma { > #endif > char name[CMA_MAX_NAME]; > #ifdef CONFIG_CMA_SYSFS > - struct cma_stat *stat; > + struct cma_stat stat; > #endif > }; > > diff --git a/mm/cma_sysfs.c b/mm/cma_sysfs.c > index 1f6b9f785825..52905694b6b7 100644 > --- a/mm/cma_sysfs.c > +++ b/mm/cma_sysfs.c > @@ -11,20 +11,18 @@ > > #include "cma.h" > > -static struct cma_stat *cma_stats; > - > void cma_sysfs_alloc_count(struct cma *cma, size_t count) > { > - spin_lock(&cma->stat->lock); > - cma->stat->pages_attempt += count; > - spin_unlock(&cma->stat->lock); > + spin_lock(&cma->stat.lock); > + cma->stat.pages_attempt += count; > + spin_unlock(&cma->stat.lock); > } > > void cma_sysfs_fail(struct cma *cma, size_t count) > { > - spin_lock(&cma->stat->lock); > - cma->stat->pages_fail += count; > - spin_unlock(&cma->stat->lock); > + spin_lock(&cma->stat.lock); > + cma->stat.pages_fail += count; > + spin_unlock(&cma->stat.lock); > } > > #define CMA_ATTR_RO(_name) \ > @@ -50,13 +48,6 @@ static ssize_t cma_alloc_pages_fail_show(struct kobject *kobj, > } > CMA_ATTR_RO(cma_alloc_pages_fail); > > -static void cma_kobj_release(struct kobject *kobj) > -{ > - struct cma_stat *stat = container_of(kobj, struct cma_stat, kobj); > - > - kfree(stat); > -} > - > static struct attribute *cma_attrs[] = { > &cma_alloc_pages_attempt_attr.attr, > &cma_alloc_pages_fail_attr.attr, > @@ -65,7 +56,6 @@ static struct attribute *cma_attrs[] = { > ATTRIBUTE_GROUPS(cma); > > static struct kobj_type cma_ktype = { > - .release = cma_kobj_release, > .sysfs_ops = &kobj_sysfs_ops, > .default_groups = cma_groups > }; > @@ -81,32 +71,23 @@ static int __init cma_sysfs_init(void) > return -ENOMEM; > } > > - cma_stats = kzalloc(array_size(sizeof(struct cma_stat), > - cma_area_count), GFP_KERNEL); > - if (!cma_stats) { > - pr_err("failed to create cma_stats\n"); > - goto out; > - } > - > do { > cma = &cma_areas[i]; > - cma->stat = &cma_stats[i]; > - spin_lock_init(&cma->stat->lock); > - if (kobject_init_and_add(&cma->stat->kobj, &cma_ktype, > + spin_lock_init(&cma->stat.lock); > + if (kobject_init_and_add(&cma->stat.kobj, &cma_ktype, > cma_kobj, "%s", cma->name)) { > - kobject_put(&cma->stat->kobj); > + kobject_put(&cma->stat.kobj); > goto out; > } > - } while (++i < cma_area_count) > + } while (++i < cma_area_count); > > return 0; > out: > while (--i >= 0) { > cma = &cma_areas[i]; > - kobject_put(&cma->stat->kobj); > + kobject_put(&cma->stat.kobj); > } > > - kfree(cma_stats); > kobject_put(cma_kobj); > > return -ENOMEM; > > > +#endif > > }; > > extern struct cma cma_areas[MAX_CMA_AREAS]; > > @@ -26,4 +37,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_count(struct cma *cma, size_t count); > > +void cma_sysfs_fail(struct cma *cma, size_t count); > > +#else > > +static inline void cma_sysfs_alloc_count(struct cma *cma, size_t count) {}; > > +static inline void cma_sysfs_fail(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..1f6b9f785825 > > --- /dev/null > > +++ b/mm/cma_sysfs.c > > @@ -0,0 +1,114 @@ > > +// 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" > > + > > +static struct cma_stat *cma_stats; > > I don't know what that's for but it definitely is not needed if you make cma.stat > not a pointer, and not in any other case either. > > > + > > +void cma_sysfs_alloc_count(struct cma *cma, size_t count) > > +{ > > + spin_lock(&cma->stat->lock); > > + cma->stat->pages_attempt += count; > > + spin_unlock(&cma->stat->lock); > > +} > > + > > +void cma_sysfs_fail(struct cma *cma, size_t count) > > +{ > > + spin_lock(&cma->stat->lock); > > + cma->stat->pages_fail += count; > > + spin_unlock(&cma->stat->lock); > > +} > > + > > +#define CMA_ATTR_RO(_name) \ > > + static struct kobj_attribute _name##_attr = __ATTR_RO(_name) > > + > > +static struct kobject *cma_kobj; > > + > > +static ssize_t cma_alloc_pages_attempt_show(struct kobject *kobj, > > + struct kobj_attribute *attr, char *buf) > > +{ > > + struct cma_stat *stat = container_of(kobj, struct cma_stat, kobj); > > + > > + return sysfs_emit(buf, "%lu\n", stat->pages_attempt); > > +} > > +CMA_ATTR_RO(cma_alloc_pages_attempt); > > + > > +static ssize_t cma_alloc_pages_fail_show(struct kobject *kobj, > > + struct kobj_attribute *attr, char *buf) > > +{ > > + struct cma_stat *stat = container_of(kobj, struct cma_stat, kobj); > > + > > + return sysfs_emit(buf, "%lu\n", stat->pages_fail); > > +} > > +CMA_ATTR_RO(cma_alloc_pages_fail); > > + > > +static void cma_kobj_release(struct kobject *kobj) > > +{ > > + struct cma_stat *stat = container_of(kobj, struct cma_stat, kobj); > > + > > + kfree(stat); > > +} > > + > > +static struct attribute *cma_attrs[] = { > > + &cma_alloc_pages_attempt_attr.attr, > > + &cma_alloc_pages_fail_attr.attr, > > + NULL, > > +}; > > +ATTRIBUTE_GROUPS(cma); > > + > > +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 = kobject_create_and_add("cma", mm_kobj); > > + if (!cma_kobj) { > > + pr_err("failed to create cma kobject\n"); > > + return -ENOMEM; > > + } > > + > > + cma_stats = kzalloc(array_size(sizeof(struct cma_stat), > > + cma_area_count), GFP_KERNEL); > > + if (!cma_stats) { > > + pr_err("failed to create cma_stats\n"); > > + goto out; > > + } > > + > > + do { > > + cma = &cma_areas[i]; > > + cma->stat = &cma_stats[i]; > > + spin_lock_init(&cma->stat->lock); > > + if (kobject_init_and_add(&cma->stat->kobj, &cma_ktype, > > + cma_kobj, "%s", cma->name)) { > > + kobject_put(&cma->stat->kobj); > > + goto out; > > + } > > + } while (++i < cma_area_count) > > This clearly is not going to compile! Don't forget to build and test the > patches. > > > + > > + return 0; > > +out: > > + while (--i >= 0) { > > + cma = &cma_areas[i]; > > + kobject_put(&cma->stat->kobj); > > + } > > + > > + kfree(cma_stats); > > + kobject_put(cma_kobj); > > + > > + return -ENOMEM; > > +} > > +subsys_initcall(cma_sysfs_init); > > > > thanks, > -- > John Hubbard > NVIDIA
