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

[Minchan Kim]

On Wed, Mar 03, 2021 at 02:44:49PM -0800, Andrew Morton wrote:
> On Wed,  3 Mar 2021 12:50:53 -0800 Minchan Kim <minchan@xxxxxxxxxx> 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?

It have a related long discussion.
https://lore.kernel.org/linux-mm/YCIoHBGELFWAyfMi@xxxxxxxxx/
https://lore.kernel.org/linux-mm/YCLLKDEQ4NYqb5Y5@xxxxxxxxx/

TLDR - Greg really want to see kobject stuff working as dynamic
property.

> 
> > ...
> >
> > +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);

Yub.

> 
> ?
> 
> > +	if (!cma_stats) {
> > +		pr_err("failed to create cma_stats\n");
> 
> Probably unneeded - the ENOMEM stack backtrace will point straight here.

I failed to find the point you mentioned to print backtrace.
Where code do you mean to dump the backtrace?