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

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

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.

Julien, would that be okay for you?

~ Oleksii