Date:	Mon, 9 Sep 2013 22:17:42 +0200
From:	Veaceslav Falico <vfalico@...hat.com>
To:	Ding Tianhong <dthxman@...il.com>
Cc:	Ding Tianhong <dingtianhong@...wei.com>,
	Nikolay Aleksandrov <nikolay@...hat.com>,
	"David S. Miller" <davem@...emloft.net>,
	Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v4 1/6] bonding: simplify and use RCU protection
 for 3ad xmit path

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@...r.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@...r.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists