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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 29/05/2024 17:36, Oleksii K. wrote:
> 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.
I actually find the second better because it explicitely spell out the 
issue. I can live with the first one though.
> If then this part of the comment is needed for test_bit() as it is
> doing only reading?
I don't think so. As Jan said, test_bit() is not a read-modify-write 
operation.
Cheers,

--
Julien Grall