Re: [PATCH] xen/bitmap: Consistently use unsigned bits values

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/02/2024 10:45 am, Jan Beulich wrote:
> On 01.02.2024 11:33, Andrew Cooper wrote:
>> Right now, most of the static inline helpers take an unsigned nbits quantity,
>> and most of the library functions take a signed quanity.  Because
>> BITMAP_LAST_WORD_MASK() is expressed as a divide, the compiler is forced to
>> emit two different paths to get the correct semantics for signed division.
>>
>> Swap all signed bit-counts to being unsigned bit-counts for the simple cases.
>> This includes the return value of bitmap_weight().
>>
>> Bloat-o-meter for a random x86 build reports:
>>   add/remove: 0/0 grow/shrink: 8/19 up/down: 167/-413 (-246)
>>
>> which all comes from compiler not emitting "dead" logic paths for negative 
>> bit
>> counts.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

> albeit with a question at the bottom.
>
>> There is much more wanting cleaning up here, but we have to start somewhere.
>> Some observations:
>>
>>  * Various of the boolean-like return values have -1 for zero-length bitmaps.
>>    I can't spot any callers which care, so this seems like a waste.
>>  * bitmap_zero() and similar clear predate us switching to use
>>    __builtin_memset(), because there's no need for bitmap_switch().
>>  * Should we consolidate 'bits' vs 'nbits'?
> This looks desirable to me.
>
>>  * The internals of these helpers want converting too.  Other helpers need
>>    more than just a parameter conversion.
> This may or may not relate to my question; it's not exactly clear what you
> mean here.
>
>> --- a/xen/include/xen/bitmap.h
>> +++ b/xen/include/xen/bitmap.h
>> @@ -66,25 +66,25 @@
>>   * lib/bitmap.c provides these functions:
>>   */
>>  
>> -extern int __bitmap_empty(const unsigned long *bitmap, int bits);
>> -extern int __bitmap_full(const unsigned long *bitmap, int bits);
>> -extern int __bitmap_equal(const unsigned long *bitmap1,
>> -                    const unsigned long *bitmap2, int bits);
>> -extern void __bitmap_complement(unsigned long *dst, const unsigned long 
>> *src,
>> -                    int bits);
>> -extern void __bitmap_and(unsigned long *dst, const unsigned long *bitmap1,
>> -                    const unsigned long *bitmap2, int bits);
>> -extern void __bitmap_or(unsigned long *dst, const unsigned long *bitmap1,
>> -                    const unsigned long *bitmap2, int bits);
>> -extern void __bitmap_xor(unsigned long *dst, const unsigned long *bitmap1,
>> -                    const unsigned long *bitmap2, int bits);
>> -extern void __bitmap_andnot(unsigned long *dst, const unsigned long 
>> *bitmap1,
>> -                    const unsigned long *bitmap2, int bits);
>> -extern int __bitmap_intersects(const unsigned long *bitmap1,
>> -                    const unsigned long *bitmap2, int bits);
>> -extern int __bitmap_subset(const unsigned long *bitmap1,
>> -                    const unsigned long *bitmap2, int bits);
>> -extern int __bitmap_weight(const unsigned long *bitmap, int bits);
>> +int __bitmap_empty(const unsigned long *bitmap, unsigned int bits);
>> +int __bitmap_full(const unsigned long *bitmap, unsigned int bits);
>> +int __bitmap_equal(const unsigned long *bitmap1,
>> +                   const unsigned long *bitmap2, unsigned int bits);
>> +void __bitmap_complement(unsigned long *dst, const unsigned long *src,
>> +                         unsigned int bits);
>> +void __bitmap_and(unsigned long *dst, const unsigned long *bitmap1,
>> +                  const unsigned long *bitmap2, unsigned int bits);
>> +void __bitmap_or(unsigned long *dst, const unsigned long *bitmap1,
>> +                 const unsigned long *bitmap2, unsigned int bits);
>> +void __bitmap_xor(unsigned long *dst, const unsigned long *bitmap1,
>> +                  const unsigned long *bitmap2, unsigned int bits);
>> +void __bitmap_andnot(unsigned long *dst, const unsigned long *bitmap1,
>> +                     const unsigned long *bitmap2, unsigned int bits);
>> +int __bitmap_intersects(const unsigned long *bitmap1,
>> +                        const unsigned long *bitmap2, unsigned int bits);
>> +int __bitmap_subset(const unsigned long *bitmap1,
>> +                    const unsigned long *bitmap2, unsigned int bits);
>> +unsigned int __bitmap_weight(const unsigned long *bitmap, unsigned int 
>> bits);
>>  extern void __bitmap_set(unsigned long *map, unsigned int start, int len);
>>  extern void __bitmap_clear(unsigned long *map, unsigned int start, int len);
> What about these two, and the subsequent (in the .c file at least)
> bitmap_*_region()? That last remark above may mean you deliberately
> left them out for now (which is okay - as you say, this is merely a
> 1st step).

They want map->bitmap and int len -> unsigned int bits for consistency,
which is more than just a typechange.

I also wasn't totally certain of the correctness of the internal logic. 
I don't have time to investigate further in the immediate future, so
left it unchanged out of caution.

~Andrew