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. Jan
