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

[Julien Grall <julien@xxxxxxx>]

Hi Oleksii,

On 24/05/2024 09:58, Oleksii K. wrote:
> On Thu, 2024-05-23 at 15:33 +0100, Julien Grall wrote:
> > > > >      #include <asm/bitops.h>
> > > > >      
> > > > > +/**
> > > > > + * 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.
> > > > > + */
> > > > Sorry for only mentioning this on v10. I think this comment
> > > > should be
> > > > duplicated (or moved to) on top of test_bit() because this is
> > > > what
> > > > everyone will use. This will avoid the developper to follow the
> > > > function
> > > > calls and only notice the x86 version which says "This function
> > > > is
> > > > atomic and may not be reordered." and would be wrong for all the
> > > > other arch.
> > > It makes sense to add this comment on top of test_bit(), but I am
> > > curious if it is needed to mention that for x86 arch_test_bit() "is
> > > atomic and may not be reordered":
> > I would say no because any developper modifying common code can't
> > relying it.
> But won't then be confusion that if not generic implementation of
> test_bit() is chosen then test_bit() can be " atomic and cannot be
> reordered " ( as it is in case of x86 )?
I don't understand what confusion could arise. A developper cannot rely 
on the x86 behavior in common code. They have to write code that works 
for every arch.
The first thing a developer will do is to check test_bit(). The comment 
on top will say they can't relying on ordering. And that what most of 
the developper needs to rely on.
If someone wants to write more optimized code for x86, they are free to 
look at the implementation. But I don't think this should be detailed on 
top of the common wrapper (the x86 implementation would still be 
compatible with the comment).
Cheers,

--
Julien Grall