Re: [PATCH v9 02/15] xen: introduce generic non-atomic test_*bit()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.05.2024 19:03, Oleksii K. wrote:
> On Wed, 2024-05-15 at 17:41 +0200, Jan Beulich wrote:
>> On 15.05.2024 17:29, Oleksii K. wrote:
>>> On Wed, 2024-05-15 at 10:52 +0200, Jan Beulich wrote:
>>>> On 06.05.2024 12:15, Oleksii Kurochko wrote:
>>>>> The following generic functions were introduced:
>>>>> * 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.
>>>>
>>>> Logically this paragraph may better move ahead of the BITOP_*
>>>> one.
>>>>
>>>>> Because of that x86 has the following check in the macros
>>>>> test_bit(),
>>>>> __test_and_set_bit(), __test_and_clear_bit(),
>>>>> __test_and_change_bit():
>>>>>     if ( bitop_bad_size(addr) ) __bitop_bad_size();
>>>>> It was necessary to make bitop bad size check generic too, so
>>>>> arch_check_bitop_size() was introduced.
>>>>
>>>> Not anymore, with the most recent adjustments? There's nothing
>>>> arch-
>>>> specific anymore in the checking.
>>>>
>>>>> @@ -183,7 +180,7 @@ static inline int test_and_set_bit(int nr,
>>>>> volatile void *addr)
>>>>>   * If two examples of this operation race, one can appear to
>>>>> succeed
>>>>>   * but actually fail.  You must protect multiple accesses with
>>>>> a
>>>>> lock.
>>>>>   */
>>>>> -static inline int __test_and_set_bit(int nr, void *addr)
>>>>> +static inline int arch__test_and_set_bit(int nr, volatile void
>>>>> *addr)
>>>>
>>>> I think I raised this point before: Why arch__ here, ...
>>>>
>>>>> @@ -232,7 +226,7 @@ static inline int test_and_clear_bit(int
>>>>> nr,
>>>>> volatile void *addr)
>>>>>   * If two examples of this operation race, one can appear to
>>>>> succeed
>>>>>   * but actually fail.  You must protect multiple accesses with
>>>>> a
>>>>> lock.
>>>>>   */
>>>>> -static inline int __test_and_clear_bit(int nr, void *addr)
>>>>> +static inline int arch__test_and_clear_bit(int nr, volatile
>>>>> void
>>>>> *addr)
>>>>
>>>> ... here, ...
>>>>
>>>>> @@ -243,13 +237,10 @@ static inline int
>>>>> __test_and_clear_bit(int
>>>>> nr, void *addr)
>>>>>  
>>>>>      return oldbit;
>>>>>  }
>>>>> -#define __test_and_clear_bit(nr, addr) ({               \
>>>>> -    if ( bitop_bad_size(addr) ) __bitop_bad_size();     \
>>>>> -    __test_and_clear_bit(nr, addr);                     \
>>>>> -})
>>>>> +#define arch__test_and_clear_bit arch__test_and_clear_bit
>>>>>  
>>>>>  /* WARNING: non atomic and it can be reordered! */
>>>>> -static inline int __test_and_change_bit(int nr, void *addr)
>>>>> +static inline int arch__test_and_change_bit(int nr, volatile
>>>>> void
>>>>> *addr)
>>>>
>>>> ... and here, while ...
>>>>
>>>>> @@ -307,8 +295,7 @@ static inline int variable_test_bit(int nr,
>>>>> const volatile void *addr)
>>>>>      return oldbit;
>>>>>  }
>>>>>  
>>>>> -#define test_bit(nr, addr) ({                           \
>>>>> -    if ( bitop_bad_size(addr) ) __bitop_bad_size();     \
>>>>> +#define arch_test_bit(nr, addr) ({                      \
>>>>
>>>> ... just arch_ here? I don't like the double underscore infixes
>>>> very
>>>> much, but I'm okay with them as long as they're applied
>>>> consistently.
>>>
>>> Common code and x86 use __test_and_clear_bit(), and this patch
>>> provides
>>> a generic version of __test_and_clear_bit(). To emphasize that
>>> generic__test_and_clear_bit() is a common implementation of
>>> __test_and_clear_bit(), the double underscore was retained. Also,
>>> test_and_clear_bit() exists and if one day it will be needed to
>>> provide
>>> a generic version of it, then it will be needed to have
>>> generic__test_and_clear_bit() and generic_test_and_clear_bit()
>>>
>>> A similar logic was chosen for test_bit.
>>
>> Right, but in all of your reply arch_ doesn't appear at all.
> I am a little confused here. According to my logic, should it be
> arch___test_and_set_bit() and generic___test_and_set_bit()?

Why 3 underscores in a row? I'm clearly not following.

> If you are asking why there is no generic implementation for
> test_and_clear_bit() without the double underscores, the reason is that
> Arm, PPC, and x86 don't use generic code and rely on specific
> instructions for this operation. Therefore, I didn't see much sense in
> providing a generic version of test_and_clear_bit(), at least, for now.

No, there was no question in that direction. And hence ...

>>  Yet the
>> question was: Why then not arch__test_bit(), to match the other arch
>> helpers?
> Because no one uses __test_bit(). Everywhere is used test_bit().

... this seems unrelated (constrained by my earlier lack of following you).

(Later) Wait, maybe I've finally figured it: You use arch__test_and_*()
because those underlie __test_and_*(), but arch_test_bit() because there's
solely test_bit() (same for the generic_* naming). I guess I can accept
that then, despite the slight anomaly you point out, resulting in the
question towards 3 underscores in a row. To clarify, my thinking was more
towards there not possibly being generic forms of test_and_*() (i.e. no
possible set of arch_test_and_*() or generic_test_and_*()), thus using
double inner underscores in arch__test_*() and generic__test_*() to
signify that those are purely internal functions, which aren't supposed to
be called directly.

Jan