Re: [ovs-dev] [PATCH] rculist: Fix iteration macros.
On 11/23/22 15:01, Ilya Maximets wrote: > On 11/21/22 16:54, Adrian Moreno wrote: >> >> >> On 11/4/22 15:25, Ilya Maximets wrote: >>> Some macros for rculist have no users and there are no unit tests >>> specific to that library as well, so broken code wasn't spotted >>> while updating to multi-variable iterators. >>> >>> Fixing multiple problems like missing commas, parenthesis, incorrect >>> variable and macro names. >>> >>> Fixes: d293965d7b06 ("rculist: use multi-variable helpers for loop macros.") >>> Reported-by: Subrata Nath >>> Co-authored-by: Dumitru Ceara >>> Signed-off-by: Dumitru Ceara >>> Signed-off-by: Ilya Maximets >>> --- >>> lib/rculist.h | 18 +- >>> 1 file changed, 9 insertions(+), 9 deletions(-) >>> >>> diff --git a/lib/rculist.h b/lib/rculist.h >>> index c0d77acf9..9bb8cbf3e 100644 >>> --- a/lib/rculist.h >>> +++ b/lib/rculist.h >>> @@ -380,18 +380,18 @@ rculist_is_singleton_protected(const struct rculist >>> *list) >>> #define RCULIST_FOR_EACH_REVERSE_PROTECTED(ITER, MEMBER, RCULIST) >>> \ >>> for (INIT_MULTIVAR(ITER, MEMBER, (RCULIST)->prev, struct rculist); >>> \ >>> CONDITION_MULTIVAR(ITER, MEMBER, ITER_VAR(ITER) != (RCULIST)); >>> \ >>> - UPDATE_MULTIVAR(ITER, ITER_VAR(VAR).prev)) >>> + UPDATE_MULTIVAR(ITER, ITER_VAR(ITER)->prev)) >>> #define RCULIST_FOR_EACH_REVERSE_PROTECTED_CONTINUE(ITER, MEMBER, >>> RCULIST) \ >>> for (INIT_MULTIVAR(ITER, MEMBER, (ITER)->MEMBER.prev, struct >>> rculist); \ >>> CONDITION_MULTIVAR(ITER, MEMBER, ITER_VAR(ITER) != (RCULIST)); >>> \ >>> - UPDATE_MULTIVAR(ITER, ITER_VAR(VAR).prev)) >>> + UPDATE_MULTIVAR(ITER, ITER_VAR(ITER)->prev)) >>> >> >> There's another hidden problem with the REVERSE iterators that has not >> popped up yet: They access 'prev' member directly which will not compile >> when using clang's thread-safety macros because of a fake mutex specifically >> added to avoid it. >> Since the macros are PROTECTED it should be OK to use >> rculist_back_protected() instead. > > Hmm, interesting. > >> >> I have written a unit test for rculist that I was planning to post soon. If >> you prefer I can fix this at the same time. > > If you can fix that in your patch that would be easier, I think. > I'll try to merge the current fix somewhere soon (Just got back > from PTO today). Applied now and backported down to 2.13. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] rculist: Fix iteration macros.
On 11/21/22 16:54, Adrian Moreno wrote: > > > On 11/4/22 15:25, Ilya Maximets wrote: >> Some macros for rculist have no users and there are no unit tests >> specific to that library as well, so broken code wasn't spotted >> while updating to multi-variable iterators. >> >> Fixing multiple problems like missing commas, parenthesis, incorrect >> variable and macro names. >> >> Fixes: d293965d7b06 ("rculist: use multi-variable helpers for loop macros.") >> Reported-by: Subrata Nath >> Co-authored-by: Dumitru Ceara >> Signed-off-by: Dumitru Ceara >> Signed-off-by: Ilya Maximets >> --- >> lib/rculist.h | 18 +- >> 1 file changed, 9 insertions(+), 9 deletions(-) >> >> diff --git a/lib/rculist.h b/lib/rculist.h >> index c0d77acf9..9bb8cbf3e 100644 >> --- a/lib/rculist.h >> +++ b/lib/rculist.h >> @@ -380,18 +380,18 @@ rculist_is_singleton_protected(const struct rculist >> *list) >> #define RCULIST_FOR_EACH_REVERSE_PROTECTED(ITER, MEMBER, RCULIST) >> \ >> for (INIT_MULTIVAR(ITER, MEMBER, (RCULIST)->prev, struct rculist); >> \ >> CONDITION_MULTIVAR(ITER, MEMBER, ITER_VAR(ITER) != (RCULIST)); >> \ >> - UPDATE_MULTIVAR(ITER, ITER_VAR(VAR).prev)) >> + UPDATE_MULTIVAR(ITER, ITER_VAR(ITER)->prev)) >> #define RCULIST_FOR_EACH_REVERSE_PROTECTED_CONTINUE(ITER, MEMBER, >> RCULIST) \ >> for (INIT_MULTIVAR(ITER, MEMBER, (ITER)->MEMBER.prev, struct rculist); >> \ >> CONDITION_MULTIVAR(ITER, MEMBER, ITER_VAR(ITER) != (RCULIST)); >> \ >> - UPDATE_MULTIVAR(ITER, ITER_VAR(VAR).prev)) >> + UPDATE_MULTIVAR(ITER, ITER_VAR(ITER)->prev)) >> > > There's another hidden problem with the REVERSE iterators that has not popped > up yet: They access 'prev' member directly which will not compile when using > clang's thread-safety macros because of a fake mutex specifically added to > avoid it. > Since the macros are PROTECTED it should be OK to use > rculist_back_protected() instead. Hmm, interesting. > > I have written a unit test for rculist that I was planning to post soon. If > you prefer I can fix this at the same time. If you can fix that in your patch that would be easier, I think. I'll try to merge the current fix somewhere soon (Just got back from PTO today). Best regards, Ilya Maximets. > > >> #define RCULIST_FOR_EACH_PROTECTED(ITER, MEMBER, RCULIST) >> \ >> for (INIT_MULTIVAR(ITER, MEMBER, rculist_next_protected(RCULIST), >> \ >> struct rculist); >> \ >> CONDITION_MULTIVAR(ITER, MEMBER, ITER_VAR(ITER) != (RCULIST)); >> \ >> - UPDATE_MULTIVAR(ITER, rculist_next_protected(ITER_VAR(ITER))) >> \ >> + UPDATE_MULTIVAR(ITER, rculist_next_protected(ITER_VAR(ITER >> \ >> #define RCULIST_FOR_EACH_SAFE_SHORT_PROTECTED(ITER, MEMBER, RCULIST) >> \ >> for (INIT_MULTIVAR_SAFE_SHORT(ITER, MEMBER, >> \ >> @@ -399,18 +399,18 @@ rculist_is_singleton_protected(const struct rculist >> *list) >> struct rculist); >> \ >> CONDITION_MULTIVAR_SAFE_SHORT(ITER, MEMBER, >> \ >> ITER_VAR(ITER) != (RCULIST), >> \ >> - ITER_NEXT_VAR(ITER) = rculist_next_protected(ITER_VAR(VAR))); >> \ >> - UPDATE_MULTIVAR_SHORT(ITER)) >> + ITER_NEXT_VAR(ITER) = rculist_next_protected(ITER_VAR(ITER))); >> \ >> + UPDATE_MULTIVAR_SAFE_SHORT(ITER)) >> #define RCULIST_FOR_EACH_SAFE_LONG_PROTECTED(ITER, NEXT, MEMBER, >> RCULIST) \ >> for (INIT_MULTIVAR_SAFE_LONG(ITER, NEXT, MEMBER, >> \ >> - rculist_next_protected(RCULIST) >> \ >> + rculist_next_protected(RCULIST), >> \ >> struct rculist); >> \ >> - CONDITION_MULTIVAR_SAFE_LONG(VAR, NEXT, MEMBER >> \ >> + CONDITION_MULTIVAR_SAFE_LONG(ITER, NEXT, MEMBER, >> \ >> ITER_VAR(ITER) != (RCULIST), >> \ >> - ITER_VAR(NEXT) = rculist_next_protected(ITER_VAR(VAR)), >> \ >> + ITER_VAR(NEXT) = rculist_next_protected(ITER_VAR(ITER)), >> \ >> ITER_VAR(NEXT) != (RCULIST)); >> \ >> - UPDATE_MULTIVAR_LONG(ITER)) >> + UPDATE_MULTIVAR_SAFE_LONG(ITER, NEXT)) >> #define RCULIST_FOR_EACH_SAFE_PROTECTED(...) >> \ >> OVERLOAD_SAFE_MACRO(RCULIST_FOR_EACH_SAFE_LONG_PROTECTED, >> \ > > Thanks ___ dev
Re: [ovs-dev] [PATCH] rculist: Fix iteration macros.
On 11/4/22 15:25, Ilya Maximets wrote: Some macros for rculist have no users and there are no unit tests specific to that library as well, so broken code wasn't spotted while updating to multi-variable iterators. Fixing multiple problems like missing commas, parenthesis, incorrect variable and macro names. Fixes: d293965d7b06 ("rculist: use multi-variable helpers for loop macros.") Reported-by: Subrata Nath Co-authored-by: Dumitru Ceara Signed-off-by: Dumitru Ceara Signed-off-by: Ilya Maximets --- lib/rculist.h | 18 +- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/rculist.h b/lib/rculist.h index c0d77acf9..9bb8cbf3e 100644 --- a/lib/rculist.h +++ b/lib/rculist.h @@ -380,18 +380,18 @@ rculist_is_singleton_protected(const struct rculist *list) #define RCULIST_FOR_EACH_REVERSE_PROTECTED(ITER, MEMBER, RCULIST) \ for (INIT_MULTIVAR(ITER, MEMBER, (RCULIST)->prev, struct rculist); \ CONDITION_MULTIVAR(ITER, MEMBER, ITER_VAR(ITER) != (RCULIST)); \ - UPDATE_MULTIVAR(ITER, ITER_VAR(VAR).prev)) + UPDATE_MULTIVAR(ITER, ITER_VAR(ITER)->prev)) #define RCULIST_FOR_EACH_REVERSE_PROTECTED_CONTINUE(ITER, MEMBER, RCULIST)\ for (INIT_MULTIVAR(ITER, MEMBER, (ITER)->MEMBER.prev, struct rculist); \ CONDITION_MULTIVAR(ITER, MEMBER, ITER_VAR(ITER) != (RCULIST)); \ - UPDATE_MULTIVAR(ITER, ITER_VAR(VAR).prev)) + UPDATE_MULTIVAR(ITER, ITER_VAR(ITER)->prev)) There's another hidden problem with the REVERSE iterators that has not popped up yet: They access 'prev' member directly which will not compile when using clang's thread-safety macros because of a fake mutex specifically added to avoid it. Since the macros are PROTECTED it should be OK to use rculist_back_protected() instead. I have written a unit test for rculist that I was planning to post soon. If you prefer I can fix this at the same time. #define RCULIST_FOR_EACH_PROTECTED(ITER, MEMBER, RCULIST) \ for (INIT_MULTIVAR(ITER, MEMBER, rculist_next_protected(RCULIST), \ struct rculist); \ CONDITION_MULTIVAR(ITER, MEMBER, ITER_VAR(ITER) != (RCULIST)); \ - UPDATE_MULTIVAR(ITER, rculist_next_protected(ITER_VAR(ITER)))\ + UPDATE_MULTIVAR(ITER, rculist_next_protected(ITER_VAR(ITER \ #define RCULIST_FOR_EACH_SAFE_SHORT_PROTECTED(ITER, MEMBER, RCULIST) \ for (INIT_MULTIVAR_SAFE_SHORT(ITER, MEMBER, \ @@ -399,18 +399,18 @@ rculist_is_singleton_protected(const struct rculist *list) struct rculist); \ CONDITION_MULTIVAR_SAFE_SHORT(ITER, MEMBER, \ ITER_VAR(ITER) != (RCULIST), \ - ITER_NEXT_VAR(ITER) = rculist_next_protected(ITER_VAR(VAR)));\ -UPDATE_MULTIVAR_SHORT(ITER)) + ITER_NEXT_VAR(ITER) = rculist_next_protected(ITER_VAR(ITER))); \ +UPDATE_MULTIVAR_SAFE_SHORT(ITER)) #define RCULIST_FOR_EACH_SAFE_LONG_PROTECTED(ITER, NEXT, MEMBER, RCULIST) \ for (INIT_MULTIVAR_SAFE_LONG(ITER, NEXT, MEMBER, \ - rculist_next_protected(RCULIST) \ + rculist_next_protected(RCULIST), \ struct rculist); \ - CONDITION_MULTIVAR_SAFE_LONG(VAR, NEXT, MEMBER \ + CONDITION_MULTIVAR_SAFE_LONG(ITER, NEXT, MEMBER, \ ITER_VAR(ITER) != (RCULIST), \ - ITER_VAR(NEXT) = rculist_next_protected(ITER_VAR(VAR)), \ + ITER_VAR(NEXT) = rculist_next_protected(ITER_VAR(ITER)), \ ITER_VAR(NEXT) != (RCULIST)); \ -UPDATE_MULTIVAR_LONG(ITER)) +UPDATE_MULTIVAR_SAFE_LONG(ITER, NEXT)) #define RCULIST_FOR_EACH_SAFE_PROTECTED(...) \ OVERLOAD_SAFE_MACRO(RCULIST_FOR_EACH_SAFE_LONG_PROTECTED, \ Thanks -- Adrián Moreno ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] rculist: Fix iteration macros.
Acked-by: Alin-Gabriel Serdean -Original Message- From: dev On Behalf Of Ilya Maximets Sent: Friday, November 4, 2022 4:26 PM To: ovs-dev@openvswitch.org Cc: Subrata Nath ; Dumitru Ceara ; Ilya Maximets Subject: [ovs-dev] [PATCH] rculist: Fix iteration macros. Some macros for rculist have no users and there are no unit tests specific to that library as well, so broken code wasn't spotted while updating to multi-variable iterators. Fixing multiple problems like missing commas, parenthesis, incorrect variable and macro names. Fixes: d293965d7b06 ("rculist: use multi-variable helpers for loop macros.") Reported-by: Subrata Nath Co-authored-by: Dumitru Ceara Signed-off-by: Dumitru Ceara Signed-off-by: Ilya Maximets --- lib/rculist.h | 18 +- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/rculist.h b/lib/rculist.h index c0d77acf9..9bb8cbf3e 100644 --- a/lib/rculist.h +++ b/lib/rculist.h @@ -380,18 +380,18 @@ rculist_is_singleton_protected(const struct rculist *list) #define RCULIST_FOR_EACH_REVERSE_PROTECTED(ITER, MEMBER, RCULIST) \ for (INIT_MULTIVAR(ITER, MEMBER, (RCULIST)->prev, struct rculist); \ CONDITION_MULTIVAR(ITER, MEMBER, ITER_VAR(ITER) != (RCULIST)); \ - UPDATE_MULTIVAR(ITER, ITER_VAR(VAR).prev)) + UPDATE_MULTIVAR(ITER, ITER_VAR(ITER)->prev)) #define RCULIST_FOR_EACH_REVERSE_PROTECTED_CONTINUE(ITER, MEMBER, RCULIST) \ for (INIT_MULTIVAR(ITER, MEMBER, (ITER)->MEMBER.prev, struct rculist); \ CONDITION_MULTIVAR(ITER, MEMBER, ITER_VAR(ITER) != (RCULIST)); \ - UPDATE_MULTIVAR(ITER, ITER_VAR(VAR).prev)) + UPDATE_MULTIVAR(ITER, ITER_VAR(ITER)->prev)) #define RCULIST_FOR_EACH_PROTECTED(ITER, MEMBER, RCULIST) \ for (INIT_MULTIVAR(ITER, MEMBER, rculist_next_protected(RCULIST), \ struct rculist); \ CONDITION_MULTIVAR(ITER, MEMBER, ITER_VAR(ITER) != (RCULIST)); \ - UPDATE_MULTIVAR(ITER, rculist_next_protected(ITER_VAR(ITER))) \ + UPDATE_MULTIVAR(ITER, rculist_next_protected(ITER_VAR(ITER \ #define RCULIST_FOR_EACH_SAFE_SHORT_PROTECTED(ITER, MEMBER, RCULIST) \ for (INIT_MULTIVAR_SAFE_SHORT(ITER, MEMBER, \ @@ -399,18 +399,18 @@ rculist_is_singleton_protected(const struct rculist *list) struct rculist); \ CONDITION_MULTIVAR_SAFE_SHORT(ITER, MEMBER, \ ITER_VAR(ITER) != (RCULIST), \ - ITER_NEXT_VAR(ITER) = rculist_next_protected(ITER_VAR(VAR))); \ -UPDATE_MULTIVAR_SHORT(ITER)) + ITER_NEXT_VAR(ITER) = rculist_next_protected(ITER_VAR(ITER))); \ +UPDATE_MULTIVAR_SAFE_SHORT(ITER)) #define RCULIST_FOR_EACH_SAFE_LONG_PROTECTED(ITER, NEXT, MEMBER, RCULIST) \ for (INIT_MULTIVAR_SAFE_LONG(ITER, NEXT, MEMBER, \ - rculist_next_protected(RCULIST) \ + rculist_next_protected(RCULIST), \ struct rculist); \ - CONDITION_MULTIVAR_SAFE_LONG(VAR, NEXT, MEMBER \ + CONDITION_MULTIVAR_SAFE_LONG(ITER, NEXT, MEMBER, \ ITER_VAR(ITER) != (RCULIST), \ - ITER_VAR(NEXT) = rculist_next_protected(ITER_VAR(VAR)), \ + ITER_VAR(NEXT) = rculist_next_protected(ITER_VAR(ITER)), \ ITER_VAR(NEXT) != (RCULIST)); \ -UPDATE_MULTIVAR_LONG(ITER)) +UPDATE_MULTIVAR_SAFE_LONG(ITER, NEXT)) #define RCULIST_FOR_EACH_SAFE_PROTECTED(...) \ OVERLOAD_SAFE_MACRO(RCULIST_FOR_EACH_SAFE_LONG_PROTECTED, \ -- 2.37.3 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev