| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <82bde114-60c0-3fde-43f4-844522b80673@gmail.com>
Date: Fri, 19 Mar 2021 20:29:29 +0300
From: Dmitry Osipenko <digetx@...il.com>
To: Minchan Kim <minchan@...nel.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Andrew Morton <akpm@...ux-foundation.org>,
linux-mm <linux-mm@...ck.org>,
LKML <linux-kernel@...r.kernel.org>, joaodias@...gle.com,
willy@...radead.org, david@...hat.com, surenb@...gle.com,
John Hubbard <jhubbard@...dia.com>,
Nicolas Chauvet <kwizart@...il.com>,
"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>
Subject: Re: [PATCH v4] mm: cma: support sysfs
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@...nel.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@...nel.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@...nel.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@...nel.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@...nel.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@...nel.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
#321: FILE: mm/cma_sysfs.c:28:
+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));
> +}
> +CMA_ATTR_RO(alloc_pages_success);
> +
> +static ssize_t alloc_pages_fail_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
CHECK: Alignment should match open parenthesis
#331: FILE: mm/cma_sysfs.c:38:
+static ssize_t alloc_pages_fail_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_failed));
> +}
> +CMA_ATTR_RO(alloc_pages_fail);
> +
> +static void cma_kobj_release(struct kobject *kobj)
> +{
> + struct cma_kobject *cma_kobj = container_of(kobj, struct cma_kobject, kobj);
> +
> + kfree(cma_kobj);
> +}
> +
> +static struct attribute *cma_attrs[] = {
> + &alloc_pages_success_attr.attr,
> + &alloc_pages_fail_attr.attr,
> + NULL,
> +};
> +ATTRIBUTE_GROUPS(cma);
> +
> +static struct cma_kobject *cma_kobjs;
> +static struct kobject *cma_kobj_root;
> +
> +static struct kobj_type cma_ktype = {
> + .release = cma_kobj_release,
> + .sysfs_ops = &kobj_sysfs_ops,
> + .default_groups = cma_groups
> +};
> +
> +static int __init cma_sysfs_init(void)
> +{
> + int i = 0;
> + struct cma *cma;
> +
> + cma_kobj_root = kobject_create_and_add("cma", mm_kobj);
> + if (!cma_kobj_root)
> + return -ENOMEM;
> +
> + cma_kobjs = kcalloc(cma_area_count, sizeof(struct cma_kobject),
> + GFP_KERNEL);
CHECK: Alignment should match open parenthesis
#373: FILE: mm/cma_sysfs.c:80:
+ cma_kobjs = kcalloc(cma_area_count, sizeof(struct cma_kobject),
+ GFP_KERNEL);
> + 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?
> + cma->kobj->cma = cma;
> + if (kobject_init_and_add(&cma->kobj->kobj, &cma_ktype,
> + cma_kobj_root, "%s", cma->name)) {
CHECK: Alignment should match open parenthesis
#382: FILE: mm/cma_sysfs.c:89:
+ if (kobject_init_and_add(&cma->kobj->kobj, &cma_ktype,
+ cma_kobj_root, "%s", cma->name)) {
> + 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?
> + }
> +
> + kfree(cma_kobjs);
> + kobject_put(cma_kobj_root);
> +
> + return -ENOMEM;
> +}
> +subsys_initcall(cma_sysfs_init);
>
Tested-by: Dmitry Osipenko <digetx@...il.com>
Powered by blists - more mailing lists