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

[Minchan Kim]

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/YCOAmXqt6dZkCQYs@xxxxxxxxx/
> > 
> > Reviewed-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Reviewed-by: John Hubbard <jhubbard@xxxxxxxxxx>
> > Signed-off-by: Minchan Kim <minchan@xxxxxxxxxx>
> > ---
> >  From v3 - https://lore.kernel.org/linux-mm/20210303205053.2906924-1-minchan@xxxxxxxxxx/
> >   * 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-minchan@xxxxxxxxxx/
> >   * sysfs doc and description modification - jhubbard
> > 
> >  From v1 - https://lore.kernel.org/linux-mm/20210203155001.4121868-1-minchan@xxxxxxxxxx/
> >   * 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 000000000000..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 <minchan@xxxxxxxxxx>
> > +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:
> > +
> > +			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?