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"? Jan
