[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 9 Sep 2013 11:57:52 +0200
From: Veaceslav Falico <vfalico@...hat.com>
To: Ding Tianhong <dingtianhong@...wei.com>
Cc: 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 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@...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