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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.02.2020 14:21, Roger Pau Monné wrote:
> 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).

So do I. Julien, Stefano?

> 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

As per my previous reply - yes, I'm okay with that, and yes,
expecting this I've also kept your patches this way in my
to-be-committed folder (pending Kevin's ack for patch 1).

Jan

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