bonding: simplify and use RCU protection for 3ad xmit path
Subsystems (3)
PATCH 1/5 bonding: simplify and use RCU protection for 3ad xmit path
The commit 278b20837511776dc9d5f6ee1c7fabd5479838bb
(bonding: initial RCU conversion) has convert the roundrobin, active-backup, broadcast and xor xmit path to rcu protection, the performance will be better for these mode, so this time, convert xmit path for 3ad mode.
Signed-off-by Ding Tianhong <[email protected]>
Signed-off-by Wang Yufen <[email protected]>
Cc Nikolay Aleksandrov <[email protected]>
Cc Veaceslav Falico <[email protected]>
--- drivers/net/bonding/bond_3ad.c | 31 +++++++++++++------------------ drivers/net/bonding/bonding.h | 22 ++++++++++++++++++++++ 2 files changed, 35 insertions(+), 18 deletions(-)
drivers/net/bonding/bond_3ad.c +13 -18
@@ -143,7 +143,7 @@ static inline struct bonding *__get_bond_by_port(struct port *port)*/ static inline struct port *__get_first_port (struct bonding *bond) { -struct slave *first_slave =bond_first_slave (bond); +struct slave *first_slave =bond_first_slave_rcu (bond);return first_slave ? &(SLAVE_AD_INFO (first_slave).port) :NULL ; }
@@ -163,7 +163,7 @@ static inline struct port *__get_next_port(struct port *port)// If there's no bond for this port, or this is the last slave if (bond ==NULL )return NULL ; - slave_next =bond_next_slave (bond, slave); + slave_next =bond_next_slave_rcu (bond, slave);if (!slave_next ||bond_is_first_slave (bond, slave_next))return NULL ;
@@ -2417,16 +2417,14 @@ int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info)int bond_3ad_xmit_xor (struct sk_buff *skb,struct net_device *dev) { -struct slave *slave, *start_at; +struct slave *slave;struct bonding *bond =netdev_priv (dev);int slave_agg_no;int slaves_in_agg;int agg_id; -int i;struct ad_info ad_info;int res =1 ; -read_lock (&bond->lock);if (__bond_3ad_get_active_agg_info (bond, &ad_info)) {pr_debug ("%s: Error: __bond_3ad_get_active_agg_info failed\n" , dev->name);
@@ -2444,13 +2442,16 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev) slave_agg_no = bond->xmit_hash_policy (skb, slaves_in_agg); -bond_for_each_slave (bond, slave) { +bond_for_each_slave_rcu (bond, slave) {struct aggregator *agg =SLAVE_AD_INFO (slave).port.aggregator;if (agg && (agg->aggregator_identifier == agg_id)) { - slave_agg_no--; -if (slave_agg_no <0 ) -break ; +if (--slave_agg_no <0 ) { +if (SLAVE_IS_OK (slave)) { + res =bond_dev_queue_xmit (bond, skb, slave->dev); + goto out; + } + } } }
@@ -2460,23 +2461,17 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev) goto out; } - start_at = slave; - -bond_for_each_slave_from (bond, slave, i, start_at) { -int slave_agg_id =0 ; +bond_for_each_slave_rcu (bond, slave) {struct aggregator *agg =SLAVE_AD_INFO (slave).port.aggregator; -if (agg) - slave_agg_id = agg->aggregator_identifier; - -if (SLAVE_IS_OK (slave) && agg && (slave_agg_id == agg_id)) { +if (SLAVE_IS_OK (slave) && agg && + (agg->aggregator_identifier == agg_id)) { res =bond_dev_queue_xmit (bond, skb, slave->dev);break ; } } out: -read_unlock (&bond->lock);if (res) {/* no suitable interface, frame not sent */ kfree_skb (skb);
drivers/net/bonding/bonding.h +22 -0
@@ -74,6 +74,9 @@/* slave list primitives */ #define bond_to_slave (ptr) list_entry(ptr, struct slave, list) +/* slave list primitives, Caller must hold rcu_read_lock */ +#define bond_to_slave_rcu (ptr) list_entry_rcu(ptr, struct slave, list) +/* IMPORTANT: bond_first/last_slave can return NULL in case of an empty list */ #define bond_first_slave (bond) \ list_first_entry_or_null(&(bond)->slave_list, struct slave, list)
@@ -81,6 +84,16 @@ (list_empty (&(bond)->slave_list) ?NULL : \bond_to_slave ((bond)->slave_list.prev)) +/** +* IMPORTANT: bond_first/last_slave_rcu can return NULL in case of an empty list +* Caller must hold rcu_read_lock + */ +#define bond_first_slave_rcu (bond) \ + list_first_or_null_rcu(&(bond)->slave_list, struct slave, list) +#define bond_last_slave_rcu (bond) \ + (list_emptry(&(bond)->slave_list) ? NULL : \ + bond_to_slave_rcu((bond)->slave_list.prev)) +#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 +106,15 @@ (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) \ + (bond_is_last_slave(bond, pos) ? bond_first_slave_rcu(bond) : \ + bond_to_slave_rcu((pos)->list.next)) + +#define bond_prev_slave_rcu (bond, pos) \ + (bond_is_first_slave(bond, pos) ? bond_last_slave_rcu(bond) : \ + bond_to_slave_rcu((pos)->list.prev)) +/** * bond_for_each_slave_from - iterate the slaves list from a starting point * @bond: the bond holding this list.
Signature · 1 line
1.8.2.1
Quoted 37 lines · Ding Tianhong
The commit
278b20837511776dc9d5f6ee1c7fabd5479838bb
(bonding: initial RCU conversion) has convert the roundrobin, active-backup, broadcast and xor xmit path to rcu protection, the performance will be better for these mode, so this time, convert xmit path for 3ad mode.Signed-off-by Ding Tianhong <[email protected]>Signed-off-by Wang Yufen <[email protected]>Cc Nikolay Aleksandrov <[email protected]>Cc Veaceslav Falico <[email protected]>--- drivers/net/bonding/bond_3ad.c | 31 +++++++++++++------------------ drivers/net/bonding/bonding.h | 22 ++++++++++++++++++++++ 2 files changed, 35 insertions(+), 18 deletions(-)drivers/net/bonding/bond_3ad.c +2
-2@@ -143,7 +143,7 @@ static inline struct bonding *__get_bond_by_port(struct port *port)*/ static inline struct port *__get_first_port (struct bonding *bond) { -struct slave *first_slave =bond_first_slave (bond); +struct slave *first_slave =bond_first_slave_rcu (bond);return first_slave ? &(SLAVE_AD_INFO (first_slave).port) :NULL ; }@@ -163,7 +163,7 @@ static inline struct port *__get_next_port(struct port *port)// If there's no bond for this port, or this is the last slave if (bond ==NULL )return NULL ; - slave_next =bond_next_slave (bond, slave); + slave_next =bond_next_slave_rcu (bond, slave);if (!slave_next ||bond_is_first_slave (bond, slave_next))return NULL ;
drivers/net/bonding/bond_3ad.c int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info) · line 2420
Quoted 5 lines
drivers/net/bonding/bond_3ad.c +1
-1@@ -2417,16 +2417,14 @@ int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info)int bond_3ad_xmit_xor (struct sk_buff *skb,struct net_device *dev) { -struct slave *slave, *start_at; +struct slave *slave;
Please re-arrange longest -> shortest where possible.
Quoted 40 lines
drivers/net/bonding/bond_3ad.c +7
-9struct bonding *bond =netdev_priv (dev);int slave_agg_no;int slaves_in_agg;int agg_id; -int i;struct ad_info ad_info;int res =1 ; -read_lock (&bond->lock);if (__bond_3ad_get_active_agg_info (bond, &ad_info)) {pr_debug ("%s: Error: __bond_3ad_get_active_agg_info failed\n" , dev->name);@@ -2444,13 +2442,16 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev) slave_agg_no = bond->xmit_hash_policy (skb, slaves_in_agg); -bond_for_each_slave (bond, slave) { +bond_for_each_slave_rcu (bond, slave) {struct aggregator *agg =SLAVE_AD_INFO (slave).port.aggregator;if (agg && (agg->aggregator_identifier == agg_id)) { - slave_agg_no--; -if (slave_agg_no <0 ) -break ; +if (--slave_agg_no <0 ) { +if (SLAVE_IS_OK (slave)) { + res =bond_dev_queue_xmit (bond, skb, slave->dev); + goto out; + } + } } }@@ -2460,23 +2461,17 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev) goto out; } - start_at = slave; - -bond_for_each_slave_from (bond, slave, i, start_at) {
drivers/net/bonding/bond_3ad.c int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev) · line 2468
Quoted 10 lines
drivers/net/bonding/bond_3ad.c +3
-5-int slave_agg_id =0 ; +bond_for_each_slave_rcu (bond, slave) {struct aggregator *agg =SLAVE_AD_INFO (slave).port.aggregator; -if (agg) - slave_agg_id = agg->aggregator_identifier; - -if (SLAVE_IS_OK (slave) && agg && (slave_agg_id == agg_id)) { +if (SLAVE_IS_OK (slave) && agg && + (agg->aggregator_identifier == agg_id)) {
Please drop the ( ) from (agg->aggregator_identifier == agg_id), also align it properly.
Quoted 25 lines
drivers/net/bonding/bond_3ad.c +0
-1res =bond_dev_queue_xmit (bond, skb, slave->dev);break ; } } out: -read_unlock (&bond->lock);if (res) {/* no suitable interface, frame not sent */ kfree_skb (skb);drivers/net/bonding/bonding.h +3
-0@@ -74,6 +74,9 @@/* slave list primitives */ #define bond_to_slave (ptr) list_entry(ptr, struct slave, list) +/* slave list primitives, Caller must hold rcu_read_lock */ +#define bond_to_slave_rcu (ptr) list_entry_rcu(ptr, struct slave, list) +/* IMPORTANT: bond_first/last_slave can return NULL in case of an empty list */ #define bond_first_slave (bond) \ list_first_entry_or_null(&(bond)->slave_list, struct slave, list)
drivers/net/bonding/bonding.h line 90
Quoted 7 lines
drivers/net/bonding/bonding.h +4
-0@@ -81,6 +84,16 @@ (list_empty (&(bond)->slave_list) ?NULL : \bond_to_slave ((bond)->slave_list.prev)) +/** +* IMPORTANT: bond_first/last_slave_rcu can return NULL in case of an empty list +* Caller must hold rcu_read_lock +*/
Styling nit: the comments in net/ should be /* text...
drivers/net/bonding/bonding.h line 94
Quoted 4 lines
drivers/net/bonding/bonding.h +4
-0+#define bond_first_slave_rcu (bond) \ + list_first_or_null_rcu(&(bond)->slave_list, struct slave, list) +#define bond_last_slave_rcu (bond) \ + (list_emptry(&(bond)->slave_list) ? NULL : \
^^^^^^^^^^^^^^^^^^^^^
Did you compile this ?
Quoted 22 lines
drivers/net/bonding/bonding.h +11
-0+ bond_to_slave_rcu((bond)->slave_list.prev)) +#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 +106,15 @@ (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) \ + (bond_is_last_slave(bond, pos) ? bond_first_slave_rcu(bond) : \ + bond_to_slave_rcu((pos)->list.next)) + +#define bond_prev_slave_rcu (bond, pos) \ + (bond_is_first_slave(bond, pos) ? bond_last_slave_rcu(bond) : \ + bond_to_slave_rcu((pos)->list.prev)) +/** * bond_for_each_slave_from - iterate the slaves list from a starting point * @bond: the bond holding this list.
Quoted 37 lines · Nikolay Aleksandrov
Quoted 36 lines · Ding Tianhong
The commit
278b20837511776dc9d5f6ee1c7fabd5479838bb
(bonding: initial RCU conversion) has convert the roundrobin, active-backup, broadcast and xor xmit path to rcu protection, the performance will be better for these mode, so this time, convert xmit path for 3ad mode.Signed-off-by Ding Tianhong <[email protected]>Signed-off-by Wang Yufen <[email protected]>Cc Nikolay Aleksandrov <[email protected]>Cc Veaceslav Falico <[email protected]>--- drivers/net/bonding/bond_3ad.c | 31 +++++++++++++------------------ drivers/net/bonding/bonding.h | 22 ++++++++++++++++++++++ 2 files changed, 35 insertions(+), 18 deletions(-)drivers/net/bonding/bond_3ad.c +2
-2@@ -143,7 +143,7 @@ static inline struct bonding *__get_bond_by_port(struct port *port)*/ static inline struct port *__get_first_port (struct bonding *bond) { -struct slave *first_slave =bond_first_slave (bond); +struct slave *first_slave =bond_first_slave_rcu (bond);return first_slave ? &(SLAVE_AD_INFO (first_slave).port) :NULL ; }@@ -163,7 +163,7 @@ static inline struct port *__get_next_port(struct port *port)// If there's no bond for this port, or this is the last slave if (bond ==NULL )return NULL ; - slave_next =bond_next_slave (bond, slave); + slave_next =bond_next_slave_rcu (bond, slave);if (!slave_next ||bond_is_first_slave (bond, slave_next))return NULL ;
Quoted 8 lines
drivers/net/bonding/bond_3ad.c int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info) · line 2420
Quoted 6 lines
drivers/net/bonding/bond_3ad.c +1
-1@@ -2417,16 +2417,14 @@ int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info)int bond_3ad_xmit_xor (struct sk_buff *skb,struct net_device *dev) { -struct slave *slave, *start_at; +struct slave *slave;Please re-arrange longest -> shortest where possible.
something wrong with my vim, so fix it, thanks.
Quoted 44 lines
Quoted 44 lines
drivers/net/bonding/bond_3ad.c +8
-10struct bonding *bond =netdev_priv (dev);int slave_agg_no;int slaves_in_agg;int agg_id; -int i;struct ad_info ad_info;int res =1 ; -read_lock (&bond->lock);if (__bond_3ad_get_active_agg_info (bond, &ad_info)) {pr_debug ("%s: Error: __bond_3ad_get_active_agg_info failed\n" , dev->name);@@ -2444,13 +2442,16 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev) slave_agg_no = bond->xmit_hash_policy (skb, slaves_in_agg); -bond_for_each_slave (bond, slave) { +bond_for_each_slave_rcu (bond, slave) {struct aggregator *agg =SLAVE_AD_INFO (slave).port.aggregator;if (agg && (agg->aggregator_identifier == agg_id)) { - slave_agg_no--; -if (slave_agg_no <0 ) -break ; +if (--slave_agg_no <0 ) { +if (SLAVE_IS_OK (slave)) { + res =bond_dev_queue_xmit (bond, skb, slave->dev); + goto out; + } + } } }@@ -2460,23 +2461,17 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev) goto out; } - start_at = slave; - -bond_for_each_slave_from (bond, slave, i, start_at) { -int slave_agg_id =0 ; +bond_for_each_slave_rcu (bond, slave) {struct aggregator *agg =SLAVE_AD_INFO (slave).port.aggregator;
Quoted 9 lines
drivers/net/bonding/bond_3ad.c int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev) · line 2468
Quoted 6 lines
drivers/net/bonding/bond_3ad.c +2
-4-if (agg) - slave_agg_id = agg->aggregator_identifier; - -if (SLAVE_IS_OK (slave) && agg && (slave_agg_id == agg_id)) { +if (SLAVE_IS_OK (slave) && agg && + (agg->aggregator_identifier == agg_id)) {Please drop the ( ) from (
agg->aggregator_identifier==agg_id), also align it properly.
my mistake, thanks.
Quoted 28 lines
Quoted 28 lines
drivers/net/bonding/bond_3ad.c +0
-1res =bond_dev_queue_xmit (bond, skb, slave->dev);break ; } } out: -read_unlock (&bond->lock);if (res) {/* no suitable interface, frame not sent */ kfree_skb (skb);drivers/net/bonding/bonding.h +3
-0@@ -74,6 +74,9 @@/* slave list primitives */ #define bond_to_slave (ptr) list_entry(ptr, struct slave, list) +/* slave list primitives, Caller must hold rcu_read_lock */ +#define bond_to_slave_rcu (ptr) list_entry_rcu(ptr, struct slave, list) +/* IMPORTANT: bond_first/last_slave can return NULL in case of an empty list */ #define bond_first_slave (bond) \ list_first_entry_or_null(&(bond)->slave_list, struct slave, list)@@ -81,6 +84,16 @@ (list_empty (&(bond)->slave_list) ?NULL : \bond_to_slave ((bond)->slave_list.prev))
Quoted 5 lines
drivers/net/bonding/bonding.h line 90
Quoted 4 lines
drivers/net/bonding/bonding.h +4
-0+/** +* IMPORTANT: bond_first/last_slave_rcu can return NULL in case of an empty list +* Caller must hold rcu_read_lock +*/ Styling nit: the comments in
net/should be /* text...
miss the net-next. add in resend.
Quoted 7 lines
drivers/net/bonding/bonding.h line 94
Quoted 5 lines
drivers/net/bonding/bonding.h +4
-0+#define bond_first_slave_rcu (bond) \ + list_first_or_null_rcu(&(bond)->slave_list, struct slave, list) +#define bond_last_slave_rcu (bond) \ + (list_emptry(&(bond)->slave_list) ? NULL : \^^^^^^^^^^^^^^^^^^^^^
Did you compile this ?
oh, I compiled, but the func did not used, so did not show any problem, it's a big error, I will review and check all above.
Quoted 27 lines
Quoted 13 lines
drivers/net/bonding/bonding.h +4
-0+ bond_to_slave_rcu((bond)->slave_list.prev)) +#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 +106,15 @@ (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) \drivers/net/bonding/bonding.h line 120
Quoted 10 lines
drivers/net/bonding/bonding.h +7
-0+ (bond_is_last_slave (bond, pos) ?bond_first_slave_rcu (bond) : \ + bond_to_slave_rcu((pos)->list.next)) + +#define bond_prev_slave_rcu (bond, pos) \ + (bond_is_first_slave(bond, pos) ? bond_last_slave_rcu(bond) : \ + bond_to_slave_rcu((pos)->list.prev)) +/** * bond_for_each_slave_from - iterate the slaves list from a starting point * @bond: the bond holding this list. .