linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 1/1] bitmap.h: add const modifier to bitmap_next_[set/clear]_region() argument
@ 2021-07-27  9:44 Andrea Merello
  2021-07-27 11:48 ` Andy Shevchenko
  0 siblings, 1 reply; 6+ messages in thread
From: Andrea Merello @ 2021-07-27  9:44 UTC (permalink / raw)
  To: Andy Shevchenko, Bartosz Golaszewski, linux-kernel
  Cc: Yury Norov, Rasmus Villemoes, Andrea Merello, Andrea Merello

Those two functions don't modify the bitmap, so their bitmap argument
should be const. This patch add this.

Signed-off-by: Andrea Merello <andrea.merello@iit.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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1 1/1] bitmap.h: add const modifier to bitmap_next_[set/clear]_region() argument
  2021-07-27  9:44 [PATCH v1 1/1] bitmap.h: add const modifier to bitmap_next_[set/clear]_region() argument Andrea Merello
@ 2021-07-27 11:48 ` Andy Shevchenko
  2021-07-27 15:26   ` Yury Norov
  0 siblings, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2021-07-27 11:48 UTC (permalink / raw)
  To: Andrea Merello
  Cc: Bartosz Golaszewski, linux-kernel, Yury Norov, Rasmus Villemoes,
	Andrea Merello

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.

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

Yuri, do you have a public repo / branch that people can base their changes to
bitmap stuff against of?

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Andrea Merello <andrea.merello@iit.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



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1 1/1] bitmap.h: add const modifier to bitmap_next_[set/clear]_region() argument
  2021-07-27 11:48 ` Andy Shevchenko
@ 2021-07-27 15:26   ` Yury Norov
  2021-07-28  6:39     ` Andrea Merello
  0 siblings, 1 reply; 6+ messages in thread
From: Yury Norov @ 2021-07-27 15:26 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Andrea Merello, Bartosz Golaszewski, linux-kernel,
	Rasmus Villemoes, Andrea Merello

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@gmail.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@linux.intel.com>
> 
> > Signed-off-by: Andrea Merello <andrea.merello@iit.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
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1 1/1] bitmap.h: add const modifier to bitmap_next_[set/clear]_region() argument
  2021-07-27 15:26   ` Yury Norov
@ 2021-07-28  6:39     ` Andrea Merello
  2021-07-28  9:18       ` Andy Shevchenko
  2021-07-28 13:46       ` Yury Norov
  0 siblings, 2 replies; 6+ messages in thread
From: Andrea Merello @ 2021-07-28  6:39 UTC (permalink / raw)
  To: Yury Norov
  Cc: Andy Shevchenko, Bartosz Golaszewski, linux-kernel,
	Rasmus Villemoes, Andrea Merello

Il giorno mar 27 lug 2021 alle ore 17:26 Yury Norov
<yury.norov@gmail.com> ha scritto:
>
> 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@gmail.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.

Ah, that's interesting to know: I'm working on a new driver that used
find_next_zero_bit() and find_next_bit() [0]; Andy told me that my
algorithm could be rewritten exploiting some bitmap_* helpers, so I
rewrote my code using bitmap_for_each_set_region(), which in turn uses
bitmap_next_set_region().

Should I revert to my previous implementation that uses
find_next_zero_bit() and find_next_bit() ?

[0] https://lore.kernel.org/linux-iio/20210715141742.15072-3-andrea.merello@gmail.com/

> 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

Looks like also bitmap_for_each_set_region() went away indeed ?

> 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@linux.intel.com>
> >
> > > Signed-off-by: Andrea Merello <andrea.merello@iit.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
> >

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1 1/1] bitmap.h: add const modifier to bitmap_next_[set/clear]_region() argument
  2021-07-28  6:39     ` Andrea Merello
@ 2021-07-28  9:18       ` Andy Shevchenko
  2021-07-28 13:46       ` Yury Norov
  1 sibling, 0 replies; 6+ messages in thread
From: Andy Shevchenko @ 2021-07-28  9:18 UTC (permalink / raw)
  To: Andrea Merello
  Cc: Yury Norov, Andy Shevchenko, Bartosz Golaszewski, linux-kernel,
	Rasmus Villemoes, Andrea Merello

On Wed, Jul 28, 2021 at 9:39 AM Andrea Merello <andrea.merello@gmail.com> wrote:
> Il giorno mar 27 lug 2021 alle ore 17:26 Yury Norov
> <yury.norov@gmail.com> ha scritto:
> > On Tue, Jul 27, 2021 at 02:48:42PM +0300, Andy Shevchenko wrote:

...

> > 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.
>
> Ah, that's interesting to know: I'm working on a new driver that used
> find_next_zero_bit() and find_next_bit() [0]; Andy told me that my
> algorithm could be rewritten exploiting some bitmap_* helpers, so I
> rewrote my code using bitmap_for_each_set_region(), which in turn uses
> bitmap_next_set_region().
>
> Should I revert to my previous implementation that uses
> find_next_zero_bit() and find_next_bit() ?

According to the

https://github.com/norov/linux/commit/a2d6e02d19450a49a55f08c151d1f704723bec1a

they are transformed to for_each_bitrange(), etc.

...

> Looks like also bitmap_for_each_set_region() went away indeed ?

See above.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1 1/1] bitmap.h: add const modifier to bitmap_next_[set/clear]_region() argument
  2021-07-28  6:39     ` Andrea Merello
  2021-07-28  9:18       ` Andy Shevchenko
@ 2021-07-28 13:46       ` Yury Norov
  1 sibling, 0 replies; 6+ messages in thread
From: Yury Norov @ 2021-07-28 13:46 UTC (permalink / raw)
  To: Andrea Merello
  Cc: Andy Shevchenko, Bartosz Golaszewski, linux-kernel,
	Rasmus Villemoes, Andrea Merello

On Wed, Jul 28, 2021 at 08:39:15AM +0200, Andrea Merello wrote:
> Il giorno mar 27 lug 2021 alle ore 17:26 Yury Norov
> <yury.norov@gmail.com> ha scritto:
> >
> > 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@gmail.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.
> 
> Ah, that's interesting to know: I'm working on a new driver that used
> find_next_zero_bit() and find_next_bit() [0]; Andy told me that my
> algorithm could be rewritten exploiting some bitmap_* helpers, so I
> rewrote my code using bitmap_for_each_set_region(), which in turn uses
> bitmap_next_set_region().
> 
> Should I revert to my previous implementation that uses
> find_next_zero_bit() and find_next_bit() ?
> 
> [0] https://lore.kernel.org/linux-iio/20210715141742.15072-3-andrea.merello@gmail.com/

No, you're doing right. I just reimplemented and renamed
bitmap_for_each_set_region to unify with other similar functions. The
transition is very stragthforward, so you have nothing to worry on.

https://patchwork.kernel.org/project/linux-mmc/patch/20210719021755.883182-3-yury.norov@gmail.com/

> 
> > 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
> 
> Looks like also bitmap_for_each_set_region() went away indeed ?

Just replaced with for_each_{set,clear}_bitrange.

Thanks,
Yury

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-07-28 13:47 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-27  9:44 [PATCH v1 1/1] bitmap.h: add const modifier to bitmap_next_[set/clear]_region() argument Andrea Merello
2021-07-27 11:48 ` Andy Shevchenko
2021-07-27 15:26   ` Yury Norov
2021-07-28  6:39     ` Andrea Merello
2021-07-28  9:18       ` Andy Shevchenko
2021-07-28 13:46       ` Yury Norov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).