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

["Oleksii K." <oleksii.kurochko@xxxxxxxxx>]

On Thu, 2024-05-16 at 09:04 +0200, Jan Beulich wrote:
> 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).
Yes, that what I meant.

>  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.
I understand your point regarding functions that start with "__".
For example, __test_and_clear_bit() is used not only internally (in
terms of architecture code) but also in common code, so it is not
strictly internal. I may have misunderstood what "internal function"
means in this context.

I thought that, at least for bit operations, "__bit_operation" means
that the bit operation is non-atomic and can be reordered, which
implies that it's not a good idea to use it in common code without
additional steps.

Anyway, I am not sure I understand which approach I should use in this
patch. You mentioned that possibly test_and_() can't have a generic
form, meaning it won't be a set of arch_test_and_() functions.

So, can I rename arch__test_() and generic__test_() to arch_test_() and
generic_test_(), respectively, and use the renamed functions in
_test_and*() in xen/bitops.h? Is my understanding correct?
If my understanding is correct, I am happy to apply mentioned changes
in the next patch version.

~ Oleksii


> 
> Jan