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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.02.2024 12:06, Oleksii wrote:
> On Mon, 2024-02-12 at 16:58 +0100, Jan Beulich wrote:
>> On 05.02.2024 16:32, Oleksii Kurochko wrote:
>>> +({                                                      \
>>> +    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.

My view: If you retain Linux style, retaining such names is also (kind
of) okay. If you convert to Xen style, then name changes are to occur
as part of that conversion.

>>> +    __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?

It's not "often used". Identifiers starting with two underscores or an
underscore and a capital letter are reserved for the implementation
(i.e. for the compiler's internal use). When not overly generic we
stand a fair chance of getting away. But NOP and NOT are pretty generic.

>>> +#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

Not sure. Depends in part on whether then you'd effectively introduce
dead code. If the introduction was such that RISC-V used the new ones
right away, then yes, that would quite likely be better.

Jan