Re: [PATCH v11 2/9] xen: introduce generic non-atomic test_*bit()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.05.2024 13:08, Oleksii Kurochko wrote:
> The following generic functions were introduced:
> * test_bit
> * generic__test_and_set_bit
> * generic__test_and_clear_bit
> * generic__test_and_change_bit
> 
> These functions and macros can be useful for architectures
> that don't have corresponding arch-specific instructions.
> 
> 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
> 
> BITS_PER_BYTE was moved to xen/bitops.h as BITS_PER_BYTE is the same
> across architectures.
> 
> The following approach was chosen for generic*() and arch*() bit
> operation functions:
> If the bit operation function that is going to be generic starts
> with the prefix "__", then the corresponding generic/arch function
> will also contain the "__" prefix. For example:
>  * test_bit() will be defined using arch_test_bit() and
>    generic_test_bit().
>  * __test_and_set_bit() will be defined using
>    arch__test_and_set_bit() and generic__test_and_set_bit().
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
>  Reviewed-by: Jan Beulich jbeulich@xxxxxxxx? Jan gave his R-by for the 
> previous
>  version of the patch, but some changes were done, so I wasn't sure if I could
>  use the R-by in this patch version.

That really depends on the nature of the changes. Seeing the pretty
long list below, I think it was appropriate to drop the R-b.

> ---
> Changes in V11:
>  - fix identation in generic_test_bit() function.
>  - move definition of BITS_PER_BYTE from <arch>/bitops.h to xen/bitops.h

I realize you were asked to do this, but I'm not overly happy with it.
BITS_PER_BYTE is far more similar to BITS_PER_LONG than to
BITOP_BITS_PER_WORD. Plus in any event that change is entirely unrelated
here.

>  - drop the changes in arm64/livepatch.c.
>  - update the the comments on top of functions: generic__test_and_set_bit(),
>    generic__test_and_clear_bit(),  generic__test_and_change_bit(),
>    generic_test_bit().
>  - Update footer after Signed-off section.
>  - Rebase the patch on top of staging branch, so it can be merged when 
> necessary
>    approves will be given.
>  - Update the commit message.
> ---
>  xen/arch/arm/include/asm/bitops.h |  69 -----------
>  xen/arch/ppc/include/asm/bitops.h |  54 ---------
>  xen/arch/ppc/include/asm/page.h   |   2 +-
>  xen/arch/ppc/mm-radix.c           |   2 +-
>  xen/arch/x86/include/asm/bitops.h |  31 ++---
>  xen/include/xen/bitops.h          | 185 ++++++++++++++++++++++++++++++
>  6 files changed, 196 insertions(+), 147 deletions(-)
> 
> --- a/xen/include/xen/bitops.h
> +++ b/xen/include/xen/bitops.h
> @@ -103,8 +103,193 @@ static inline int generic_flsl(unsigned long x)
>   * Include this here because some architectures need generic_ffs/fls in
>   * scope
>   */
> +
> +#define BITOP_BITS_PER_WORD 32
> +typedef uint32_t bitop_uint_t;
> +
> +#define BITOP_MASK(nr)  ((bitop_uint_t)1 << ((nr) % BITOP_BITS_PER_WORD))
> +
> +#define BITOP_WORD(nr)  ((nr) / BITOP_BITS_PER_WORD)
> +
> +#define BITS_PER_BYTE 8

This, if to be moved at all, very clearly doesn't belong here. As much
as it's unrelated to this change, it's also unrelated to bitops as a
whole.

> +extern void __bitop_bad_size(void);
> +
> +#define bitop_bad_size(addr) (sizeof(*(addr)) < sizeof(bitop_uint_t))
> +
> +/* --------------------- Please tidy above here --------------------- */
> +
>  #include <asm/bitops.h>
>  
> +/**
> + * 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

Why "examples"? Do you mean "instances"? It's further unclear what
"succeed" and "fail" here mean. The function after all has two
purposes: Checking and setting the specified bit. Therefore I think
you mean "succeed in updating the bit in memory", yet then it's
still unclear what the "appear" here means: The caller has no
indication of success/failure. Therefore I think "appear to" also
wants dropping.

> + * but actually fail.  You must protect multiple accesses with a lock.

That's not "must". An often better alternative is to use the atomic
forms instead. "Multiple" is imprecise, too: "Potentially racy" or
some such would come closer.

Also did Julien(?) really ask that these comments be reproduced on
both the functions supposed to be used throughout the codebase _and_
the generic_*() ones (being merely internal helpers/fallbacks)?

> +/**
> + * generic_test_bit - Determine whether a bit is set
> + * @nr: bit number to test
> + * @addr: Address to start counting 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.
> + */

You got carried away updating comments - there's no raciness for
simple test_bit(). As is also expressed by its name not having those
double underscores that the others have.

> +static always_inline bool generic_test_bit(int nr, const volatile void *addr)
> +{
> +    bitop_uint_t mask = BITOP_MASK(nr);
> +    const volatile bitop_uint_t *p =
> +        (const volatile bitop_uint_t *)addr + BITOP_WORD(nr);
> +
> +    return (*p & mask);
> +}
> +
> +/**
> + * __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 bool
> +__test_and_set_bit(int 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);
> +}

See Julien's comments on the earlier version as well as what Andrew has
now done for ffs()/fls(), avoiding the largely pointless fallback
#define.

Jan