linux-kernel - Re: [PATCH v1 1/1] bitmap.h: add const modifier to bitmap_next_[set/clear]

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 27 Jul 2021 08:26:16 -0700
From:   Yury Norov <yury.norov@...il.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Andrea Merello <andrea.merello@...il.com>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        linux-kernel@...r.kernel.org,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Andrea Merello <andrea.merello@....it>
Subject: Re: [PATCH v1 1/1] bitmap.h: add const modifier to
 bitmap_next_[set/clear]_region() argument

On Tue, Jul 27, 2021 at 02:48:42PM +0300, Andy Shevchenko wrote:
> On Tue, Jul 27, 2021 at 11:44:41AM +0200, Andrea Merello wrote:
> > Those two functions don't modify the bitmap, so their bitmap argument
> > should be const. This patch add this.

Thanks Andrea.

Acked-by: Yury Norov <yury.norov@...il.com>

> Constification is always a good thing. No objections from me,
> although Yuri is doing something with them in one of his patch series.

I try to remove those two because in practice they bring more mess
than good. All real use-cases for bitmap_next_{set,clear}_region 
relate to iterating the whole bitmap, ie nobody just wants to find a
next region. Untill recently there was only a single user of the API,
so I easily reworked the code to use find_first_bit/find_next bit and
by chance return faster.

https://github.com/norov/linux/commit/1c870b5c3fcd2eea9b351a1e0af8d1e93be78e1e

Recently in next-20210716, there appeared another user in fs/btrfs/extent_io.c:
find_next_dirty_byte(). The fun is that in that case the length of bitmap
is 16 bit, so it's probably simpler to return the bitmap by value,
instead of calling prologue code up to 8 times. Anyways, I'll contact
authors of the find_next_dirty_byte() and ask if it's possible to rework
their code. 
 
> Yuri, do you have a public repo / branch that people can base their changes to
> bitmap stuff against of?

I collected not yet upstreamed bitmap patches here:

https://github.com/norov/linux/commits/bitmap-20210716

Please let me know if you believe we need a more official branch for
bitmaps. I'd encourage people to review what we already have and
upstream it.

Thanks,
Yury

> Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> 
> > Signed-off-by: Andrea Merello <andrea.merello@....it>
> > ---
> >  include/linux/bitmap.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> > index a36cfcec4e77..ea4a8f1a2545 100644
> > --- a/include/linux/bitmap.h
> > +++ b/include/linux/bitmap.h
> > @@ -458,7 +458,7 @@ static inline void bitmap_replace(unsigned long *dst,
> >  		__bitmap_replace(dst, old, new, mask, nbits);
> >  }
> >  
> > -static inline void bitmap_next_clear_region(unsigned long *bitmap,
> > +static inline void bitmap_next_clear_region(const unsigned long *bitmap,
> >  					    unsigned int *rs, unsigned int *re,
> >  					    unsigned int end)
> >  {
> > @@ -466,7 +466,7 @@ static inline void bitmap_next_clear_region(unsigned long *bitmap,
> >  	*re = find_next_bit(bitmap, end, *rs + 1);
> >  }
> >  
> > -static inline void bitmap_next_set_region(unsigned long *bitmap,
> > +static inline void bitmap_next_set_region(const unsigned long *bitmap,
> >  					  unsigned int *rs, unsigned int *re,
> >  					  unsigned int end)
> >  {
> > -- 
> > 2.17.1
> > 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
>