Re: [PATCH v6 07/20] xen/riscv: introduce bitops.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.03.2024 19:06, Oleksii Kurochko wrote:
> Taken from Linux-6.4.0-rc1
> 
> Xen's bitops.h consists of several Linux's headers:
> * linux/arch/include/asm/bitops.h:
>   * The following function were removed as they aren't used in Xen:
>         * test_and_set_bit_lock
>         * clear_bit_unlock
>         * __clear_bit_unlock
>   * The following functions were renamed in the way how they are
>     used by common code:
>         * __test_and_set_bit
>         * __test_and_clear_bit
>   * The declaration and implementation of the following functios
>     were updated to make Xen build happy:
>         * clear_bit
>         * set_bit
>         * __test_and_clear_bit
>         * __test_and_set_bit
>   * linux/include/asm-generic/bitops/generic-non-atomic.h with the
>     following changes:
>      * Only functions that can be reused in Xen were left;
>        others were removed.
>      * 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.

This last middle level bullet point looks stale here here, with that header
now being introduced in a separate patch.

> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/bitops.h
> @@ -0,0 +1,144 @@
> +/* 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>
> +
> +#define BITOP_BITS_PER_WORD BITS_PER_LONG
> +
> +#define BITOP_TYPE
> +typedef uint64_t bitops_uint_t;
> +
> +#include <asm-generic/bitops/bitops-bits.h>
> +
> +#define __set_bit(n, p)      set_bit(n, p)
> +#define __clear_bit(n, p)    clear_bit(n, p)

If these cam with a TODO, I wouldn't say anything. But without I take it
they're meant to remain that way, at which point I'd like to ask about
the performance aspect: Surely the AMO insns are more expensive than
whatever more basic insns could be used instead? I'd even go as far as
wondering whether

#define __set_bit(n, p)      ((void)__test_and_set_bit(n, p))
#define __clear_bit(n, p)    ((void)__test_and_clear_bit(n, p))

wouldn't be cheaper (the compiler would recognize the unused result
and eliminate its calculation, I'm pretty sure).

> +/* Based on linux/arch/include/asm/bitops.h */
> +
> +#if BITS_PER_LONG == 64
> +#define __AMO(op)   "amo" #op ".d"
> +#elif BITS_PER_LONG == 32
> +#define __AMO(op)   "amo" #op ".w"

This isn't in line with bitops_uint_t above. With BITOP_BITS_PER_WORD 
also expanding to BITS_PER_LONG, I think you mean to use unsigned long
there. Or else you could move stuff into the conditional here (or
really move the conditional here further up).

However, if you look at Arm64 and x86 code, you will notice that they
avoid 64-bit operations here. I'm afraid I can't easily tell whether
the reason(s) for this have meanwhile disappeared; I fear some of that
is still around.

> +#else
> +#error "Unexpected BITS_PER_LONG"
> +#endif
> +
> +#define test_and_op_bit_ord(op, mod, nr, addr, ord)     \
> +({                                                      \
> +    unsigned long res, mask;                            \
> +    mask = BITOP_MASK(nr);                              \
> +    __asm__ __volatile__ (                              \
> +        __AMO(op) #ord " %0, %2, %1"                    \
> +        : "=r" (res), "+A" (addr[BITOP_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[BITOP_WORD(nr)])           \
> +        : "r" (mod(BITOP_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, )

Why are double underscores left on the non-test_and_ macros? Even
without name space considerations that looks odd here, when talk
is of the atomic forms of the operations, which by convention have
no (double) underscore at their front.

> --- a/xen/arch/riscv/include/asm/config.h
> +++ b/xen/arch/riscv/include/asm/config.h
> @@ -113,6 +113,8 @@
>  # error "Unsupported RISCV variant"
>  #endif
>  
> +#define BITS_PER_BYTE 8
> +
>  #define BYTES_PER_LONG (1 << LONG_BYTEORDER)
>  #define BITS_PER_LONG  (BYTES_PER_LONG << 3)
>  #define POINTER_ALIGN  BYTES_PER_LONG

How's this addition related to the change at hand?

Jan