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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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
>
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;
?
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
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
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");
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...
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");
>
>
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
>
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
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