Re: [PATCH v6 04/20] xen/asm-generic: introduce generic non-atomic test_*bit()

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Wed, 2024-03-20 at 16:30 +0100, Jan Beulich wrote:
> On 15.03.2024 19:06, 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.
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> 
> First of all: How much syncing has there been versus Andrew's plans?
> I don't want to end up in the middle of two entirely different
> approaches to the same generalization goal.
We don't have synced about bit operations mentioned in this patch, so I
decided to follow an approach I have for them. But if Andrew's approach
is better, or it would be better to follow one approach instead of
introducing two approaches, I will be happy to rework this patch.

Andrew, what do you think about this bit ops?

> 
> > --- /dev/null
> > +++ b/xen/include/asm-generic/bitops/bitops-bits.h
> > @@ -0,0 +1,21 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _ASM_GENERIC_BITOPS_BITS_H_
> > +#define _ASM_GENERIC_BITOPS_BITS_H_
> > +
> > +#ifndef BITOP_BITS_PER_WORD
> > +#define BITOP_BITS_PER_WORD     32
> > +#endif
> > +
> > +#ifndef BITOP_MASK
> > +#define BITOP_MASK(nr)          (1U << ((nr) %
> > BITOP_BITS_PER_WORD))
> > +#endif
> > +
> > +#ifndef BITOP_WORD
> > +#define BITOP_WORD(nr)          ((nr) / BITOP_BITS_PER_WORD)
> > +#endif
> > +
> > +#ifndef BITOP_TYPE
> > +typedef uint32_t bitops_uint_t;
> > +#endif
> 
> Is it of practical use to permit overriding of one of BITOP_TYPE and
> BITOP_BITS_PER_WORD? I think both want tying together.
> 
> Is it further of any use to allow overriding of BITOP_{MASK,WORD}?
> There's minimal generalization needed to allow an override of
> BITOP_TYPE, though:
> 
> #define BITOP_MASK(nr)          ((bitop_uint_t)1 << ((nr) %
> BITOP_BITS_PER_WORD))
> 
> Note that I've omitted the 's' from the typedef name - the macros
> all having no 'S', imo the type also shouldn't (or the other way
> around).
If to generalize BITOP_MASK in way you suggest, then there is no need
to allow overriding of BITOP_{MASK,WORD}. I'll update this part.

> 
> > --- /dev/null
> > +++ b/xen/include/asm-generic/bitops/generic-non-atomic.h
> > @@ -0,0 +1,89 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * The file is based on Linux ( 6.4.0 ) header:
> > + *   include/asm-generic/bitops/generic-non-atomic.h
> > + * 
> > + * Only functions that can be reused in Xen were left; others were
> > removed.
> > + * 
> > + * Also, the following changes were done:
> > + *  - it was updated the message inside #ifndef ... #endif.
> > + *  - __always_inline -> always_inline to be align with definition
> > in
> > + *    xen/compiler.h.
> > + *  - update function prototypes from
> > + *    generic___test_and_*(unsigned long nr nr, volatile unsigned
> > long *addr) to
> > + *    generic___test_and_*(unsigned long nr, volatile void *addr)
> > to be
> 
> What's the point of having a whopping 3 successive inner underscores?
> Which btw ...
> 
> > + *    consistent with other related macros/defines.
> > + *  - convert identations from tabs to spaces.
> > + *  - inside generic__test_and_* use 'bitops_uint_t' instead of
> > 'unsigned long'
> > + *    to be generic.
> 
> ... shrink to just 2 here.
> 
> Oh, and there's no generic__test_bit(), but just test_bit() in a
> separate
> header.
I just wanted to show that it is a generic version of __test_and_*
operations. If there is no sense in that, I am okay to update function
names.

~ Oleksii
> 
> > + */
> > +
> > +#ifndef __ASM_GENERIC_BITOPS_GENERIC_NON_ATOMIC_H
> > +#define __ASM_GENERIC_BITOPS_GENERIC_NON_ATOMIC_H
> > +
> > +#include <xen/compiler.h>
> > +
> > +#include <asm-generic/bitops/bitops-bits.h>
> > +
> > +#ifndef XEN_BITOPS_H
> > +#error only <xen/bitops.h> can be included directly
> > +#endif
> > +
> > +/*
> > + * Generic definitions for bit operations, should not be used in
> > regular code
> > + * directly.
> > + */
> > +
> > +/**
> > + * 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 bool
> > +generic___test_and_set_bit(unsigned long nr, volatile void *addr)
> > +{
> > +    bitops_uint_t mask = BITOP_MASK(nr);
> > +    bitops_uint_t *p = ((bitops_uint_t *)addr) + BITOP_WORD(nr);
> 
> If you cast away the volatile here, what's the point of having it in
> the parameter declaration? Also such a cast doesn't need an outer
> pair of parentheses.
> 
> > +    bitops_uint_t old = *p;
> > +
> > +    *p = old | mask;
> > +    return (old & mask) != 0;
> 
> Minor: The function returning bool, the "!= 0" (and then also the
> parentheses) could be omitted.
> 
> > --- /dev/null
> > +++ b/xen/include/asm-generic/bitops/test-bit.h
> > @@ -0,0 +1,18 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _ASM_GENERIC_BITOPS_TESTBIT_H_
> > +#define _ASM_GENERIC_BITOPS_TESTBIT_H_
> > +
> > +#include <asm-generic/bitops/bitops-bits.h>
> > +
> > +/**
> > + * test_bit - Determine whether a bit is set
> > + * @nr: bit number to test
> > + * @addr: Address to start counting from
> > + */
> > +static inline int test_bit(int nr, const volatile void *addr)
> > +{
> > +    const volatile bitops_uint_t *p = addr;
> > +    return 1 & (p[BITOP_WORD(nr)] >> (nr & (BITOP_BITS_PER_WORD -
> > 1)));
> > +}
> 
> Interestingly you don't lose the volatile here. Overall the way this
> is written would likely benefit the other functions too. There's no
> cast needed here and thus also not there.
> 
> Jan