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.kuroc...@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
