[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4 09/30] xen/riscv: introduce bitops.h
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |