Re: [Xen-devel] [PATCH v4 2/3] bitmap: import bitmap_{set/clear} from Linux 5.5

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Feb 05, 2020 at 09:46:25AM +0100, Jan Beulich wrote:
> On 04.02.2020 18:34, Roger Pau Monne wrote:
> > Import the functions and it's dependencies. Based on Linux 5.5, commit
> > id d5226fa6dbae0569ee43ecfc08bdcd6770fc4755.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> Thanks for going this route; two remarks / requests:
> 
> > --- a/xen/common/bitmap.c
> > +++ b/xen/common/bitmap.c
> > @@ -212,6 +212,47 @@ int __bitmap_weight(const unsigned long *bitmap, int 
> > bits)
> >  #endif
> >  EXPORT_SYMBOL(__bitmap_weight);
> >  
> > +void __bitmap_set(unsigned long *map, unsigned int start, int len)
> > +{
> > +   unsigned long *p = map + BIT_WORD(start);
> > +   const unsigned int size = start + len;
> > +   int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG);
> > +   unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start);
> > +
> > +   while (len - bits_to_set >= 0) {
> > +           *p |= mask_to_set;
> > +           len -= bits_to_set;
> > +           bits_to_set = BITS_PER_LONG;
> > +           mask_to_set = ~0UL;
> > +           p++;
> > +   }
> > +   if (len) {
> > +           mask_to_set &= BITMAP_LAST_WORD_MASK(size);
> > +           *p |= mask_to_set;
> > +   }
> > +}
> > +EXPORT_SYMBOL(__bitmap_set);
> > +
> > +void __bitmap_clear(unsigned long *map, unsigned int start, int len)
> > +{
> > +   unsigned long *p = map + BIT_WORD(start);
> > +   const unsigned int size = start + len;
> > +   int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG);
> > +   unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start);
> > +
> > +   while (len - bits_to_clear >= 0) {
> > +           *p &= ~mask_to_clear;
> > +           len -= bits_to_clear;
> > +           bits_to_clear = BITS_PER_LONG;
> > +           mask_to_clear = ~0UL;
> > +           p++;
> > +   }
> > +   if (len) {
> > +           mask_to_clear &= BITMAP_LAST_WORD_MASK(size);
> > +           *p &= ~mask_to_clear;
> > +   }
> > +}
> > +EXPORT_SYMBOL(__bitmap_clear);
> 
> Despite all the other EXPORT_SYMBOL() in this file, personally I
> would suggest to refrain from adding more. But I'm not going to
> insist (until such time that they all get cleaned up).
> 
> > --- a/xen/include/asm-x86/bitops.h
> > +++ b/xen/include/asm-x86/bitops.h
> > @@ -480,4 +480,6 @@ static inline int fls(unsigned int x)
> >  #define hweight16(x) generic_hweight16(x)
> >  #define hweight8(x) generic_hweight8(x)
> >  
> > +#define BIT_WORD(nr) ((nr) / BITS_PER_LONG)
> 
> At first I thought - why for x86 only? Then I noticed Arm has an
> almost identical #define already. Which in turn made me look at
> Linux, where that #define lives in a common header. I think you
> want to move the Arm one. Or wait, no - Arm's isn't even
> compatible with the implementations of the functions you add.
> This definitely needs taking care of, perhaps by way of ignoring
> my request to go this route (as getting too involved).

Urg, yes, I didn't realize that BIT_WORD on ARM is only meant to be
used when the bitmap is mapped to an array of 32bit type elements.

I could introduce BIT_LONG that would have the same definition on Arm
and x86, and then modify the imported functions to use it, but IMO the
right solution would be to change the Arm BIT_WORD macro to also use
BITS_PER_LONG (and adjust the callers).

This seems quite far off, so if you don't mind I would rather have the
original v3 2/2 using set_bit:

https://lists.xenproject.org/archives/html/xen-devel/2020-02/msg00190.html

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel