diff mbox series

[v2,2/6] bitmap: replace _reg_op(REG_OP_ALLOC) with bitmap_set()

Message ID 20230811005732.107718-3-yury.norov@gmail.com
State New
Headers
Series bitmap: cleanup bitmap_*_region() implementation |

Commit Message

Yury Norov Aug. 11, 2023, 12:57 a.m. UTC 
  _reg_op(REG_OP_ALLOC) duplicates bitmap_set(). Fix it.

Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 lib/bitmap.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Rasmus Villemoes Aug. 11, 2023, 6:21 a.m. UTC | #1 
On 11/08/2023 02.57, Yury Norov wrote:
> _reg_op(REG_OP_ALLOC) duplicates bitmap_set(). Fix it.
> 
> Signed-off-by: Yury Norov <yury.norov@gmail.com>
> ---
>  lib/bitmap.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index 3a589016f5e0..c9afe704fe4b 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -1352,9 +1352,12 @@ EXPORT_SYMBOL(bitmap_release_region);
>   */
>  int bitmap_allocate_region(unsigned long *bitmap, unsigned int pos, int order)
>  {
> +	unsigned int nbits = pos + BIT(order);
> +

That really doesn't sound right. Have you added self-tests for these
functions first and then used those to catch regressions?

Rasmus
Andy Shevchenko Aug. 11, 2023, 9:25 a.m. UTC | #2 
On Thu, Aug 10, 2023 at 05:57:28PM -0700, Yury Norov wrote:
> _reg_op(REG_OP_ALLOC) duplicates bitmap_set(). Fix it.

...

>  int bitmap_allocate_region(unsigned long *bitmap, unsigned int pos, int order)
>  {
> +	unsigned int nbits = pos + BIT(order);

Shouldn't this be unsigned long?
As the prototype of the used later unsigned long find_next_bit().

>  	if (!__reg_op(bitmap, pos, order, REG_OP_ISFREE))
>  		return -EBUSY;
> -	return __reg_op(bitmap, pos, order, REG_OP_ALLOC);
> +	bitmap_set(bitmap, pos, nbits);
> +	return 0;
>  }
Yury Norov Aug. 11, 2023, 12:56 p.m. UTC | #3 
On Fri, Aug 11, 2023 at 08:21:33AM +0200, Rasmus Villemoes wrote:
> On 11/08/2023 02.57, Yury Norov wrote:
> > _reg_op(REG_OP_ALLOC) duplicates bitmap_set(). Fix it.
> > 
> > Signed-off-by: Yury Norov <yury.norov@gmail.com>
> > ---
> >  lib/bitmap.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/bitmap.c b/lib/bitmap.c
> > index 3a589016f5e0..c9afe704fe4b 100644
> > --- a/lib/bitmap.c
> > +++ b/lib/bitmap.c
> > @@ -1352,9 +1352,12 @@ EXPORT_SYMBOL(bitmap_release_region);
> >   */
> >  int bitmap_allocate_region(unsigned long *bitmap, unsigned int pos, int order)
> >  {
> > +	unsigned int nbits = pos + BIT(order);
> > +
> 
> That really doesn't sound right. Have you added self-tests for these
> functions first and then used those to catch regressions?

When bitmap_allocate_region() is broken, almost every arch build fails
to boot. Can you explain what exactly looks wrong to you?

Thanks,
Yury
Yury Norov Aug. 11, 2023, 1:01 p.m. UTC | #4 
On Fri, Aug 11, 2023 at 12:25:02PM +0300, Andy Shevchenko wrote:
> On Thu, Aug 10, 2023 at 05:57:28PM -0700, Yury Norov wrote:
> > _reg_op(REG_OP_ALLOC) duplicates bitmap_set(). Fix it.
> 
> ...
> 
> >  int bitmap_allocate_region(unsigned long *bitmap, unsigned int pos, int order)
> >  {
> > +	unsigned int nbits = pos + BIT(order);
> 
> Shouldn't this be unsigned long?
> As the prototype of the used later unsigned long find_next_bit().

Linux doesn't support bitmaps longer than 2^32 bits. I really doubt
such huge bitmaps would ever exist, but if that will happen, too many
bitmap functions will have to be revisited.

For the reference, check the code of __reg_op(), which I remove in the
series - all indexes and offsets are int's there.

Thanks,
Yury
 
> >  	if (!__reg_op(bitmap, pos, order, REG_OP_ISFREE))
> >  		return -EBUSY;
> > -	return __reg_op(bitmap, pos, order, REG_OP_ALLOC);
> > +	bitmap_set(bitmap, pos, nbits);
> > +	return 0;
> >  }
> 
> -- 
> With Best Regards,
> Andy Shevchenko
>
Rasmus Villemoes Aug. 11, 2023, 1:31 p.m. UTC | #5 
On 11/08/2023 14.56, Yury Norov wrote:
> On Fri, Aug 11, 2023 at 08:21:33AM +0200, Rasmus Villemoes wrote:
>> On 11/08/2023 02.57, Yury Norov wrote:
>>> _reg_op(REG_OP_ALLOC) duplicates bitmap_set(). Fix it.
>>>
>>> Signed-off-by: Yury Norov <yury.norov@gmail.com>
>>> ---
>>>  lib/bitmap.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/bitmap.c b/lib/bitmap.c
>>> index 3a589016f5e0..c9afe704fe4b 100644
>>> --- a/lib/bitmap.c
>>> +++ b/lib/bitmap.c
>>> @@ -1352,9 +1352,12 @@ EXPORT_SYMBOL(bitmap_release_region);
>>>   */
>>>  int bitmap_allocate_region(unsigned long *bitmap, unsigned int pos, int order)
>>>  {
>>> +	unsigned int nbits = pos + BIT(order);
>>> +
>>
>> That really doesn't sound right. Have you added self-tests for these
>> functions first and then used those to catch regressions?
> 
> When bitmap_allocate_region() is broken, almost every arch build fails
> to boot. Can you explain what exactly looks wrong to you?

The number of bits we are about to set should not be [position in bitmap
to start from] + [2^order]. The second half of that patch was

-	return __reg_op(bitmap, pos, order, REG_OP_ALLOC);
+	bitmap_set(bitmap, pos, nbits);
+	return 0;

so instead of setting 1<<nbits starting at pos, you're now setting
pos+(1<<nbits) starting at pos. How is that correct?

Rasmus
Yury Norov Aug. 14, 2023, 12:39 a.m. UTC | #6 
On Fri, Aug 11, 2023 at 03:31:01PM +0200, Rasmus Villemoes wrote:
> On 11/08/2023 14.56, Yury Norov wrote:
> > On Fri, Aug 11, 2023 at 08:21:33AM +0200, Rasmus Villemoes wrote:
> >> On 11/08/2023 02.57, Yury Norov wrote:
> >>> _reg_op(REG_OP_ALLOC) duplicates bitmap_set(). Fix it.
> >>>
> >>> Signed-off-by: Yury Norov <yury.norov@gmail.com>
> >>> ---
> >>>  lib/bitmap.c | 5 ++++-
> >>>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/lib/bitmap.c b/lib/bitmap.c
> >>> index 3a589016f5e0..c9afe704fe4b 100644
> >>> --- a/lib/bitmap.c
> >>> +++ b/lib/bitmap.c
> >>> @@ -1352,9 +1352,12 @@ EXPORT_SYMBOL(bitmap_release_region);
> >>>   */
> >>>  int bitmap_allocate_region(unsigned long *bitmap, unsigned int pos, int order)
> >>>  {
> >>> +	unsigned int nbits = pos + BIT(order);
> >>> +
> >>
> >> That really doesn't sound right. Have you added self-tests for these
> >> functions first and then used those to catch regressions?
> > 
> > When bitmap_allocate_region() is broken, almost every arch build fails
> > to boot. Can you explain what exactly looks wrong to you?
> 
> The number of bits we are about to set should not be [position in bitmap
> to start from] + [2^order]. The second half of that patch was
> 
> -	return __reg_op(bitmap, pos, order, REG_OP_ALLOC);
> +	bitmap_set(bitmap, pos, nbits);
> +	return 0;
> 
> so instead of setting 1<<nbits starting at pos, you're now setting
> pos+(1<<nbits) starting at pos. How is that correct?

You're right. Bad on me. :/

I'll send v3 with all the discussed cleared shortly.

Thanks,
YUry
diff mbox series

Patch

diff --git a/lib/bitmap.c b/lib/bitmap.c
index 3a589016f5e0..c9afe704fe4b 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -1352,9 +1352,12 @@  EXPORT_SYMBOL(bitmap_release_region);
  */
 int bitmap_allocate_region(unsigned long *bitmap, unsigned int pos, int order)
 {
+	unsigned int nbits = pos + BIT(order);
+
 	if (!__reg_op(bitmap, pos, order, REG_OP_ISFREE))
 		return -EBUSY;
-	return __reg_op(bitmap, pos, order, REG_OP_ALLOC);
+	bitmap_set(bitmap, pos, nbits);
+	return 0;
 }
 EXPORT_SYMBOL(bitmap_allocate_region);