Re: [PATCH v7 04/19] xen: introduce generic non-atomic test_*bit()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.04.2024 12:19, Oleksii Kurochko wrote:
> The patch introduces the following generic functions:
> * test_bit
> * generic__test_and_set_bit
> * generic__test_and_clear_bit
> * generic__test_and_change_bit
> 
> Also, the patch introduces the following generics which are
> used by the functions mentioned above:
> * BITOP_BITS_PER_WORD
> * BITOP_MASK
> * BITOP_WORD
> * BITOP_TYPE
> 
> These functions and macros can be useful for architectures
> that don't have corresponding arch-specific instructions.
> 
> Because of that x86 has the following check in the macros test_bit(),
> __test_and_set_bit(), __test_and_clear_bit(), __test_and_change_bit():
>     if ( bitop_bad_size(addr) ) __bitop_bad_size();
> It was necessary to move the check to generic code and define as 0
> for other architectures as they do not require this check.

Hmm, yes, the checks need to be in the outermost wrapper macros. While
you're abstracting other stuff to arch_*(), wouldn't it make sense to
also abstract this to e.g. arch_check_bitop_size(), with the expansion
simply being (effectively) empty in the generic fallback case?

> --- a/xen/include/xen/bitops.h
> +++ b/xen/include/xen/bitops.h
> @@ -65,10 +65,164 @@ static inline int generic_flsl(unsigned long x)
>   * scope
>   */
>  
> +#define BITOP_BITS_PER_WORD 32
> +/* typedef uint32_t bitop_uint_t; */
> +#define bitop_uint_t uint32_t

So no arch overrides permitted anymore at all?

> +#define BITOP_MASK(nr)  ((bitop_uint_t)1 << ((nr) % BITOP_BITS_PER_WORD))
> +
> +#define BITOP_WORD(nr)  ((nr) / BITOP_BITS_PER_WORD)
> +
>  /* --------------------- Please tidy above here --------------------- */
>  
>  #include <asm/bitops.h>
>  
> +#ifndef bitop_bad_size
> +extern void __bitop_bad_size(void);

If not switching to arch_check_bitop_size() or alike as suggested above,
why exactly does this need duplicating here and in x86? Can't the decl
simply move ahead of the #include right above? (Sure, this will then
require that nothing needing any of the functions you move here would
still include asm/bitops.h; it would need to be xen/bitops.h everywhere.)

> +#define bitop_bad_size(addr) 0
> +#endif
> +
> +/**
> + * generic__test_and_set_bit - Set a bit and return its old value
> + * @nr: Bit to set
> + * @addr: Address to count from
> + *
> + * This operation is non-atomic and can be reordered.
> + * If two examples of this operation race, one can appear to succeed
> + * but actually fail.  You must protect multiple accesses with a lock.
> + */
> +static always_inline __pure bool
> +generic__test_and_set_bit(unsigned long nr, volatile void *addr)

Does __pure actually fit with the use of volatile? The former says multiple
accesses may be folded; the latter says they must not be.

> +{
> +    bitop_uint_t mask = BITOP_MASK(nr);
> +    volatile bitop_uint_t *p = ((volatile bitop_uint_t *)addr) + 
> BITOP_WORD(nr);

Nit: Slightly shorter line possible:

    volatile bitop_uint_t *p = (volatile bitop_uint_t *)addr + BITOP_WORD(nr);

> +    bitop_uint_t old = *p;
> +
> +    *p = old | mask;
> +    return (old & mask);
> +}
> +
> +/**
> + * generic__test_and_clear_bit - Clear a bit and return its old value
> + * @nr: Bit to clear
> + * @addr: Address to count from
> + *
> + * This operation is non-atomic and can be reordered.
> + * If two examples of this operation race, one can appear to succeed
> + * but actually fail.  You must protect multiple accesses with a lock.
> + */
> +static always_inline __pure bool
> +generic__test_and_clear_bit(bitop_uint_t nr, volatile void *addr)
> +{
> +    bitop_uint_t mask = BITOP_MASK(nr);
> +    volatile bitop_uint_t *p = ((volatile bitop_uint_t *)addr) + 
> BITOP_WORD(nr);
> +    bitop_uint_t old = *p;
> +
> +    *p = old & ~mask;
> +    return (old & mask);
> +}
> +
> +/* WARNING: non atomic and it can be reordered! */
> +static always_inline __pure bool
> +generic__test_and_change_bit(unsigned long nr, volatile void *addr)
> +{
> +    bitop_uint_t mask = BITOP_MASK(nr);
> +    volatile bitop_uint_t *p = ((volatile bitop_uint_t *)addr) + 
> BITOP_WORD(nr);
> +    bitop_uint_t old = *p;
> +
> +    *p = old ^ mask;
> +    return (old & mask);
> +}
> +/**
> + * generic_test_bit - Determine whether a bit is set
> + * @nr: bit number to test
> + * @addr: Address to start counting from
> + */
> +static always_inline __pure int generic_test_bit(int nr, const volatile void 
> *addr)

Further up you use bool; why int here?

> +{
> +    const volatile bitop_uint_t *p = addr;
> +    return 1 & (p[BITOP_WORD(nr)] >> (nr & (BITOP_BITS_PER_WORD - 1)));

And reason not to use BITOP_MASK() here as well (once having switched to
bool return type)?

> +}
> +
> +static always_inline __pure bool
> +__test_and_set_bit(unsigned long nr, volatile void *addr)
> +{
> +#ifndef arch__test_and_set_bit
> +#define arch__test_and_set_bit generic__test_and_set_bit
> +#endif
> +
> +    return arch__test_and_set_bit(nr, addr);
> +}
> +#define __test_and_set_bit(nr, addr) ({             \
> +    if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
> +    __test_and_set_bit(nr, addr);                   \
> +})
> +
> +static always_inline __pure bool
> +__test_and_clear_bit(bitop_uint_t nr, volatile void *addr)
> +{
> +#ifndef arch__test_and_clear_bit
> +#define arch__test_and_clear_bit generic__test_and_clear_bit
> +#endif
> +
> +    return arch__test_and_clear_bit(nr, addr);
> +}
> +#define __test_and_clear_bit(nr, addr) ({           \
> +    if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
> +    __test_and_clear_bit(nr, addr);                 \
> +})
> +
> +static always_inline __pure bool
> +__test_and_change_bit(unsigned long nr, volatile void *addr)
> +{
> +#ifndef arch__test_and_change_bit
> +#define arch__test_and_change_bit generic__test_and_change_bit
> +#endif
> +
> +    return arch__test_and_change_bit(nr, addr);
> +}
> +#define __test_and_change_bit(nr, addr) ({              \
> +    if ( bitop_bad_size(addr) ) __bitop_bad_size();     \
> +    __test_and_change_bit(nr, addr);                    \
> +})
> +
> +static always_inline __pure int test_bit(int nr, const volatile void *addr)

Further up you use bool; why int here?

> +{
> +#ifndef arch_test_bit
> +#define arch_test_bit generic_test_bit
> +#endif
> +
> +    return arch_test_bit(nr, addr);
> +}
> +#define test_bit(nr, addr) ({                           \
> +    if ( bitop_bad_size(addr) ) __bitop_bad_size();     \
> +    test_bit(nr, addr);                                 \
> +})

>From here onwards, ...

> +static always_inline __pure int fls(unsigned int x)
> +{
> +    if (__builtin_constant_p(x))
> +        return generic_fls(x);
> +
> +#ifndef arch_fls
> +#define arch_fls generic_fls
> +#endif
> +
> +    return arch_fls(x);
> +}
> +
> +static always_inline __pure int flsl(unsigned long x)
> +{
> +    if (__builtin_constant_p(x))
> +        return generic_flsl(x);
> +
> +#ifndef arch_flsl
> +#define arch_flsl generic_flsl
> +#endif
> +
> +    return arch_flsl(x);
> +}

... does all of this really belong here? Neither title nor description have
any hint towards this.

>  /*
>   * Find First Set bit.  Bits are labelled from 1.
>   */

This context suggests there's a dependency on an uncommitted patch. Nothing
above says so. I guess you have a remark in the cover letter, yet imo that's
only partly helpful.

Jan