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

[Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>]

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