Re: [PATCH 4/6] xen/bitops: Introduce for_each_set_bit()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.06.2024 21:07, Andrew Cooper wrote:
> The prior version (renamed to bitmap_for_each()) was inefficeint when used
> over a scalar, but this is the more common usage even before accounting for
> the many opencoded forms.
> 
> Introduce a new version which operates on scalars only and does so without
> spilling them to memory.  This in turn requires the addition of a type-generic
> form of ffs().
> 
> Add testing for the new construct alongside the ffs/fls testing.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with two remarks:

> The naming of ffs_g() is taken from the new compiler builtins which are using
> a g suffix to mean type-generic.

As you say, a g suffix, not a _g one; gcc14 documents __builtin_ffsg().
(Seeing it exists I wonder whether we wouldn't want to use it when
available, and only fall back to the macro for older versions.) Any
specific reason you use _g?

> --- a/xen/include/xen/bitops.h
> +++ b/xen/include/xen/bitops.h
> @@ -56,6 +56,16 @@ static always_inline __pure unsigned int ffs64(uint64_t x)
>          return !x || (uint32_t)x ? ffs(x) : ffs(x >> 32) + 32;
>  }
>  
> +/*
> + * A type-generic ffs() which picks the appropriate ffs{,l,64}() based on 
> it's
> + * argument.
> + */
> +#define ffs_g(x)                                        \
> +    sizeof(x) <= sizeof(int) ? ffs(x) :                 \
> +        sizeof(x) <= sizeof(long) ? ffsl(x) :           \
> +        sizeof(x) <= sizeof(uint64_t) ? ffs64(x) :      \
> +        ({ BUILD_ERROR("ffs_g() Bad input type"); 0; })

Related to the patch introducing it: I can see how BUILD_ERROR_ON() may
be deemed sub-optimal here. Nevertheless I think we should be able to
find some common ground there.

Jan