[PATCH] xen-blkfront: Use the bitmap API when applicable

[PATCH] xen-blkfront: Use the bitmap API when applicable

[Christophe JAILLET]

Use 'bitmap_zalloc()' to simplify code, improve the semantic and avoid some
open-coded arithmetic in allocator arguments.

Also change the corresponding 'kfree()' into 'bitmap_free()' to keep
consistency.

Use 'bitmap_copy()' to avoid an explicit 'memcpy()'

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/block/xen-blkfront.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 700c765a759a..fe4d69cf9469 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -442,16 +442,14 @@ static int xlbd_reserve_minors(unsigned int minor, unsigned int nr)
 	if (end > nr_minors) {
 		unsigned long *bitmap, *old;
 
-		bitmap = kcalloc(BITS_TO_LONGS(end), sizeof(*bitmap),
-				 GFP_KERNEL);
+		bitmap = bitmap_zalloc(end, GFP_KERNEL);
 		if (bitmap == NULL)
 			return -ENOMEM;
 
 		spin_lock(&minor_lock);
 		if (end > nr_minors) {
 			old = minors;
-			memcpy(bitmap, minors,
-			       BITS_TO_LONGS(nr_minors) * sizeof(*bitmap));
+			bitmap_copy(bitmap, minors, nr_minors);
 			minors = bitmap;
 			nr_minors = BITS_TO_LONGS(end) * BITS_PER_LONG;
 		} else
@@ -2610,7 +2608,7 @@ static void __exit xlblk_exit(void)
 
 	xenbus_unregister_driver(&blkfront_driver);
 	unregister_blkdev(XENVBD_MAJOR, DEV_NAME);
-	kfree(minors);
+	bitmap_free(minors);
 }
 module_exit(xlblk_exit);
 
-- 
2.30.2

Re: [PATCH] xen-blkfront: Use the bitmap API when applicable

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1689 bytes --]

On 01.12.21 22:10, Christophe JAILLET wrote:
> Use 'bitmap_zalloc()' to simplify code, improve the semantic and avoid some
> open-coded arithmetic in allocator arguments.
> 
> Also change the corresponding 'kfree()' into 'bitmap_free()' to keep
> consistency.
> 
> Use 'bitmap_copy()' to avoid an explicit 'memcpy()'
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>   drivers/block/xen-blkfront.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index 700c765a759a..fe4d69cf9469 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -442,16 +442,14 @@ static int xlbd_reserve_minors(unsigned int minor, unsigned int nr)
>   	if (end > nr_minors) {
>   		unsigned long *bitmap, *old;
>   
> -		bitmap = kcalloc(BITS_TO_LONGS(end), sizeof(*bitmap),
> -				 GFP_KERNEL);
> +		bitmap = bitmap_zalloc(end, GFP_KERNEL);
>   		if (bitmap == NULL)
>   			return -ENOMEM;
>   
>   		spin_lock(&minor_lock);
>   		if (end > nr_minors) {
>   			old = minors;
> -			memcpy(bitmap, minors,
> -			       BITS_TO_LONGS(nr_minors) * sizeof(*bitmap));
> +			bitmap_copy(bitmap, minors, nr_minors);
>   			minors = bitmap;
>   			nr_minors = BITS_TO_LONGS(end) * BITS_PER_LONG;
>   		} else

Shouldn't you use bitmap_free(old) some lines down?

> @@ -2610,7 +2608,7 @@ static void __exit xlblk_exit(void)
>   
>   	xenbus_unregister_driver(&blkfront_driver);
>   	unregister_blkdev(XENVBD_MAJOR, DEV_NAME);
> -	kfree(minors);
> +	bitmap_free(minors);
>   }
>   module_exit(xlblk_exit);


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] xen-blkfront: Use the bitmap API when applicable

[Christophe JAILLET]

Le 02/12/2021 à 07:12, Juergen Gross a écrit :
> On 01.12.21 22:10, Christophe JAILLET wrote:
>> Use 'bitmap_zalloc()' to simplify code, improve the semantic and avoid 
>> some
>> open-coded arithmetic in allocator arguments.
>>
>> Also change the corresponding 'kfree()' into 'bitmap_free()' to keep
>> consistency.
>>
>> Use 'bitmap_copy()' to avoid an explicit 'memcpy()'
>>
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>>   drivers/block/xen-blkfront.c | 8 +++-----
>>   1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
>> index 700c765a759a..fe4d69cf9469 100644
>> --- a/drivers/block/xen-blkfront.c
>> +++ b/drivers/block/xen-blkfront.c
>> @@ -442,16 +442,14 @@ static int xlbd_reserve_minors(unsigned int 
>> minor, unsigned int nr)
>>       if (end > nr_minors) {
>>           unsigned long *bitmap, *old;
>> -        bitmap = kcalloc(BITS_TO_LONGS(end), sizeof(*bitmap),
>> -                 GFP_KERNEL);
>> +        bitmap = bitmap_zalloc(end, GFP_KERNEL);
>>           if (bitmap == NULL)
>>               return -ENOMEM;
>>           spin_lock(&minor_lock);
>>           if (end > nr_minors) {
>>               old = minors;
>> -            memcpy(bitmap, minors,
>> -                   BITS_TO_LONGS(nr_minors) * sizeof(*bitmap));
>> +            bitmap_copy(bitmap, minors, nr_minors);
>>               minors = bitmap;
>>               nr_minors = BITS_TO_LONGS(end) * BITS_PER_LONG;
>>           } else
> 
> Shouldn't you use bitmap_free(old) some lines down?

Obvious.
I'll send a V2, Thx for the review.

CJ

> 
>> @@ -2610,7 +2608,7 @@ static void __exit xlblk_exit(void)
>>       xenbus_unregister_driver(&blkfront_driver);
>>       unregister_blkdev(XENVBD_MAJOR, DEV_NAME);
>> -    kfree(minors);
>> +    bitmap_free(minors);
>>   }
>>   module_exit(xlblk_exit);
> 
> 
> Juergen
> 

Re: [PATCH] xen-blkfront: Use the bitmap API when applicable

[Joe Perches]

On Thu, 2021-12-02 at 19:12 +0100, Christophe JAILLET wrote:
> Le 02/12/2021 à 07:12, Juergen Gross a écrit :
> > On 01.12.21 22:10, Christophe JAILLET wrote:
> > > Use 'bitmap_zalloc()' to simplify code, improve the semantic and avoid 
> > > some open-coded arithmetic in allocator arguments.
> > > 
> > > Also change the corresponding 'kfree()' into 'bitmap_free()' to keep
> > > consistency.
> > > 
> > > Use 'bitmap_copy()' to avoid an explicit 'memcpy()'
[]
> > > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
[]
> > > @@ -442,16 +442,14 @@ static int xlbd_reserve_minors(unsigned int 
> > > minor, unsigned int nr)
> > >       if (end > nr_minors) {
> > >           unsigned long *bitmap, *old;
> > > -        bitmap = kcalloc(BITS_TO_LONGS(end), sizeof(*bitmap),
> > > -                 GFP_KERNEL);
> > > +        bitmap = bitmap_zalloc(end, GFP_KERNEL);
> > >           if (bitmap == NULL)
> > >               return -ENOMEM;
> > >           spin_lock(&minor_lock);
> > >           if (end > nr_minors) {
> > >               old = minors;
> > > -            memcpy(bitmap, minors,
> > > -                   BITS_TO_LONGS(nr_minors) * sizeof(*bitmap));
> > > +            bitmap_copy(bitmap, minors, nr_minors);
> > >               minors = bitmap;
> > >               nr_minors = BITS_TO_LONGS(end) * BITS_PER_LONG;

		nr_minors = end;
?

Re: [PATCH] xen-blkfront: Use the bitmap API when applicable

[Christophe JAILLET]

Le 02/12/2021 à 19:16, Joe Perches a écrit :
> On Thu, 2021-12-02 at 19:12 +0100, Christophe JAILLET wrote:
>> Le 02/12/2021 à 07:12, Juergen Gross a écrit :
>>> On 01.12.21 22:10, Christophe JAILLET wrote:
>>>> Use 'bitmap_zalloc()' to simplify code, improve the semantic and avoid
>>>> some open-coded arithmetic in allocator arguments.
>>>>
>>>> Also change the corresponding 'kfree()' into 'bitmap_free()' to keep
>>>> consistency.
>>>>
>>>> Use 'bitmap_copy()' to avoid an explicit 'memcpy()'
> []
>>>> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> []
>>>> @@ -442,16 +442,14 @@ static int xlbd_reserve_minors(unsigned int
>>>> minor, unsigned int nr)
>>>>        if (end > nr_minors) {
>>>>            unsigned long *bitmap, *old;
>>>> -        bitmap = kcalloc(BITS_TO_LONGS(end), sizeof(*bitmap),
>>>> -                 GFP_KERNEL);
>>>> +        bitmap = bitmap_zalloc(end, GFP_KERNEL);
>>>>            if (bitmap == NULL)
>>>>                return -ENOMEM;
>>>>            spin_lock(&minor_lock);
>>>>            if (end > nr_minors) {
>>>>                old = minors;
>>>> -            memcpy(bitmap, minors,
>>>> -                   BITS_TO_LONGS(nr_minors) * sizeof(*bitmap));
>>>> +            bitmap_copy(bitmap, minors, nr_minors);
>>>>                minors = bitmap;
>>>>                nr_minors = BITS_TO_LONGS(end) * BITS_PER_LONG;
> 
> 		nr_minors = end;
> ?
> 

No,
My understanding of the code is that if we lack space (end > nr_minors), 
we need to allocate more. In such a case, we want to keep track of what 
we have allocated, not what we needed.
The "padding" bits in the "long align" allocation, can be used later.

first call
----------
end = 65
nr_minors = 63

--> we need some space
--> we allocate 2 longs = 128 bits
--> we now use 65 bits of these 128 bits

new call
--------
end = 68
nr_minors = 128 (from previous call)
--> no need to reallocate

CJ

Re: [PATCH] xen-blkfront: Use the bitmap API when applicable

[Joe Perches]

On Thu, 2021-12-02 at 20:07 +0100, Christophe JAILLET wrote:
> Le 02/12/2021 à 19:16, Joe Perches a écrit :
> > On Thu, 2021-12-02 at 19:12 +0100, Christophe JAILLET wrote:
> > > Le 02/12/2021 à 07:12, Juergen Gross a écrit :
> > > > On 01.12.21 22:10, Christophe JAILLET wrote:
> > > > > Use 'bitmap_zalloc()' to simplify code, improve the semantic and avoid
> > > > > some open-coded arithmetic in allocator arguments.
> > > > > 
> > > > > Also change the corresponding 'kfree()' into 'bitmap_free()' to keep
> > > > > consistency.
> > > > > 
> > > > > Use 'bitmap_copy()' to avoid an explicit 'memcpy()'
> > []
> > > > > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> > []
> > > > > @@ -442,16 +442,14 @@ static int xlbd_reserve_minors(unsigned int
> > > > > minor, unsigned int nr)
> > > > >        if (end > nr_minors) {
> > > > >            unsigned long *bitmap, *old;
> > > > > -        bitmap = kcalloc(BITS_TO_LONGS(end), sizeof(*bitmap),
> > > > > -                 GFP_KERNEL);
> > > > > +        bitmap = bitmap_zalloc(end, GFP_KERNEL);
> > > > >            if (bitmap == NULL)
> > > > >                return -ENOMEM;
> > > > >            spin_lock(&minor_lock);
> > > > >            if (end > nr_minors) {
> > > > >                old = minors;
> > > > > -            memcpy(bitmap, minors,
> > > > > -                   BITS_TO_LONGS(nr_minors) * sizeof(*bitmap));
> > > > > +            bitmap_copy(bitmap, minors, nr_minors);
> > > > >                minors = bitmap;
> > > > >                nr_minors = BITS_TO_LONGS(end) * BITS_PER_LONG;
> > 
> > 		nr_minors = end;
> > ?
> > 
> 
> No,
> My understanding of the code is that if we lack space (end > nr_minors), 
> we need to allocate more. In such a case, we want to keep track of what 
> we have allocated, not what we needed.
> The "padding" bits in the "long align" allocation, can be used later.

> 
> first call
> ----------
> end = 65
> nr_minors = 63
> 
> --> we need some space
> --> we allocate 2 longs = 128 bits
> --> we now use 65 bits of these 128 bits

or 96, 32 or 64 bit longs remember.

> 
> new call
> --------
> end = 68
> nr_minors = 128 (from previous call)

The initial allocation is now bitmap_zalloc which
specifies only bits and the nr_minors is then in
BITS_TO_LONGS(bits) * BITS_PER_LONG

Perhaps that assumes too much about the internal
implementation of bitmap_alloc

Re: [PATCH] xen-blkfront: Use the bitmap API when applicable

[Christophe JAILLET]

Le 03/12/2021 à 04:03, Joe Perches a écrit :
> On Thu, 2021-12-02 at 20:07 +0100, Christophe JAILLET wrote:
>> Le 02/12/2021 à 19:16, Joe Perches a écrit :
>>> On Thu, 2021-12-02 at 19:12 +0100, Christophe JAILLET wrote:
>>>> Le 02/12/2021 à 07:12, Juergen Gross a écrit :
>>>>> On 01.12.21 22:10, Christophe JAILLET wrote:
>>>>>> Use 'bitmap_zalloc()' to simplify code, improve the semantic and avoid
>>>>>> some open-coded arithmetic in allocator arguments.
>>>>>>
>>>>>> Also change the corresponding 'kfree()' into 'bitmap_free()' to keep
>>>>>> consistency.
>>>>>>
>>>>>> Use 'bitmap_copy()' to avoid an explicit 'memcpy()'
>>> []
>>>>>> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
>>> []
>>>>>> @@ -442,16 +442,14 @@ static int xlbd_reserve_minors(unsigned int
>>>>>> minor, unsigned int nr)
>>>>>>         if (end > nr_minors) {
>>>>>>             unsigned long *bitmap, *old;
>>>>>> -        bitmap = kcalloc(BITS_TO_LONGS(end), sizeof(*bitmap),
>>>>>> -                 GFP_KERNEL);
>>>>>> +        bitmap = bitmap_zalloc(end, GFP_KERNEL);
>>>>>>             if (bitmap == NULL)
>>>>>>                 return -ENOMEM;
>>>>>>             spin_lock(&minor_lock);
>>>>>>             if (end > nr_minors) {
>>>>>>                 old = minors;
>>>>>> -            memcpy(bitmap, minors,
>>>>>> -                   BITS_TO_LONGS(nr_minors) * sizeof(*bitmap));
>>>>>> +            bitmap_copy(bitmap, minors, nr_minors);
>>>>>>                 minors = bitmap;
>>>>>>                 nr_minors = BITS_TO_LONGS(end) * BITS_PER_LONG;
>>>
>>> 		nr_minors = end;
>>> ?
>>>
>>
>> No,
>> My understanding of the code is that if we lack space (end > nr_minors),
>> we need to allocate more. In such a case, we want to keep track of what
>> we have allocated, not what we needed.
>> The "padding" bits in the "long align" allocation, can be used later.
> 
>>
>> first call
>> ----------
>> end = 65
>> nr_minors = 63
>>
>> --> we need some space
>> --> we allocate 2 longs = 128 bits
>> --> we now use 65 bits of these 128 bits
> 
> or 96, 32 or 64 bit longs remember.

32 and 64 for sure, but I was not aware of 96. On which arch?

> 
>>
>> new call
>> --------
>> end = 68
>> nr_minors = 128 (from previous call)
> 
> The initial allocation is now bitmap_zalloc which
> specifies only bits and the nr_minors is then in
> BITS_TO_LONGS(bits) * BITS_PER_LONG
> 
> Perhaps that assumes too much about the internal
> implementation of bitmap_alloc
> 
> 

I get your point now, and I agree with you.

Maybe something as what is done in mc-entity.c?
Explicitly require more bits (which will be allocated anyway), instead 
of taking advantage (read "hoping") that it will be done.

Could be:

@@ -440,26 +440,25 @@ static int xlbd_reserve_minors(unsigned int minor, 
unsigned int nr)
  	int rc;

  	if (end > nr_minors) {
  		unsigned long *bitmap, *old;

-		bitmap = kcalloc(BITS_TO_LONGS(end), sizeof(*bitmap),
-				 GFP_KERNEL);
+		end = ALIGN(end, BITS_PER_LONG);
+		bitmap = bitmap_zalloc(end, GFP_KERNEL);
  		if (bitmap == NULL)
  			return -ENOMEM;

  		spin_lock(&minor_lock);
  		if (end > nr_minors) {
  			old = minors;
-			memcpy(bitmap, minors,
-			       BITS_TO_LONGS(nr_minors) * sizeof(*bitmap));
+			bitmap_copy(bitmap, minors, nr_minors);
  			minors = bitmap;
-			nr_minors = BITS_TO_LONGS(end) * BITS_PER_LONG;
+			nr_minors = end;
  		} else
  			old = bitmap;
  		spin_unlock(&minor_lock);
-		kfree(old);
+		bitmap_free(old);
  	}

  	spin_lock(&minor_lock);
  	if (find_next_bit(minors, end, minor) >= end) {
  		bitmap_set(minors, minor, nr);
@@ -2608,11 +2607,11 @@ static void __exit xlblk_exit(void)
  {
  	cancel_delayed_work_sync(&blkfront_work);

  	xenbus_unregister_driver(&blkfront_driver);
  	unregister_blkdev(XENVBD_MAJOR, DEV_NAME);
-	kfree(minors);
+	bitmap_free(minors);
  }
  module_exit(xlblk_exit);

  MODULE_DESCRIPTION("Xen virtual block device frontend");
  MODULE_LICENSE("GPL");

Re: [PATCH] xen-blkfront: Use the bitmap API when applicable

[Joe Perches]

On Fri, 2021-12-03 at 16:54 +0100, Christophe JAILLET wrote:
> Le 03/12/2021 à 04:03, Joe Perches a écrit :
> > On Thu, 2021-12-02 at 20:07 +0100, Christophe JAILLET wrote:
> > > Le 02/12/2021 à 19:16, Joe Perches a écrit :
> > > > On Thu, 2021-12-02 at 19:12 +0100, Christophe JAILLET wrote:
> > > > > Le 02/12/2021 à 07:12, Juergen Gross a écrit :
> > > > > > On 01.12.21 22:10, Christophe JAILLET wrote:
> > > > > > > Use 'bitmap_zalloc()' to simplify code, improve the semantic and avoid
> > > > > > > some open-coded arithmetic in allocator arguments.
> > > > > > > 
> > > > > > > Also change the corresponding 'kfree()' into 'bitmap_free()' to keep
> > > > > > > consistency.
> > > > > > > 
> > > > > > > Use 'bitmap_copy()' to avoid an explicit 'memcpy()'
> > > > []
> > > > > > > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> > > > []
> > > > > > > @@ -442,16 +442,14 @@ static int xlbd_reserve_minors(unsigned int
> > > > > > > minor, unsigned int nr)
> > > > > > >         if (end > nr_minors) {
> > > > > > >             unsigned long *bitmap, *old;
> > > > > > > -        bitmap = kcalloc(BITS_TO_LONGS(end), sizeof(*bitmap),
> > > > > > > -                 GFP_KERNEL);
> > > > > > > +        bitmap = bitmap_zalloc(end, GFP_KERNEL);
> > > > > > >             if (bitmap == NULL)
> > > > > > >                 return -ENOMEM;
> > > > > > >             spin_lock(&minor_lock);
> > > > > > >             if (end > nr_minors) {
> > > > > > >                 old = minors;
> > > > > > > -            memcpy(bitmap, minors,
> > > > > > > -                   BITS_TO_LONGS(nr_minors) * sizeof(*bitmap));
> > > > > > > +            bitmap_copy(bitmap, minors, nr_minors);
> > > > > > >                 minors = bitmap;
> > > > > > >                 nr_minors = BITS_TO_LONGS(end) * BITS_PER_LONG;
> > > > 
> > > > 		nr_minors = end;
> > > > ?
> > > > 
> > > 
> > > No,
> > > My understanding of the code is that if we lack space (end > nr_minors),
> > > we need to allocate more. In such a case, we want to keep track of what
> > > we have allocated, not what we needed.
> > > The "padding" bits in the "long align" allocation, can be used later.
> > 
> > > 
> > > first call
> > > ----------
> > > end = 65
> > > nr_minors = 63
> > > 
> > > --> we need some space
> > > --> we allocate 2 longs = 128 bits
> > > --> we now use 65 bits of these 128 bits
> > 
> > or 96, 32 or 64 bit longs remember.
> 
> 32 and 64 for sure, but I was not aware of 96. On which arch?

For more clarity that should have been a period instead of comma after 96.


> > The initial allocation is now bitmap_zalloc which
> > specifies only bits and the nr_minors is then in
> > BITS_TO_LONGS(bits) * BITS_PER_LONG
> > 
> > Perhaps that assumes too much about the internal
> > implementation of bitmap_alloc
> 
> I get your point now, and I agree with you.
> 
> Maybe something as what is done in mc-entity.c?
> Explicitly require more bits (which will be allocated anyway), instead 
> of taking advantage (read "hoping") that it will be done.

Sure, that's sensible.

> Could be:
> 
> @@ -440,26 +440,25 @@ static int xlbd_reserve_minors(unsigned int minor, 
> unsigned int nr)
>   	int rc;
> 
>   	if (end > nr_minors) {
>   		unsigned long *bitmap, *old;
> 
> -		bitmap = kcalloc(BITS_TO_LONGS(end), sizeof(*bitmap),
> -				 GFP_KERNEL);
> +		end = ALIGN(end, BITS_PER_LONG);

Though it may be more sensible to use some other alignment
like round_up and not use BITS_PER_LONG at all as the
number of these may not be dependent on 32/64 bit arches
at all.

Maybe something like:

#define GROW_MINORS	64

		end = round_up(nr_minors, GROW_MINORS);

etc...

Re: [PATCH] xen-blkfront: Use the bitmap API when applicable

[Boris Ostrovsky]

On 12/3/21 10:54 AM, Christophe JAILLET wrote:
> Le 03/12/2021 à 04:03, Joe Perches a écrit :
>> On Thu, 2021-12-02 at 20:07 +0100, Christophe JAILLET wrote:
>>> Le 02/12/2021 à 19:16, Joe Perches a écrit :
>>>> On Thu, 2021-12-02 at 19:12 +0100, Christophe JAILLET wrote:
>>>>> Le 02/12/2021 à 07:12, Juergen Gross a écrit :
>>>>>> On 01.12.21 22:10, Christophe JAILLET wrote:
>>>>>>> Use 'bitmap_zalloc()' to simplify code, improve the semantic and avoid
>>>>>>> some open-coded arithmetic in allocator arguments.
>>>>>>>
>>>>>>> Also change the corresponding 'kfree()' into 'bitmap_free()' to keep
>>>>>>> consistency.
>>>>>>>
>>>>>>> Use 'bitmap_copy()' to avoid an explicit 'memcpy()'
>>>> []
>>>>>>> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
>>>> []
>>>>>>> @@ -442,16 +442,14 @@ static int xlbd_reserve_minors(unsigned int
>>>>>>> minor, unsigned int nr)
>>>>>>>         if (end > nr_minors) {
>>>>>>>             unsigned long *bitmap, *old;
>>>>>>> -        bitmap = kcalloc(BITS_TO_LONGS(end), sizeof(*bitmap),
>>>>>>> -                 GFP_KERNEL);
>>>>>>> +        bitmap = bitmap_zalloc(end, GFP_KERNEL);
>>>>>>>             if (bitmap == NULL)
>>>>>>>                 return -ENOMEM;
>>>>>>>             spin_lock(&minor_lock);
>>>>>>>             if (end > nr_minors) {
>>>>>>>                 old = minors;
>>>>>>> -            memcpy(bitmap, minors,
>>>>>>> -                   BITS_TO_LONGS(nr_minors) * sizeof(*bitmap));
>>>>>>> +            bitmap_copy(bitmap, minors, nr_minors);
>>>>>>>                 minors = bitmap;
>>>>>>>                 nr_minors = BITS_TO_LONGS(end) * BITS_PER_LONG;
>>>>
>>>>         nr_minors = end;
>>>> ?
>>>>
>>>
>>> No,
>>> My understanding of the code is that if we lack space (end > nr_minors),
>>> we need to allocate more. In such a case, we want to keep track of what
>>> we have allocated, not what we needed.
>>> The "padding" bits in the "long align" allocation, can be used later.
>>
>>>
>>> first call
>>> ----------
>>> end = 65
>>> nr_minors = 63
>>>
>>> --> we need some space
>>> --> we allocate 2 longs = 128 bits
>>> --> we now use 65 bits of these 128 bits
>>
>> or 96, 32 or 64 bit longs remember.
>
> 32 and 64 for sure, but I was not aware of 96. On which arch?
>
>>
>>>
>>> new call
>>> --------
>>> end = 68
>>> nr_minors = 128 (from previous call)
>>
>> The initial allocation is now bitmap_zalloc which
>> specifies only bits and the nr_minors is then in
>> BITS_TO_LONGS(bits) * BITS_PER_LONG
>>
>> Perhaps that assumes too much about the internal
>> implementation of bitmap_alloc
>>
>>
>
> I get your point now, and I agree with you.
>
> Maybe something as what is done in mc-entity.c?
> Explicitly require more bits (which will be allocated anyway), instead of taking advantage (read "hoping") that it will be done.
>
> Could be:
>
> @@ -440,26 +440,25 @@ static int xlbd_reserve_minors(unsigned int minor, unsigned int nr)
>      int rc;
>
>      if (end > nr_minors) {
>          unsigned long *bitmap, *old;
>
> -        bitmap = kcalloc(BITS_TO_LONGS(end), sizeof(*bitmap),
> -                 GFP_KERNEL);
> +        end = ALIGN(end, BITS_PER_LONG);
> +        bitmap = bitmap_zalloc(end, GFP_KERNEL);
>          if (bitmap == NULL)
>              return -ENOMEM;
>
>          spin_lock(&minor_lock);
>          if (end > nr_minors) {
>              old = minors;
> -            memcpy(bitmap, minors,
> -                   BITS_TO_LONGS(nr_minors) * sizeof(*bitmap));
> +            bitmap_copy(bitmap, minors, nr_minors);
>              minors = bitmap;
> -            nr_minors = BITS_TO_LONGS(end) * BITS_PER_LONG;
> +            nr_minors = end;
>          } else
>              old = bitmap;
>          spin_unlock(&minor_lock);
> -        kfree(old);
> +        bitmap_free(old);
>      }
>
>      spin_lock(&minor_lock);
>      if (find_next_bit(minors, end, minor) >= end) {


I don't think this will work anymore, we may now fail if another thread gets a minor above the original (i.e. no aligned) @end.


-boris


> bitmap_set(minors, minor, nr);
> @@ -2608,11 +2607,11 @@ static void __exit xlblk_exit(void)
>  {
>      cancel_delayed_work_sync(&blkfront_work);
>
>      xenbus_unregister_driver(&blkfront_driver);
>      unregister_blkdev(XENVBD_MAJOR, DEV_NAME);
> -    kfree(minors);
> +    bitmap_free(minors);
>  }
>  module_exit(xlblk_exit);
>
>  MODULE_DESCRIPTION("Xen virtual block device frontend");
>  MODULE_LICENSE("GPL");
>
>

Re: [PATCH] xen-blkfront: Use the bitmap API when applicable

[Christophe JAILLET]

Le 03/12/2021 à 22:04, Boris Ostrovsky a écrit :
> 
> On 12/3/21 10:54 AM, Christophe JAILLET wrote:
>> Le 03/12/2021 à 04:03, Joe Perches a écrit :
>>
>> I get your point now, and I agree with you.
>>
>> Maybe something as what is done in mc-entity.c?
>> Explicitly require more bits (which will be allocated anyway), instead 
>> of taking advantage (read "hoping") that it will be done.
>>
>> Could be:
>>
>> @@ -440,26 +440,25 @@ static int xlbd_reserve_minors(unsigned int 
>> minor, unsigned int nr)
>>      int rc;
>>
>>      if (end > nr_minors) {
>>          unsigned long *bitmap, *old;
>>
>> -        bitmap = kcalloc(BITS_TO_LONGS(end), sizeof(*bitmap),
>> -                 GFP_KERNEL);
>> +        end = ALIGN(end, BITS_PER_LONG);
>> +        bitmap = bitmap_zalloc(end, GFP_KERNEL);
>>          if (bitmap == NULL)
>>              return -ENOMEM;
>>
>>          spin_lock(&minor_lock);
>>          if (end > nr_minors) {
>>              old = minors;
>> -            memcpy(bitmap, minors,
>> -                   BITS_TO_LONGS(nr_minors) * sizeof(*bitmap));
>> +            bitmap_copy(bitmap, minors, nr_minors);
>>              minors = bitmap;
>> -            nr_minors = BITS_TO_LONGS(end) * BITS_PER_LONG;
>> +            nr_minors = end;
>>          } else
>>              old = bitmap;
>>          spin_unlock(&minor_lock);
>> -        kfree(old);
>> +        bitmap_free(old);
>>      }
>>
>>      spin_lock(&minor_lock);
>>      if (find_next_bit(minors, end, minor) >= end) {
> 
> 
> I don't think this will work anymore, we may now fail if another thread 
> gets a minor above the original (i.e. no aligned) @end.
> 

So, maybe adding an "official" 'bitmap_size()' (which is already 
existing and duplicated in a few places) would ease things.

It would replace the 'nr_minors = BITS_TO_LONGS(end) * BITS_PER_LONG;' 
and hide the implementation details of the bitmap API.

Something like:
static __always_inline size_t bitmap_size(unsigned long nr_bits)
{
	return BITS_TO_LONGS(nr_bits) * sizeof(long);
}

CJ

> 
> -boris
> 

Re: [PATCH] xen-blkfront: Use the bitmap API when applicable

[Joe Perches]

On Sat, 2021-12-04 at 07:57 +0100, Christophe JAILLET wrote:
> So, maybe adding an "official" 'bitmap_size()' (which is already 
> existing and duplicated in a few places) would ease things.
> 
> It would replace the 'nr_minors = BITS_TO_LONGS(end) * BITS_PER_LONG;' 
> and hide the implementation details of the bitmap API.
> 
> Something like:
> static __always_inline size_t bitmap_size(unsigned long nr_bits)
> {
> 	return BITS_TO_LONGS(nr_bits) * sizeof(long);
> }

Or maybe a bitmap_realloc

Re: [PATCH] xen-blkfront: Use the bitmap API when applicable

[Boris Ostrovsky]

On 12/4/21 1:57 AM, Christophe JAILLET wrote:
>
> So, maybe adding an "official" 'bitmap_size()' (which is already existing and duplicated in a few places) would ease things.
>
> It would replace the 'nr_minors = BITS_TO_LONGS(end) * BITS_PER_LONG;' and hide the implementation details of the bitmap API.
>
> Something like:
> static __always_inline size_t bitmap_size(unsigned long nr_bits)
> {
>     return BITS_TO_LONGS(nr_bits) * sizeof(long);
> }
>

Yes, I think this would be a useful helper. Should be sizeof(unsigned long) though to keep things consistent.


-boris