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

["Oleksii K." <oleksii.kurochko@xxxxxxxxx>]

On Tue, 2024-05-28 at 08:20 +0200, Jan Beulich wrote:
> 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.
So where then it should be introduced? x86 introduces that in config.h,
Arm in asm/bitops.h.
I am okay to revert this change and make a separate patch.

> 
> >  - 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.
I think we can go only with:
 " This operation is non-atomic and can be reordered."
and drop:
 " If two examples of this operation race, one can appear to ... "

> 
> 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)?
At least, I understood that in this way.

> 
> > +/**
> > + * 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.
Then it is true for every function in this header. Based on the naming
the conclusion can be done if it is atomic/npn-atomic and can/can't be
reordered. So the comments aren't seem very useful.

~ Oleksii

> 
> > +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