Re: [PATCH v11 2/9] xen: introduce generic non-atomic test_*bit()
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
Re: [PATCH v11 2/9] xen: introduce generic non-atomic test_*bit()
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. If then this part of the comment is needed for test_bit() as it is doing only reading? > Julien, would that be okay for you? ~ Oleksii
Re: [PATCH v11 2/9] xen: introduce generic non-atomic test_*bit()
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. Julien, would that be okay for you? ~ Oleksii
Re: [PATCH v11 2/9] xen: introduce generic non-atomic test_*bit()
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
Re: [PATCH v11 2/9] xen: introduce generic non-atomic test_*bit()
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 exampl
Re: [PATCH v11 2/9] xen: introduce generic non-atomic test_*bit()
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
Re: [PATCH v11 2/9] xen: introduce generic non-atomic test_*bit()
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
Re: [PATCH v11 2/9] xen: introduce generic non-atomic test_*bit()
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. > 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. Jan
Re: [PATCH v11 2/9] xen: introduce generic non-atomic test_*bit()
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.". 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. ~ Oleksii
Re: [PATCH v11 2/9] xen: introduce generic non-atomic test_*bit()
On 28.05.2024 10:37, Oleksii K. wrote: > On Tue, 2024-05-28 at 08:20 +0200, Jan Beulich wrote: >> On 24.05.2024 13:08, Oleksii Kurochko 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 the comments aren't seem very useful. I disagree - test_bit() is different, in not being a read-modify-write operation. Jan
Re: [PATCH v11 2/9] xen: introduce generic non-atomic test_*bit()
Hi Oleksii, On 28/05/2024 09:37, Oleksii K. wrote: On Tue, 2024-05-28 at 08:20 +0200, Jan Beulich wrote: On 24.05.2024 13:08, Oleksii Kurochko wrote: The following generic functions were introduced: * test_bit * generic__test_and_set_bit * generic__test_and_clear_bit * generic__test_and_change_bit These functions and macros can be useful for architectures that don't have corresponding arch-specific instructions. Also, the patch introduces the following generics which are used by the functions mentioned above: * BITOP_BITS_PER_WORD * BITOP_MASK * BITOP_WORD * BITOP_TYPE BITS_PER_BYTE was moved to xen/bitops.h as BITS_PER_BYTE is the same across architectures. The following approach was chosen for generic*() and arch*() bit operation functions: If the bit operation function that is going to be generic starts with the prefix "__", then the corresponding generic/arch function will also contain the "__" prefix. For example: * test_bit() will be defined using arch_test_bit() and generic_test_bit(). * __test_and_set_bit() will be defined using arch__test_and_set_bit() and generic__test_and_set_bit(). Signed-off-by: Oleksii Kurochko --- Reviewed-by: Jan Beulich jbeul...@suse.com? Jan gave his R-by for the previous version of the patch, but some changes were done, so I wasn't sure if I could use the R-by in this patch version. That really depends on the nature of the changes. Seeing the pretty long list below, I think it was appropriate to drop the R-b. --- Changes in V11: - fix identation in generic_test_bit() function. - move definition of BITS_PER_BYTE from /bitops.h to xen/bitops.h I realize you were asked to do this, but I'm not overly happy with it. BITS_PER_BYTE is far more similar to BITS_PER_LONG than to BITOP_BITS_PER_WORD. Plus in any event that change is entirely unrelated here. So where then it should be introduced? x86 introduces that in config.h, Arm in asm/bitops.h. I would be fine if it is defined in config.h for Arm. I am okay to revert this change and make a separate patch. [...] Also did Julien(?) really ask that these comments be reproduced on both the functions supposed to be used throughout the codebase _and_ the generic_*() ones (being merely internal helpers/fallbacks)? At least, I understood that in this way. I suggested to duplicate. But I also proposed an alternative to move the comment: "I think this comment should be duplicated (or moved to) on top of" I don't have a strong preference which one is used. +/** + * 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. > So the comments aren't seem very useful. The comments *are* useful. We had several instances where non-Arm folks thought all the operations were atomic on Arm as well. And the usual suggestion is to add barriers in the Arm version which is a no-go. Cheers, -- Julien Grall
Re: [PATCH v11 2/9] xen: introduce generic non-atomic test_*bit()
On Tue, 2024-05-28 at 08:20 +0200, Jan Beulich wrote: > On 24.05.2024 13:08, Oleksii Kurochko wrote: > > The following generic functions were introduced: > > * test_bit > > * generic__test_and_set_bit > > * generic__test_and_clear_bit > > * generic__test_and_change_bit > > > > These functions and macros can be useful for architectures > > that don't have corresponding arch-specific instructions. > > > > Also, the patch introduces the following generics which are > > used by the functions mentioned above: > > * BITOP_BITS_PER_WORD > > * BITOP_MASK > > * BITOP_WORD > > * BITOP_TYPE > > > > BITS_PER_BYTE was moved to xen/bitops.h as BITS_PER_BYTE is the > > same > > across architectures. > > > > The following approach was chosen for generic*() and arch*() bit > > operation functions: > > If the bit operation function that is going to be generic starts > > with the prefix "__", then the corresponding generic/arch function > > will also contain the "__" prefix. For example: > > * test_bit() will be defined using arch_test_bit() and > > generic_test_bit(). > > * __test_and_set_bit() will be defined using > > arch__test_and_set_bit() and generic__test_and_set_bit(). > > > > Signed-off-by: Oleksii Kurochko > > --- > > Reviewed-by: Jan Beulich jbeul...@suse.com? Jan gave his R-by for > > the previous > > version of the patch, but some changes were done, so I wasn't sure > > if I could > > use the R-by in this patch version. > > That really depends on the nature of the changes. Seeing the pretty > long list below, I think it was appropriate to drop the R-b. > > > --- > > Changes in V11: > > - fix identation in generic_test_bit() function. > > - move definition of BITS_PER_BYTE from /bitops.h to > > xen/bitops.h > > I realize you were asked to do this, but I'm not overly happy with > it. > BITS_PER_BYTE is far more similar to BITS_PER_LONG than to > BITOP_BITS_PER_WORD. Plus in any event that change is entirely > unrelated > here. So where then it should be introduced? x86 introduces that in config.h, Arm in asm/bitops.h. I am okay to revert this change and make a separate patch. > > > - drop the changes in arm64/livepatch.c. > > - update the the comments on top of functions: > > generic__test_and_set_bit(), > > generic__test_and_clear_bit(), generic__test_and_change_bit(), > > generic_test_bit(). > > - Update footer after Signed-off section. > > - Rebase the patch on top of staging branch, so it can be merged > > when necessary > > approves will be given. > > - Update the commit message. > > --- > > xen/arch/arm/include/asm/bitops.h | 69 --- > > xen/arch/ppc/include/asm/bitops.h | 54 - > > xen/arch/ppc/include/asm/page.h | 2 +- > > xen/arch/ppc/mm-radix.c | 2 +- > > xen/arch/x86/include/asm/bitops.h | 31 ++--- > > xen/include/xen/bitops.h | 185 > > ++ > > 6 files changed, 196 insertions(+), 147 deletions(-) > > > > --- a/xen/include/xen/bitops.h > > +++ b/xen/include/xen/bitops.h > > @@ -103,8 +103,193 @@ static inline int generic_flsl(unsigned long > > x) > > * Include this here because some architectures need > > generic_ffs/fls in > > * scope > > */ > > + > > +#define BITOP_BITS_PER_WORD 32 > > +typedef uint32_t bitop_uint_t; > > + > > +#define BITOP_MASK(nr) ((bitop_uint_t)1 << ((nr) % > > BITOP_BITS_PER_WORD)) > > + > > +#define BITOP_WORD(nr) ((nr) / BITOP_BITS_PER_WORD) > > + > > +#define BITS_PER_BYTE 8 > > This, if to be moved at all, very clearly doesn't belong here. As > much > as it's unrelated to this change, it's also unrelated to bitops as a > whole. > > > +extern void __bitop_bad_size(void); > > + > > +#define bitop_bad_size(addr) (sizeof(*(addr)) < > > sizeof(bitop_uint_t)) > > + > > +/* - Please tidy above here -- > > --- */ > > + > > #include > > > > +/** > > + * generic__test_and_set_bit - Set a bit and return its old value > > + * @nr: Bit to set > > + * @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 > > Why "examples"? Do you mean "instances"? It's further unclear what > "succeed" and "fail" here mean. The function after all has two > purposes: Checking and setting the specified bit. Therefore I think > you mean "succeed in updating the bit in memory", yet then it's > still unclear what the "appear" here means: The caller has no > indication of success/failure. Therefore I think "appear to" also > wants dropping. > > > + * but actually fail. You must protect multiple accesses with a > > lock. > > That's not "must". An often better alternative is to use the atomic > forms instead. "Multiple" is imprecise, too: "Potentially racy" or > some such would come closer. I think we can go only with: " This operation is non-atomic and can be reordered." and drop: " If two examples of this operation race, one
Re: [PATCH v11 2/9] xen: introduce generic non-atomic test_*bit()
On 24.05.2024 13:08, Oleksii Kurochko wrote: > The following generic functions were introduced: > * test_bit > * generic__test_and_set_bit > * generic__test_and_clear_bit > * generic__test_and_change_bit > > These functions and macros can be useful for architectures > that don't have corresponding arch-specific instructions. > > Also, the patch introduces the following generics which are > used by the functions mentioned above: > * BITOP_BITS_PER_WORD > * BITOP_MASK > * BITOP_WORD > * BITOP_TYPE > > BITS_PER_BYTE was moved to xen/bitops.h as BITS_PER_BYTE is the same > across architectures. > > The following approach was chosen for generic*() and arch*() bit > operation functions: > If the bit operation function that is going to be generic starts > with the prefix "__", then the corresponding generic/arch function > will also contain the "__" prefix. For example: > * test_bit() will be defined using arch_test_bit() and >generic_test_bit(). > * __test_and_set_bit() will be defined using >arch__test_and_set_bit() and generic__test_and_set_bit(). > > Signed-off-by: Oleksii Kurochko > --- > Reviewed-by: Jan Beulich jbeul...@suse.com? Jan gave his R-by for the > previous > version of the patch, but some changes were done, so I wasn't sure if I could > use the R-by in this patch version. That really depends on the nature of the changes. Seeing the pretty long list below, I think it was appropriate to drop the R-b. > --- > Changes in V11: > - fix identation in generic_test_bit() function. > - move definition of BITS_PER_BYTE from /bitops.h to xen/bitops.h I realize you were asked to do this, but I'm not overly happy with it. BITS_PER_BYTE is far more similar to BITS_PER_LONG than to BITOP_BITS_PER_WORD. Plus in any event that change is entirely unrelated here. > - drop the changes in arm64/livepatch.c. > - update the the comments on top of functions: generic__test_and_set_bit(), >generic__test_and_clear_bit(), generic__test_and_change_bit(), >generic_test_bit(). > - Update footer after Signed-off section. > - Rebase the patch on top of staging branch, so it can be merged when > necessary >approves will be given. > - Update the commit message. > --- > xen/arch/arm/include/asm/bitops.h | 69 --- > xen/arch/ppc/include/asm/bitops.h | 54 - > xen/arch/ppc/include/asm/page.h | 2 +- > xen/arch/ppc/mm-radix.c | 2 +- > xen/arch/x86/include/asm/bitops.h | 31 ++--- > xen/include/xen/bitops.h | 185 ++ > 6 files changed, 196 insertions(+), 147 deletions(-) > > --- a/xen/include/xen/bitops.h > +++ b/xen/include/xen/bitops.h > @@ -103,8 +103,193 @@ static inline int generic_flsl(unsigned long x) > * Include this here because some architectures need generic_ffs/fls in > * scope > */ > + > +#define BITOP_BITS_PER_WORD 32 > +typedef uint32_t bitop_uint_t; > + > +#define BITOP_MASK(nr) ((bitop_uint_t)1 << ((nr) % BITOP_BITS_PER_WORD)) > + > +#define BITOP_WORD(nr) ((nr) / BITOP_BITS_PER_WORD) > + > +#define BITS_PER_BYTE 8 This, if to be moved at all, very clearly doesn't belong here. As much as it's unrelated to this change, it's also unrelated to bitops as a whole. > +extern void __bitop_bad_size(void); > + > +#define bitop_bad_size(addr) (sizeof(*(addr)) < sizeof(bitop_uint_t)) > + > +/* - Please tidy above here - */ > + > #include > > +/** > + * generic__test_and_set_bit - Set a bit and return its old value > + * @nr: Bit to set > + * @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 Why "examples"? Do you mean "instances"? It's further unclear what "succeed" and "fail" here mean. The function after all has two purposes: Checking and setting the specified bit. Therefore I think you mean "succeed in updating the bit in memory", yet then it's still unclear what the "appear" here means: The caller has no indication of success/failure. Therefore I think "appear to" also wants dropping. > + * but actually fail. You must protect multiple accesses with a lock. That's not "must". An often better alternative is to use the atomic forms instead. "Multiple" is imprecise, too: "Potentially racy" or some such would come closer. Also did Julien(?) really ask that these comments be reproduced on both the functions supposed to be used throughout the codebase _and_ the generic_*() ones (being merely internal helpers/fallbacks)? > +/** > + * 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_bi
[PATCH v11 2/9] xen: introduce generic non-atomic test_*bit()
The following generic functions were introduced:
* test_bit
* generic__test_and_set_bit
* generic__test_and_clear_bit
* generic__test_and_change_bit
These functions and macros can be useful for architectures
that don't have corresponding arch-specific instructions.
Also, the patch introduces the following generics which are
used by the functions mentioned above:
* BITOP_BITS_PER_WORD
* BITOP_MASK
* BITOP_WORD
* BITOP_TYPE
BITS_PER_BYTE was moved to xen/bitops.h as BITS_PER_BYTE is the same
across architectures.
The following approach was chosen for generic*() and arch*() bit
operation functions:
If the bit operation function that is going to be generic starts
with the prefix "__", then the corresponding generic/arch function
will also contain the "__" prefix. For example:
* test_bit() will be defined using arch_test_bit() and
generic_test_bit().
* __test_and_set_bit() will be defined using
arch__test_and_set_bit() and generic__test_and_set_bit().
Signed-off-by: Oleksii Kurochko
---
Reviewed-by: Jan Beulich jbeul...@suse.com? Jan gave his R-by for the previous
version of the patch, but some changes were done, so I wasn't sure if I could
use the R-by in this patch version.
The current patch can be merged without waiting for Andrew's patch series as
it was rebased on top of staging.
---
Changes in V11:
- fix identation in generic_test_bit() function.
- move definition of BITS_PER_BYTE from /bitops.h to xen/bitops.h
- drop the changes in arm64/livepatch.c.
- update the the comments on top of functions: generic__test_and_set_bit(),
generic__test_and_clear_bit(), generic__test_and_change_bit(),
generic_test_bit().
- Update footer after Signed-off section.
- Rebase the patch on top of staging branch, so it can be merged when necessary
approves will be given.
- Update the commit message.
---
Changes in V10:
- update the commit message. ( re-order paragraphs and add explanation usage
of prefix "__" in bit
operation function names )
- add parentheses around the whole expression of bitop_bad_size() macros.
- move macros bitop_bad_size() above asm/bitops.h as it is not arch-specific
anymore and there is
no need for overriding it.
- drop macros check_bitop_size() and use "if ( bitop_bad_size(addr) )
__bitop_bad_size();" implictly
where it is needed.
- in use 'int' as a first parameter for __test_and_*(),
generic__test_and_*() to be
consistent with how the mentioned functions were declared in the original
per-arch functions.
- add 'const' to p variable in generic_test_bit().
- move definition of BITOP_BITS_PER_WORD and bitop_uint_t to xen/bitops.h as
we don't allow for arch
overrides these definitions anymore.
---
Changes in V9:
- move up xen/bitops.h in ppc/asm/page.h.
- update defintion of arch_check_bitop_size.
And drop correspondent macros from x86/asm/bitops.h
- drop parentheses in generic__test_and_set_bit() for definition of
local variable p.
- fix indentation inside #ifndef BITOP_TYPE...#endif
- update the commit message.
---
Changes in V8:
- drop __pure for function which uses volatile.
- drop unnessary () in generic__test_and_change_bit() for addr casting.
- update prototype of generic_test_bit() and test_bit(): now it returns bool
instead of int.
- update generic_test_bit() to use BITOP_MASK().
- Deal with fls{l} changes: it should be in the patch with introduced generic
fls{l}.
- add a footer with explanation of dependency on an uncommitted patch after
Signed-off.
- abstract bitop_size().
- move BITOP_TYPE define to .
---
Changes in V7:
- move everything to xen/bitops.h to follow the same approach for all generic
bit ops.
- put together BITOP_BITS_PER_WORD and bitops_uint_t.
- make BITOP_MASK more generic.
- drop #ifdef ... #endif around BITOP_MASK, BITOP_WORD as they are generic
enough.
- drop "_" for generic__{test_and_set_bit,...}().
- drop " != 0" for functions which return bool.
- add volatile during the cast for generic__{...}().
- update the commit message.
- update arch related code to follow the proposed generic approach.
---
Changes in V6:
- Nothing changed ( only rebase )
---
Changes in V5:
- new patch
---
xen/arch/arm/include/asm/bitops.h | 69 ---
xen/arch/ppc/include/asm/bitops.h | 54 -
xen/arch/ppc/include/asm/page.h | 2 +-
xen/arch/ppc/mm-radix.c | 2 +-
xen/arch/x86/include/asm/bitops.h | 31 ++---
xen/include/xen/bitops.h | 185 ++
6 files changed, 196 insertions(+), 147 deletions(-)
diff --git a/xen/arch/arm/include/asm/bitops.h
b/xen/arch/arm/include/asm/bitops.h
index ab030b6cb0..af38fbffdd 100644
--- a/xen/arch/arm/include/asm/bitops.h
+++ b/xen/arch/arm/include/asm/bitops.h
@@ -22,11 +22,6 @@
#define __set_bit(n,p)set_bit(n,p)
#define __clear_bit(n,p) clear_bit(n,p)
-#define BITOP_BITS_PER_WORD 32
-#define BITOP_MASK(nr) (1UL << ((n
