Re: [PATCH] mm: cma: support sysfs

[John Hubbard <jhubbard@nvidia.com>]

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
/sys/kernel/debug/cma, right?

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.
> 
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>   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@kernel.org>
> +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_*.

> +
> +What:		/sys/kernel/mm/cma/<cma-heap-name>/cma_alloc_attempt
> +Date:		Feb 2021
> +Contact:	Minchan Kim <minchan@kernel.org>
> +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@kernel.org>
> +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@kernel.org>
> +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@kernel.org>
> +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. :)

thanks,
-- 
John Hubbard
NVIDIA