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/ycoamxqt6dzkc...@kroah.com/
> >
> > Reported-by: Dmitry Osipenko <dig...@gmail.com>
> > Tested-by: Dmitry Osipenko <dig...@gmail.com>
> > Suggested-by: Dmitry Osipenko <dig...@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 <jhubb...@nvidia.com>
> > Suggested-by: Matthew Wilcox <wi...@infradead.org>
> > Reviewed-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
> > Reviewed-by: John Hubbard <jhubb...@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.k...@canonical.com/
>
> > Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> > Signed-off-by: Minchan Kim <minc...@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 <dig...@gmail.com>
> Tested-by: Dmitry Osipenko <dig...@gmail.com>
Thanks!
