| 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: <a74b62ef71be4348889bfc8c620e7b77@hisilicon.com>
Date: Fri, 16 Jul 2021 08:49:58 +0000
From: "Song Bao Hua (Barry Song)" <song.bao.hua@...ilicon.com>
To: "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
"andriy.shevchenko@...ux.intel.com"
<andriy.shevchenko@...ux.intel.com>,
"yury.norov@...il.com" <yury.norov@...il.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC: "dave.hansen@...el.com" <dave.hansen@...el.com>,
"linux@...musvillemoes.dk" <linux@...musvillemoes.dk>,
"rafael@...nel.org" <rafael@...nel.org>,
"rdunlap@...radead.org" <rdunlap@...radead.org>,
"agordeev@...ux.ibm.com" <agordeev@...ux.ibm.com>,
"sbrivio@...hat.com" <sbrivio@...hat.com>,
"jianpeng.ma@...el.com" <jianpeng.ma@...el.com>,
"valentin.schneider@....com" <valentin.schneider@....com>,
"peterz@...radead.org" <peterz@...radead.org>,
"bristot@...hat.com" <bristot@...hat.com>,
"guodong.xu@...aro.org" <guodong.xu@...aro.org>,
tangchengchang <tangchengchang@...wei.com>,
"Zengtao (B)" <prime.zeng@...ilicon.com>,
yangyicong <yangyicong@...wei.com>,
"tim.c.chen@...ux.intel.com" <tim.c.chen@...ux.intel.com>,
Linuxarm <linuxarm@...wei.com>,
"tiantao (H)" <tiantao6@...ilicon.com>
Subject: RE: [PATCH v7 2/4] topology: use bin_attribute to break the size
limitation of cpumap ABI
Hi Yury,
Not sure if I have totally got your idea. But please see if the below
is closer to what you prefer.
I haven't really tested it. But this approach can somehow solve the
problem you mentioned(malloc/free and printing is done 1000times for
a 1MB buffer which is read 1K each time).
Bitmap provides some API to alloc and return print_buf:
+ssize_t bitmap_get_print_buf(bool list, char **buf, const unsigned long *maskp,
+ int nmaskbits)
+{
+ const char *fmt = list ? "%*pbl\n" : "%*pb\n";
+ ssize_t size;
+
+ size = snprintf(NULL, 0, fmt, nmaskbits, maskp);
+ *buf = kmalloc_track_caller(size + 1, GFP_KERNEL);
+ scnprintf(*buf, size + 1, fmt, nmaskbits, maskp);
+
+ return size + 1;
+}
+
+static inline ssize_t
+cpumap_get_print_buf(bool list, char **buf, const struct cpumask *mask)
+{
+ return bitmap_get_print_buf(list, buf, cpumask_bits(mask),
+ nr_cpu_ids);
+}
+
+struct bitmap_print_buf
+{
+ char *buf;
+ ssize_t size;
+};
+
In bin_attribute, move to get and save the buffer while sysfs entry is
read at the first time and free it when file arrives EOF:
#define define_id_show_func(name) \
static ssize_t name##_show(struct device *dev, \
struct device_attribute *attr, char *buf) \
@@ -27,9 +53,27 @@ static ssize_t name##_read(struct file *file, struct kobject *kobj, \
loff_t off, size_t count) \
{ \
struct device *dev = kobj_to_dev(kobj); \
- \
- return cpumap_print_to_buf(false, buf, topology_##mask(dev->id), \
- off, count); \
+ struct bitmap_print_buf *bmb = dev_get_drvdata(dev); \
+ if (!bmb) { \
+ bmb = devm_kzalloc(dev, sizeof(*bmb), GFP_KERNEL); \
+ if (!bmb) \
+ return -ENOMEM; \
+ dev_set_drvdata(dev, bmb); \
+ } \
+ /* for the 1st time, getting the printed buffer */ \
+ if (!bmb->buf) \
+ bmb->size = cpumap_get_print_buf(false, &bmb->buf, \
+ topology_##mask(dev->id)); \
+ /* when we arrive EOF, free the printed buffer */ \
+ if (off >= bmb->size) { \
+ kfree(bmb->buf); bmb->buf = NULL; \
+ return 0; \
+ } \
+ /* while a large printed buffer is read many times, we reuse \
+ * the buffer we get at the 1st time \
+ */ \
+ strncpy(buf, bmb->buf + off, count); \
+ return min(count, bmb->size - off); \
} \
\
This means a huge change in drivers though I am not sure Greg is
a fan of this approach. Anyway, "1000 times" is not a real case.
Typically we will arrive EOF after one time.
Thanks
Barry
> -----Original Message-----
> From: Song Bao Hua (Barry Song)
> Sent: Thursday, July 15, 2021 11:59 PM
> To: gregkh@...uxfoundation.org; akpm@...ux-foundation.org;
> andriy.shevchenko@...ux.intel.com; yury.norov@...il.com;
> linux-kernel@...r.kernel.org
> Cc: dave.hansen@...el.com; linux@...musvillemoes.dk; rafael@...nel.org;
> rdunlap@...radead.org; agordeev@...ux.ibm.com; sbrivio@...hat.com;
> jianpeng.ma@...el.com; valentin.schneider@....com; peterz@...radead.org;
> bristot@...hat.com; guodong.xu@...aro.org; tangchengchang
> <tangchengchang@...wei.com>; Zengtao (B) <prime.zeng@...ilicon.com>;
> yangyicong <yangyicong@...wei.com>; tim.c.chen@...ux.intel.com; Linuxarm
> <linuxarm@...wei.com>; tiantao (H) <tiantao6@...ilicon.com>; Song Bao Hua
> (Barry Song) <song.bao.hua@...ilicon.com>
> Subject: [PATCH v7 2/4] topology: use bin_attribute to break the size limitation
> of cpumap ABI
>
> From: Tian Tao <tiantao6@...ilicon.com>
>
> Reading /sys/devices/system/cpu/cpuX/topology/ returns cpu topology.
> However, the size of this file is limited to PAGE_SIZE because of
> the limitation for sysfs attribute.
> This patch moves to use bin_attribute to extend the ABI to be more
> than one page so that cpumap bitmask and list won't be potentially
> trimmed.
>
> Signed-off-by: Tian Tao <tiantao6@...ilicon.com>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@...nel.org>
> Signed-off-by: Barry Song <song.bao.hua@...ilicon.com>
> ---
> drivers/base/topology.c | 115 ++++++++++++++++++++++------------------
> 1 file changed, 63 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/base/topology.c b/drivers/base/topology.c
> index 4d254fcc93d1..dd3980124e33 100644
> --- a/drivers/base/topology.c
> +++ b/drivers/base/topology.c
> @@ -21,25 +21,27 @@ static ssize_t name##_show(struct device *dev,
> \
> return sysfs_emit(buf, "%d\n", topology_##name(dev->id)); \
> }
>
> -#define define_siblings_show_map(name, mask) \
> -static ssize_t name##_show(struct device *dev, \
> - struct device_attribute *attr, char *buf) \
> -{ \
> - return cpumap_print_to_pagebuf(false, buf, topology_##mask(dev->id));\
> +#define define_siblings_read_func(name, mask) \
> +static ssize_t name##_read(struct file *file, struct kobject *kobj, \
> + struct bin_attribute *attr, char *buf, \
> + loff_t off, size_t count) \
> +{ \
> + struct device *dev = kobj_to_dev(kobj); \
> + \
> + return cpumap_print_to_buf(false, buf, topology_##mask(dev->id), \
> + off, count); \
> +} \
> + \
> +static ssize_t name##_list_read(struct file *file, struct kobject *kobj, \
> + struct bin_attribute *attr, char *buf, \
> + loff_t off, size_t count) \
> +{ \
> + struct device *dev = kobj_to_dev(kobj); \
> + \
> + return cpumap_print_to_buf(true, buf, topology_##mask(dev->id), \
> + off, count); \
> }
>
> -#define define_siblings_show_list(name, mask) \
> -static ssize_t name##_list_show(struct device *dev, \
> - struct device_attribute *attr, \
> - char *buf) \
> -{ \
> - return cpumap_print_to_pagebuf(true, buf, topology_##mask(dev->id));\
> -}
> -
> -#define define_siblings_show_func(name, mask) \
> - define_siblings_show_map(name, mask); \
> - define_siblings_show_list(name, mask)
> -
> define_id_show_func(physical_package_id);
> static DEVICE_ATTR_RO(physical_package_id);
>
> @@ -49,71 +51,80 @@ static DEVICE_ATTR_RO(die_id);
> define_id_show_func(core_id);
> static DEVICE_ATTR_RO(core_id);
>
> -define_siblings_show_func(thread_siblings, sibling_cpumask);
> -static DEVICE_ATTR_RO(thread_siblings);
> -static DEVICE_ATTR_RO(thread_siblings_list);
> +define_siblings_read_func(thread_siblings, sibling_cpumask);
> +static BIN_ATTR_RO(thread_siblings, 0);
> +static BIN_ATTR_RO(thread_siblings_list, 0);
>
> -define_siblings_show_func(core_cpus, sibling_cpumask);
> -static DEVICE_ATTR_RO(core_cpus);
> -static DEVICE_ATTR_RO(core_cpus_list);
> +define_siblings_read_func(core_cpus, sibling_cpumask);
> +static BIN_ATTR_RO(core_cpus, 0);
> +static BIN_ATTR_RO(core_cpus_list, 0);
>
> -define_siblings_show_func(core_siblings, core_cpumask);
> -static DEVICE_ATTR_RO(core_siblings);
> -static DEVICE_ATTR_RO(core_siblings_list);
> +define_siblings_read_func(core_siblings, core_cpumask);
> +static BIN_ATTR_RO(core_siblings, 0);
> +static BIN_ATTR_RO(core_siblings_list, 0);
>
> -define_siblings_show_func(die_cpus, die_cpumask);
> -static DEVICE_ATTR_RO(die_cpus);
> -static DEVICE_ATTR_RO(die_cpus_list);
> +define_siblings_read_func(die_cpus, die_cpumask);
> +static BIN_ATTR_RO(die_cpus, 0);
> +static BIN_ATTR_RO(die_cpus_list, 0);
>
> -define_siblings_show_func(package_cpus, core_cpumask);
> -static DEVICE_ATTR_RO(package_cpus);
> -static DEVICE_ATTR_RO(package_cpus_list);
> +define_siblings_read_func(package_cpus, core_cpumask);
> +static BIN_ATTR_RO(package_cpus, 0);
> +static BIN_ATTR_RO(package_cpus_list, 0);
>
> #ifdef CONFIG_SCHED_BOOK
> define_id_show_func(book_id);
> static DEVICE_ATTR_RO(book_id);
> -define_siblings_show_func(book_siblings, book_cpumask);
> -static DEVICE_ATTR_RO(book_siblings);
> -static DEVICE_ATTR_RO(book_siblings_list);
> +define_siblings_read_func(book_siblings, book_cpumask);
> +static BIN_ATTR_RO(book_siblings, 0);
> +static BIN_ATTR_RO(book_siblings_list, 0);
> #endif
>
> #ifdef CONFIG_SCHED_DRAWER
> define_id_show_func(drawer_id);
> static DEVICE_ATTR_RO(drawer_id);
> -define_siblings_show_func(drawer_siblings, drawer_cpumask);
> -static DEVICE_ATTR_RO(drawer_siblings);
> -static DEVICE_ATTR_RO(drawer_siblings_list);
> +define_siblings_read_func(drawer_siblings, drawer_cpumask);
> +static BIN_ATTR_RO(drawer_siblings, 0);
> +static BIN_ATTR_RO(drawer_siblings_list, 0);
> #endif
>
> +static struct bin_attribute *bin_attrs[] = {
> + &bin_attr_core_cpus,
> + &bin_attr_core_cpus_list,
> + &bin_attr_thread_siblings,
> + &bin_attr_thread_siblings_list,
> + &bin_attr_core_siblings,
> + &bin_attr_core_siblings_list,
> + &bin_attr_die_cpus,
> + &bin_attr_die_cpus_list,
> + &bin_attr_package_cpus,
> + &bin_attr_package_cpus_list,
> +#ifdef CONFIG_SCHED_BOOK
> + &bin_attr_book_siblings,
> + &bin_attr_book_siblings_list,
> +#endif
> +#ifdef CONFIG_SCHED_DRAWER
> + &bin_attr_drawer_siblings,
> + &bin_attr_drawer_siblings_list,
> +#endif
> + NULL
> +};
> +
> static struct attribute *default_attrs[] = {
> &dev_attr_physical_package_id.attr,
> &dev_attr_die_id.attr,
> &dev_attr_core_id.attr,
> - &dev_attr_thread_siblings.attr,
> - &dev_attr_thread_siblings_list.attr,
> - &dev_attr_core_cpus.attr,
> - &dev_attr_core_cpus_list.attr,
> - &dev_attr_core_siblings.attr,
> - &dev_attr_core_siblings_list.attr,
> - &dev_attr_die_cpus.attr,
> - &dev_attr_die_cpus_list.attr,
> - &dev_attr_package_cpus.attr,
> - &dev_attr_package_cpus_list.attr,
> #ifdef CONFIG_SCHED_BOOK
> &dev_attr_book_id.attr,
> - &dev_attr_book_siblings.attr,
> - &dev_attr_book_siblings_list.attr,
> #endif
> #ifdef CONFIG_SCHED_DRAWER
> &dev_attr_drawer_id.attr,
> - &dev_attr_drawer_siblings.attr,
> - &dev_attr_drawer_siblings_list.attr,
> #endif
> NULL
> };
>
> static const struct attribute_group topology_attr_group = {
> .attrs = default_attrs,
> + .bin_attrs = bin_attrs,
> .name = "topology"
> };
>
> --
> 2.25.1
Powered by blists - more mailing lists