Re: [PATCH] x86/bitmap: Even more signed-ness fixes

[Julien Grall <julien@xxxxxxx>]

Hi Andrew,

title: s/x86// as this is common code.

On 05/02/2024 15:14, Andrew Cooper wrote:
> Further to commit 558e84b7ffec ("xen/bitmap: Consistently use unsigned bits
> values"), it turns out that GCC's range analysis isn't good enough to notice
> that 'bits / BITS_PER_LONG' is always within the unsigned range of k.
I would suggest to add the compiler version. This would be helpful if we 
need to revisit this decision in the future. Althougth, I don't expect 
to be the case here (switching the bitmap functions to unsigned is a 
good move and IIRC was discussed during the MISRA meeting).
> As a consequence, it re-sign-extends 'k' when calculating the pointer holding
> 'bitmap[k]' on each loop iteration.  Switch all internal variables to unsigned
> int, which allows for better-still code generation.
>
> Also clean up the prototypes for __bitmap_{set,clear} and the *_region()
> helpers, along with the internal variables.
>
> Bloat-o-meter for a random x86 build reports:
>    add/remove: 0/0 grow/shrink: 0/10 up/down: 0/-277 (-277)
>
> No functional change.
I am not sure about this...

> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
[...]

> -int bitmap_find_free_region(unsigned long *bitmap, int bits, int order)
> +int bitmap_find_free_region(unsigned long *bitmap, unsigned int bits, unsigned 
> int order)
>   {
>         unsigned long mask;
> -       int pages = 1 << order;
> -       int i;
> +       unsigned int pages = 1 << order;
> +       unsigned int i;
... I think your other patch is fixing a latent bug you introduced here. 
Before hand, if bits was "negative", we would return -ENOMEM. Now if we 
pass 2GB or higher, we would go through the loop.
So I would fold the hunk from common/bitmap.c here.

>   
>   	if(pages > BITS_PER_LONG)
>                 return -EINVAL;
[...]

> -int bitmap_allocate_region(unsigned long *bitmap, int pos, int order)
> +int bitmap_allocate_region(unsigned long *bitmap, unsigned int pos,
> +                          unsigned int order)
>   {
> -       int pages = 1 << order;
> +       unsigned int pages = 1 << order;
>         unsigned long mask = (1ul << (pages - 1));
> -       int index = pos/BITS_PER_LONG;
> -       int offset = pos - (index * BITS_PER_LONG);
> +	unsigned int index = pos/BITS_PER_LONG; 
NIT: While you modify the line, can you add a space before after / as 
you did above?
Cheers,

--
Julien Grall