Re: [PATCH v4] mm: cma: support sysfs
20.03.2021 10:52, Greg Kroah-Hartman пишет: .. >> I found the Greg's original argument and not sure that it's really >> worthwhile to worry about the copycats since this is not a driver's code.. >> >> Maybe we could just add a clarifying comment for the kobj, telling why >> it's okay for CMA. Greg, doesn't it sound like a good compromise to you? > > Please no. > In the case of a static objects, like CMA, this creates more bad than good, IMO. Even experienced developers are getting confused. In the end it's up to you guys to decide what to choose, either way will work.
Re: [PATCH v4] mm: cma: support sysfs
On Fri, Mar 19, 2021 at 10:24:03PM +0300, Dmitry Osipenko wrote:
> 19.03.2021 22:03, Minchan Kim пишет:
> > On Fri, Mar 19, 2021 at 09:48:11PM +0300, Dmitry Osipenko wrote:
> >> 19.03.2021 21:21, Minchan Kim пишет:
> >>> On Fri, Mar 19, 2021 at 08:56:06PM +0300, Dmitry Osipenko wrote:
> 19.03.2021 19:30, Minchan Kim пишет:
> > +static void cma_kobj_release(struct kobject *kobj)
> > +{
> > + struct cma_kobject *cma_kobj = container_of(kobj, struct
> > cma_kobject, kobj);
> > +
> > + kfree(cma_kobj);
> > +}
>
> Oh, wait.. I think this kfree wrong since cma_kobj belongs to the array.
> >>>
> >>> Oh, good spot. Let me use kzalloc.
> >>>
> >>
> >> Thinking a bit more about this.. it looks like actually it should be
> >> better to get back to the older variant of cma_stat, but allocate at the
> >> time of CMA initialization, rather than at the time of sysfs
> >> initialization. Then the cma_stat will be decoupled from the cma struct
> >
> > IIRC, the problem was slab was not initiaized at CMA init point.
> > That's why I liked your suggestion.
>
> Alright, if CMA init time is a problem, then the recent variant should
> be okay.
>
> >> and cma_stat will be a self-contained object.
> >
> > Yeah, self-contained is better but it's already weird to
> > have differnt lifetime for one object since CMA object
> > never die, technically.
> >
>
> Indeed.
>
> I found the Greg's original argument and not sure that it's really
> worthwhile to worry about the copycats since this is not a driver's code..
>
> Maybe we could just add a clarifying comment for the kobj, telling why
> it's okay for CMA. Greg, doesn't it sound like a good compromise to you?
Please no.
Re: [PATCH v4] mm: cma: support sysfs
19.03.2021 22:03, Minchan Kim пишет:
> On Fri, Mar 19, 2021 at 09:48:11PM +0300, Dmitry Osipenko wrote:
>> 19.03.2021 21:21, Minchan Kim пишет:
>>> On Fri, Mar 19, 2021 at 08:56:06PM +0300, Dmitry Osipenko wrote:
19.03.2021 19:30, Minchan Kim пишет:
> +static void cma_kobj_release(struct kobject *kobj)
> +{
> + struct cma_kobject *cma_kobj = container_of(kobj, struct cma_kobject,
> kobj);
> +
> + kfree(cma_kobj);
> +}
Oh, wait.. I think this kfree wrong since cma_kobj belongs to the array.
>>>
>>> Oh, good spot. Let me use kzalloc.
>>>
>>
>> Thinking a bit more about this.. it looks like actually it should be
>> better to get back to the older variant of cma_stat, but allocate at the
>> time of CMA initialization, rather than at the time of sysfs
>> initialization. Then the cma_stat will be decoupled from the cma struct
>
> IIRC, the problem was slab was not initiaized at CMA init point.
> That's why I liked your suggestion.
Alright, if CMA init time is a problem, then the recent variant should
be okay.
>> and cma_stat will be a self-contained object.
>
> Yeah, self-contained is better but it's already weird to
> have differnt lifetime for one object since CMA object
> never die, technically.
>
Indeed.
I found the Greg's original argument and not sure that it's really
worthwhile to worry about the copycats since this is not a driver's code..
Maybe we could just add a clarifying comment for the kobj, telling why
it's okay for CMA. Greg, doesn't it sound like a good compromise to you?
Re: [PATCH v4] mm: cma: support sysfs
On Fri, Mar 19, 2021 at 09:48:11PM +0300, Dmitry Osipenko wrote:
> 19.03.2021 21:21, Minchan Kim пишет:
> > On Fri, Mar 19, 2021 at 08:56:06PM +0300, Dmitry Osipenko wrote:
> >> 19.03.2021 19:30, Minchan Kim пишет:
> >>> +static void cma_kobj_release(struct kobject *kobj)
> >>> +{
> >>> + struct cma_kobject *cma_kobj = container_of(kobj, struct cma_kobject,
> >>> kobj);
> >>> +
> >>> + kfree(cma_kobj);
> >>> +}
> >>
> >> Oh, wait.. I think this kfree wrong since cma_kobj belongs to the array.
> >
> > Oh, good spot. Let me use kzalloc.
> >
>
> Thinking a bit more about this.. it looks like actually it should be
> better to get back to the older variant of cma_stat, but allocate at the
> time of CMA initialization, rather than at the time of sysfs
> initialization. Then the cma_stat will be decoupled from the cma struct
IIRC, the problem was slab was not initiaized at CMA init point.
That's why I liked your suggestion.
> and cma_stat will be a self-contained object.
Yeah, self-contained is better but it's already weird to
have differnt lifetime for one object since CMA object
never die, technically.
Re: [PATCH v4] mm: cma: support sysfs
19.03.2021 21:18, Minchan Kim пишет: >>> +#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 > from ply import lex, yacc > > > < snip > > That's probably because I use the --strict option of checkpatch by default.
Re: [PATCH v4] mm: cma: support sysfs
19.03.2021 21:18, Minchan Kim пишет:
>>> + if (ZERO_OR_NULL_PTR(cma_kobjs))
>>> + goto out;
>>> +
>>> + do {
>>> + cma = _areas[i];
>>> + cma->kobj = _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(>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.
I meant that cma_kobj->cma is okay, but cma->kobj wasn't really used
anywhere since struct cma can't be released. I.e. we could write
kobject_put(>kobj->kobj) as kobject_put(_kobjs[i].kobj);
But since we won't use the array anymore and maybe will get back to use
the cma_stat, then it will be fine to add the pointer.
Re: [PATCH v4] mm: cma: support sysfs
19.03.2021 21:21, Minchan Kim пишет:
> On Fri, Mar 19, 2021 at 08:56:06PM +0300, Dmitry Osipenko wrote:
>> 19.03.2021 19:30, Minchan Kim пишет:
>>> +static void cma_kobj_release(struct kobject *kobj)
>>> +{
>>> + struct cma_kobject *cma_kobj = container_of(kobj, struct cma_kobject,
>>> kobj);
>>> +
>>> + kfree(cma_kobj);
>>> +}
>>
>> Oh, wait.. I think this kfree wrong since cma_kobj belongs to the array.
>
> Oh, good spot. Let me use kzalloc.
>
Thinking a bit more about this.. it looks like actually it should be
better to get back to the older variant of cma_stat, but allocate at the
time of CMA initialization, rather than at the time of sysfs
initialization. Then the cma_stat will be decoupled from the cma struct
and cma_stat will be a self-contained object.
Re: [PATCH v4] mm: cma: support sysfs
On Fri, Mar 19, 2021 at 08:56:06PM +0300, Dmitry Osipenko wrote:
> 19.03.2021 19:30, Minchan Kim пишет:
> > +static void cma_kobj_release(struct kobject *kobj)
> > +{
> > + struct cma_kobject *cma_kobj = container_of(kobj, struct cma_kobject,
> > kobj);
> > +
> > + kfree(cma_kobj);
> > +}
>
> Oh, wait.. I think this kfree wrong since cma_kobj belongs to the array.
Oh, good spot. Let me use kzalloc.
Re: [PATCH v4] mm: cma: support sysfs
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 > > 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 > > --- > > 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 ..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 > > +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/ directory) contains the > > + following items: > > + > > + alloc_pages_success > > + alloc_pages_fail > > + > > +What: /sys/kernel/mm/cma//alloc_pages_success > > +Date: Feb 2021 > > +Contact: Minchan Kim > > +Description: > > + the number of pages CMA API succeeded to allocate > > + > > +What: /sys/kernel/mm/cma//alloc_pages_fail > > +Date: Feb 2021 > > +Contact: Minchan Kim > > +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 > >
Re: [PATCH v4] mm: cma: support sysfs
On Fri, Mar 19, 2021 at 08:29:29PM +0300, Dmitry Osipenko wrote: > > +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 > #321: FILE: mm/cma_sysfs.c:28: > +static ssize_t alloc_pages_success_show(struct kobject *kobj, > + struct kobj_attribute *attr, char *buf) This is bullshit. Do not waste people's time with this frivolity.
Re: [PATCH v4] mm: cma: support sysfs
19.03.2021 19:30, Minchan Kim пишет:
> +static void cma_kobj_release(struct kobject *kobj)
> +{
> + struct cma_kobject *cma_kobj = container_of(kobj, struct cma_kobject,
> kobj);
> +
> + kfree(cma_kobj);
> +}
Oh, wait.. I think this kfree wrong since cma_kobj belongs to the array.
Re: [PATCH v4] mm: cma: support sysfs
19.03.2021 20:41, Dmitry Osipenko пишет:
> 19.03.2021 20:29, Dmitry Osipenko пишет:
>> +void cma_sysfs_alloc_pages_count(struct cma *cma, size_t count)
>> +{
>> +atomic64_add(count, >nr_pages_succeeded);
>> +}
>> +
>> +void cma_sysfs_fail_pages_count(struct cma *cma, size_t count)
>> +{
>> +atomic64_add(count, >nr_pages_failed);
>> +}
>
> The atomic looks good, but aren't CMA allocations already protected by
> the CMA core? Do we really need to worry about racing here?
>
Although, please scratch that. I see now that these functions are called
outside of the lock.
Re: [PATCH v4] mm: cma: support sysfs
19.03.2021 20:29, Dmitry Osipenko пишет:
> +void cma_sysfs_alloc_pages_count(struct cma *cma, size_t count)
> +{
> + atomic64_add(count, >nr_pages_succeeded);
> +}
> +
> +void cma_sysfs_fail_pages_count(struct cma *cma, size_t count)
> +{
> + atomic64_add(count, >nr_pages_failed);
> +}
The atomic looks good, but aren't CMA allocations already protected by
the CMA core? Do we really need to worry about racing here?
Re: [PATCH v4] mm: cma: support sysfs
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 > 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 > --- > 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 ..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 > +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/ directory) contains the > + following items: > + > + alloc_pages_success > + alloc_pages_fail > + > +What:/sys/kernel/mm/cma//alloc_pages_success > +Date:Feb 2021 > +Contact: Minchan Kim > +Description: > + the number of pages CMA API succeeded to allocate > + > +What:/sys/kernel/mm/cma//alloc_pages_fail > +Date:Feb 2021 > +Contact: Minchan Kim > +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,
Re: [PATCH v4] mm: cma: support sysfs
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
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
---
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 ..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
+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/ directory) contains the
+ following items:
+
+ alloc_pages_success
+ alloc_pages_fail
+
+What: /sys/kernel/mm/cma//alloc_pages_success
+Date: Feb 2021
+Contact: Minchan Kim
+Description:
+ the number of pages CMA API succeeded to allocate
+
+What: /sys/kernel/mm/cma//alloc_pages_fail
+Date: Feb 2021
+Contact: Minchan Kim
+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);
+
Re: [PATCH v4] mm: cma: support sysfs
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. > 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.
Re: [PATCH v4] mm: cma: support sysfs
On Fri, Mar 19, 2021 at 06:38:00PM +0300, Dmitry Osipenko wrote:
> 19.03.2021 17:27, Greg Kroah-Hartman пишет:
> > On Fri, Mar 19, 2021 at 05:19:47PM +0300, Dmitry Osipenko wrote:
> >> 19.03.2021 16:51, Dmitry Osipenko пишет:
> >>> 19.03.2021 16:47, Greg Kroah-Hartman пишет:
> On Fri, Mar 19, 2021 at 04:45:21PM +0300, Dmitry Osipenko wrote:
> > 19.03.2021 16:42, Greg Kroah-Hartman пишет:
> >> On Fri, Mar 19, 2021 at 04:39:41PM +0300, Dmitry Osipenko wrote:
> >>> 19.03.2021 15:44, Dmitry Osipenko пишет:
> >>> ...
> > #include
> > +#include
> > +
> > +struct cma_stat {
> > + spinlock_t lock;
> > + /* the number of CMA page successful allocations */
> > + unsigned long nr_pages_succeeded;
> > + /* the number of CMA page allocation failures */
> > + unsigned long nr_pages_failed;
> > + struct kobject kobj;
> > +};
> >
> > struct cma {
> > unsigned long base_pfn;
> > @@ -16,6 +26,9 @@ struct cma {
> > struct debugfs_u32_array dfs_bitmap;
> > #endif
> > char name[CMA_MAX_NAME];
> > +#ifdef CONFIG_CMA_SYSFS
> > + struct cma_stat *stat;
> > +#endif
> >>>
> >>> What is the point of allocating stat dynamically?
> >>
> >> Because static kobjects make me cry.
> >>
> >
> > I meant that it's already a part of struct cma, it looks like the stat
> > could be embedded into struct cma and then kobj could be initialized
> > separately.
>
> But that structure is statically allocated, so it can not be. This has
> been discussed in the past threads for when this was reviewed if you are
> curious :)
> >>>
> >>> Indeed, I missed that cma_areas[] is static, thank you.
> >>>
> >>
> >> And in this case should be better to make only the kobj allocated
> >> dynamically instead of the whole cma_stat.
> >
> > Why does it matter?
> >
>
> Then initialization order won't be a problem.
I don't understand the problem here, what is wrong with the patch as-is?
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?
thanks,
greg k-h
Re: [PATCH v4] mm: cma: support sysfs
19.03.2021 17:27, Greg Kroah-Hartman пишет:
> On Fri, Mar 19, 2021 at 05:19:47PM +0300, Dmitry Osipenko wrote:
>> 19.03.2021 16:51, Dmitry Osipenko пишет:
>>> 19.03.2021 16:47, Greg Kroah-Hartman пишет:
On Fri, Mar 19, 2021 at 04:45:21PM +0300, Dmitry Osipenko wrote:
> 19.03.2021 16:42, Greg Kroah-Hartman пишет:
>> On Fri, Mar 19, 2021 at 04:39:41PM +0300, Dmitry Osipenko wrote:
>>> 19.03.2021 15:44, Dmitry Osipenko пишет:
>>> ...
> #include
> +#include
> +
> +struct cma_stat {
> + spinlock_t lock;
> + /* the number of CMA page successful allocations */
> + unsigned long nr_pages_succeeded;
> + /* the number of CMA page allocation failures */
> + unsigned long nr_pages_failed;
> + struct kobject kobj;
> +};
>
> struct cma {
> unsigned long base_pfn;
> @@ -16,6 +26,9 @@ struct cma {
> struct debugfs_u32_array dfs_bitmap;
> #endif
> char name[CMA_MAX_NAME];
> +#ifdef CONFIG_CMA_SYSFS
> + struct cma_stat *stat;
> +#endif
>>>
>>> What is the point of allocating stat dynamically?
>>
>> Because static kobjects make me cry.
>>
>
> I meant that it's already a part of struct cma, it looks like the stat
> could be embedded into struct cma and then kobj could be initialized
> separately.
But that structure is statically allocated, so it can not be. This has
been discussed in the past threads for when this was reviewed if you are
curious :)
>>>
>>> Indeed, I missed that cma_areas[] is static, thank you.
>>>
>>
>> And in this case should be better to make only the kobj allocated
>> dynamically instead of the whole cma_stat.
>
> Why does it matter?
>
Then initialization order won't be a problem.
Re: [PATCH v4] mm: cma: support sysfs
On Fri, Mar 19, 2021 at 05:19:47PM +0300, Dmitry Osipenko wrote:
> 19.03.2021 16:51, Dmitry Osipenko пишет:
> > 19.03.2021 16:47, Greg Kroah-Hartman пишет:
> >> On Fri, Mar 19, 2021 at 04:45:21PM +0300, Dmitry Osipenko wrote:
> >>> 19.03.2021 16:42, Greg Kroah-Hartman пишет:
> On Fri, Mar 19, 2021 at 04:39:41PM +0300, Dmitry Osipenko wrote:
> > 19.03.2021 15:44, Dmitry Osipenko пишет:
> > ...
> >>> #include
> >>> +#include
> >>> +
> >>> +struct cma_stat {
> >>> + spinlock_t lock;
> >>> + /* the number of CMA page successful allocations */
> >>> + unsigned long nr_pages_succeeded;
> >>> + /* the number of CMA page allocation failures */
> >>> + unsigned long nr_pages_failed;
> >>> + struct kobject kobj;
> >>> +};
> >>>
> >>> struct cma {
> >>> unsigned long base_pfn;
> >>> @@ -16,6 +26,9 @@ struct cma {
> >>> struct debugfs_u32_array dfs_bitmap;
> >>> #endif
> >>> char name[CMA_MAX_NAME];
> >>> +#ifdef CONFIG_CMA_SYSFS
> >>> + struct cma_stat *stat;
> >>> +#endif
> >
> > What is the point of allocating stat dynamically?
>
> Because static kobjects make me cry.
>
> >>>
> >>> I meant that it's already a part of struct cma, it looks like the stat
> >>> could be embedded into struct cma and then kobj could be initialized
> >>> separately.
> >>
> >> But that structure is statically allocated, so it can not be. This has
> >> been discussed in the past threads for when this was reviewed if you are
> >> curious :)
> >
> > Indeed, I missed that cma_areas[] is static, thank you.
> >
>
> And in this case should be better to make only the kobj allocated
> dynamically instead of the whole cma_stat.
Why does it matter?
Re: [PATCH v4] mm: cma: support sysfs
19.03.2021 16:51, Dmitry Osipenko пишет:
> 19.03.2021 16:47, Greg Kroah-Hartman пишет:
>> On Fri, Mar 19, 2021 at 04:45:21PM +0300, Dmitry Osipenko wrote:
>>> 19.03.2021 16:42, Greg Kroah-Hartman пишет:
On Fri, Mar 19, 2021 at 04:39:41PM +0300, Dmitry Osipenko wrote:
> 19.03.2021 15:44, Dmitry Osipenko пишет:
> ...
>>> #include
>>> +#include
>>> +
>>> +struct cma_stat {
>>> + spinlock_t lock;
>>> + /* the number of CMA page successful allocations */
>>> + unsigned long nr_pages_succeeded;
>>> + /* the number of CMA page allocation failures */
>>> + unsigned long nr_pages_failed;
>>> + struct kobject kobj;
>>> +};
>>>
>>> struct cma {
>>> unsigned long base_pfn;
>>> @@ -16,6 +26,9 @@ struct cma {
>>> struct debugfs_u32_array dfs_bitmap;
>>> #endif
>>> char name[CMA_MAX_NAME];
>>> +#ifdef CONFIG_CMA_SYSFS
>>> + struct cma_stat *stat;
>>> +#endif
>
> What is the point of allocating stat dynamically?
Because static kobjects make me cry.
>>>
>>> I meant that it's already a part of struct cma, it looks like the stat
>>> could be embedded into struct cma and then kobj could be initialized
>>> separately.
>>
>> But that structure is statically allocated, so it can not be. This has
>> been discussed in the past threads for when this was reviewed if you are
>> curious :)
>
> Indeed, I missed that cma_areas[] is static, thank you.
>
And in this case should be better to make only the kobj allocated
dynamically instead of the whole cma_stat.
Re: [PATCH v4] mm: cma: support sysfs
19.03.2021 16:47, Greg Kroah-Hartman пишет:
> On Fri, Mar 19, 2021 at 04:45:21PM +0300, Dmitry Osipenko wrote:
>> 19.03.2021 16:42, Greg Kroah-Hartman пишет:
>>> On Fri, Mar 19, 2021 at 04:39:41PM +0300, Dmitry Osipenko wrote:
19.03.2021 15:44, Dmitry Osipenko пишет:
...
>> #include
>> +#include
>> +
>> +struct cma_stat {
>> +spinlock_t lock;
>> +/* the number of CMA page successful allocations */
>> +unsigned long nr_pages_succeeded;
>> +/* the number of CMA page allocation failures */
>> +unsigned long nr_pages_failed;
>> +struct kobject kobj;
>> +};
>>
>> struct cma {
>> unsigned long base_pfn;
>> @@ -16,6 +26,9 @@ struct cma {
>> struct debugfs_u32_array dfs_bitmap;
>> #endif
>> char name[CMA_MAX_NAME];
>> +#ifdef CONFIG_CMA_SYSFS
>> +struct cma_stat *stat;
>> +#endif
What is the point of allocating stat dynamically?
>>>
>>> Because static kobjects make me cry.
>>>
>>
>> I meant that it's already a part of struct cma, it looks like the stat
>> could be embedded into struct cma and then kobj could be initialized
>> separately.
>
> But that structure is statically allocated, so it can not be. This has
> been discussed in the past threads for when this was reviewed if you are
> curious :)
Indeed, I missed that cma_areas[] is static, thank you.
Re: [PATCH v4] mm: cma: support sysfs
On Fri, Mar 19, 2021 at 04:45:21PM +0300, Dmitry Osipenko wrote:
> 19.03.2021 16:42, Greg Kroah-Hartman пишет:
> > On Fri, Mar 19, 2021 at 04:39:41PM +0300, Dmitry Osipenko wrote:
> >> 19.03.2021 15:44, Dmitry Osipenko пишет:
> >> ...
> #include
> +#include
> +
> +struct cma_stat {
> +spinlock_t lock;
> +/* the number of CMA page successful allocations */
> +unsigned long nr_pages_succeeded;
> +/* the number of CMA page allocation failures */
> +unsigned long nr_pages_failed;
> +struct kobject kobj;
> +};
>
> struct cma {
> unsigned long base_pfn;
> @@ -16,6 +26,9 @@ struct cma {
> struct debugfs_u32_array dfs_bitmap;
> #endif
> char name[CMA_MAX_NAME];
> +#ifdef CONFIG_CMA_SYSFS
> +struct cma_stat *stat;
> +#endif
> >>
> >> What is the point of allocating stat dynamically?
> >
> > Because static kobjects make me cry.
> >
>
> I meant that it's already a part of struct cma, it looks like the stat
> could be embedded into struct cma and then kobj could be initialized
> separately.
But that structure is statically allocated, so it can not be. This has
been discussed in the past threads for when this was reviewed if you are
curious :)
thanks,
greg k-h
Re: [PATCH v4] mm: cma: support sysfs
19.03.2021 16:39, Dmitry Osipenko пишет:
> 19.03.2021 15:44, Dmitry Osipenko пишет:
> ...
>>> #include
>>> +#include
>>> +
>>> +struct cma_stat {
>>> + spinlock_t lock;
>>> + /* the number of CMA page successful allocations */
>>> + unsigned long nr_pages_succeeded;
>>> + /* the number of CMA page allocation failures */
>>> + unsigned long nr_pages_failed;
>>> + struct kobject kobj;
>>> +};
>>>
>>> struct cma {
>>> unsigned long base_pfn;
>>> @@ -16,6 +26,9 @@ struct cma {
>>> struct debugfs_u32_array dfs_bitmap;
>>> #endif
>>> char name[CMA_MAX_NAME];
>>> +#ifdef CONFIG_CMA_SYSFS
>>> + struct cma_stat *stat;
>>> +#endif
>
> What is the point of allocating stat dynamically?
>
> ...
>>> +void cma_sysfs_alloc_pages_count(struct cma *cma, size_t count)
>>> +{
>>> + spin_lock(>stat->lock);
>>> + cma->stat->nr_pages_succeeded += count;
>>> + spin_unlock(>stat->lock);
>>> +}
>>> +
>>> +void cma_sysfs_fail_pages_count(struct cma *cma, size_t count)
>>> +{
>>> + spin_lock(>stat->lock);
>>> + cma->stat->nr_pages_failed += count;
>>> + spin_unlock(>stat->lock);
>>> +}
>
> You could use atomic increment and then locking isn't needed.
>
Actually, the counter should be u64 in order not to worry about overflow.
Re: [PATCH v4] mm: cma: support sysfs
19.03.2021 16:42, Greg Kroah-Hartman пишет:
> On Fri, Mar 19, 2021 at 04:39:41PM +0300, Dmitry Osipenko wrote:
>> 19.03.2021 15:44, Dmitry Osipenko пишет:
>> ...
#include
+#include
+
+struct cma_stat {
+ spinlock_t lock;
+ /* the number of CMA page successful allocations */
+ unsigned long nr_pages_succeeded;
+ /* the number of CMA page allocation failures */
+ unsigned long nr_pages_failed;
+ struct kobject kobj;
+};
struct cma {
unsigned long base_pfn;
@@ -16,6 +26,9 @@ struct cma {
struct debugfs_u32_array dfs_bitmap;
#endif
char name[CMA_MAX_NAME];
+#ifdef CONFIG_CMA_SYSFS
+ struct cma_stat *stat;
+#endif
>>
>> What is the point of allocating stat dynamically?
>
> Because static kobjects make me cry.
>
I meant that it's already a part of struct cma, it looks like the stat
could be embedded into struct cma and then kobj could be initialized
separately.
Re: [PATCH v4] mm: cma: support sysfs
On Fri, Mar 19, 2021 at 04:39:41PM +0300, Dmitry Osipenko wrote:
> 19.03.2021 15:44, Dmitry Osipenko пишет:
> ...
> >> #include
> >> +#include
> >> +
> >> +struct cma_stat {
> >> + spinlock_t lock;
> >> + /* the number of CMA page successful allocations */
> >> + unsigned long nr_pages_succeeded;
> >> + /* the number of CMA page allocation failures */
> >> + unsigned long nr_pages_failed;
> >> + struct kobject kobj;
> >> +};
> >>
> >> struct cma {
> >>unsigned long base_pfn;
> >> @@ -16,6 +26,9 @@ struct cma {
> >>struct debugfs_u32_array dfs_bitmap;
> >> #endif
> >>char name[CMA_MAX_NAME];
> >> +#ifdef CONFIG_CMA_SYSFS
> >> + struct cma_stat *stat;
> >> +#endif
>
> What is the point of allocating stat dynamically?
Because static kobjects make me cry.
Re: [PATCH v4] mm: cma: support sysfs
19.03.2021 15:44, Dmitry Osipenko пишет:
...
>> #include
>> +#include
>> +
>> +struct cma_stat {
>> +spinlock_t lock;
>> +/* the number of CMA page successful allocations */
>> +unsigned long nr_pages_succeeded;
>> +/* the number of CMA page allocation failures */
>> +unsigned long nr_pages_failed;
>> +struct kobject kobj;
>> +};
>>
>> struct cma {
>> unsigned long base_pfn;
>> @@ -16,6 +26,9 @@ struct cma {
>> struct debugfs_u32_array dfs_bitmap;
>> #endif
>> char name[CMA_MAX_NAME];
>> +#ifdef CONFIG_CMA_SYSFS
>> +struct cma_stat *stat;
>> +#endif
What is the point of allocating stat dynamically?
...
>> +void cma_sysfs_alloc_pages_count(struct cma *cma, size_t count)
>> +{
>> +spin_lock(>stat->lock);
>> +cma->stat->nr_pages_succeeded += count;
>> +spin_unlock(>stat->lock);
>> +}
>> +
>> +void cma_sysfs_fail_pages_count(struct cma *cma, size_t count)
>> +{
>> +spin_lock(>stat->lock);
>> +cma->stat->nr_pages_failed += count;
>> +spin_unlock(>stat->lock);
>> +}
You could use atomic increment and then locking isn't needed.
Re: [PATCH v4] mm: cma: support sysfs
09.03.2021 09:23, 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/
>
> Reviewed-by: Greg Kroah-Hartman
> Reviewed-by: John Hubbard
> Signed-off-by: Minchan Kim
> ---
> From v3 -
> https://lore.kernel.org/linux-mm/20210303205053.2906924-1-minc...@kernel.org/
> * fix ZERO_OR_NULL_PTR - kernel test robot
> * remove prefix cma - david@
> * resolve conflict with vmstat cma in mmotm - akpm@
> * rename stat name with success|fail
>
> From v2 -
> https://lore.kernel.org/linux-mm/20210208180142.2765456-1-minc...@kernel.org/
> * sysfs doc and description modification - jhubbard
>
> From v1 -
> https://lore.kernel.org/linux-mm/20210203155001.4121868-1-minc...@kernel.org/
> * fix sysfs build and refactoring - willy
> * rename and drop some attributes - jhubbard
>
> 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| 110 ++
> 6 files changed, 168 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 ..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
> +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/ directory) contains the
> + following items:
> +
> + alloc_pages_success
> + alloc_pages_fail
> +
> +What:/sys/kernel/mm/cma//alloc_pages_success
> +Date:Feb 2021
> +Contact: Minchan Kim
> +Description:
> + the number of pages CMA API succeeded to allocate
> +
> +What:/sys/kernel/mm/cma//alloc_pages_fail
> +Date:Feb 2021
> +Contact: Minchan Kim
> +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
[PATCH v4] 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/
Reviewed-by: Greg Kroah-Hartman
Reviewed-by: John Hubbard
Signed-off-by: Minchan Kim
---
>From v3 -
>https://lore.kernel.org/linux-mm/20210303205053.2906924-1-minc...@kernel.org/
* fix ZERO_OR_NULL_PTR - kernel test robot
* remove prefix cma - david@
* resolve conflict with vmstat cma in mmotm - akpm@
* rename stat name with success|fail
>From v2 -
>https://lore.kernel.org/linux-mm/20210208180142.2765456-1-minc...@kernel.org/
* sysfs doc and description modification - jhubbard
>From v1 -
>https://lore.kernel.org/linux-mm/20210203155001.4121868-1-minc...@kernel.org/
* fix sysfs build and refactoring - willy
* rename and drop some attributes - jhubbard
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| 110 ++
6 files changed, 168 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 ..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
+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/ directory) contains the
+ following items:
+
+ alloc_pages_success
+ alloc_pages_fail
+
+What: /sys/kernel/mm/cma//alloc_pages_success
+Date: Feb 2021
+Contact: Minchan Kim
+Description:
+ the number of pages CMA API succeeded to allocate
+
+What: /sys/kernel/mm/cma//alloc_pages_fail
+Date: Feb 2021
+Contact: Minchan Kim
+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..95d1aa2d808a 100644
--- a/mm/cma.h
+++ b/mm/cma.h
@@ -3,6 +3,16 @@
#define __MM_CMA_H__
#include
+#include
+
+struct cma_stat {
+ spinlock_t lock;
+ /* the number of CMA page successful allocations */
+ unsigned long nr_pages_succeeded;
+ /*
Re: [PATCH v4] mm: cma: support sysfs
On Fri, Mar 05, 2021 at 06:34:22PM +0100, David Hildenbrand wrote: > On 04.03.21 17:17, Minchan Kim wrote: > > 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 allocation attempts > > * 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/cma_alloc_pages_[attempts|fails] > >/sys/kernel/mm/cma/SENSOR/cma_alloc_pages_[attempts|fails] > >/sys/kernel/mm/cma/BLUETOOTH/cma_alloc_pages_[attempts|fails] > > > > The cma_stat was intentionally allocated by dynamic allocation > > to harmonize with kobject lifetime management. > > https://lore.kernel.org/linux-mm/ycoamxqt6dzkc...@kroah.com/ > > > > Reviewed-by: Greg Kroah-Hartman > > Reviewed-by: John Hubbard > > Signed-off-by: Minchan Kim > > --- > > From v3 - > > https://lore.kernel.org/linux-mm/20210303205053.2906924-1-minc...@kernel.org/ > > * kmalloc_array - akpm > > * add why cma_stat was implemented by dynamic allocation - akpm > > * use !__GFP_NOWARN facility to print error - akpm > > > > From v2 - > > https://lore.kernel.org/linux-mm/20210208180142.2765456-1-minc...@kernel.org/ > > * sysfs doc and description modification - jhubbard > > > > From v1 - > > https://lore.kernel.org/linux-mm/20210203155001.4121868-1-minc...@kernel.org/ > > * fix sysfs build and refactoring - willy > > * rename and drop some attributes - jhubbard > > > > Documentation/ABI/testing/sysfs-kernel-mm-cma | 25 > > mm/Kconfig| 7 ++ > > mm/Makefile | 1 + > > mm/cma.c | 6 +- > > mm/cma.h | 18 +++ > > mm/cma_sysfs.c| 110 ++ > > 6 files changed, 166 insertions(+), 1 deletion(-) > > 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 ..f518af819cee > > --- /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 > > +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/ directory) contains the > > + following items: > > + > > + cma_alloc_pages_attempts > > + cma_alloc_pages_fails > > Nit: why "cma_" again when we are already under "/cma/" ? Originally, there was desire to add cma_alloc_attempts as well as cma_alloc_pages_attempts. > > I'd simply go with something like > > "total_alloc_attempts" > "failed_alloc_attempts" If we really want to remove the cma prefix, maybe, alloc_pages_attempts alloc_pages_fails If someone want to count cma_alloc itself, Then alloc_success alloc_fail Does that make sense?
Re: [PATCH v4] mm: cma: support sysfs
On 04.03.21 17:17, Minchan Kim wrote: 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 allocation attempts * 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/cma_alloc_pages_[attempts|fails] /sys/kernel/mm/cma/SENSOR/cma_alloc_pages_[attempts|fails] /sys/kernel/mm/cma/BLUETOOTH/cma_alloc_pages_[attempts|fails] The cma_stat was intentionally allocated by dynamic allocation to harmonize with kobject lifetime management. https://lore.kernel.org/linux-mm/ycoamxqt6dzkc...@kroah.com/ Reviewed-by: Greg Kroah-Hartman Reviewed-by: John Hubbard Signed-off-by: Minchan Kim --- From v3 - https://lore.kernel.org/linux-mm/20210303205053.2906924-1-minc...@kernel.org/ * kmalloc_array - akpm * add why cma_stat was implemented by dynamic allocation - akpm * use !__GFP_NOWARN facility to print error - akpm From v2 - https://lore.kernel.org/linux-mm/20210208180142.2765456-1-minc...@kernel.org/ * sysfs doc and description modification - jhubbard From v1 - https://lore.kernel.org/linux-mm/20210203155001.4121868-1-minc...@kernel.org/ * fix sysfs build and refactoring - willy * rename and drop some attributes - jhubbard Documentation/ABI/testing/sysfs-kernel-mm-cma | 25 mm/Kconfig| 7 ++ mm/Makefile | 1 + mm/cma.c | 6 +- mm/cma.h | 18 +++ mm/cma_sysfs.c| 110 ++ 6 files changed, 166 insertions(+), 1 deletion(-) 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 ..f518af819cee --- /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 +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/ directory) contains the + following items: + + cma_alloc_pages_attempts + cma_alloc_pages_fails Nit: why "cma_" again when we are already under "/cma/" ? I'd simply go with something like "total_alloc_attempts" "failed_alloc_attempts" But maybe this has been discussed already. + +What: /sys/kernel/mm/cma//cma_alloc_pages_attempts +Date: Feb 2021 +Contact: Minchan Kim +Description: + the number of pages CMA API tried to allocate + +What: /sys/kernel/mm/cma//cma_alloc_pages_fails +Date: Feb 2021 +Contact: Minchan Kim +Description: + the number of pages CMA API failed to allocate This will be useful. -- Thanks, David / dhildenb
[PATCH v4] 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 allocation attempts
* 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/cma_alloc_pages_[attempts|fails]
/sys/kernel/mm/cma/SENSOR/cma_alloc_pages_[attempts|fails]
/sys/kernel/mm/cma/BLUETOOTH/cma_alloc_pages_[attempts|fails]
The cma_stat was intentionally allocated by dynamic allocation
to harmonize with kobject lifetime management.
https://lore.kernel.org/linux-mm/ycoamxqt6dzkc...@kroah.com/
Reviewed-by: Greg Kroah-Hartman
Reviewed-by: John Hubbard
Signed-off-by: Minchan Kim
---
>From v3 -
>https://lore.kernel.org/linux-mm/20210303205053.2906924-1-minc...@kernel.org/
* kmalloc_array - akpm
* add why cma_stat was implemented by dynamic allocation - akpm
* use !__GFP_NOWARN facility to print error - akpm
>From v2 -
>https://lore.kernel.org/linux-mm/20210208180142.2765456-1-minc...@kernel.org/
* sysfs doc and description modification - jhubbard
>From v1 -
>https://lore.kernel.org/linux-mm/20210203155001.4121868-1-minc...@kernel.org/
* fix sysfs build and refactoring - willy
* rename and drop some attributes - jhubbard
Documentation/ABI/testing/sysfs-kernel-mm-cma | 25
mm/Kconfig| 7 ++
mm/Makefile | 1 +
mm/cma.c | 6 +-
mm/cma.h | 18 +++
mm/cma_sysfs.c| 110 ++
6 files changed, 166 insertions(+), 1 deletion(-)
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 ..f518af819cee
--- /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
+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/ directory) contains the
+ following items:
+
+ cma_alloc_pages_attempts
+ cma_alloc_pages_fails
+
+What: /sys/kernel/mm/cma//cma_alloc_pages_attempts
+Date: Feb 2021
+Contact: Minchan Kim
+Description:
+ the number of pages CMA API tried to allocate
+
+What: /sys/kernel/mm/cma//cma_alloc_pages_fails
+Date: Feb 2021
+Contact: Minchan Kim
+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 54eee2119822..551b704faeaf 100644
--- a/mm/cma.c
+++ b/mm/cma.c
@@ -447,9 +447,10 @@ struct page *cma_alloc(struct cma *cma, size_t count,
unsigned int align,
offset = cma_bitmap_aligned_offset(cma, align);
bitmap_maxno = cma_bitmap_maxno(cma);
bitmap_count = cma_bitmap_pages_to_bits(cma, count);
+ cma_sysfs_alloc_count(cma, count);
if (bitmap_count > bitmap_maxno)
- return NULL;
+ goto out;
for (;;) {
mutex_lock(>lock);
@@ -504,6 +505,9 @@ struct page *cma_alloc(struct cma *cma, size_t count,
unsigned int align,
__func__, cma->name, count, ret);
cma_debug_show_areas(cma);
}
+out:
+ if (!page)
+ cma_sysfs_fail_count(cma, count);
pr_debug("%s(): returned %p\n", __func__, page);
