RE: [PATCH v4 1/4] lib: bitmap: introduce bitmap_print_to_buf

["Song Bao Hua (Barry Song)" <song.bao.hua@hisilicon.com>]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Thursday, June 17, 2021 10:47 PM
> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>
> Cc: andriy.shevchenko@linux.intel.com; linux-kernel@vger.kernel.org;
> linux@rasmusvillemoes.dk; rafael@kernel.org; akpm@linux-foundation.org;
> rdunlap@infradead.org; agordeev@linux.ibm.com; sbrivio@redhat.com;
> jianpeng.ma@intel.com; yury.norov@gmail.com; valentin.schneider@arm.com;
> peterz@infradead.org; bristot@redhat.com; guodong.xu@linaro.org;
> tangchengchang <tangchengchang@huawei.com>; Zengtao (B)
> <prime.zeng@hisilicon.com>; yangyicong <yangyicong@huawei.com>;
> tim.c.chen@linux.intel.com; tiantao (H) <tiantao6@hisilicon.com>; Jonathan
> Cameron <jonathan.cameron@huawei.com>; Linuxarm <linuxarm@huawei.com>
> Subject: Re: [PATCH v4 1/4] lib: bitmap: introduce bitmap_print_to_buf
> 
> On Thu, Jun 17, 2021 at 10:19:07PM +1200, Barry Song wrote:
> > From: Tian Tao <tiantao6@hisilicon.com>
> >
> > New API bitmap_print_to_buf() with bin_attribute to avoid maskp
> > exceeding PAGE_SIZE.
> 
> I can not parse this sentance at all, sorry.
> 
> What does a "bin_attribute" mean to this change?  What does PAGE_SIZE
> have to do with anything at all here?
> 
> > bitmap_print_to_pagebuf() is a special case
> > of bitmap_print_to_buf(), so in bitmap_print_to_pagebuf() call
> > bitmap_print_to_buf().
> 
> Again, I can not parse this, sorry.  Can you please try writing this in
> a different way to be more descriptive?
> 
> >
> > Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> > Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Randy Dunlap <rdunlap@infradead.org>
> > Cc: Stefano Brivio <sbrivio@redhat.com>
> > Cc: Alexander Gordeev <agordeev@linux.ibm.com>
> > Cc: "Ma, Jianpeng" <jianpeng.ma@intel.com>
> > Cc: Yury Norov <yury.norov@gmail.com>
> > Cc: Valentin Schneider <valentin.schneider@arm.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Daniel Bristot de Oliveira <bristot@redhat.com>
> > ---
> >  include/linux/bitmap.h  |  2 ++
> >  include/linux/cpumask.h | 21 +++++++++++++++++++++
> >  lib/bitmap.c            | 37 +++++++++++++++++++++++++++++++++++--
> >  3 files changed, 58 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> > index a36cfcec4e77..0de6effa2797 100644
> > --- a/include/linux/bitmap.h
> > +++ b/include/linux/bitmap.h
> > @@ -226,6 +226,8 @@ void bitmap_copy_le(unsigned long *dst, const unsigned
> long *src, unsigned int n
> >  unsigned int bitmap_ord_to_pos(const unsigned long *bitmap, unsigned int ord,
> unsigned int nbits);
> >  int bitmap_print_to_pagebuf(bool list, char *buf,
> >  				   const unsigned long *maskp, int nmaskbits);
> > +int bitmap_print_to_buf(bool list, char *buf, const unsigned long *maskp,
> > +			int nmaskbits, loff_t off, size_t count);
> >
> >  #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 bfc4690de4f4..1bef69e4b950 100644
> > --- a/include/linux/cpumask.h
> > +++ b/include/linux/cpumask.h
> > @@ -983,6 +983,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
> 
> How do you choose between a comma or a hex value?
> 
> > + * @list: indicates whether the cpumap must be list
> 
> Should this be the thing that determines a comma or not?  If true does
> that mean a comma?  Or if false?  Can you please document this?
> 
> 
> > + * @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 maximum number of bytes to print
> > + *
> > + * The role of cpumap_print_to_buf() and cpumap_print_to_pagebuf() is
> > + * 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 74ceb02f45e3..ed7ef6e2edca 100644
> > --- a/lib/bitmap.c
> > +++ b/lib/bitmap.c
> > @@ -461,6 +461,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: the kernel space buffer to read to
> > + * @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 print
> > + *
> > + * The role of bitmap_print_to_buf() and bitmap_print_to_pagebuf() is
> > + * the same, the difference is that buf of bitmap_print_to_buf()
> > + * can be more than one pagesize.
> 
> "pagesize" does not make sense here.
> 
> Why does PAGE_SIZE have anything to do with this function at all?
> 
> > + */
> > +int bitmap_print_to_buf(bool list, char *buf, const unsigned long *maskp,
> > +			int nmaskbits, loff_t off, size_t count)
> > +{
> > +	const char *fmt = list ? "%*pbl\n" : "%*pb\n";
> > +	ssize_t size;
> > +	void *data;
> > +
> > +	if (off == LLONG_MAX && count == PAGE_SIZE - offset_in_page(buf))
> > +		return scnprintf(buf, count, fmt, nmaskbits, maskp);
> 
> I do not understand this check, can you please comment why you are doing
> it this way?

Discussed with Tiantao and figured out the reason. In the previous
version, this trick didn't exist.

In order that bitmap_print_to_pagebuf() can directly call
bitmap_print_to_buf() with the awareness of 
" ptrdiff_t len = PAGE_SIZE - offset_in_page(buf);" of the
original code:

int bitmap_print_to_pagebuf(bool list, char *buf, const unsigned long *maskp,
			    int nmaskbits)
{
	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);
}
EXPORT_SYMBOL(bitmap_print_to_pagebuf);

This version is playing the trick by using LLONG_MAX as a "magic"
offset so that bitmap_print_to_buf() can do the particular job for
bitmap_print_to_pagebuf(). bitmap_print_to_pagebuf is working
under the assumption the buffer is just ONE page so it always
uses the unfinished part of the page as the length to print.

So the trick in bitmap_print_to_pagebuf() is like:

@@ -482,8 +516,7 @@ 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);

This is weird and this is ugly. So my understanding is that
is no necessity to touch bitmap_print_to_pagebuf? We can keep
bitmap_print_to_pagebuf as is. If users are quite sure their
ABIs won't be more than 1 page, they can still use this version
which doesn't have the complexity to calculate the exact
size of the final bitmap.


> 
> 
> thanks,
> 
> greg k-h

Thanks
Barry