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@xxxxxxxxx/ > > > > Reported-by: Dmitry Osipenko <digetx@xxxxxxxxx> > > Tested-by: Dmitry Osipenko <digetx@xxxxxxxxx> > > Suggested-by: Dmitry Osipenko <digetx@xxxxxxxxx> > > 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@xxxxxxxxxx> > > Suggested-by: Matthew Wilcox <willy@xxxxxxxxxxxxx> > > Reviewed-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> > > Reviewed-by: John Hubbard <jhubbard@xxxxxxxxxx> > > > 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@xxxxxxxxxxxxx/ > > > Signed-off-by: Colin Ian King <colin.king@xxxxxxxxxxxxx> > > Signed-off-by: Minchan Kim <minchan@xxxxxxxxxx> > > --- > ... > > > #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@xxxxxxxxx> > Tested-by: Dmitry Osipenko <digetx@xxxxxxxxx> Thanks!
