_reg_op(REG_OP_ALLOC) duplicates bitmap_set(). Fix it.
Signed-off-by: Yury Norov <[email protected]>
---
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);
+
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);
--
2.39.2
On 11/08/2023 02.57, Yury Norov wrote:
> _reg_op(REG_OP_ALLOC) duplicates bitmap_set(). Fix it.
>
> Signed-off-by: Yury Norov <[email protected]>
> ---
> 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
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;
> }
--
With Best Regards,
Andy Shevchenko
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 <[email protected]>
> > ---
> > 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
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
>
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 <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
> >>> ---
> >>> 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