Message ID | 20210324205503.2132082-1-minchan@kernel.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v7] mm: cma: support sysfs | expand |
24.03.2021 23:55, 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 > failure rate for each CMA area. > > e.g.) > /sys/kernel/mm/cma/WIFI/alloc_pages_[success|fail] > /sys/kernel/mm/cma/SENSOR/alloc_pages_[success|fail] > /sys/kernel/mm/cma/BLUETOOTH/alloc_pages_[success|fail] > > The cma_stat was intentionally allocated by dynamic allocation > to harmonize with kobject lifetime management. > https://lore.kernel.org/linux-mm/YCOAmXqt6dZkCQYs@kroah.com/ > > Reported-by: Dmitry Osipenko <digetx@gmail.com> > Tested-by: Dmitry Osipenko <digetx@gmail.com> > Suggested-by: Dmitry Osipenko <digetx@gmail.com> The tags are incorrect, I haven't suggested this change. > Suggested-by: John Hubbard <jhubbard@nvidia.com> > Suggested-by: Matthew Wilcox <willy@infradead.org> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Reviewed-by: John Hubbard <jhubbard@nvidia.com> > Addresses-Coverity: ("Dereference after null check") There are no dereferences fixed by this patch. > Signed-off-by: Colin Ian King <colin.king@canonical.com> > Signed-off-by: Minchan Kim <minchan@kernel.org> > --- ... > #include <linux/debugfs.h> > +#include <linux/kobject.h> > + > +struct cma_kobject { > + struct cma *cma; > + struct kobject kobj; If you'll place the kobj as the first member of the struct, then container_of will be a no-op. ... > +#include <linux/cma.h> > +#include <linux/kernel.h> > +#include <linux/slab.h> > + > +#include "cma.h" > + > +void cma_sysfs_account_success_pages(struct cma *cma, unsigned long nr_pages) > +{ > + atomic64_add(nr_pages, &cma->nr_pages_succeeded); > +} > + > +void cma_sysfs_account_fail_pages(struct cma *cma, unsigned long nr_pages) > +{ > + atomic64_add(nr_pages, &cma->nr_pages_failed); > +} > + > +#define CMA_ATTR_RO(_name) \ > + static struct kobj_attribute _name##_attr = __ATTR_RO(_name) nit: #defines and inlined helpers typically are placed at the top of the code, after includes. > +static inline struct cma *cma_from_kobj(struct kobject *kobj) > +{ > + struct cma_kobject *cma_kobj = container_of(kobj, struct cma_kobject, > + kobj); > + struct cma *cma = cma_kobj->cma; > + > + return cma; nit: you can write this as: return container_of(kobj, struct cma_kobject, kobj)->cma; > +} > + > +static ssize_t alloc_pages_success_show(struct kobject *kobj, > + struct kobj_attribute *attr, char *buf) > +{ > + struct cma *cma = cma_from_kobj(kobj); > + > + return sysfs_emit(buf, "%llu\n", > + atomic64_read(&cma->nr_pages_succeeded)); nit: Algnment isn't right, should be better to write it as single line, IMO. ... > +static int __init cma_sysfs_init(void) > +{ > + struct kobject *cma_kobj_root; > + struct cma_kobject *cma_kobj; > + struct cma *cma; > + int i, err; > + > + cma_kobj_root = kobject_create_and_add("cma", mm_kobj); > + if (!cma_kobj_root) > + return -ENOMEM; > + > + for (i = 0; i < cma_area_count; i++) { > + cma_kobj = kzalloc(sizeof(*cma_kobj), GFP_KERNEL); > + if (!cma_kobj) { > + err = -ENOMEM; > + goto out; > + } > + > + cma = &cma_areas[i]; > + cma->cma_kobj = cma_kobj; > + cma_kobj->cma = cma; > + err = kobject_init_and_add(&cma_kobj->kobj, &cma_ktype, > + cma_kobj_root, "%s", cma->name); nit: Previousy algnment of the code was better here. Otherwise this is okay to me: Reviewed-by: Dmitry Osipenko <digetx@gmail.com> Tested-by: Dmitry Osipenko <digetx@gmail.com>
25.03.2021 00:31, Dmitry Osipenko пишет: >> Reported-by: Dmitry Osipenko <digetx@gmail.com> >> Tested-by: Dmitry Osipenko <digetx@gmail.com> >> Suggested-by: Dmitry Osipenko <digetx@gmail.com> > The tags are incorrect, I haven't suggested this change. The reported-by also should be removed.
On Thu, Mar 25, 2021 at 12:31:51AM +0300, Dmitry Osipenko wrote: > 24.03.2021 23:55, 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 > > failure rate for each CMA area. > > > > e.g.) > > /sys/kernel/mm/cma/WIFI/alloc_pages_[success|fail] > > /sys/kernel/mm/cma/SENSOR/alloc_pages_[success|fail] > > /sys/kernel/mm/cma/BLUETOOTH/alloc_pages_[success|fail] > > > > The cma_stat was intentionally allocated by dynamic allocation > > to harmonize with kobject lifetime management. > > https://lore.kernel.org/linux-mm/YCOAmXqt6dZkCQYs@kroah.com/ > > > > Reported-by: Dmitry Osipenko <digetx@gmail.com> > > Tested-by: Dmitry Osipenko <digetx@gmail.com> > > Suggested-by: Dmitry Osipenko <digetx@gmail.com> > > The tags are incorrect, I haven't suggested this change. During the development, you have suggested many things to make it clean. That suggested-by couldn't represent all the detail but wanted to give credit for you, too since you spent the time to make it better. Okay, since you didn't like it, I will remove it. > > > Suggested-by: John Hubbard <jhubbard@nvidia.com> > > Suggested-by: Matthew Wilcox <willy@infradead.org> > > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Reviewed-by: John Hubbard <jhubbard@nvidia.com> > > > Addresses-Coverity: ("Dereference after null check") > > There are no dereferences fixed by this patch. Let me add this: https://lore.kernel.org/linux-mm/20210316100433.17665-1-colin.king@canonical.com/ > > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > Signed-off-by: Minchan Kim <minchan@kernel.org> > > --- > ... > > > #include <linux/debugfs.h> > > +#include <linux/kobject.h> > > + > > +struct cma_kobject { > > + struct cma *cma; > > + struct kobject kobj; > > If you'll place the kobj as the first member of the struct, then > container_of will be a no-op. Cool. > > ... > > +#include <linux/cma.h> > > +#include <linux/kernel.h> > > +#include <linux/slab.h> > > + > > +#include "cma.h" > > + > > +void cma_sysfs_account_success_pages(struct cma *cma, unsigned long nr_pages) > > +{ > > + atomic64_add(nr_pages, &cma->nr_pages_succeeded); > > +} > > + > > +void cma_sysfs_account_fail_pages(struct cma *cma, unsigned long nr_pages) > > +{ > > + atomic64_add(nr_pages, &cma->nr_pages_failed); > > +} > > + > > +#define CMA_ATTR_RO(_name) \ > > + static struct kobj_attribute _name##_attr = __ATTR_RO(_name) > > nit: #defines and inlined helpers typically are placed at the top of the > code, after includes. No problem since I should resend anyway. > > > +static inline struct cma *cma_from_kobj(struct kobject *kobj) > > +{ > > + struct cma_kobject *cma_kobj = container_of(kobj, struct cma_kobject, > > + kobj); > > + struct cma *cma = cma_kobj->cma; > > + > > + return cma; > > nit: you can write this as: > > return container_of(kobj, struct cma_kobject, kobj)->cma; Better. > > > +} > > + > > +static ssize_t alloc_pages_success_show(struct kobject *kobj, > > + struct kobj_attribute *attr, char *buf) > > +{ > > + struct cma *cma = cma_from_kobj(kobj); > > + > > + return sysfs_emit(buf, "%llu\n", > > + atomic64_read(&cma->nr_pages_succeeded)); > > nit: Algnment isn't right, should be better to write it as single line, IMO. Let me make it align rather than single line since I know someone who keep asking us to not overflow 80 columns unless it's special. > > ... > > +static int __init cma_sysfs_init(void) > > +{ > > + struct kobject *cma_kobj_root; > > + struct cma_kobject *cma_kobj; > > + struct cma *cma; > > + int i, err; > > + > > + cma_kobj_root = kobject_create_and_add("cma", mm_kobj); > > + if (!cma_kobj_root) > > + return -ENOMEM; > > + > > + for (i = 0; i < cma_area_count; i++) { > > + cma_kobj = kzalloc(sizeof(*cma_kobj), GFP_KERNEL); > > + if (!cma_kobj) { > > + err = -ENOMEM; > > + goto out; > > + } > > + > > + cma = &cma_areas[i]; > > + cma->cma_kobj = cma_kobj; > > + cma_kobj->cma = cma; > > + err = kobject_init_and_add(&cma_kobj->kobj, &cma_ktype, > > + cma_kobj_root, "%s", cma->name); > > nit: Previousy algnment of the code was better here. Yub. > > Otherwise this is okay to me: > > Reviewed-by: Dmitry Osipenko <digetx@gmail.com> > Tested-by: Dmitry Osipenko <digetx@gmail.com> Thanks!
On 3/24/21 2:31 PM, Dmitry Osipenko wrote: > ... >> +#include <linux/kobject.h> >> + >> +struct cma_kobject { >> + struct cma *cma; >> + struct kobject kobj; > > If you'll place the kobj as the first member of the struct, then > container_of will be a no-op. > However, *this does not matter*. Let's not get carried away. If container_of() ends up as a compile-time addition of +8, instead of +0, there is not going to be a visible effect in the world. Or do you have some perf data to the contrary? Sometimes these kinds of things matter. But other times, they are just pointless to fret about, and this is once such case. thanks,
25.03.2021 00:55, Minchan Kim пишет: >> There are no dereferences fixed by this patch. > Let me add this: > https://lore.kernel.org/linux-mm/20210316100433.17665-1-colin.king@canonical.com/ > The tag is invalid now, since you squashed the fix. I think you may add "Co-developed-by: Colin Ian King <colin.king@canonical.com>", but this also should require to add the s-b from Colin, if he doesn't mind.
25.03.2021 01:01, John Hubbard пишет: > On 3/24/21 2:31 PM, Dmitry Osipenko wrote: >> ... >>> +#include <linux/kobject.h> >>> + >>> +struct cma_kobject { >>> + struct cma *cma; >>> + struct kobject kobj; >> >> If you'll place the kobj as the first member of the struct, then >> container_of will be a no-op. >> > > However, *this does not matter*. Let's not get carried away. If > container_of() ends up as a compile-time addition of +8, instead > of +0, there is not going to be a visible effect in the world. > Or do you have some perf data to the contrary? > > Sometimes these kinds of things matter. But other times, they are > just pointless to fret about, and this is once such case. Performance is out of question here, my main point is about maintaining a good coding style. Otherwise there is no point in not embedding kobj into cma struct as well, IMO.
25.03.2021 01:10, Dmitry Osipenko пишет: > 25.03.2021 00:55, Minchan Kim пишет: >>> There are no dereferences fixed by this patch. >> Let me add this: >> https://lore.kernel.org/linux-mm/20210316100433.17665-1-colin.king@canonical.com/ >> > > The tag is invalid now, since you squashed the fix. I think you may add > "Co-developed-by: Colin Ian King <colin.king@canonical.com>", but this > also should require to add the s-b from Colin, if he doesn't mind. > Ah, I now see that the s-b tag is already there.
25.03.2021 00:55, Minchan Kim пишет: >>> +static ssize_t alloc_pages_success_show(struct kobject *kobj, >>> + struct kobj_attribute *attr, char *buf) >>> +{ >>> + struct cma *cma = cma_from_kobj(kobj); >>> + >>> + return sysfs_emit(buf, "%llu\n", >>> + atomic64_read(&cma->nr_pages_succeeded)); >> nit: Algnment isn't right, should be better to write it as single line, IMO. > Let me make it align rather than single line since I know someone > who keep asking us to not overflow 80 columns unless it's special. > I'm actually one of those guys who complains about 80 chars (where necessary), but in this particular case it only hurts readability of the code, IMO.
25.03.2021 00:55, Minchan Kim пишет: >> The tags are incorrect, I haven't suggested this change. > During the development, you have suggested many things > to make it clean. That suggested-by couldn't represent > all the detail but wanted to give credit for you, too > since you spent the time to make it better. > > Okay, since you didn't like it, I will remove it. > That's what the r-b tag is for, the suggested-by is about suggesting the patch in general. Looking forward to the final v8, thank you for yours efforts!
On 3/24/21 3:11 PM, Dmitry Osipenko wrote: > 25.03.2021 01:01, John Hubbard пишет: >> On 3/24/21 2:31 PM, Dmitry Osipenko wrote: >>> ... >>>> +#include <linux/kobject.h> >>>> + >>>> +struct cma_kobject { >>>> + struct cma *cma; >>>> + struct kobject kobj; >>> >>> If you'll place the kobj as the first member of the struct, then >>> container_of will be a no-op. >>> >> >> However, *this does not matter*. Let's not get carried away. If >> container_of() ends up as a compile-time addition of +8, instead >> of +0, there is not going to be a visible effect in the world. >> Or do you have some perf data to the contrary? >> >> Sometimes these kinds of things matter. But other times, they are >> just pointless to fret about, and this is once such case. > > Performance is out of question here, my main point is about maintaining In that case, there is even less reason to harass people about the order of members of a struct. > a good coding style. Otherwise there is no point in not embedding kobj > into cma struct as well, IMO. We really don't need to worry about the order of members in a struct, from a "coding style" point of view. It is a solid step too far. Sorry if that sounds a little too direct. But this review has tended to go quite too far into nitpicks that are normally left as-is, and I've merely picked one that is particularly questionable. I realize that other coding communities have their own standards. Here, I'm explaining what I have observed about linux-mm and linux-kernel, which needs to be respected. thanks,
25.03.2021 01:23, John Hubbard пишет: > On 3/24/21 3:11 PM, Dmitry Osipenko wrote: >> 25.03.2021 01:01, John Hubbard пишет: >>> On 3/24/21 2:31 PM, Dmitry Osipenko wrote: >>>> ... >>>>> +#include <linux/kobject.h> >>>>> + >>>>> +struct cma_kobject { >>>>> + struct cma *cma; >>>>> + struct kobject kobj; >>>> >>>> If you'll place the kobj as the first member of the struct, then >>>> container_of will be a no-op. >>>> >>> >>> However, *this does not matter*. Let's not get carried away. If >>> container_of() ends up as a compile-time addition of +8, instead >>> of +0, there is not going to be a visible effect in the world. >>> Or do you have some perf data to the contrary? >>> >>> Sometimes these kinds of things matter. But other times, they are >>> just pointless to fret about, and this is once such case. >> >> Performance is out of question here, my main point is about maintaining > > In that case, there is even less reason to harass people about the order > of members of a struct. > >> a good coding style. Otherwise there is no point in not embedding kobj >> into cma struct as well, IMO. > > > We really don't need to worry about the order of members in a struct, > from a "coding style" point of view. It is a solid step too far. > > Sorry if that sounds a little too direct. But this review has tended to > go quite too far into nitpicks that are normally left as-is, and I've > merely picked one that is particularly questionable. I realize that other > coding communities have their own standards. Here, I'm explaining what > I have observed about linux-mm and linux-kernel, which needs to be > respected. I tried to help as much as I could, sorry if this felt annoying to you or anyone else. I assume that linux-mm maintainers, like any other maintainers, should skip all suggestions that are deemed as inappropriate to them.
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 23a0e3c98ff0..3823a2314256 100644 --- a/mm/Kconfig +++ b/mm/Kconfig @@ -524,6 +524,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 9e284dba50ef..788c5ce5c0ef 100644 --- a/mm/Makefile +++ b/mm/Makefile @@ -112,6 +112,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 0361e289c31a..08c45157911a 100644 --- a/mm/cma.c +++ b/mm/cma.c @@ -507,10 +507,14 @@ 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_account_success_pages(cma, count); + } else { count_vm_event(CMA_ALLOC_FAIL); + if (cma) + cma_sysfs_account_fail_pages(cma, count); + } return page; } diff --git a/mm/cma.h b/mm/cma.h index 42ae082cb067..37b9b7858c8e 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 object */ + struct cma_kobject *cma_kobj; +#endif }; extern struct cma cma_areas[MAX_CMA_AREAS]; @@ -26,4 +40,13 @@ static inline unsigned long cma_bitmap_maxno(struct cma *cma) return cma->count >> cma->order_per_bit; } +#ifdef CONFIG_CMA_SYSFS +void cma_sysfs_account_success_pages(struct cma *cma, unsigned long nr_pages); +void cma_sysfs_account_fail_pages(struct cma *cma, unsigned long nr_pages); +#else +static inline void cma_sysfs_account_success_pages(struct cma *cma, + unsigned long nr_pages) {}; +static inline void cma_sysfs_account_fail_pages(struct cma *cma, + unsigned long nr_pages) {}; +#endif #endif diff --git a/mm/cma_sysfs.c b/mm/cma_sysfs.c new file mode 100644 index 000000000000..73463be08df7 --- /dev/null +++ b/mm/cma_sysfs.c @@ -0,0 +1,116 @@ +// 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_account_success_pages(struct cma *cma, unsigned long nr_pages) +{ + atomic64_add(nr_pages, &cma->nr_pages_succeeded); +} + +void cma_sysfs_account_fail_pages(struct cma *cma, unsigned long nr_pages) +{ + atomic64_add(nr_pages, &cma->nr_pages_failed); +} + +#define CMA_ATTR_RO(_name) \ + static struct kobj_attribute _name##_attr = __ATTR_RO(_name) + +static inline struct cma *cma_from_kobj(struct kobject *kobj) +{ + struct cma_kobject *cma_kobj = container_of(kobj, struct cma_kobject, + kobj); + struct cma *cma = cma_kobj->cma; + + return cma; +} + +static ssize_t alloc_pages_success_show(struct kobject *kobj, + struct kobj_attribute *attr, char *buf) +{ + struct cma *cma = cma_from_kobj(kobj); + + 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 *cma = cma_from_kobj(kobj); + + 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 *cma = cma_from_kobj(kobj); + struct cma_kobject *cma_kobj = cma->cma_kobj; + + kfree(cma_kobj); + cma->cma_kobj = NULL; +} + +static struct attribute *cma_attrs[] = { + &alloc_pages_success_attr.attr, + &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) +{ + struct kobject *cma_kobj_root; + struct cma_kobject *cma_kobj; + struct cma *cma; + int i, err; + + cma_kobj_root = kobject_create_and_add("cma", mm_kobj); + if (!cma_kobj_root) + return -ENOMEM; + + for (i = 0; i < cma_area_count; i++) { + cma_kobj = kzalloc(sizeof(*cma_kobj), GFP_KERNEL); + if (!cma_kobj) { + err = -ENOMEM; + goto out; + } + + cma = &cma_areas[i]; + cma->cma_kobj = cma_kobj; + 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); + goto out; + } + } + + return 0; +out: + while (--i >= 0) { + cma = &cma_areas[i]; + kobject_put(&cma->cma_kobj->kobj); + } + kobject_put(cma_kobj_root); + + return err; +} +subsys_initcall(cma_sysfs_init);