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

[Jan Beulich <jbeulich@xxxxxxxx>]

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).

> @@ -227,6 +229,42 @@ static inline int bitmap_weight(const unsigned long 
> *src, int nbits)
>       return __bitmap_weight(src, nbits);
>  }
>  
> +#ifdef __LITTLE_ENDIAN
> +#define BITMAP_MEM_ALIGNMENT 8
> +#else
> +#define BITMAP_MEM_ALIGNMENT (8 * sizeof(unsigned long))
> +#endif

For __LITTLE_ENDIAN to be consistently defined (or not), don't
you need to include <asm/byteorder.h> here?

Jan

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