Message ID | 20130907142041.GA20237@redhat.com |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
On 09/07/2013 04:20 PM, Veaceslav Falico wrote: > On Fri, Sep 06, 2013 at 03:28:07PM +0800, Ding Tianhong wrote: <snip> > (Also, mind the "likely(__ptr != __next)" - usually, and bonding is a very > good example, we always have at least one element in the list.) > > This way, I'd recommend you to do bond_last_slave_rcu() the same way as > here - you, though, might omit saving the slave_list pointer, it's not > needed in case of bonding AFAIK. Something like that (I'm writing it in my > mail editor - so it's only for the reference): > > #define bond_last_slave_rcu(bond) \ > ({struct list_head *__slave_ptr = list_next_rcu(&bond->slave_list); \ > likely(__slave_ptr != &bond->slave_list) ? \ > bond_to_slave_rcu(__slave_ptr) : NULL;}) > > > Or, even better (from my POV), add a generic macro to rculist.h (again, > didn't even compile it) - it can be used later on: > > diff --git a/include/linux/rculist.h b/include/linux/rculist.h > index f4b1001..37b49d1 100644 > --- a/include/linux/rculist.h > +++ b/include/linux/rculist.h > @@ -23,6 +23,7 @@ > * way, we must not access it directly > */ > #define list_next_rcu(list) (*((struct list_head __rcu > **)(&(list)->next))) > +#define list_prev_rcu(list) (*((struct list_head __rcu > **)(&(list)->prev))) > > /* > * Insert a new entry between two known consecutive entries. > @@ -271,6 +272,12 @@ static inline void list_splice_init_rcu(struct > list_head *list, > likely(__ptr != __next) ? container_of(__next, type, member) : NULL; \ > }) > > +#define list_last_or_null_rcu(ptr, type, member) \ > + ({struct list_head *__ptr = (ptr); \ > + struct list_head __rcu *__last = list_prev_rcu(__ptr); \ > + likely(__ptr != __last) ? container_of(__prev, type, member) : NULL; \ > + }) > + Hi, Actually I don't think you can dereference ->prev and use the standard list_del_rcu because it guarantees only the ->next ptr will be valid and ->prev is set to LIST_POISON2. IMO, you'll need something like this: https://lkml.org/lkml/2012/7/25/193 with the bidir_del and all that. But in any case I complete agree with Veaceslav here. Read all the documentation carefully :-) Cheers, Nik > /** > * list_for_each_entry_rcu - iterate over rcu list of given type > * @pos: the type * to use as a loop cursor. > ------- END OF PATCH ------ > > Anyway, it's up to you. > > Hope that helps. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Sep 07, 2013 at 04:45:05PM +0200, Nikolay Aleksandrov wrote: > >On 09/07/2013 04:20 PM, Veaceslav Falico wrote: >> On Fri, Sep 06, 2013 at 03:28:07PM +0800, Ding Tianhong wrote: ...snip... >> diff --git a/include/linux/rculist.h b/include/linux/rculist.h >> index f4b1001..37b49d1 100644 >> --- a/include/linux/rculist.h >> +++ b/include/linux/rculist.h >> @@ -23,6 +23,7 @@ >> * way, we must not access it directly >> */ >> #define list_next_rcu(list) (*((struct list_head __rcu >> **)(&(list)->next))) >> +#define list_prev_rcu(list) (*((struct list_head __rcu >> **)(&(list)->prev))) >> >> /* >> * Insert a new entry between two known consecutive entries. >> @@ -271,6 +272,12 @@ static inline void list_splice_init_rcu(struct >> list_head *list, >> likely(__ptr != __next) ? container_of(__next, type, member) : NULL; \ >> }) >> >> +#define list_last_or_null_rcu(ptr, type, member) \ >> + ({struct list_head *__ptr = (ptr); \ >> + struct list_head __rcu *__last = list_prev_rcu(__ptr); \ >> + likely(__ptr != __last) ? container_of(__prev, type, member) : NULL; \ >> + }) >> + >Hi, >Actually I don't think you can dereference ->prev and use the standard >list_del_rcu because it guarantees only the ->next ptr will be valid and >->prev is set to LIST_POISON2. >IMO, you'll need something like this: https://lkml.org/lkml/2012/7/25/193 >with the bidir_del and all that. Yeah, right, my bad - we can rely only on the ->next pointer, indeed, missed that part. RCU is hard :). So it'll be a lot harder to implement bond_last_slave_rcu() in a 'straightforward' approach. I'd rather go in the opposite direction here - i.e. drop the 'reverse' traversal completely, and all the use cases for bond_last_slave_rcu(). I've got some patches already - http://patchwork.ozlabs.org/patch/272076/ doing that, and hopefully will remove the whole 'backword' traversal completely in the future. > >But in any case I complete agree with Veaceslav here. Read all the >documentation carefully :-) > >Cheers, > Nik > >> /** >> * list_for_each_entry_rcu - iterate over rcu list of given type >> * @pos: the type * to use as a loop cursor. >> ------- END OF PATCH ------ >> >> Anyway, it's up to you. >> >> Hope that helps. > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
于 2013/9/7 23:03, Veaceslav Falico 写道: > On Sat, Sep 07, 2013 at 04:45:05PM +0200, Nikolay Aleksandrov wrote: >> >> On 09/07/2013 04:20 PM, Veaceslav Falico wrote: >>> On Fri, Sep 06, 2013 at 03:28:07PM +0800, Ding Tianhong wrote: > ...snip... >>> diff --git a/include/linux/rculist.h b/include/linux/rculist.h >>> index f4b1001..37b49d1 100644 >>> --- a/include/linux/rculist.h >>> +++ b/include/linux/rculist.h >>> @@ -23,6 +23,7 @@ >>> * way, we must not access it directly >>> */ >>> #define list_next_rcu(list) (*((struct list_head __rcu >>> **)(&(list)->next))) >>> +#define list_prev_rcu(list) (*((struct list_head __rcu >>> **)(&(list)->prev))) >>> >>> /* >>> * Insert a new entry between two known consecutive entries. >>> @@ -271,6 +272,12 @@ static inline void list_splice_init_rcu(struct >>> list_head *list, >>> likely(__ptr != __next) ? container_of(__next, type, member) : NULL; \ >>> }) >>> >>> +#define list_last_or_null_rcu(ptr, type, member) \ >>> + ({struct list_head *__ptr = (ptr); \ >>> + struct list_head __rcu *__last = list_prev_rcu(__ptr); \ >>> + likely(__ptr != __last) ? container_of(__prev, type, member) : >>> NULL; \ >>> + }) >>> + >> Hi, >> Actually I don't think you can dereference ->prev and use the standard >> list_del_rcu because it guarantees only the ->next ptr will be valid and >> ->prev is set to LIST_POISON2. >> IMO, you'll need something like this: >> https://lkml.org/lkml/2012/7/25/193 >> with the bidir_del and all that. > > Yeah, right, my bad - we can rely only on the ->next pointer, indeed, > missed that part. RCU is hard :). > > So it'll be a lot harder to implement bond_last_slave_rcu() in a > 'straightforward' approach. > > I'd rather go in the opposite direction here - i.e. drop the 'reverse' > traversal completely, and all the use cases for bond_last_slave_rcu(). > I've > got some patches already - http://patchwork.ozlabs.org/patch/272076/ > doing > that, and hopefully will remove the whole 'backword' traversal completely > in the future. > Although RCU is truely difficult to understand, but it is very interesting and beautiful, I will follow your footsteps and finish it. >> >> But in any case I complete agree with Veaceslav here. Read all the >> documentation carefully :-) >> >> Cheers, >> Nik >> good lession, read it complete and agreed, it is a hard work to do. I need more time to review the Veaceslav's patchset and improve my thought, thanks Nik and Veaceslav. Best regards Ding >>> /** >>> * list_for_each_entry_rcu - iterate over rcu list of given type >>> * @pos: the type * to use as a loop cursor. >>> ------- END OF PATCH ------ >>> >>> Anyway, it's up to you. >>> >>> Hope that helps. >> > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013/9/8 14:05, Ding Tianhong wrote: Hi Veaceslav and Nik: please take a moment to reveiw the function just modify for bond_XXX_rcu, and give me some advice. thanks for the help again.:) +#define bond_first_slave_rcu(bond) \ + list_first_or_null_rcu(&(bond)->slave_list, struct slave, list); +#define bond_last_slave_rcu(bond) \ + ({struct list_head *__slave_list = &(bond)->slave_list; \ + struct list_head __rcu *__prev = \ + (*((struct list_head __rcu **)(&(__slave_list)->prev)));\ + likely(__slave_list != __prev) ? \ + container_of(__prev, struct slave, list) : NULL;}) + #define bond_is_first_slave(bond, pos) ((pos)->list.prev == &(bond)->slave_list) #define bond_is_last_slave(bond, pos) ((pos)->list.next == &(bond)->slave_list) @@ -93,6 +117,29 @@ (bond_is_first_slave(bond, pos) ? bond_last_slave(bond) : \ bond_to_slave((pos)->list.prev)) +/* Since bond_first/last_slave_rcu can return NULL, these can return NULL too */ +#define bond_next_slave_rcu(bond, pos) \ + ({struct list_head *__slave_list = &(bond)->slave_list; \ + struct list_head __rcu *__next = list_next_rcu(__slave_list); \ + struct list_head *__pos_list = &(pos)->list; \ + struct list_head __rcu *__pos_next = list_next_rcu(__pos_list); \ + likely(__pos_next != __slave_list) ? \ + container_of(__pos_next, struct slave, list) : \ + container_of(__next, struct slave, list); \ + }) + +#define bond_prev_slave_rcu(bond, pos) \ + ({struct list_head *__slave_list = &(bond)->slave_list; \ + struct list_head __rcu *__prev = \ + (*((struct list_head __rcu **)(&(__slave_list)->prev)));\ + struct list_head *__pos_list = &(pos)->list; \ + struct list_head __rcu *__pos_prev = (__pos_list->prev != LIST_POISON2) ? \ + (*((struct list_head __rcu **)(&(__pos_list)->prev))) : NULL; \ + likely(__pos_prev != __slave_list) ? \ + ((__pos_prev) ? list_entry_rcu(__pos_prev, struct slave, list) : NULL;) : \ + (list_entry_rcu(__prev, struct slave, list)); \ + }) + -#define bond_for_each_slave_from(bond, pos, cnt, start) \ - for (cnt = 0, pos = start; pos && cnt < (bond)->slave_cnt; \ - cnt++, pos = bond_next_slave(bond, pos)) - +#define bond_for_each_slave_from(bond, pos, start) \ + for (pos = start; pos; (pos = bond_next_slave(bond, pos)) != start ? \ + (pos) : (pos = NULL)) + +#define bond_for_each_slave_from_rcu(bond, pos, start) \ + for ({struct list_head *__start = &(start)->list; \ + struct list_head *__slave_list = &(bond)->slave_list; \ + pos = list_entry_rcu(__start, struct slave, list);}; \ + pos; \ + {struct list_head __rcu *__next = list_next_rcu(pos->next); \ + __next != __slave_list ? \ + __next : __next = list_next_rcu(__next->next); \ + __next != __start ? \ + pos = list_entry_rcu(__next, struct slave, list) : \ + pos = NULL; \ + }) + Best regards Ding >> -- >> To unsubscribe from this list: send the line "unsubsc ribe netdev" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > > . > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 09, 2013 at 04:58:29PM +0800, Ding Tianhong wrote: >On 2013/9/8 14:05, Ding Tianhong wrote: > >Hi Veaceslav and Nik: > >please take a moment to reveiw the function just modify for bond_XXX_rcu, >and give me some advice. thanks for the help again.:) > >+#define bond_first_slave_rcu(bond) \ >+ list_first_or_null_rcu(&(bond)->slave_list, struct slave, list); >+#define bond_last_slave_rcu(bond) \ >+ ({struct list_head *__slave_list = &(bond)->slave_list; \ >+ struct list_head __rcu *__prev = \ >+ (*((struct list_head __rcu **)(&(__slave_list)->prev)));\ >+ likely(__slave_list != __prev) ? \ >+ container_of(__prev, struct slave, list) : NULL;}) Please take a look at Nikolay's reply to my RCU email - http://www.spinics.net/lists/netdev/msg249805.html . And mine also, to his email. In short - RCU doesn't guarantee ->prev, so better take the approach of eliminating bond_last/prev_slave completely. >+ > #define bond_is_first_slave(bond, pos) ((pos)->list.prev == &(bond)->slave_list) > #define bond_is_last_slave(bond, pos) ((pos)->list.next == &(bond)->slave_list) > >@@ -93,6 +117,29 @@ > (bond_is_first_slave(bond, pos) ? bond_last_slave(bond) : \ > bond_to_slave((pos)->list.prev)) > >+/* Since bond_first/last_slave_rcu can return NULL, these can return NULL too */ >+#define bond_next_slave_rcu(bond, pos) \ >+ ({struct list_head *__slave_list = &(bond)->slave_list; \ >+ struct list_head __rcu *__next = list_next_rcu(__slave_list); \ >+ struct list_head *__pos_list = &(pos)->list; \ >+ struct list_head __rcu *__pos_next = list_next_rcu(__pos_list); \ >+ likely(__pos_next != __slave_list) ? \ >+ container_of(__pos_next, struct slave, list) : \ >+ container_of(__next, struct slave, list); \ >+ }) Nice, but can be shortened - we know that pos won't go away. >+ >+#define bond_prev_slave_rcu(bond, pos) \ >+ ({struct list_head *__slave_list = &(bond)->slave_list; \ >+ struct list_head __rcu *__prev = \ >+ (*((struct list_head __rcu **)(&(__slave_list)->prev)));\ >+ struct list_head *__pos_list = &(pos)->list; \ >+ struct list_head __rcu *__pos_prev = (__pos_list->prev != LIST_POISON2) ? \ >+ (*((struct list_head __rcu **)(&(__pos_list)->prev))) : NULL; \ >+ likely(__pos_prev != __slave_list) ? \ >+ ((__pos_prev) ? list_entry_rcu(__pos_prev, struct slave, list) : NULL;) : \ >+ (list_entry_rcu(__prev, struct slave, list)); \ >+ }) Same remark as above about prev. >+ > > >-#define bond_for_each_slave_from(bond, pos, cnt, start) \ >- for (cnt = 0, pos = start; pos && cnt < (bond)->slave_cnt; \ >- cnt++, pos = bond_next_slave(bond, pos)) >- >+#define bond_for_each_slave_from(bond, pos, start) \ >+ for (pos = start; pos; (pos = bond_next_slave(bond, pos)) != start ? \ >+ (pos) : (pos = NULL)) >+ >+#define bond_for_each_slave_from_rcu(bond, pos, start) \ >+ for ({struct list_head *__start = &(start)->list; \ >+ struct list_head *__slave_list = &(bond)->slave_list; \ >+ pos = list_entry_rcu(__start, struct slave, list);}; \ >+ pos; \ >+ {struct list_head __rcu *__next = list_next_rcu(pos->next); \ >+ __next != __slave_list ? \ >+ __next : __next = list_next_rcu(__next->next); \ >+ __next != __start ? \ >+ pos = list_entry_rcu(__next, struct slave, list) : \ >+ pos = NULL; \ >+ }) Jeez, I don't even want to review it. It's too complex and too hard to maintain, even if it works. Can you please make something shorter/easier to understand? >+ > >Best regards >Ding > > >>> -- >>> To unsubscribe from this list: send the line "unsubsc ribe netdev" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >> >> >> . >> > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
于 2013/9/9 17:57, Veaceslav Falico 写道: > On Mon, Sep 09, 2013 at 04:58:29PM +0800, Ding Tianhong wrote: >> On 2013/9/8 14:05, Ding Tianhong wrote: >> >> Hi Veaceslav and Nik: >> >> please take a moment to reveiw the function just modify for >> bond_XXX_rcu, >> and give me some advice. thanks for the help again.:) >> >> +#define bond_first_slave_rcu(bond) \ >> + list_first_or_null_rcu(&(bond)->slave_list, struct slave, list); >> +#define bond_last_slave_rcu(bond) \ >> + ({struct list_head *__slave_list = &(bond)->slave_list; \ >> + struct list_head __rcu *__prev = \ >> + (*((struct list_head __rcu **)(&(__slave_list)->prev)));\ >> + likely(__slave_list != __prev) ? \ >> + container_of(__prev, struct slave, list) : NULL;}) > > Please take a look at Nikolay's reply to my RCU email - > http://www.spinics.net/lists/netdev/msg249805.html . And mine also, to > his > email. In short - RCU doesn't guarantee ->prev, so better take the > approach > of eliminating bond_last/prev_slave completely. > yes, I see the message, the list_del_rcu will make the slave->list ->prev = LIST_POISON2, the bond->slave_list will not be set to the messae, the prev will point a slave->list or itself, so I think it will be ok here, please correct me if I miss something. Best Regards Ding >> + >> #define bond_is_first_slave(bond, pos) ((pos)->list.prev == >> &(bond)->slave_list) >> #define bond_is_last_slave(bond, pos) ((pos)->list.next == >> &(bond)->slave_list) >> >> @@ -93,6 +117,29 @@ >> (bond_is_first_slave(bond, pos) ? bond_last_slave(bond) : \ >> bond_to_slave((pos)->list.prev)) >> >> +/* Since bond_first/last_slave_rcu can return NULL, these can return >> NULL too */ >> +#define bond_next_slave_rcu(bond, pos) \ >> + ({struct list_head *__slave_list = &(bond)->slave_list; \ >> + struct list_head __rcu *__next = list_next_rcu(__slave_list); \ >> + struct list_head *__pos_list = &(pos)->list; \ >> + struct list_head __rcu *__pos_next = list_next_rcu(__pos_list); \ >> + likely(__pos_next != __slave_list) ? \ >> + container_of(__pos_next, struct slave, list) : \ >> + container_of(__next, struct slave, list); \ >> + }) > > Nice, but can be shortened - we know that pos won't go away. OK, clean it soon. > >> + >> +#define bond_prev_slave_rcu(bond, pos) \ >> + ({struct list_head *__slave_list = &(bond)->slave_list; \ >> + struct list_head __rcu *__prev = \ >> + (*((struct list_head __rcu **)(&(__slave_list)->prev)));\ >> + struct list_head *__pos_list = &(pos)->list; \ >> + struct list_head __rcu *__pos_prev = (__pos_list->prev >> !=LIST_POISON2) ? \ yes, the pos->list will be set to LIST_POISON2 by list_del_rcu, so I add a check for it, But take the approach of eliminating bond_last/prev_slave completely is a wise decision, I agree. >> + (*((struct list_head __rcu **)(&(__pos_list)->prev))) : NULL; \ >> + likely(__pos_prev != __slave_list) ? \ >> + ((__pos_prev) ? list_entry_rcu(__pos_prev, struct slave, list) : >> NULL;) : \ >> + (list_entry_rcu(__prev, struct slave, list)); \ >> + }) > > Same remark as above about prev. > >> + >> >> >> -#define bond_for_each_slave_from(bond, pos, cnt, start) \ >> - for (cnt = 0, pos = start; pos && cnt < (bond)->slave_cnt; \ >> - cnt++, pos = bond_next_slave(bond, pos)) >> - >> +#define bond_for_each_slave_from(bond, pos, start) \ >> + for (pos = start; pos; (pos = bond_next_slave(bond, pos)) != start ? \ >> + (pos) : (pos = NULL)) >> + >> +#define bond_for_each_slave_from_rcu(bond, pos, start) \ yes, it is a little tedious. I think it could be more easier and shorter. >> + for ({struct list_head *__start = &(start)->list; \ >> + struct list_head *__slave_list = &(bond)->slave_list; \ >> + pos = list_entry_rcu(__start, struct slave, list);}; \ >> + pos; \ the only way to get out of the loop is that pos is NULL. >> + {struct list_head __rcu *__next = list_next_rcu(pos->next); \ >> + __next != __slave_list ? \ >> + __next : __next = list_next_rcu(__next->next); \ first, check whether the pos->next is the last one in the slave_list, if it does, get the first slave of the bond->slave_list. >> >> + __next != __start ? \ >> + pos = list_entry_rcu(__next, struct slave, list) : \ >> + pos = NULL; \ second, check whether the pos is reach the start, if not, continue, otherwise, the pos will be set to NULL, so break the loop. >> + }) > > Jeez, I don't even want to review it. It's too complex and too hard to > maintain, even if it works. Can you please make something > shorter/easier to > understand? > Best Regards. Ding >> + >> >> Best regards >> Ding >> >> >>>> -- >>>> To unsubscribe from this list: send the line "unsubsc ribe netdev" in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>> >>> >>> >>> . >>> >> >> > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 09, 2013 at 10:53:20PM +0800, Ding Tianhong wrote: >于 2013/9/9 17:57, Veaceslav Falico 写道: >>On Mon, Sep 09, 2013 at 04:58:29PM +0800, Ding Tianhong wrote: >>>On 2013/9/8 14:05, Ding Tianhong wrote: >>> >>>Hi Veaceslav and Nik: >>> >>>please take a moment to reveiw the function just modify for >>>bond_XXX_rcu, >>>and give me some advice. thanks for the help again.:) >>> >>>+#define bond_first_slave_rcu(bond) \ >>>+ list_first_or_null_rcu(&(bond)->slave_list, struct slave, list); >>>+#define bond_last_slave_rcu(bond) \ >>>+ ({struct list_head *__slave_list = &(bond)->slave_list; \ >>>+ struct list_head __rcu *__prev = \ >>>+ (*((struct list_head __rcu **)(&(__slave_list)->prev)));\ >>>+ likely(__slave_list != __prev) ? \ >>>+ container_of(__prev, struct slave, list) : NULL;}) >> >>Please take a look at Nikolay's reply to my RCU email - >>http://www.spinics.net/lists/netdev/msg249805.html . And mine also, >>to his >>email. In short - RCU doesn't guarantee ->prev, so better take the >>approach >>of eliminating bond_last/prev_slave completely. >> >yes, I see the message, the list_del_rcu will make the slave->list >->prev = LIST_POISON2, >the bond->slave_list will not be set to the messae, the prev will >point a slave->list or itself, >so I think it will be ok here, please correct me if I miss something. Hi Ding, Please take a look at the patchset I've just sent to net-next: [PATCH net-next 0/26] bonding: use neighbours instead of own lists It removes all the ->prev usage, and uses completely different primitives for list traversal, and - *drops* the bond->slave_list and slave->list completely. I think it would be a lot easier to RCUify bonding further providing this patchset. Thanks. > >Best Regards >Ding > >>>+ >>>#define bond_is_first_slave(bond, pos) ((pos)->list.prev == >>>&(bond)->slave_list) >>>#define bond_is_last_slave(bond, pos) ((pos)->list.next == >>>&(bond)->slave_list) >>> >>>@@ -93,6 +117,29 @@ >>>(bond_is_first_slave(bond, pos) ? bond_last_slave(bond) : \ >>>bond_to_slave((pos)->list.prev)) >>> >>>+/* Since bond_first/last_slave_rcu can return NULL, these can >>>return NULL too */ >>>+#define bond_next_slave_rcu(bond, pos) \ >>>+ ({struct list_head *__slave_list = &(bond)->slave_list; \ >>>+ struct list_head __rcu *__next = list_next_rcu(__slave_list); \ >>>+ struct list_head *__pos_list = &(pos)->list; \ >>>+ struct list_head __rcu *__pos_next = list_next_rcu(__pos_list); \ >>>+ likely(__pos_next != __slave_list) ? \ >>>+ container_of(__pos_next, struct slave, list) : \ >>>+ container_of(__next, struct slave, list); \ >>>+ }) >> >>Nice, but can be shortened - we know that pos won't go away. > >OK, clean it soon. > >> >>>+ >>>+#define bond_prev_slave_rcu(bond, pos) \ >>>+ ({struct list_head *__slave_list = &(bond)->slave_list; \ >>>+ struct list_head __rcu *__prev = \ >>>+ (*((struct list_head __rcu **)(&(__slave_list)->prev)));\ >>>+ struct list_head *__pos_list = &(pos)->list; \ >>>+ struct list_head __rcu *__pos_prev = (__pos_list->prev >>>!=LIST_POISON2) ? \ >yes, the pos->list will be set to LIST_POISON2 by list_del_rcu, so I >add a check for it, But >take the approach of eliminating bond_last/prev_slave completely is a >wise decision, I agree. > >>>+ (*((struct list_head __rcu **)(&(__pos_list)->prev))) : NULL; \ >>>+ likely(__pos_prev != __slave_list) ? \ >>>+ ((__pos_prev) ? list_entry_rcu(__pos_prev, struct slave, list) >>>: NULL;) : \ >>>+ (list_entry_rcu(__prev, struct slave, list)); \ >>>+ }) >> >>Same remark as above about prev. >> >>>+ >>> >>> >>>-#define bond_for_each_slave_from(bond, pos, cnt, start) \ >>>- for (cnt = 0, pos = start; pos && cnt < (bond)->slave_cnt; \ >>>- cnt++, pos = bond_next_slave(bond, pos)) >>>- >>>+#define bond_for_each_slave_from(bond, pos, start) \ >>>+ for (pos = start; pos; (pos = bond_next_slave(bond, pos)) != start ? \ >>>+ (pos) : (pos = NULL)) >>>+ >>>+#define bond_for_each_slave_from_rcu(bond, pos, start) \ >yes, it is a little tedious. I think it could be more easier and shorter. > >>>+ for ({struct list_head *__start = &(start)->list; \ >>>+ struct list_head *__slave_list = &(bond)->slave_list; \ >>>+ pos = list_entry_rcu(__start, struct slave, list);}; \ >>>+ pos; \ >the only way to get out of the loop is that pos is NULL. >>>+ {struct list_head __rcu *__next = list_next_rcu(pos->next); \ >>>+ __next != __slave_list ? \ >>>+ __next : __next = list_next_rcu(__next->next); \ >first, check whether the pos->next is the last one in the slave_list, >if it does, get the >first slave of the bond->slave_list. >>> >>>+ __next != __start ? \ >>>+ pos = list_entry_rcu(__next, struct slave, list) : \ >>>+ pos = NULL; \ >second, check whether the pos is reach the start, if not, continue, >otherwise, the pos >will be set to NULL, so break the loop. >>>+ }) >> >>Jeez, I don't even want to review it. It's too complex and too hard to >>maintain, even if it works. Can you please make something >>shorter/easier to >>understand? >> > >Best Regards. >Ding > >>>+ >>> >>>Best regards >>>Ding >>> >>> >>>>>-- >>>>>To unsubscribe from this list: send the line "unsubsc ribe netdev" in >>>>>the body of a message to majordomo@vger.kernel.org >>>>>More majordomo info at http://vger.kernel.org/majordomo-info.html >>>>> >>>> >>>> >>>>. >>>> >>> >>> >>-- >>To unsubscribe from this list: send the line "unsubscribe netdev" in >>the body of a message to majordomo@vger.kernel.org >>More majordomo info at http://vger.kernel.org/majordomo-info.html >> > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Sep 07, 2013 at 05:03:50PM +0200, Veaceslav Falico wrote: > On Sat, Sep 07, 2013 at 04:45:05PM +0200, Nikolay Aleksandrov wrote: > > > >On 09/07/2013 04:20 PM, Veaceslav Falico wrote: > >>On Fri, Sep 06, 2013 at 03:28:07PM +0800, Ding Tianhong wrote: > ...snip... > >>diff --git a/include/linux/rculist.h b/include/linux/rculist.h > >>index f4b1001..37b49d1 100644 > >>--- a/include/linux/rculist.h > >>+++ b/include/linux/rculist.h > >>@@ -23,6 +23,7 @@ > >> * way, we must not access it directly > >> */ > >> #define list_next_rcu(list) (*((struct list_head __rcu > >>**)(&(list)->next))) > >>+#define list_prev_rcu(list) (*((struct list_head __rcu > >>**)(&(list)->prev))) > >> > >> /* > >> * Insert a new entry between two known consecutive entries. > >>@@ -271,6 +272,12 @@ static inline void list_splice_init_rcu(struct > >>list_head *list, > >> likely(__ptr != __next) ? container_of(__next, type, member) : NULL; \ > >> }) > >> > >>+#define list_last_or_null_rcu(ptr, type, member) \ > >>+ ({struct list_head *__ptr = (ptr); \ > >>+ struct list_head __rcu *__last = list_prev_rcu(__ptr); \ > >>+ likely(__ptr != __last) ? container_of(__prev, type, member) : NULL; \ > >>+ }) > >>+ > >Hi, > >Actually I don't think you can dereference ->prev and use the standard > >list_del_rcu because it guarantees only the ->next ptr will be valid and > >->prev is set to LIST_POISON2. > >IMO, you'll need something like this: https://lkml.org/lkml/2012/7/25/193 > >with the bidir_del and all that. > > Yeah, right, my bad - we can rely only on the ->next pointer, indeed, > missed that part. RCU is hard :). > > So it'll be a lot harder to implement bond_last_slave_rcu() in a > 'straightforward' approach. > > I'd rather go in the opposite direction here - i.e. drop the 'reverse' > traversal completely, and all the use cases for bond_last_slave_rcu(). I've > got some patches already - http://patchwork.ozlabs.org/patch/272076/ doing > that, and hopefully will remove the whole 'backword' traversal completely > in the future. If it is important, it would be possible to create an RCU-protected linked list that allowed readers to go in both directions -- mostly just leave out the poisoning of the ->prev pointers. We do -not- want to do this for the normal RCU-protected linked lists because the poisoning does find bugs. However, you would have to be quite careful with a bi-directional RCU-protected linked list, because p->next->prev will not necessarily be equal to p, for all the reasons that you guys listed out earlier in this thread. So be very careful what you wish for! ;-) Thanx, Paul > >But in any case I complete agree with Veaceslav here. Read all the > >documentation carefully :-) > > > >Cheers, > >Nik > > > >> /** > >> * list_for_each_entry_rcu - iterate over rcu list of given type > >> * @pos: the type * to use as a loop cursor. > >>------- END OF PATCH ------ > >> > >>Anyway, it's up to you. > >> > >>Hope that helps. > > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/rculist.h b/include/linux/rculist.h index f4b1001..37b49d1 100644 --- a/include/linux/rculist.h +++ b/include/linux/rculist.h @@ -23,6 +23,7 @@ * way, we must not access it directly */ #define list_next_rcu(list) (*((struct list_head __rcu **)(&(list)->next))) +#define list_prev_rcu(list) (*((struct list_head __rcu **)(&(list)->prev))) /* * Insert a new entry between two known consecutive entries. @@ -271,6 +272,12 @@ static inline void list_splice_init_rcu(struct list_head *list, likely(__ptr != __next) ? container_of(__next, type, member) : NULL; \ }) +#define list_last_or_null_rcu(ptr, type, member) \ + ({struct list_head *__ptr = (ptr); \ + struct list_head __rcu *__last = list_prev_rcu(__ptr); \ + likely(__ptr != __last) ? container_of(__prev, type, member) : NULL; \ + }) + /** * list_for_each_entry_rcu - iterate over rcu list of given type * @pos: the type * to use as a loop cursor.