Re: [PATCH v11 2/9] xen: introduce generic non-atomic test_*bit()

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

On Wed, 2024-05-29 at 18:29 +0200, Oleksii K. wrote:
> On Wed, 2024-05-29 at 17:22 +0200, Jan Beulich wrote:
> > On 29.05.2024 16:58, Oleksii K. wrote:
> > > static always_inline bool test_bit(int nr, const volatile void
> > > *addr)On
> > > Wed, 2024-05-29 at 12:06 +0200, Jan Beulich wrote:
> > > > On 29.05.2024 11:59, Julien Grall wrote:
> > > > > I didn't realise this was an existing comment. I think the
> > > > > suggestion is 
> > > > > a little bit odd because you could use the atomic version of
> > > > > the
> > > > > helper.
> > > > > 
> > > > > Looking at Linux, the second sentence was dropped. But not
> > > > > the
> > > > > first 
> > > > > one. I would suggest to do the same. IOW keep:
> > > > > 
> > > > > "
> > > > > If two examples of this operation race, one can appear to
> > > > > succeed
> > > > > but 
> > > > > actually fail.
> > > > > "
> > > > 
> > > > As indicated, I'm okay with that being retained, but only in a
> > > > form
> > > > that
> > > > actually makes sense. I've explained before (to Oleksii) what I
> > > > consider
> > > > wrong in this way of wording things.
> > > 
> > > Would it be suitable to rephrase it in the following way:
> > >      * 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.
> > >    + * If two instances of this operation race, one may succeed
> > > in
> > >    updating
> > >    + * the bit in memory, but actually fail. It should be
> > > protected
> > >    from
> > >    + * potentially racy behavior.
> > >      */
> > >     static always_inline bool
> > >     __test_and_set_bit(int nr, volatile void *addr)
> > 
> > You've lost the "appear to" ahead of "succeed". Yet even with the
> > adjustment
> > I still don't see what the "appear to succeed" really is supposed
> > to
> > mean
> > here. The issue (imo) isn't with success or failure, but with the
> > write of
> > one CPU potentially being immediately overwritten by another CPU,
> > not
> > observing the bit change that the first CPU did. IOW both will
> > succeed (or
> > appear to succeed), but one update may end up being lost. Maybe
> > "...,
> > both
> > may update memory with their view of the new value, not taking into
> > account
> > the update the respectively other one did"? Or "..., both will
> > appear
> > to
> > succeed, but one of the updates may be lost"?
> For me, the first one is clearer.
If then this part of the comment is needed for test_bit() as it is
doing only reading?

> Julien, would that be okay for you?

~ Oleksii