Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: Minchan Kim <minchan.kim@gmail.com>
Date:   Fri, 19 Mar 2021 11:18:38 -0700
From:   Minchan Kim <minchan@kernel.org>
To:     Dmitry Osipenko <digetx@gmail.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        linux-mm <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>, joaodias@google.com,
        willy@infradead.org, david@redhat.com, surenb@google.com,
        John Hubbard <jhubbard@nvidia.com>,
        Nicolas Chauvet <kwizart@gmail.com>,
        "linux-tegra@vger.kernel.org" <linux-tegra@vger.kernel.org>
Subject: Re: [PATCH v4] mm: cma: support sysfs
Message-ID: <YFTq/nhQaeNSWPWs@google.com>
References: <b3cfe38f-bfd0-043a-6063-f5178d4a9b09@gmail.com>
 <YFSrgfAyp+dYWi7k@kroah.com>
 <33ec18ef-8652-643a-1a53-ff7c3caf4399@gmail.com>
 <c61e58ca-6495-fd47-0138-5bbfe0b3dd20@gmail.com>
 <YFS06OLp70nWWLFi@kroah.com>
 <78883205-e6da-5bc4-dcec-b6eb921567b1@gmail.com>
 <YFTITw73Wga0/F0V@kroah.com>
 <72db59eb-75dc-d1ed-7a83-17052e8f22a8@gmail.com>
 <YFTRkBEr5T37NFpV@google.com>
 <82bde114-60c0-3fde-43f4-844522b80673@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <82bde114-60c0-3fde-43f4-844522b80673@gmail.com>
Precedence: bulk

On Fri, Mar 19, 2021 at 08:29:29PM +0300, Dmitry Osipenko wrote:
> 19.03.2021 19:30, Minchan Kim пишет:
> > On Fri, Mar 19, 2021 at 07:24:05PM +0300, Dmitry Osipenko wrote:
> >> 19.03.2021 18:50, Greg Kroah-Hartman пишет:
> >>>> Then initialization order won't be a problem.
> >>> I don't understand the problem here, what is wrong with the patch as-is?
> >>
> >> The cma->stat is NULL at the time when CMA is used on ARM because
> >> cma->stat is initialized at the subsys level. This is the problem,
> >> apparently.
> > 
> > That's true.
> > 
> >>
> >>> Also, watch out, if you only make the kobject dynamic, how are you going
> >>> to get backwards from the kobject to the values you want to send to
> >>> userspace in the show functions?
> >>
> >> Still there should be a wrapper around the kobj with a back reference to
> >> the cma entry. If you're suggesting that I should write a patch, then I
> >> may take a look at it later on. Although, I assume that Minchan could
> >> just correct this patch and re-push it to -next.
> > 
> > This is ateempt to address it. Unless any objection, let me send it to
> > akpm.
> > 
> > From 29a9fb4f300b754ebf55e6182ba84127658ef504 Mon Sep 17 00:00:00 2001
> > From: Minchan Kim <minchan@kernel.org>
> > Date: Fri, 22 Jan 2021 12:31:56 -0800
> > Subject: [PATCH] mm: cma: support sysfs
> > 
> > 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 successful allocations
> >  * 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/alloc_pages_[success|fail]
> >   /sys/kernel/mm/cma/SENSOR/alloc_pages_[success|fail]
> >   /sys/kernel/mm/cma/BLUETOOTH/alloc_pages_[success|fail]
> > 
> > The cma_stat was intentionally allocated by dynamic allocation
> > to harmonize with kobject lifetime management.
> > https://lore.kernel.org/linux-mm/YCOAmXqt6dZkCQYs@kroah.com/
> > 
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> >  Documentation/ABI/testing/sysfs-kernel-mm-cma |  25 ++++
> >  mm/Kconfig                                    |   7 ++
> >  mm/Makefile                                   |   1 +
> >  mm/cma.c                                      |   7 +-
> >  mm/cma.h                                      |  20 ++++
> >  mm/cma_sysfs.c                                | 107 ++++++++++++++++++
> >  6 files changed, 165 insertions(+), 2 deletions(-)
> >  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..02b2bb60c296
> > --- /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@kernel.org>
> > +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:
> > +
> > +			alloc_pages_success
> > +			alloc_pages_fail
> > +
> > +What:		/sys/kernel/mm/cma/<cma-heap-name>/alloc_pages_success
> > +Date:		Feb 2021
> > +Contact:	Minchan Kim <minchan@kernel.org>
> > +Description:
> > +		the number of pages CMA API succeeded 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/mm/Kconfig b/mm/Kconfig
> > index 24c045b24b95..febb7e8e24de 100644
> > --- a/mm/Kconfig
> > +++ b/mm/Kconfig
> > @@ -513,6 +513,13 @@ config CMA_DEBUGFS
> >  	help
> >  	  Turns on the DebugFS interface for CMA.
> >  
> > +config CMA_SYSFS
> > +	bool "CMA information through sysfs interface"
> > +	depends on CMA && SYSFS
> > +	help
> > +	  This option exposes some sysfs attributes to get information
> > +	  from CMA.
> > +
> >  config CMA_AREAS
> >  	int "Maximum count of the CMA areas"
> >  	depends on CMA
> > diff --git a/mm/Makefile b/mm/Makefile
> > index 72227b24a616..56968b23ed7a 100644
> > --- a/mm/Makefile
> > +++ b/mm/Makefile
> > @@ -109,6 +109,7 @@ obj-$(CONFIG_CMA)	+= cma.o
> >  obj-$(CONFIG_MEMORY_BALLOON) += balloon_compaction.o
> >  obj-$(CONFIG_PAGE_EXTENSION) += page_ext.o
> >  obj-$(CONFIG_CMA_DEBUGFS) += cma_debug.o
> > +obj-$(CONFIG_CMA_SYSFS) += cma_sysfs.o
> >  obj-$(CONFIG_USERFAULTFD) += userfaultfd.o
> >  obj-$(CONFIG_IDLE_PAGE_TRACKING) += page_idle.o
> >  obj-$(CONFIG_DEBUG_PAGE_REF) += debug_page_ref.o
> > diff --git a/mm/cma.c b/mm/cma.c
> > index 908f04775686..ac050359faae 100644
> > --- a/mm/cma.c
> > +++ b/mm/cma.c
> > @@ -507,10 +507,13 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
> >  
> >  	pr_debug("%s(): returned %p\n", __func__, page);
> >  out:
> > -	if (page)
> > +	if (page) {
> >  		count_vm_event(CMA_ALLOC_SUCCESS);
> > -	else
> > +		cma_sysfs_alloc_pages_count(cma, count);
> > +	} else {
> >  		count_vm_event(CMA_ALLOC_FAIL);
> > +		cma_sysfs_fail_pages_count(cma, count);
> > +	}
> >  
> >  	return page;
> >  }
> > diff --git a/mm/cma.h b/mm/cma.h
> > index 42ae082cb067..70fd7633fe01 100644
> > --- a/mm/cma.h
> > +++ b/mm/cma.h
> > @@ -3,6 +3,12 @@
> >  #define __MM_CMA_H__
> >  
> >  #include <linux/debugfs.h>
> > +#include <linux/kobject.h>
> > +
> > +struct cma_kobject {
> > +	struct cma *cma;
> > +	struct kobject kobj;
> > +};
> >  
> >  struct cma {
> >  	unsigned long   base_pfn;
> > @@ -16,6 +22,13 @@ struct cma {
> >  	struct debugfs_u32_array dfs_bitmap;
> >  #endif
> >  	char name[CMA_MAX_NAME];
> > +#ifdef CONFIG_CMA_SYSFS
> > +	/* the number of CMA page successful allocations */
> > +	atomic64_t nr_pages_succeeded;
> > +	/* the number of CMA page allocation failures */
> > +	atomic64_t nr_pages_failed;
> > +	struct cma_kobject *kobj;
> > +#endif
> >  };
> >  
> >  extern struct cma cma_areas[MAX_CMA_AREAS];
> > @@ -26,4 +39,11 @@ static inline unsigned long cma_bitmap_maxno(struct cma *cma)
> >  	return cma->count >> cma->order_per_bit;
> >  }
> >  
> > +#ifdef CONFIG_CMA_SYSFS
> > +void cma_sysfs_alloc_pages_count(struct cma *cma, size_t count);
> > +void cma_sysfs_fail_pages_count(struct cma *cma, size_t count);
> > +#else
> > +static inline void cma_sysfs_alloc_pages_count(struct cma *cma, size_t count) {};
> > +static inline void cma_sysfs_fail_pages_count(struct cma *cma, size_t count) {};
> > +#endif
> >  #endif
> > diff --git a/mm/cma_sysfs.c b/mm/cma_sysfs.c
> > new file mode 100644
> > index 000000000000..ca093e9e9f64
> > --- /dev/null
> > +++ b/mm/cma_sysfs.c
> > @@ -0,0 +1,107 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * CMA SysFS Interface
> > + *
> > + * Copyright (c) 2021 Minchan Kim <minchan@kernel.org>
> > + */
> > +
> > +#include <linux/cma.h>
> > +#include <linux/kernel.h>
> > +#include <linux/slab.h>
> > +
> > +#include "cma.h"
> > +
> > +void cma_sysfs_alloc_pages_count(struct cma *cma, size_t count)
> > +{
> > +	atomic64_add(count, &cma->nr_pages_succeeded);
> > +}
> > +
> > +void cma_sysfs_fail_pages_count(struct cma *cma, size_t count)
> > +{
> > +	atomic64_add(count, &cma->nr_pages_failed);
> > +}
> > +
> > +#define CMA_ATTR_RO(_name) \
> > +	static struct kobj_attribute _name##_attr = __ATTR_RO(_name)
> > +
> > +static ssize_t alloc_pages_success_show(struct kobject *kobj,
> > +			struct kobj_attribute *attr, char *buf)
> 
> The indentations are still wrong.
> 
> CHECK: Alignment should match open parenthesis

Hmm, I didn't know that we have that kinds of rule.
Maybe, my broken checkpatch couldn't catch it up.
Thanks.

$ ./scripts/checkpatch.pl 0001-mm-cma-support-sysfs.patch
Traceback (most recent call last):
  File "scripts/spdxcheck.py", line 6, in <module>
    from ply import lex, yacc


< snip >

> 
> > +	if (ZERO_OR_NULL_PTR(cma_kobjs))
> > +		goto out;
> > +
> > +	do {
> > +		cma = &cma_areas[i];
> > +		cma->kobj = &cma_kobjs[i];
> 
> Does cma really need are pointer to cma_kobj?

Do you mean something like this?

struct cma {
	..
	..
	struct kobject *kobj;
};

Then, how could we we make kobject dynamic model?

We need to get struct cma from kboj like below.

static ssize_t alloc_pages_success_show(struct kobject *kobj,
                                        struct kobj_attribute *attr, char *buf)
{
        struct cma_kobject *cma_kobj = container_of(kobj,
                                                    struct cma_kobject, kobj);
        struct cma *cma = cma_kobj->cma;

        return sysfs_emit(buf, "%llu\n",
                          atomic64_read(&cma->nr_pages_succeeded));
}

So kobj should be not a pointer in the container struct.
Am I missing your point? Otherwise, it would be great if you suggest
better idea.


< snip>

> > +			kobject_put(&cma->kobj->kobj);
> > +			goto out;
> > +		}
> > +	} while (++i < cma_area_count);
> > +
> > +	return 0;
> > +out:
> > +	while (--i >= 0) {
> > +		cma = &cma_areas[i];
> > +		kobject_put(&cma->kobj->kobj);
> 
> Should the cma->kobj be set to NULL by cma_kobj_release() if cma->kobj is really needed?

True. Then, let's fix cma_kobj_release, too.

> 
> > +	}
> > +
> > +	kfree(cma_kobjs);
> > +	kobject_put(cma_kobj_root);
> > +
> > +	return -ENOMEM;
> > +}
> > +subsys_initcall(cma_sysfs_init);
> > 
> 
> Tested-by: Dmitry Osipenko <digetx@gmail.com>

Thanks!