* [PATCH v2] mm: cma: support sysfs @ 2021-02-08 18:01 Minchan Kim 2021-02-08 21:34 ` John Hubbard 0 siblings, 1 reply; 20+ messages in thread From: Minchan Kim @ 2021-02-08 18:01 UTC (permalink / raw) To: Andrew Morton Cc: linux-mm, LKML, gregkh, surenb, joaodias, jhubbard, willy, 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 for the CMA and exposes stats below to keep monitor for telemetric in the system. * 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. 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@kernel.org> --- 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 | 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@kernel.org> +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 + +What: /sys/kernel/mm/cma/<cma-heap-name>/cma_alloc_pages_attempt +Date: Feb 2021 +Contact: Minchan Kim <minchan@kernel.org> +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@kernel.org> +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 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; +#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@kernel.org> + */ + +#include <linux/cma.h> +#include <linux/kernel.h> +#include <linux/slab.h> + +#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); +} + +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) + + 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); -- 2.30.0.478.g8a0d178c01-goog ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2] mm: cma: support sysfs 2021-02-08 18:01 [PATCH v2] mm: cma: support sysfs Minchan Kim @ 2021-02-08 21:34 ` John Hubbard 2021-02-08 23:36 ` Minchan Kim 0 siblings, 1 reply; 20+ messages in thread From: John Hubbard @ 2021-02-08 21:34 UTC (permalink / raw) To: Minchan Kim, Andrew Morton Cc: linux-mm, LKML, gregkh, surenb, joaodias, willy 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. > > * 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. > > 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@kernel.org> > --- > > 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 | 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@kernel.org> > +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 > + > +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. > +Date: Feb 2021 > +Contact: Minchan Kim <minchan@kernel.org> > +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@kernel.org> > +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. > 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. 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@kernel.org> 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@kernel.org> > + */ > + > +#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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2] mm: cma: support sysfs 2021-02-08 21:34 ` John Hubbard @ 2021-02-08 23:36 ` Minchan Kim 2021-02-09 1:57 ` John Hubbard 0 siblings, 1 reply; 20+ messages in thread From: Minchan Kim @ 2021-02-08 23:36 UTC (permalink / raw) To: John Hubbard Cc: Andrew Morton, linux-mm, LKML, gregkh, surenb, joaodias, willy 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@kernel.org> > > --- > > > > 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 | 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@kernel.org> > > +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@kernel.org> > > +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@kernel.org> > > +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@kernel.org> > 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@kernel.org> > > + */ > > + > > +#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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] mm: cma: support sysfs 2021-02-08 23:36 ` Minchan Kim @ 2021-02-09 1:57 ` John Hubbard 2021-02-09 4:19 ` Minchan Kim 2021-02-09 6:13 ` Greg KH 0 siblings, 2 replies; 20+ messages in thread From: John Hubbard @ 2021-02-09 1:57 UTC (permalink / raw) To: Minchan Kim Cc: Andrew Morton, linux-mm, LKML, gregkh, surenb, joaodias, willy On 2/8/21 3:36 PM, Minchan Kim wrote: ... >>> 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. Are you talking about the kobj in your new struct cma_stat? That seems like circular logic if so. I'm guessing Greg just wanted kobj methods to be used *if* you are dealing with kobjects. That's a narrower point. I can't imagine that he would have insisted on having additional allocations just so that kobj freeing methods could be used. :) thanks, -- John Hubbard NVIDIA ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] mm: cma: support sysfs 2021-02-09 1:57 ` John Hubbard @ 2021-02-09 4:19 ` Minchan Kim 2021-02-09 5:18 ` John Hubbard 2021-02-09 6:13 ` Greg KH 1 sibling, 1 reply; 20+ messages in thread From: Minchan Kim @ 2021-02-09 4:19 UTC (permalink / raw) To: John Hubbard Cc: Andrew Morton, linux-mm, LKML, gregkh, surenb, joaodias, willy On Mon, Feb 08, 2021 at 05:57:17PM -0800, John Hubbard wrote: > On 2/8/21 3:36 PM, Minchan Kim wrote: > ... > > > > 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. > > Are you talking about the kobj in your new struct cma_stat? That seems > like circular logic if so. I'm guessing Greg just wanted kobj methods > to be used *if* you are dealing with kobjects. That's a narrower point. > > I can't imagine that he would have insisted on having additional > allocations just so that kobj freeing methods could be used. :) I have no objection if Greg agree static kobject is okay in this case. Greg? ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] mm: cma: support sysfs 2021-02-09 4:19 ` Minchan Kim @ 2021-02-09 5:18 ` John Hubbard 2021-02-09 5:27 ` John Hubbard 0 siblings, 1 reply; 20+ messages in thread From: John Hubbard @ 2021-02-09 5:18 UTC (permalink / raw) To: Minchan Kim Cc: Andrew Morton, linux-mm, LKML, gregkh, surenb, joaodias, willy On 2/8/21 8:19 PM, Minchan Kim wrote: > On Mon, Feb 08, 2021 at 05:57:17PM -0800, John Hubbard wrote: >> On 2/8/21 3:36 PM, Minchan Kim wrote: >> ... >>>>> 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. >> >> Are you talking about the kobj in your new struct cma_stat? That seems >> like circular logic if so. I'm guessing Greg just wanted kobj methods >> to be used *if* you are dealing with kobjects. That's a narrower point. >> >> I can't imagine that he would have insisted on having additional >> allocations just so that kobj freeing methods could be used. :) > > I have no objection if Greg agree static kobject is okay in this > case. Greg? > What I meant is, no kobject at all in the struct cma_stat member variable. The lifetime of the cma_stat member is the same as the containing struct, so no point in putting a kobject into it. thanks, -- John Hubbard NVIDIA ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] mm: cma: support sysfs 2021-02-09 5:18 ` John Hubbard @ 2021-02-09 5:27 ` John Hubbard 0 siblings, 0 replies; 20+ messages in thread From: John Hubbard @ 2021-02-09 5:27 UTC (permalink / raw) To: Minchan Kim Cc: Andrew Morton, linux-mm, LKML, gregkh, surenb, joaodias, willy On 2/8/21 9:18 PM, John Hubbard wrote: > On 2/8/21 8:19 PM, Minchan Kim wrote: >> On Mon, Feb 08, 2021 at 05:57:17PM -0800, John Hubbard wrote: >>> On 2/8/21 3:36 PM, Minchan Kim wrote: >>> ... >>>>>> 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. >>> >>> Are you talking about the kobj in your new struct cma_stat? That seems >>> like circular logic if so. I'm guessing Greg just wanted kobj methods >>> to be used *if* you are dealing with kobjects. That's a narrower point. >>> >>> I can't imagine that he would have insisted on having additional >>> allocations just so that kobj freeing methods could be used. :) >> >> I have no objection if Greg agree static kobject is okay in this >> case. Greg? >> > > What I meant is, no kobject at all in the struct cma_stat member > variable. The lifetime of the cma_stat member is the same as the > containing struct, so no point in putting a kobject into it. > ...unless...are you actually *wanting* to keep the lifetimes separate? Hmmm, given the short nature of sysfs reads, though, I'd be inclined to just let the parent object own the lifetime. But maybe I'm missing some design point here? thanks, -- John Hubbard NVIDIA ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] mm: cma: support sysfs 2021-02-09 1:57 ` John Hubbard 2021-02-09 4:19 ` Minchan Kim @ 2021-02-09 6:13 ` Greg KH 2021-02-09 6:27 ` John Hubbard 1 sibling, 1 reply; 20+ messages in thread From: Greg KH @ 2021-02-09 6:13 UTC (permalink / raw) To: John Hubbard Cc: Minchan Kim, Andrew Morton, linux-mm, LKML, surenb, joaodias, willy On Mon, Feb 08, 2021 at 05:57:17PM -0800, John Hubbard wrote: > On 2/8/21 3:36 PM, Minchan Kim wrote: > ... > > > > 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. > > Are you talking about the kobj in your new struct cma_stat? That seems > like circular logic if so. I'm guessing Greg just wanted kobj methods > to be used *if* you are dealing with kobjects. That's a narrower point. > > I can't imagine that he would have insisted on having additional > allocations just so that kobj freeing methods could be used. :) Um, yes, I was :) You can not add a kobject to a structure and then somehow think you can just ignore the reference counting issues involved. If a kobject is part of a structure then the kobject is responsible for controling the lifespan of the memory, nothing else can be. So by making the kobject dynamic, you properly handle that memory lifespan of the object, instead of having to worry about the lifespan of the larger object (which the original patch was not doing.) Does that make sense? thanks, greg k-h ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] mm: cma: support sysfs 2021-02-09 6:13 ` Greg KH @ 2021-02-09 6:27 ` John Hubbard 2021-02-09 6:34 ` John Hubbard 0 siblings, 1 reply; 20+ messages in thread From: John Hubbard @ 2021-02-09 6:27 UTC (permalink / raw) To: Greg KH Cc: Minchan Kim, Andrew Morton, linux-mm, LKML, surenb, joaodias, willy On 2/8/21 10:13 PM, Greg KH wrote: > On Mon, Feb 08, 2021 at 05:57:17PM -0800, John Hubbard wrote: >> On 2/8/21 3:36 PM, Minchan Kim wrote: >> ... >>>>> 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. >> >> Are you talking about the kobj in your new struct cma_stat? That seems >> like circular logic if so. I'm guessing Greg just wanted kobj methods >> to be used *if* you are dealing with kobjects. That's a narrower point. >> >> I can't imagine that he would have insisted on having additional >> allocations just so that kobj freeing methods could be used. :) > > Um, yes, I was :) > > You can not add a kobject to a structure and then somehow think you can > just ignore the reference counting issues involved. If a kobject is > part of a structure then the kobject is responsible for controling the > lifespan of the memory, nothing else can be. > > So by making the kobject dynamic, you properly handle that memory > lifespan of the object, instead of having to worry about the lifespan of > the larger object (which the original patch was not doing.) > > Does that make sense? > That part makes sense, yes, thanks. The part that I'm trying to straighten out is, why was kobject even added to the struct cma_stat in the first place? Why not just leave .stat as a static member variable, without a kobject in it, and done? thanks, -- John Hubbard NVIDIA ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] mm: cma: support sysfs 2021-02-09 6:27 ` John Hubbard @ 2021-02-09 6:34 ` John Hubbard 2021-02-09 6:56 ` Greg KH 0 siblings, 1 reply; 20+ messages in thread From: John Hubbard @ 2021-02-09 6:34 UTC (permalink / raw) To: Greg KH Cc: Minchan Kim, Andrew Morton, linux-mm, LKML, surenb, joaodias, willy On 2/8/21 10:27 PM, John Hubbard wrote: > On 2/8/21 10:13 PM, Greg KH wrote: >> On Mon, Feb 08, 2021 at 05:57:17PM -0800, John Hubbard wrote: >>> On 2/8/21 3:36 PM, Minchan Kim wrote: >>> ... >>>>>> 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. >>> >>> Are you talking about the kobj in your new struct cma_stat? That seems >>> like circular logic if so. I'm guessing Greg just wanted kobj methods >>> to be used *if* you are dealing with kobjects. That's a narrower point. >>> >>> I can't imagine that he would have insisted on having additional >>> allocations just so that kobj freeing methods could be used. :) >> >> Um, yes, I was :) >> >> You can not add a kobject to a structure and then somehow think you can >> just ignore the reference counting issues involved. If a kobject is >> part of a structure then the kobject is responsible for controling the >> lifespan of the memory, nothing else can be. >> >> So by making the kobject dynamic, you properly handle that memory >> lifespan of the object, instead of having to worry about the lifespan of >> the larger object (which the original patch was not doing.) >> >> Does that make sense? >> > That part makes sense, yes, thanks. The part that I'm trying to straighten > out is, why was kobject even added to the struct cma_stat in the first > place? Why not just leave .stat as a static member variable, without > a kobject in it, and done? > Sorry, I think I get it now: this is in order to allow a separate lifetime for the .stat member. I was sort of implicitly assuming that the "right" way to do it is just have the whole object use one lifetime management, but as you say, there is no kobject being added to the parent. I still feel odd about the allocation and freeing of something that seems to be logically the same lifetime (other than perhaps a few, briefly pending sysfs reads, at the end of life). So I'd still think that the kobject should be added to the parent... thanks, -- John Hubbard NVIDIA ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] mm: cma: support sysfs 2021-02-09 6:34 ` John Hubbard @ 2021-02-09 6:56 ` Greg KH 2021-02-09 15:55 ` Minchan Kim 0 siblings, 1 reply; 20+ messages in thread From: Greg KH @ 2021-02-09 6:56 UTC (permalink / raw) To: John Hubbard Cc: Minchan Kim, Andrew Morton, linux-mm, LKML, surenb, joaodias, willy On Mon, Feb 08, 2021 at 10:34:51PM -0800, John Hubbard wrote: > On 2/8/21 10:27 PM, John Hubbard wrote: > > On 2/8/21 10:13 PM, Greg KH wrote: > > > On Mon, Feb 08, 2021 at 05:57:17PM -0800, John Hubbard wrote: > > > > On 2/8/21 3:36 PM, Minchan Kim wrote: > > > > ... > > > > > > > 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. > > > > > > > > Are you talking about the kobj in your new struct cma_stat? That seems > > > > like circular logic if so. I'm guessing Greg just wanted kobj methods > > > > to be used *if* you are dealing with kobjects. That's a narrower point. > > > > > > > > I can't imagine that he would have insisted on having additional > > > > allocations just so that kobj freeing methods could be used. :) > > > > > > Um, yes, I was :) > > > > > > You can not add a kobject to a structure and then somehow think you can > > > just ignore the reference counting issues involved. If a kobject is > > > part of a structure then the kobject is responsible for controling the > > > lifespan of the memory, nothing else can be. > > > > > > So by making the kobject dynamic, you properly handle that memory > > > lifespan of the object, instead of having to worry about the lifespan of > > > the larger object (which the original patch was not doing.) > > > > > > Does that make sense? > > > > > That part makes sense, yes, thanks. The part that I'm trying to straighten > > out is, why was kobject even added to the struct cma_stat in the first > > place? Why not just leave .stat as a static member variable, without > > a kobject in it, and done? > > > > Sorry, I think I get it now: this is in order to allow a separate lifetime > for the .stat member. I was sort of implicitly assuming that the "right" > way to do it is just have the whole object use one lifetime management, > but as you say, there is no kobject being added to the parent. > > I still feel odd about the allocation and freeing of something that seems > to be logically the same lifetime (other than perhaps a few, briefly pending > sysfs reads, at the end of life). So I'd still think that the kobject should > be added to the parent... That's fine if you want to add it to the parent. If so, then the kobject controls the lifetime of the structure, nothing else can. Either is fine with me, what is "forbidden" is having a kobject and somehow thinking that it does not control the lifetime of the structure. thanks, greg k-h ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] mm: cma: support sysfs 2021-02-09 6:56 ` Greg KH @ 2021-02-09 15:55 ` Minchan Kim 2021-02-09 17:49 ` Greg KH 0 siblings, 1 reply; 20+ messages in thread From: Minchan Kim @ 2021-02-09 15:55 UTC (permalink / raw) To: Greg KH Cc: John Hubbard, Andrew Morton, linux-mm, LKML, surenb, joaodias, willy On Tue, Feb 09, 2021 at 07:56:30AM +0100, Greg KH wrote: > On Mon, Feb 08, 2021 at 10:34:51PM -0800, John Hubbard wrote: > > On 2/8/21 10:27 PM, John Hubbard wrote: > > > On 2/8/21 10:13 PM, Greg KH wrote: > > > > On Mon, Feb 08, 2021 at 05:57:17PM -0800, John Hubbard wrote: > > > > > On 2/8/21 3:36 PM, Minchan Kim wrote: > > > > > ... > > > > > > > > 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. > > > > > > > > > > Are you talking about the kobj in your new struct cma_stat? That seems > > > > > like circular logic if so. I'm guessing Greg just wanted kobj methods > > > > > to be used *if* you are dealing with kobjects. That's a narrower point. > > > > > > > > > > I can't imagine that he would have insisted on having additional > > > > > allocations just so that kobj freeing methods could be used. :) > > > > > > > > Um, yes, I was :) > > > > > > > > You can not add a kobject to a structure and then somehow think you can > > > > just ignore the reference counting issues involved. If a kobject is > > > > part of a structure then the kobject is responsible for controling the > > > > lifespan of the memory, nothing else can be. > > > > > > > > So by making the kobject dynamic, you properly handle that memory > > > > lifespan of the object, instead of having to worry about the lifespan of > > > > the larger object (which the original patch was not doing.) > > > > > > > > Does that make sense? > > > > > > > That part makes sense, yes, thanks. The part that I'm trying to straighten > > > out is, why was kobject even added to the struct cma_stat in the first > > > place? Why not just leave .stat as a static member variable, without > > > a kobject in it, and done? > > > > > > > Sorry, I think I get it now: this is in order to allow a separate lifetime > > for the .stat member. I was sort of implicitly assuming that the "right" > > way to do it is just have the whole object use one lifetime management, > > but as you say, there is no kobject being added to the parent. > > > > I still feel odd about the allocation and freeing of something that seems > > to be logically the same lifetime (other than perhaps a few, briefly pending > > sysfs reads, at the end of life). So I'd still think that the kobject should > > be added to the parent... sruct cma_stat { spinlock_t lock; unsigned long pages_attemtp; unsigned long pages_fail; }; struct cma { .. .. struct kobject kobj; struct cma_stat stat; }; I guess this is what Johan suggested. I agree with it. > > That's fine if you want to add it to the parent. If so, then the > kobject controls the lifetime of the structure, nothing else can. The problem was parent object(i.e., struct cma cma_areas) is static arrary so kobj->release function will be NULL or just dummy. Is it okay? I thought it was one of the what you wanted to avoid it. > > Either is fine with me, what is "forbidden" is having a kobject and > somehow thinking that it does not control the lifetime of the structure. Since parent object is static arrary, there is no need to control the lifetime so I am curious if parent object approach is okay from kobject handling point of view. If it's no problem, I am happy to change it. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] mm: cma: support sysfs 2021-02-09 15:55 ` Minchan Kim @ 2021-02-09 17:49 ` Greg KH 2021-02-09 20:11 ` John Hubbard 0 siblings, 1 reply; 20+ messages in thread From: Greg KH @ 2021-02-09 17:49 UTC (permalink / raw) To: Minchan Kim Cc: John Hubbard, Andrew Morton, linux-mm, LKML, surenb, joaodias, willy On Tue, Feb 09, 2021 at 07:55:59AM -0800, Minchan Kim wrote: > On Tue, Feb 09, 2021 at 07:56:30AM +0100, Greg KH wrote: > > On Mon, Feb 08, 2021 at 10:34:51PM -0800, John Hubbard wrote: > > > On 2/8/21 10:27 PM, John Hubbard wrote: > > > > On 2/8/21 10:13 PM, Greg KH wrote: > > > > > On Mon, Feb 08, 2021 at 05:57:17PM -0800, John Hubbard wrote: > > > > > > On 2/8/21 3:36 PM, Minchan Kim wrote: > > > > > > ... > > > > > > > > > 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. > > > > > > > > > > > > Are you talking about the kobj in your new struct cma_stat? That seems > > > > > > like circular logic if so. I'm guessing Greg just wanted kobj methods > > > > > > to be used *if* you are dealing with kobjects. That's a narrower point. > > > > > > > > > > > > I can't imagine that he would have insisted on having additional > > > > > > allocations just so that kobj freeing methods could be used. :) > > > > > > > > > > Um, yes, I was :) > > > > > > > > > > You can not add a kobject to a structure and then somehow think you can > > > > > just ignore the reference counting issues involved. If a kobject is > > > > > part of a structure then the kobject is responsible for controling the > > > > > lifespan of the memory, nothing else can be. > > > > > > > > > > So by making the kobject dynamic, you properly handle that memory > > > > > lifespan of the object, instead of having to worry about the lifespan of > > > > > the larger object (which the original patch was not doing.) > > > > > > > > > > Does that make sense? > > > > > > > > > That part makes sense, yes, thanks. The part that I'm trying to straighten > > > > out is, why was kobject even added to the struct cma_stat in the first > > > > place? Why not just leave .stat as a static member variable, without > > > > a kobject in it, and done? > > > > > > > > > > Sorry, I think I get it now: this is in order to allow a separate lifetime > > > for the .stat member. I was sort of implicitly assuming that the "right" > > > way to do it is just have the whole object use one lifetime management, > > > but as you say, there is no kobject being added to the parent. > > > > > > I still feel odd about the allocation and freeing of something that seems > > > to be logically the same lifetime (other than perhaps a few, briefly pending > > > sysfs reads, at the end of life). So I'd still think that the kobject should > > > be added to the parent... > > sruct cma_stat { > spinlock_t lock; > unsigned long pages_attemtp; > unsigned long pages_fail; > }; > > struct cma { > .. > .. > struct kobject kobj; > struct cma_stat stat; > }; > > I guess this is what Johan suggested. I agree with it. > > > > > That's fine if you want to add it to the parent. If so, then the > > kobject controls the lifetime of the structure, nothing else can. > > The problem was parent object(i.e., struct cma cma_areas) is > static arrary so kobj->release function will be NULL or just > dummy. Is it okay? I thought it was one of the what you wanted > to avoid it. No, that is not ok. > > Either is fine with me, what is "forbidden" is having a kobject and > > somehow thinking that it does not control the lifetime of the structure. > > Since parent object is static arrary, there is no need to control the > lifetime so I am curious if parent object approach is okay from kobject > handling point of view. So the array is _NEVER_ freed? If not, fine, don't provide a release function for the kobject, but ick, just make a dynamic kobject I don't see the problem for something so tiny and not very many... I worry that any static kobject might be copied/pasted as someone might think this is an ok thing to do. And it's not an ok thing to do. thanks, greg k-h ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] mm: cma: support sysfs 2021-02-09 17:49 ` Greg KH @ 2021-02-09 20:11 ` John Hubbard 2021-02-09 21:13 ` Minchan Kim 0 siblings, 1 reply; 20+ messages in thread From: John Hubbard @ 2021-02-09 20:11 UTC (permalink / raw) To: Greg KH, Minchan Kim Cc: Andrew Morton, linux-mm, LKML, surenb, joaodias, willy On 2/9/21 9:49 AM, Greg KH wrote: >>> That's fine if you want to add it to the parent. If so, then the >>> kobject controls the lifetime of the structure, nothing else can. >> >> The problem was parent object(i.e., struct cma cma_areas) is >> static arrary so kobj->release function will be NULL or just >> dummy. Is it okay? I thought it was one of the what you wanted >> to avoid it. > > No, that is not ok. > >>> Either is fine with me, what is "forbidden" is having a kobject and >>> somehow thinking that it does not control the lifetime of the structure. >> >> Since parent object is static arrary, there is no need to control the >> lifetime so I am curious if parent object approach is okay from kobject >> handling point of view. > > So the array is _NEVER_ freed? If not, fine, don't provide a release > function for the kobject, but ick, just make a dynamic kobject I don't > see the problem for something so tiny and not very many... > Yeah, I wasn't trying to generate so much discussion, I initially thought it would be a minor comment: "just use an embedded struct and avoid some extra code", at first. > I worry that any static kobject might be copied/pasted as someone might > think this is an ok thing to do. And it's not an ok thing to do. > Overall, then, we're seeing that there is a small design hole: in order to use sysfs most naturally, you either much provide a dynamically allocated item for it, or you must use the static kobject, and the second one sets a bad example. I think we should just use a static kobject, with a cautionary comment to would-be copy-pasters, that explains the design constraints above. That way, we still get a nice, less-code implementation, a safe design, and it only really costs us a single carefully written comment. thanks, -- John Hubbard NVIDIA ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] mm: cma: support sysfs 2021-02-09 20:11 ` John Hubbard @ 2021-02-09 21:13 ` Minchan Kim 2021-02-10 6:43 ` Greg KH 0 siblings, 1 reply; 20+ messages in thread From: Minchan Kim @ 2021-02-09 21:13 UTC (permalink / raw) To: John Hubbard Cc: Greg KH, Andrew Morton, linux-mm, LKML, surenb, joaodias, willy On Tue, Feb 09, 2021 at 12:11:20PM -0800, John Hubbard wrote: > On 2/9/21 9:49 AM, Greg KH wrote: > > > > That's fine if you want to add it to the parent. If so, then the > > > > kobject controls the lifetime of the structure, nothing else can. > > > > > > The problem was parent object(i.e., struct cma cma_areas) is > > > static arrary so kobj->release function will be NULL or just > > > dummy. Is it okay? I thought it was one of the what you wanted > > > to avoid it. > > > > No, that is not ok. > > > > > > Either is fine with me, what is "forbidden" is having a kobject and > > > > somehow thinking that it does not control the lifetime of the structure. > > > > > > Since parent object is static arrary, there is no need to control the > > > lifetime so I am curious if parent object approach is okay from kobject > > > handling point of view. > > > > So the array is _NEVER_ freed? If not, fine, don't provide a release > > function for the kobject, but ick, just make a dynamic kobject I don't > > see the problem for something so tiny and not very many... > > > > Yeah, I wasn't trying to generate so much discussion, I initially thought it > would be a minor comment: "just use an embedded struct and avoid some extra > code", at first. > > > I worry that any static kobject might be copied/pasted as someone might > > think this is an ok thing to do. And it's not an ok thing to do. > > > > Overall, then, we're seeing that there is a small design hole: in order > to use sysfs most naturally, you either much provide a dynamically allocated > item for it, or you must use the static kobject, and the second one sets a > bad example. > > I think we should just use a static kobject, with a cautionary comment to > would-be copy-pasters, that explains the design constraints above. That way, > we still get a nice, less-code implementation, a safe design, and it only > really costs us a single carefully written comment. > > thanks, Agreed. How about this for the warning part? + +/* + * note: kobj_type should provide a release function to free dynamically + * allocated object since kobject is responsible for controlling lifespan + * of the object. However, cma_area is static object so technially, it + * doesn't need release function. It's very exceptional case so pleaes + * do not follow this model. + */ static struct kobj_type cma_ktype = { .sysfs_ops = &kobj_sysfs_ops, .default_groups = cma_groups + .release = NULL, /* do not follow. See above */ }; ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] mm: cma: support sysfs 2021-02-09 21:13 ` Minchan Kim @ 2021-02-10 6:43 ` Greg KH 2021-02-10 7:12 ` Minchan Kim 0 siblings, 1 reply; 20+ messages in thread From: Greg KH @ 2021-02-10 6:43 UTC (permalink / raw) To: Minchan Kim Cc: John Hubbard, Andrew Morton, linux-mm, LKML, surenb, joaodias, willy On Tue, Feb 09, 2021 at 01:13:17PM -0800, Minchan Kim wrote: > On Tue, Feb 09, 2021 at 12:11:20PM -0800, John Hubbard wrote: > > On 2/9/21 9:49 AM, Greg KH wrote: > > > > > That's fine if you want to add it to the parent. If so, then the > > > > > kobject controls the lifetime of the structure, nothing else can. > > > > > > > > The problem was parent object(i.e., struct cma cma_areas) is > > > > static arrary so kobj->release function will be NULL or just > > > > dummy. Is it okay? I thought it was one of the what you wanted > > > > to avoid it. > > > > > > No, that is not ok. > > > > > > > > Either is fine with me, what is "forbidden" is having a kobject and > > > > > somehow thinking that it does not control the lifetime of the structure. > > > > > > > > Since parent object is static arrary, there is no need to control the > > > > lifetime so I am curious if parent object approach is okay from kobject > > > > handling point of view. > > > > > > So the array is _NEVER_ freed? If not, fine, don't provide a release > > > function for the kobject, but ick, just make a dynamic kobject I don't > > > see the problem for something so tiny and not very many... > > > > > > > Yeah, I wasn't trying to generate so much discussion, I initially thought it > > would be a minor comment: "just use an embedded struct and avoid some extra > > code", at first. > > > > > I worry that any static kobject might be copied/pasted as someone might > > > think this is an ok thing to do. And it's not an ok thing to do. > > > > > > > Overall, then, we're seeing that there is a small design hole: in order > > to use sysfs most naturally, you either much provide a dynamically allocated > > item for it, or you must use the static kobject, and the second one sets a > > bad example. > > > > I think we should just use a static kobject, with a cautionary comment to > > would-be copy-pasters, that explains the design constraints above. That way, > > we still get a nice, less-code implementation, a safe design, and it only > > really costs us a single carefully written comment. > > > > thanks, > > Agreed. How about this for the warning part? > > + > +/* > + * note: kobj_type should provide a release function to free dynamically > + * allocated object since kobject is responsible for controlling lifespan > + * of the object. However, cma_area is static object so technially, it > + * doesn't need release function. It's very exceptional case so pleaes > + * do not follow this model. > + */ > static struct kobj_type cma_ktype = { > .sysfs_ops = &kobj_sysfs_ops, > .default_groups = cma_groups > + .release = NULL, /* do not follow. See above */ > }; > No, please no. Just do it the correct way, what is the objection to creating a few dynamic kobjects from the heap? How many of these are you going to have that it will somehow be "wasteful"? Please do it properly. thanks, greg k-h ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] mm: cma: support sysfs 2021-02-10 6:43 ` Greg KH @ 2021-02-10 7:12 ` Minchan Kim 2021-02-10 7:16 ` John Hubbard 0 siblings, 1 reply; 20+ messages in thread From: Minchan Kim @ 2021-02-10 7:12 UTC (permalink / raw) To: Greg KH Cc: John Hubbard, Andrew Morton, linux-mm, LKML, surenb, joaodias, willy On Wed, Feb 10, 2021 at 07:43:37AM +0100, Greg KH wrote: > On Tue, Feb 09, 2021 at 01:13:17PM -0800, Minchan Kim wrote: > > On Tue, Feb 09, 2021 at 12:11:20PM -0800, John Hubbard wrote: > > > On 2/9/21 9:49 AM, Greg KH wrote: > > > > > > That's fine if you want to add it to the parent. If so, then the > > > > > > kobject controls the lifetime of the structure, nothing else can. > > > > > > > > > > The problem was parent object(i.e., struct cma cma_areas) is > > > > > static arrary so kobj->release function will be NULL or just > > > > > dummy. Is it okay? I thought it was one of the what you wanted > > > > > to avoid it. > > > > > > > > No, that is not ok. > > > > > > > > > > Either is fine with me, what is "forbidden" is having a kobject and > > > > > > somehow thinking that it does not control the lifetime of the structure. > > > > > > > > > > Since parent object is static arrary, there is no need to control the > > > > > lifetime so I am curious if parent object approach is okay from kobject > > > > > handling point of view. > > > > > > > > So the array is _NEVER_ freed? If not, fine, don't provide a release > > > > function for the kobject, but ick, just make a dynamic kobject I don't > > > > see the problem for something so tiny and not very many... > > > > > > > > > > Yeah, I wasn't trying to generate so much discussion, I initially thought it > > > would be a minor comment: "just use an embedded struct and avoid some extra > > > code", at first. > > > > > > > I worry that any static kobject might be copied/pasted as someone might > > > > think this is an ok thing to do. And it's not an ok thing to do. > > > > > > > > > > Overall, then, we're seeing that there is a small design hole: in order > > > to use sysfs most naturally, you either much provide a dynamically allocated > > > item for it, or you must use the static kobject, and the second one sets a > > > bad example. > > > > > > I think we should just use a static kobject, with a cautionary comment to > > > would-be copy-pasters, that explains the design constraints above. That way, > > > we still get a nice, less-code implementation, a safe design, and it only > > > really costs us a single carefully written comment. > > > > > > thanks, > > > > Agreed. How about this for the warning part? > > > > + > > +/* > > + * note: kobj_type should provide a release function to free dynamically > > + * allocated object since kobject is responsible for controlling lifespan > > + * of the object. However, cma_area is static object so technially, it > > + * doesn't need release function. It's very exceptional case so pleaes > > + * do not follow this model. > > + */ > > static struct kobj_type cma_ktype = { > > .sysfs_ops = &kobj_sysfs_ops, > > .default_groups = cma_groups > > + .release = NULL, /* do not follow. See above */ > > }; > > > > No, please no. Just do it the correct way, what is the objection to > creating a few dynamic kobjects from the heap? How many of these are > you going to have that it will somehow be "wasteful"? > > Please do it properly. Oh, I misunderstood your word "don't provide a release function for the kobject" so thought you agreed on John. If you didn't, we are stuck again: IIUC, the objection from John was the cma_stat lifetime should be on parent object, which is reasonable and make code simple. Frankly speaking, I don't have strong opinion about either approach. John? ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] mm: cma: support sysfs 2021-02-10 7:12 ` Minchan Kim @ 2021-02-10 7:16 ` John Hubbard 2021-02-10 7:26 ` Greg KH 0 siblings, 1 reply; 20+ messages in thread From: John Hubbard @ 2021-02-10 7:16 UTC (permalink / raw) To: Minchan Kim, Greg KH Cc: Andrew Morton, linux-mm, LKML, surenb, joaodias, willy On 2/9/21 11:12 PM, Minchan Kim wrote: ... >>> Agreed. How about this for the warning part? >>> >>> + >>> +/* >>> + * note: kobj_type should provide a release function to free dynamically >>> + * allocated object since kobject is responsible for controlling lifespan >>> + * of the object. However, cma_area is static object so technially, it >>> + * doesn't need release function. It's very exceptional case so pleaes >>> + * do not follow this model. >>> + */ >>> static struct kobj_type cma_ktype = { >>> .sysfs_ops = &kobj_sysfs_ops, >>> .default_groups = cma_groups >>> + .release = NULL, /* do not follow. See above */ >>> }; >>> >> >> No, please no. Just do it the correct way, what is the objection to >> creating a few dynamic kobjects from the heap? How many of these are >> you going to have that it will somehow be "wasteful"? >> >> Please do it properly. > > Oh, I misunderstood your word "don't provide a release function for the > kobject" so thought you agreed on John. If you didn't, we are stuck again: > IIUC, the objection from John was the cma_stat lifetime should be on parent > object, which is reasonable and make code simple. > Frankly speaking, I don't have strong opinion about either approach. > John? > We should do it as Greg requests, now that it's quite clear that he's insisting on this. Not a big deal. I just am not especially happy about the inability to do natural, efficient things here, such as use a statically allocated set of things with sysfs. And I remain convinced that the above is not "improper"; it's a reasonable step, given the limitations of the current sysfs design. I just wanted to say that out loud, as my proposal sinks to the bottom of the trench here. haha :) thanks, -- John Hubbard NVIDIA ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] mm: cma: support sysfs 2021-02-10 7:16 ` John Hubbard @ 2021-02-10 7:26 ` Greg KH 2021-02-10 7:50 ` John Hubbard 0 siblings, 1 reply; 20+ messages in thread From: Greg KH @ 2021-02-10 7:26 UTC (permalink / raw) To: John Hubbard Cc: Minchan Kim, Andrew Morton, linux-mm, LKML, surenb, joaodias, willy On Tue, Feb 09, 2021 at 11:16:07PM -0800, John Hubbard wrote: > On 2/9/21 11:12 PM, Minchan Kim wrote: > ... > > > > Agreed. How about this for the warning part? > > > > > > > > + > > > > +/* > > > > + * note: kobj_type should provide a release function to free dynamically > > > > + * allocated object since kobject is responsible for controlling lifespan > > > > + * of the object. However, cma_area is static object so technially, it > > > > + * doesn't need release function. It's very exceptional case so pleaes > > > > + * do not follow this model. > > > > + */ > > > > static struct kobj_type cma_ktype = { > > > > .sysfs_ops = &kobj_sysfs_ops, > > > > .default_groups = cma_groups > > > > + .release = NULL, /* do not follow. See above */ > > > > }; > > > > > > > > > > No, please no. Just do it the correct way, what is the objection to > > > creating a few dynamic kobjects from the heap? How many of these are > > > you going to have that it will somehow be "wasteful"? > > > > > > Please do it properly. > > > > Oh, I misunderstood your word "don't provide a release function for the > > kobject" so thought you agreed on John. If you didn't, we are stuck again: > > IIUC, the objection from John was the cma_stat lifetime should be on parent > > object, which is reasonable and make code simple. > > Frankly speaking, I don't have strong opinion about either approach. > > John? > > > > We should do it as Greg requests, now that it's quite clear that he's insisting > on this. Not a big deal. > > I just am not especially happy about the inability to do natural, efficient > things here, such as use a statically allocated set of things with sysfs. And > I remain convinced that the above is not "improper"; it's a reasonable > step, given the limitations of the current sysfs design. I just wanted to say > that out loud, as my proposal sinks to the bottom of the trench here. haha :) What is "odd" is that you are creating an object in the kernel that you _never_ free. That's not normal at all in the kernel, and so, your wish to have a kobject that you never free represent this object also is not normal :) thanks, greg k-h ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] mm: cma: support sysfs 2021-02-10 7:26 ` Greg KH @ 2021-02-10 7:50 ` John Hubbard 0 siblings, 0 replies; 20+ messages in thread From: John Hubbard @ 2021-02-10 7:50 UTC (permalink / raw) To: Greg KH Cc: Minchan Kim, Andrew Morton, linux-mm, LKML, surenb, joaodias, willy On 2/9/21 11:26 PM, Greg KH wrote: ... >> I just am not especially happy about the inability to do natural, efficient >> things here, such as use a statically allocated set of things with sysfs. And >> I remain convinced that the above is not "improper"; it's a reasonable >> step, given the limitations of the current sysfs design. I just wanted to say >> that out loud, as my proposal sinks to the bottom of the trench here. haha :) > > What is "odd" is that you are creating an object in the kernel that you > _never_ free. That's not normal at all in the kernel, and so, your wish > to have a kobject that you never free represent this object also is not > normal :) > OK, thanks for taking the time to explain that, much appreciated! thanks, -- John Hubbard NVIDIA ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2021-02-10 7:50 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-08 18:01 [PATCH v2] mm: cma: support sysfs Minchan Kim 2021-02-08 21:34 ` John Hubbard 2021-02-08 23:36 ` Minchan Kim 2021-02-09 1:57 ` John Hubbard 2021-02-09 4:19 ` Minchan Kim 2021-02-09 5:18 ` John Hubbard 2021-02-09 5:27 ` John Hubbard 2021-02-09 6:13 ` Greg KH 2021-02-09 6:27 ` John Hubbard 2021-02-09 6:34 ` John Hubbard 2021-02-09 6:56 ` Greg KH 2021-02-09 15:55 ` Minchan Kim 2021-02-09 17:49 ` Greg KH 2021-02-09 20:11 ` John Hubbard 2021-02-09 21:13 ` Minchan Kim 2021-02-10 6:43 ` Greg KH 2021-02-10 7:12 ` Minchan Kim 2021-02-10 7:16 ` John Hubbard 2021-02-10 7:26 ` Greg KH 2021-02-10 7:50 ` John Hubbard
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).