Re: [PATCH v7] mm: cma: support sysfs

From: Dmitry Osipenko
Date: Wed Mar 24 2021 - 17:32:50 EST


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.

> 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.

> 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.

...
> +#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@xxxxxxxxx>
Tested-by: Dmitry Osipenko <digetx@xxxxxxxxx>