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

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

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:
> > Hi,
> > 
> > On 29/05/2024 09:36, Jan Beulich wrote:
> > > On 29.05.2024 09:50, Oleksii K. wrote:
> > > > On Tue, 2024-05-28 at 09:53 +0100, Julien Grall wrote:
> > > > > > > > +/**
> > > > > > > > + * generic_test_bit - Determine whether a bit is set
> > > > > > > > + * @nr: bit number to test
> > > > > > > > + * @addr: Address to start counting 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.
> > > > > > > > + */
> > > > > > > 
> > > > > > > You got carried away updating comments - there's no
> > > > > > > raciness for
> > > > > > > simple test_bit(). As is also expressed by its name not
> > > > > > > having
> > > > > > > those
> > > > > > > double underscores that the others have.
> > > > > > Then it is true for every function in this header. Based on
> > > > > > the
> > > > > > naming
> > > > > > the conclusion can be done if it is atomic/npn-atomic and
> > > > > > can/can't
> > > > > > be
> > > > > > reordered.
> > > > > 
> > > > > So let me start with that my only request is to keep the
> > > > > existing
> > > > > comments as you move it. It looks like there were none of
> > > > > test_bit()
> > > > > before.
> > > > Just to clarify that I understand correctly.
> > > > 
> > > > Do we need any comment above functions generic_*()? Based on
> > > > that they
> > > > are implemented in generic way they will be always "non-atomic
> > > > and can
> > > > be reordered.".
> > > 
> > > I indicated before that I think reproducing the same comments
> > > __test_and_*
> > > already have also for generic_* isn't overly useful. If someone
> > > insisted
> > > on them being there as well, I could live with that, though.
> > 
> > Would you be ok if the comment is only on top of the __test_and_* 
> > version? (So no comments on top of the generic_*)
> 
> That's my preferred variant, actually. The alternative I would also
> be
> okay-ish with is to have the comments also ahead of generic_*.
> 
> > > > Do you find the following comment useful?
> > > > 
> > > > " * If two examples of this operation race, one can appear to
> > > > succeed
> > > >   * but actually fail.  You must protect multiple accesses with
> > > > a lock."
> > > > 
> > > > It seems to me that it can dropped as basically "non-atomic and
> > > > can be
> > > > reordered." means that.
> > > 
> > > I agree, or else - as indicated before - the wording would need
> > > to further
> > > change. Yet iirc you've added that in response to a comment from
> > > Julien,
> > > so you'll primarily want his input as to the presence of
> > > something along
> > > these lines.
> > 
> > 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)
   @@ -228,8 +191,9 @@ __test_and_set_bit(int nr, volatile void *addr)
     * @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.
   + * If two instances of this operation race, one may succeed in
   clearing
   + * the bit in memory, but actually fail. It should be protected
   from
   + * potentially racy behavior.
     */
    static always_inline bool
    __test_and_clear_bit(int nr, volatile void *addr)
   @@ -251,8 +215,9 @@ __test_and_clear_bit(int nr, volatile void
   *addr)
     * @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.
   + * If two instances of this operation race, one may succeed in
   changing
   + * the bit in memory, but actually fail. It should be protected
   from
   + * potentially racy behavior.
     */
    static always_inline bool
    __test_and_change_bit(int nr, volatile void *addr)
   @@ -274,8 +239,9 @@ __test_and_change_bit(int nr, volatile void
   *addr)
     * @addr: Address to start counting 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.
   + * If two instances of this operation race, one may succeed in
   testing
   + * the bit in memory, but actually fail. It should be protected
   from
   + * potentially racy behavior.
     */
    static always_inline bool test_bit(int nr, const volatile void
   *addr)

~ Oleksii
> 
> Jan