| Message ID | 48b7dfafccc7a0ed814b5dfb0f109a0473a1b4b4.1710517542.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable build of full Xen for RISC-V | expand |
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@gmail.com> 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. > --- /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). > --- /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. > + */ > + > +#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
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@gmail.com> > > 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
diff --git a/xen/include/asm-generic/bitops/bitops-bits.h b/xen/include/asm-generic/bitops/bitops-bits.h new file mode 100644 index 0000000000..4ece2affd6 --- /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 + +#endif /* _ASM_GENERIC_BITOPS_BITS_H_ */ diff --git a/xen/include/asm-generic/bitops/generic-non-atomic.h b/xen/include/asm-generic/bitops/generic-non-atomic.h new file mode 100644 index 0000000000..02d5721bfe --- /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 + * 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. + */ + +#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); + bitops_uint_t old = *p; + + *p = old | mask; + return (old & mask) != 0; +} + +/** + * generic___test_and_clear_bit - Clear a bit and return its old value + * @nr: Bit to clear + * @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_clear_bit(bitops_uint_t nr, volatile void *addr) +{ + bitops_uint_t mask = BITOP_MASK(nr); + bitops_uint_t *p = ((bitops_uint_t *)addr) + BITOP_WORD(nr); + bitops_uint_t old = *p; + + *p = old & ~mask; + return (old & mask) != 0; +} + +/* WARNING: non atomic and it can be reordered! */ +static always_inline bool +generic___test_and_change_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); + bitops_uint_t old = *p; + + *p = old ^ mask; + return (old & mask) != 0; +} + +#endif /* __ASM_GENERIC_BITOPS_GENERIC_NON_ATOMIC_H */ diff --git a/xen/include/asm-generic/bitops/test-bit.h b/xen/include/asm-generic/bitops/test-bit.h new file mode 100644 index 0000000000..6fb414d808 --- /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))); +} + +#endif /* _ASM_GENERIC_BITOPS_TESTBIT_H_ */
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@gmail.com> --- Changes in V6: - Nothing changed ( only rebase ) --- Changes in V5: - new patch --- xen/include/asm-generic/bitops/bitops-bits.h | 21 +++++ .../asm-generic/bitops/generic-non-atomic.h | 89 +++++++++++++++++++ xen/include/asm-generic/bitops/test-bit.h | 18 ++++ 3 files changed, 128 insertions(+) create mode 100644 xen/include/asm-generic/bitops/bitops-bits.h create mode 100644 xen/include/asm-generic/bitops/generic-non-atomic.h create mode 100644 xen/include/asm-generic/bitops/test-bit.h