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

[Andrew Morton <akpm@linux-foundation.org>]

On Wed,  3 Mar 2021 12:50:53 -0800 Minchan Kim <minchan@kernel.org> 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]
> 
> ...
>
> --- a/mm/cma.h
> +++ b/mm/cma.h
> @@ -3,6 +3,14 @@
>  #define __MM_CMA_H__
>  
>  #include <linux/debugfs.h>
> +#include <linux/kobject.h>
> +
> +struct cma_stat {
> +	spinlock_t lock;
> +	unsigned long pages_attempts;	/* the number of CMA page allocation attempts */
> +	unsigned long pages_fails;	/* the number of CMA page allocation failures */
> +	struct kobject kobj;
> +};
>  
>  struct cma {
>  	unsigned long   base_pfn;
> @@ -16,6 +24,9 @@ struct cma {
>  	struct debugfs_u32_array dfs_bitmap;
>  #endif
>  	char name[CMA_MAX_NAME];
> +#ifdef CONFIG_CMA_SYSFS
> +	struct cma_stat	*stat;
> +#endif
>  };

Why aren't the stat fields simply placed directly into struct cma_stat?

> ...
>
> +static int __init cma_sysfs_init(void)
> +{
> +	int i = 0;
> +	struct cma *cma;
> +
> +	cma_kobj = kobject_create_and_add("cma", mm_kobj);
> +	if (!cma_kobj) {
> +		pr_err("failed to create cma kobject\n");
> +		return -ENOMEM;
> +	}
> +
> +	cma_stats = kzalloc(array_size(sizeof(struct cma_stat),
> +				cma_area_count), GFP_KERNEL);

kmalloc_array(..., GFP_KERNEL|__GFP_ZERO);

?

> +	if (!cma_stats) {
> +		pr_err("failed to create cma_stats\n");

Probably unneeded - the ENOMEM stack backtrace will point straight here.

> +		goto out;
> +	}
> +
> +	do {
> +		cma = &cma_areas[i];
> +		cma->stat = &cma_stats[i];
> +		spin_lock_init(&cma->stat->lock);
> +		if (kobject_init_and_add(&cma->stat->kobj, &cma_ktype,
> +					cma_kobj, "%s", cma->name)) {
> +			kobject_put(&cma->stat->kobj);
> +			goto out;
> +		}
> +	} while (++i < cma_area_count);
> +
> +	return 0;
> +out:
> +	while (--i >= 0) {
> +		cma = &cma_areas[i];
> +		kobject_put(&cma->stat->kobj);
> +	}
> +
> +	kfree(cma_stats);
> +	kobject_put(cma_kobj);
> +
> +	return -ENOMEM;
> +}
> +subsys_initcall(cma_sysfs_init);