Re: [PATCH] mm: cma: support sysfs

[Minchan Kim]

On Thu, Feb 04, 2021 at 12:50:58AM -0800, John Hubbard wrote:
> On 2/3/21 7:50 AM, 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 for the CMA and exposes stats below
> > to keep monitor for telemetric in the system.
> > 
> >   * the number of CMA allocation attempts
> >   * the number of CMA allocation failures
> >   * the number of CMA page allocation attempts
> >   * the number of CMA page allocation failures
> 
> The desire to report CMA data is understandable, but there are a few
> odd things here:
> 
> 1) First of all, this has significant overlap with /sys/kernel/debug/cma
> items. I suspect that all of these items could instead go into

At this moment, I don't see any overlap with item from cma_debugfs.
Could you specify what item you are mentioning?

> /sys/kernel/debug/cma, right?

Anyway, thing is I need an stable interface for that and need to use
it in Android production build, too(Unfortunately, Android deprecated
the debugfs
https://source.android.com/setup/start/android-11-release#debugfs
)

What should be in debugfs and in sysfs? What's the criteria?

Some statistic could be considered about debugging aid or telemetric
depening on view point and usecase. And here, I want to use it for
telemetric, get an stable interface and use it in production build
of Android. In this chance, I'd like to get concrete guideline
what should be in sysfs and debugfs so that pointing out this thread
whenever folks dump their stat into sysfs to avoid waste of time
for others in future. :)

> 
> 2) The overall CMA allocation attempts/failures (first two items above) seem
> an odd pair of things to track. Maybe that is what was easy to track, but I'd
> vote for just omitting them.

Then, how to know how often CMA API failed?
There are various size allocation request for a CMA so only page
allocation stat are not enough to know it.

> > 
> > Signed-off-by: Minchan Kim <minchan@xxxxxxxxxx>
> > ---
> >   Documentation/ABI/testing/sysfs-kernel-mm-cma |  39 +++++
> >   include/linux/cma.h                           |   1 +
> >   mm/Makefile                                   |   1 +
> >   mm/cma.c                                      |   6 +-
> >   mm/cma.h                                      |  20 +++
> >   mm/cma_sysfs.c                                | 143 ++++++++++++++++++
> >   6 files changed, 209 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 000000000000..2a43c0aacc39
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-kernel-mm-cma
> > @@ -0,0 +1,39 @@
> > +What:		/sys/kernel/mm/cma/
> > +Date:		Feb 2021
> > +Contact:	Minchan Kim <minchan@xxxxxxxxxx>
> > +Description:
> > +		/sys/kernel/mm/cma/ contains a number of subdirectories by
> > +		cma-heap name. The subdirectory contains a number of files
> > +		to represent cma allocation statistics.
> 
> Somewhere, maybe here, there should be a mention of the closely related
> /sys/kernel/debug/cma files.
> 
> > +
> > +		There are number of files under
> > +				/sys/kernel/mm/cma/<cma-heap-name> directory
> > +
> > +			- cma_alloc_attempt
> > +			- cma_alloc_fail
> 
> Are these really useful? They a summary of the alloc_pages items, really.
> 
> > +			- alloc_pages_attempt
> > +			- alloc_pages_fail
> 
> This should also have "cma" in the name, really: cma_alloc_pages_*.

No problem.

> 
> > +
> > +What:		/sys/kernel/mm/cma/<cma-heap-name>/cma_alloc_attempt
> > +Date:		Feb 2021
> > +Contact:	Minchan Kim <minchan@xxxxxxxxxx>
> > +Description:
> > +		the number of cma_alloc API attempted
> > +
> > +What:		/sys/kernel/mm/cma/<cma-heap-name>/cma_alloc_fail
> > +Date:		Feb 2021
> > +Contact:	Minchan Kim <minchan@xxxxxxxxxx>
> > +Description:
> > +		the number of CMA_alloc API failed
> > +
> > +What:		/sys/kernel/mm/cma/<cma-heap-name>/alloc_pages_attempt
> > +Date:		Feb 2021
> > +Contact:	Minchan Kim <minchan@xxxxxxxxxx>
> > +Description:
> > +		the number of pages CMA API tried to allocate
> > +
> > +What:		/sys/kernel/mm/cma/<cma-heap-name>/alloc_pages_fail
> > +Date:		Feb 2021
> > +Contact:	Minchan Kim <minchan@xxxxxxxxxx>
> > +Description:
> > +		the number of pages CMA API failed to allocate
> > diff --git a/include/linux/cma.h b/include/linux/cma.h
> > index 217999c8a762..71a28a5bb54e 100644
> > --- a/include/linux/cma.h
> > +++ b/include/linux/cma.h
> > @@ -49,4 +49,5 @@ extern struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
> >   extern bool cma_release(struct cma *cma, const struct page *pages, unsigned int count);
> >   extern int cma_for_each_area(int (*it)(struct cma *cma, void *data), void *data);
> > +
> 
> A single additional blank line seems to be the only change to this file. :)

Oops.