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

[Julien Grall <julien@xxxxxxx>]

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_*)
> > 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.
"

--
Julien Grall