On Fri, Mar 19, 2021 at 08:29:29PM +0300, Dmitry Osipenko wrote:
> 19.03.2021 19:30, Minchan Kim пишет:
> > On Fri, Mar 19, 2021 at 07:24:05PM +0300, Dmitry Osipenko wrote:
> >> 19.03.2021 18:50, Greg Kroah-Hartman пишет:
> >>>> Then initialization order won't be a problem.
> >>> I don't understand the problem here, what is wrong with the patch as-is?
> >>
> >> The cma->stat is NULL at the time when CMA is used on ARM because
> >> cma->stat is initialized at the subsys level. This is the problem,
> >> apparently.
> >
> > That's true.
> >
> >>
> >>> Also, watch out, if you only make the kobject dynamic, how are you going
> >>> to get backwards from the kobject to the values you want to send to
> >>> userspace in the show functions?
> >>
> >> Still there should be a wrapper around the kobj with a back reference to
> >> the cma entry. If you're suggesting that I should write a patch, then I
> >> may take a look at it later on. Although, I assume that Minchan could
> >> just correct this patch and re-push it to -next.
> >
> > This is ateempt to address it. Unless any objection, let me send it to
> > akpm.
> >
> > From 29a9fb4f300b754ebf55e6182ba84127658ef504 Mon Sep 17 00:00:00 2001
> > From: Minchan Kim <minc...@kernel.org>
> > Date: Fri, 22 Jan 2021 12:31:56 -0800
> > Subject: [PATCH] mm: cma: support sysfs
> >
> > 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/
> >
> > Signed-off-by: Minchan Kim <minc...@kernel.org>
> > ---
> > Documentation/ABI/testing/sysfs-kernel-mm-cma | 25 ++++
> > mm/Kconfig | 7 ++
> > mm/Makefile | 1 +
> > mm/cma.c | 7 +-
> > mm/cma.h | 20 ++++
> > mm/cma_sysfs.c | 107 ++++++++++++++++++
> > 6 files changed, 165 insertions(+), 2 deletions(-)
> > create mode 100644 Documentation/ABI/testing/sysfs-kernel-mm-cma
> > create mode 100644 mm/cma_sysfs.c
> >
> > 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 <minc...@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 <minc...@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 <minc...@kernel.org>
> > +Description:
> > + the number of pages CMA API failed to allocate
> > diff --git a/mm/Kconfig b/mm/Kconfig
> > index 24c045b24b95..febb7e8e24de 100644
> > --- a/mm/Kconfig
> > +++ b/mm/Kconfig
> > @@ -513,6 +513,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 72227b24a616..56968b23ed7a 100644
> > --- a/mm/Makefile
> > +++ b/mm/Makefile
> > @@ -109,6 +109,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 908f04775686..ac050359faae 100644
> > --- a/mm/cma.c
> > +++ b/mm/cma.c
> > @@ -507,10 +507,13 @@ 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_alloc_pages_count(cma, count);
> > + } else {
> > count_vm_event(CMA_ALLOC_FAIL);
> > + cma_sysfs_fail_pages_count(cma, count);
> > + }
> >
> > return page;
> > }
> > diff --git a/mm/cma.h b/mm/cma.h
> > index 42ae082cb067..70fd7633fe01 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,13 @@ 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;
> > + struct cma_kobject *kobj;
> > +#endif
> > };
> >
> > extern struct cma cma_areas[MAX_CMA_AREAS];
> > @@ -26,4 +39,11 @@ static inline unsigned long cma_bitmap_maxno(struct cma
> > *cma)
> > return cma->count >> cma->order_per_bit;
> > }
> >
> > +#ifdef CONFIG_CMA_SYSFS
> > +void cma_sysfs_alloc_pages_count(struct cma *cma, size_t count);
> > +void cma_sysfs_fail_pages_count(struct cma *cma, size_t count);
> > +#else
> > +static inline void cma_sysfs_alloc_pages_count(struct cma *cma, size_t
> > count) {};
> > +static inline void cma_sysfs_fail_pages_count(struct cma *cma, size_t
> > count) {};
> > +#endif
> > #endif
> > diff --git a/mm/cma_sysfs.c b/mm/cma_sysfs.c
> > new file mode 100644
> > index 000000000000..ca093e9e9f64
> > --- /dev/null
> > +++ b/mm/cma_sysfs.c
> > @@ -0,0 +1,107 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * CMA SysFS Interface
> > + *
> > + * Copyright (c) 2021 Minchan Kim <minc...@kernel.org>
> > + */
> > +
> > +#include <linux/cma.h>
> > +#include <linux/kernel.h>
> > +#include <linux/slab.h>
> > +
> > +#include "cma.h"
> > +
> > +void cma_sysfs_alloc_pages_count(struct cma *cma, size_t count)
> > +{
> > + atomic64_add(count, &cma->nr_pages_succeeded);
> > +}
> > +
> > +void cma_sysfs_fail_pages_count(struct cma *cma, size_t count)
> > +{
> > + atomic64_add(count, &cma->nr_pages_failed);
> > +}
> > +
> > +#define CMA_ATTR_RO(_name) \
> > + static struct kobj_attribute _name##_attr = __ATTR_RO(_name)
> > +
> > +static ssize_t alloc_pages_success_show(struct kobject *kobj,
> > + struct kobj_attribute *attr, char *buf)
>
> The indentations are still wrong.
>
> CHECK: Alignment should match open parenthesis
Hmm, I didn't know that we have that kinds of rule.
Maybe, my broken checkpatch couldn't catch it up.
Thanks.
$ ./scripts/checkpatch.pl 0001-mm-cma-support-sysfs.patch
Traceback (most recent call last):
File "scripts/spdxcheck.py", line 6, in <module>
from ply import lex, yacc
< snip >
>
> > + if (ZERO_OR_NULL_PTR(cma_kobjs))
> > + goto out;
> > +
> > + do {
> > + cma = &cma_areas[i];
> > + cma->kobj = &cma_kobjs[i];
>
> Does cma really need are pointer to cma_kobj?
Do you mean something like this?
struct cma {
..
..
struct kobject *kobj;
};
Then, how could we we make kobject dynamic model?
We need to get struct cma from kboj like below.
static ssize_t alloc_pages_success_show(struct kobject *kobj,
struct kobj_attribute *attr, char *buf)
{
struct cma_kobject *cma_kobj = container_of(kobj,
struct cma_kobject, kobj);
struct cma *cma = cma_kobj->cma;
return sysfs_emit(buf, "%llu\n",
atomic64_read(&cma->nr_pages_succeeded));
}
So kobj should be not a pointer in the container struct.
Am I missing your point? Otherwise, it would be great if you suggest
better idea.
< snip>
> > + kobject_put(&cma->kobj->kobj);
> > + goto out;
> > + }
> > + } while (++i < cma_area_count);
> > +
> > + return 0;
> > +out:
> > + while (--i >= 0) {
> > + cma = &cma_areas[i];
> > + kobject_put(&cma->kobj->kobj);
>
> Should the cma->kobj be set to NULL by cma_kobj_release() if cma->kobj is
> really needed?
True. Then, let's fix cma_kobj_release, too.
>
> > + }
> > +
> > + kfree(cma_kobjs);
> > + kobject_put(cma_kobj_root);
> > +
> > + return -ENOMEM;
> > +}
> > +subsys_initcall(cma_sysfs_init);
> >
>
> Tested-by: Dmitry Osipenko <dig...@gmail.com>
Thanks!
