Re: [PATCH v4 09/30] xen/riscv: introduce bitops.h

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2024-02-12 at 16:58 +0100, Jan Beulich wrote:
> On 05.02.2024 16:32, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/bitops.h
> > @@ -0,0 +1,164 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Copyright (C) 2012 Regents of the University of California */
> > +
> > +#ifndef _ASM_RISCV_BITOPS_H
> > +#define _ASM_RISCV_BITOPS_H
> > +
> > +#include <asm/system.h>
> > +
> > +#include <asm-generic/bitops/bitops-bits.h>
> 
> Especially with ...
> 
> > +/* Based on linux/arch/include/linux/bits.h */
> > +
> > +#define BIT_MASK(nr)        (1UL << ((nr) % BITS_PER_LONG))
> > +#define BIT_WORD(nr)        ((nr) / BITS_PER_LONG)
> 
> ... these it's not entirely obvious why bitops-bits.h would be needed
> here.
They are needed as __test_and_op_bit_ord(), __op_bit_ord() macros are
used them, but probably it makes sense to drop BIT_MASK() and
BIT_WORD(), and just use BITOPS_MASK() and BITOPS_WORD() from asm-
generic/bitops-bits.h or re-define BITOPS_MASK() and BITOPS_WORD()
before inclusion of bitops-bits.h in the way as BIT_MASK and BIT_WORD
macros are defined now to be aligned with Linux.

> 
> > +#define __set_bit(n,p)      set_bit(n,p)
> > +#define __clear_bit(n,p)    clear_bit(n,p)
> 
> Nit (as before?): Missing blanks after commas.
Thanks. I'll add blanks.

> 
> > +/* Based on linux/arch/include/asm/bitops.h */
> > +
> > +#if ( BITS_PER_LONG == 64 )
> 
> Imo the parentheses here make things only harder to read.
I can drop them, this part was copied from Linux, so it was decided to
leave it as is.

> 
> > +#define __AMO(op)   "amo" #op ".d"
> > +#elif ( BITS_PER_LONG == 32 )
> > +#define __AMO(op)   "amo" #op ".w"
> > +#else
> > +#error "Unexpected BITS_PER_LONG"
> > +#endif
> > +
> > +#define __test_and_op_bit_ord(op, mod, nr, addr, ord)   \
> 
> The revision log says __test_and_* were renamed. Same anomaly for
> __test_and_op_bit() then.
I'll double check the namings. Thanks.
> 
> > +({                                                      \
> > +    unsigned long __res, __mask;                        \
> 
> Leftover leading underscores?
It is how it was defined in Linux, so I thought that I've to leave it
as it, but I am OK to rename this variables in next patch version.

> 
> > +    __mask = BIT_MASK(nr);                              \
> > +    __asm__ __volatile__ (                              \
> > +        __AMO(op) #ord " %0, %2, %1"                    \
> > +        : "=r" (__res), "+A" (addr[BIT_WORD(nr)])       \
> > +        : "r" (mod(__mask))                             \
> > +        : "memory");                                    \
> > +    ((__res & __mask) != 0);                            \
> > +})
> > +
> > +#define __op_bit_ord(op, mod, nr, addr, ord)    \
> > +    __asm__ __volatile__ (                      \
> > +        __AMO(op) #ord " zero, %1, %0"          \
> > +        : "+A" (addr[BIT_WORD(nr)])             \
> > +        : "r" (mod(BIT_MASK(nr)))               \
> > +        : "memory");
> > +
> > +#define __test_and_op_bit(op, mod, nr, addr)    \
> > +    __test_and_op_bit_ord(op, mod, nr, addr, .aqrl)
> > +#define __op_bit(op, mod, nr, addr) \
> > +    __op_bit_ord(op, mod, nr, addr, )
> > +
> > +/* Bitmask modifiers */
> > +#define __NOP(x)    (x)
> > +#define __NOT(x)    (~(x))
> 
> Here the (double) leading underscores are truly worrying: Simple
> names like this aren't impossible to be assigned meaninb by a
> compiler.
I am not really understand what is the difference for a compiler
between NOP and __NOP? Do you mean that the leading double underscores
(__) are often used to indicate that these macros are implementation-
specific and might be reserved for the compiler or the standard
library?

> 
> > +/**
> > + * __test_and_set_bit - Set a bit and return its old value
> > + * @nr: Bit to set
> > + * @addr: Address to count from
> > + *
> > + * This operation may be reordered on other architectures than
> > x86.
> > + */
> > +static inline int test_and_set_bit(int nr, volatile void *p)
> > +{
> > +    volatile uint32_t *addr = p;
> 
> With BIT_WORD() / BIT_MASK() being long-based, is the use of uint32_t
> here actually correct?
No, it is not correct. It seems to me it would be better to use
BITOPS_WORD(), BITOPS_MASK() and bitops_uint_t() and just redefine them
before inclusion of bitops-bit.h to be aligned with Linux
implementation.

> 
> > +    return __test_and_op_bit(or, __NOP, nr, addr);
> > +}
> > +
> > +/**
> > + * __test_and_clear_bit - Clear a bit and return its old value
> > + * @nr: Bit to clear
> > + * @addr: Address to count from
> > + *
> > + * This operation can be reordered on other architectures other
> > than x86.
> 
> Nit: double "other" (and I think it's the 1st one that wants
> dropping,
> not - as the earlier comment suggests - the 2nd one). Question is:
> Are
> the comments correct? Both resolve to something which is (also) at
> least a compiler barrier. Same concern also applies further down, to
> at least set_bit() and clear_bit().
It looks like comments aren't correct as operation inside is atomic,
also it implies compiler memory barrier. So the comments related to
'reordering' should be dropped.
I am not sure that I know why in Linux these comments were left.

> 
> > + */
> > +static inline int test_and_clear_bit(int nr, volatile void *p)
> > +{
> > +    volatile uint32_t *addr = p;
> > +
> > +    return __test_and_op_bit(and, __NOT, nr, addr);
> > +}
> > +
> > +/**
> > + * set_bit - Atomically set a bit in memory
> > + * @nr: the bit to set
> > + * @addr: the address to start counting from
> > + *
> > + * Note: there are no guarantees that this function will not be
> > reordered
> > + * on non x86 architectures, so if you are writing portable code,
> > + * make sure not to rely on its reordering guarantees.
> > + *
> > + * Note that @nr may be almost arbitrarily large; this function is
> > not
> > + * restricted to acting on a single-word quantity.
> > + */
> > +static inline void set_bit(int nr, volatile void *p)
> > +{
> > +    volatile uint32_t *addr = p;
> > +
> > +    __op_bit(or, __NOP, nr, addr);
> > +}
> > +
> > +/**
> > + * clear_bit - Clears a bit in memory
> > + * @nr: Bit to clear
> > + * @addr: Address to start counting from
> > + *
> > + * Note: there are no guarantees that this function will not be
> > reordered
> > + * on non x86 architectures, so if you are writing portable code,
> > + * make sure not to rely on its reordering guarantees.
> > + */
> > +static inline void clear_bit(int nr, volatile void *p)
> > +{
> > +    volatile uint32_t *addr = p;
> > +
> > +    __op_bit(and, __NOT, nr, addr);
> > +}
> > +
> > +/**
> > + * test_and_change_bit - Change a bit and return its old value
> 
> How come this one's different? I notice the comments are the same
> (and
> hence as confusing) in Linux; are you sure they're applicable there?
No, I am not sure. As I mentioned above, all this functions are atomic
and uses compiler memory barrier, so it looks like the comment for 
clear_bit isn't really correct.
In case if all such functions are uses __test_and_op_bit_ord() and
__op_bit_ord() which are atomic and with compiler barrier, it seems
that we can just drop all such comments or even all the comments. I am
not sure that anyone is needed the comment as by default this function
is safe to use:
 * This operation is atomic and cannot be reordered.
 * It also implies a memory barrier.

> 
> > + * @nr: Bit to change
> > + * @addr: Address to count from
> > + *
> > + * This operation is atomic and cannot be reordered.
> > + * It also implies a memory barrier.
> > + */
> > +static inline int test_and_change_bit(int nr, volatile unsigned
> > long *addr)
> > +{
> > +   return __test_and_op_bit(xor, __NOP, nr, addr);
> > +}
> > +
> > +#undef __test_and_op_bit
> > +#undef __op_bit
> > +#undef __NOP
> > +#undef __NOT
> > +#undef __AMO
> > +
> > +#include <asm-generic/bitops/generic-non-atomic.h>
> > +
> > +#define __test_and_set_bit generic___test_and_set_bit
> > +#define __test_and_clear_bit generic___test_and_clear_bit
> > +#define __test_and_change_bit generic___test_and_change_bit
> > +
> > +#include <asm-generic/bitops/fls.h>
> > +#include <asm-generic/bitops/flsl.h>
> > +#include <asm-generic/bitops/__ffs.h>
> > +#include <asm-generic/bitops/ffs.h>
> > +#include <asm-generic/bitops/ffsl.h>
> > +#include <asm-generic/bitops/ffz.h>
> > +#include <asm-generic/bitops/find-first-set-bit.h>
> > +#include <asm-generic/bitops/hweight.h>
> > +#include <asm-generic/bitops/test-bit.h>
> 
> To be honest there's too much stuff being added here to asm-generic/,
> all in one go. I'll see about commenting on the remaining parts here,
> but I'd like to ask that you seriously consider splitting.
Would it be better to send it outside of this patch series? I can
create a separate patch series with a separate patch for each asm-
generic/bitops/*.h

~ Oleksii