RE: [PATCH v7 2/4] topology: use bin_attribute to break the size limitation of cpumap ABI

From: Song Bao Hua (Barry Song)
Date: Fri Jul 16 2021 - 04:50:07 EST

Next message: Lino Sanfilippo: "Aw: Re: [PATCH 0/2] Fixes for KSZ DSA switch"
Previous message: Martin Kepplinger: "Re: [PATCH v6 2/3] media: imx: add a driver for i.MX8MQ mipi csi rx phy and controller"
Next in thread: Yury Norov: "Re: [PATCH v7 2/4] topology: use bin_attribute to break the size limitation of cpumap ABI"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxxxxxxxxxx; akpm@xxxxxxxxxxxxxxxxxxxx;
> andriy.shevchenko@xxxxxxxxxxxxxxx; yury.norov@xxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Cc: dave.hansen@xxxxxxxxx; linux@xxxxxxxxxxxxxxxxxx; rafael@xxxxxxxxxx;
> rdunlap@xxxxxxxxxxxxx; agordeev@xxxxxxxxxxxxx; sbrivio@xxxxxxxxxx;
> jianpeng.ma@xxxxxxxxx; valentin.schneider@xxxxxxx; peterz@xxxxxxxxxxxxx;
> bristot@xxxxxxxxxx; guodong.xu@xxxxxxxxxx; tangchengchang
> <tangchengchang@xxxxxxxxxx>; Zengtao (B) <prime.zeng@xxxxxxxxxxxxx>;
> yangyicong <yangyicong@xxxxxxxxxx>; tim.c.chen@xxxxxxxxxxxxxxx; Linuxarm
> <linuxarm@xxxxxxxxxx>; tiantao (H) <tiantao6@xxxxxxxxxxxxx>; Song Bao Hua
> (Barry Song) <song.bao.hua@xxxxxxxxxxxxx>
> Subject: [PATCH v7 2/4] topology: use bin_attribute to break the size limitation
> of cpumap ABI
>
> From: Tian Tao <tiantao6@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
> Signed-off-by: Barry Song <song.bao.hua@xxxxxxxxxxxxx>
> ---
> 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

Next message: Lino Sanfilippo: "Aw: Re: [PATCH 0/2] Fixes for KSZ DSA switch"
Previous message: Martin Kepplinger: "Re: [PATCH v6 2/3] media: imx: add a driver for i.MX8MQ mipi csi rx phy and controller"
Next in thread: Yury Norov: "Re: [PATCH v7 2/4] topology: use bin_attribute to break the size limitation of cpumap ABI"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]