Messages in this thread | | | Subject | Re: [PATCH v7] mm: cma: support sysfs | From | Dmitry Osipenko <> | Date | Thu, 25 Mar 2021 00:31:51 +0300 |
| |
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>
| |