| Message ID | 1c73cf8eaff02ea19439ec676c063e592d273cfe.1638392965.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen-blkfront: Use the bitmap API when applicable | expand |
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
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 >
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
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);
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(-)