[Linuxarm] Re: [PATCH v2 1/3] lib: bitmap: introduce bitmap_print_to_buf

On Wed, 2 Jun 2021 19:02:48 +0800 Tian Tao tiantao6@hisilicon.com wrote:

new API bitmap_print_to_buf with bin_attribute to avoid maskp

New API...

Not sure for this particular bit of the kernel, but personally I prefer function names in patch descriptions to have () after them to make it clear that is what they are. Note other people disagree :)

exceeding PAGE_SIZE. bitmap_print_to_pagebuf is a special case of bitmap_print_to_buf, so in bitmap_print_to_pagebuf call bitmap_print_to_buf.

A few comments inline. Mostly editorial, but you also forgot to update your kvfree() to kfree() after moving away from kvmalloc() in this version.

Thanks,

Jonathan

Signed-off-by: Tian Tao tiantao6@hisilicon.com

include/linux/bitmap.h | 3 +++ include/linux/cpumask.h | 21 +++++++++++++++++++++ lib/bitmap.c | 38 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h index 70a9324..bc401bd9b 100644 --- a/include/linux/bitmap.h +++ b/include/linux/bitmap.h @@ -219,6 +219,9 @@ extern unsigned int bitmap_ord_to_pos(const unsigned long *bitmap, unsigned int extern int bitmap_print_to_pagebuf(bool list, char *buf, const unsigned long *maskp, int nmaskbits);

+extern int bitmap_print_to_buf(bool list, char *buf,

• 	       const unsigned long *maskp, int nmaskbits, loff_t off, size_t count);
  

Nice to keep this under 80 chars. We can go past that if it hurts readability to keep the line short, but here that isn't the case.

#define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) & (BITS_PER_LONG - 1))) #define BITMAP_LAST_WORD_MASK(nbits) (~0UL >> (-(nbits) & (BITS_PER_LONG - 1)))

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h index 383684e..114ed44 100644 --- a/include/linux/cpumask.h +++ b/include/linux/cpumask.h @@ -928,6 +928,27 @@ cpumap_print_to_pagebuf(bool list, char *buf, const struct cpumask *mask) nr_cpu_ids); }

+/**

• • cpumap_print_to_buf - copies the cpumask into the buffer either

• •  as comma-separated list of cpus or hex values of cpumask
    

• • @list: indicates whether the cpumap must be list
• • @mask: the cpumask to copy
• • @buf: the buffer to copy into
• • @off: in the string from which we are copying, We copy to @buf + off
• • @count: the length that we will copy into

I would use more similar text to below. Nothing stops the buffer being bigger than the copy we ask for after all.

"the maximum number of bytes to print"

• • the role of cpumap_print_to_buf and cpumap_print_to_pagebuf is

The...

• • the same, the difference is that buf of bitmap_print_to_buf
• • can be more than one pagesize.
• */

+static inline ssize_t +cpumap_print_to_buf(bool list, char *buf, const struct cpumask *mask,

•     loff_t off, size_t count)
  

+{

• return bitmap_print_to_buf(list, buf, cpumask_bits(mask),

• 		   nr_cpu_ids, off, count);
  

+}

#if NR_CPUS <= BITS_PER_LONG #define CPU_MASK_ALL \ (cpumask_t) { { \ diff --git a/lib/bitmap.c b/lib/bitmap.c index 75006c4..d814b32 100644 --- a/lib/bitmap.c +++ b/lib/bitmap.c @@ -460,6 +460,40 @@ int bitmap_parse_user(const char __user *ubuf, EXPORT_SYMBOL(bitmap_parse_user);

/**

• • bitmap_print_to_buf - convert bitmap to list or hex format ASCII string
• • @list: indicates whether the bitmap must be list
• • @buf: page which string is placed

No particular reason it should be a page. It's an arbitrary address I think. I would use the text from the memory_read_from_buffer docs. "the kernel space buffer to read to"

Whilst here I notice that bitmap_print_to_pagebuf() has @buf: page aligned buffer into which string is places but then uses ptrdiff_t len = PAGE_SIZE - offset_in_page(buf); which seems to suggest that comment for @buf is inaccurate as the buffer does not need to be page aligned.

• • @maskp: pointer to bitmap to convert
• • @nmaskbits: size of bitmap, in bits
• • @off: offset in data buffer below
• • @count: the maximum number of bytes to read
• • the role of bitmap_print_to_buf and bitmap_print_to_pagebuf is

The...

Also, I'm fairly sure kernel-doc referencing will only work if those function names have () after them so please add those.

• • the same, the difference is that buf of bitmap_print_to_buf
• • can be more than one pagesize.
• */

+int bitmap_print_to_buf(bool list, char *buf, const unsigned long *maskp,

• 	int nmaskbits, loff_t off, size_t count)
  

+{

• int size;
• void *data;
• const char *fmt = list ? "%*pbl\n" : "%*pb\n";
• if (off == LLONG_MAX && count == PAGE_SIZE - offset_in_page(buf))

• return scnprintf(buf, count, fmt, nmaskbits, maskp);
  

• data = kasprintf(GFP_KERNEL, fmt, nmaskbits, maskp);
• if (!data)

• return -ENOMEM;
  

• size = memory_read_from_buffer(buf, count, &off, data, strlen(data));
• kvfree(data);

kfree(data)

• return size;

+} +EXPORT_SYMBOL(bitmap_print_to_buf);

+/**

• bitmap_print_to_pagebuf - convert bitmap to list or hex format ASCII string
• @list: indicates whether the bitmap must be list
• @buf: page aligned buffer into which string is placed

@@ -480,8 +514,8 @@ int bitmap_print_to_pagebuf(bool list, char *buf, const unsigned long *maskp, { ptrdiff_t len = PAGE_SIZE - offset_in_page(buf);

• return list ? scnprintf(buf, len, "%*pbl\n", nmaskbits, maskp) :

•       scnprintf(buf, len, "%*pb\n", nmaskbits, maskp);
  

• return bitmap_print_to_buf(list, buf, maskp, nmaskbits,

• 		   LLONG_MAX, len);
  

} EXPORT_SYMBOL(bitmap_print_to_pagebuf);