Message ID | 20210323195050.2577017-1-minchan@kernel.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v5] mm: cma: support sysfs | expand |
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.
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; }
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.
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.
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.
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. >
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!
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!
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);