bonding: rebuild the lock use for bond_alb_monitor()
Subsystems (3)
PATCH v2 3/10 bonding: rebuild the lock use for bond_alb_monitor()
The bond_alb_monitor use bond lock to protect the bond slave list, it is no effect here, we need to use RTNL or RCU to replace bond lock, the bond_alb_monitor will called 10 times one second, RTNL may loss performance here, so the bond lock replace with RCU to protect the bond slave list, also the RTNL is preserved, the logic of the monitor did not changed.
--- drivers/net/bonding/bond_alb.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-)
drivers/net/bonding/bond_alb.c +9 -12
@@ -816,7 +816,7 @@ static void rlb_rebalance(struct bonding *bond)for (; hash_index !=RLB_NULL_INDEX ; hash_index = client_info->used_next) { client_info = &(bond_info->rx_hashtbl[hash_index]); - assigned_slave =rlb_next_rx_slave (bond); + assigned_slave =__rlb_next_rx_slave (bond);if (assigned_slave && (client_info->slave != assigned_slave)) { client_info->slave = assigned_slave; client_info->ntt =1 ;
@@ -1495,9 +1495,10 @@ void bond_alb_monitor(struct work_struct *work)struct list_head *iter;struct slave *slave; -read_lock (&bond->lock); +rcu_read_lock (); -if (!bond_has_slaves (bond)) { +if (!bond_has_slaves_rcu (bond)) { +rcu_read_unlock (); bond_info->tx_rebalance_counter =0 ; bond_info->lp_counter =0 ; goto re_arm;
@@ -1528,7 +1529,7 @@ void bond_alb_monitor(struct work_struct *work)read_lock (&bond->curr_slave_lock); -bond_for_each_slave (bond, slave, iter) { +bond_for_each_slave_rcu (bond, slave, iter) {tlb_clear_slave (bond, slave,1 );if (slave == bond->curr_active_slave) { SLAVE_TLB_INFO(slave).load =
@@ -1552,11 +1553,9 @@ void bond_alb_monitor(struct work_struct *work)* dev_set_promiscuity requires rtnl and * nothing else. Avoid race with bond_close. */ -read_unlock (&bond->lock); -if (!rtnl_trylock ()) { -read_lock (&bond->lock); +rcu_read_unlock (); +if (!rtnl_trylock ()) goto re_arm; - } bond_info->rlb_promisc_timeout_counter =0 ;
@@ -1568,7 +1567,7 @@ void bond_alb_monitor(struct work_struct *work) bond_info->primary_is_promisc =0 ;rtnl_unlock (); -read_lock (&bond->lock); +rcu_read_lock (); }if (bond_info->rlb_rebalance) {
@@ -1590,11 +1589,9 @@ void bond_alb_monitor(struct work_struct *work) } } } - +rcu_read_unlock (); re_arm:queue_delayed_work (bond->wq, &bond->alb_work, alb_delta_in_ticks); - -read_unlock (&bond->lock); }/* assumption: called before the slave is attached to the bond
Signature · 1 line
1.8.2.1
Quoted 30 lines · Ding Tianhong
The
bond_alb_monitoruse bond lock to protect the bond slave list, it is no effect here, we need to use RTNL or RCU to replace bond lock, thebond_alb_monitorwill called 10 times one second, RTNL may loss performance here, so the bond lock replace with RCU to protect the bond slave list, also the RTNL is preserved, the logic of the monitor did not changed.Suggested-by Jay Vosburgh <[email protected]>Suggested-by Veaceslav Falico <[email protected]>Signed-off-by Ding Tianhong <[email protected]>--- drivers/net/bonding/bond_alb.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-)drivers/net/bonding/bond_alb.c +1
-1@@ -816,7 +816,7 @@ static void rlb_rebalance(struct bonding *bond)for (; hash_index !=RLB_NULL_INDEX ; hash_index = client_info->used_next) { client_info = &(bond_info->rx_hashtbl[hash_index]); - assigned_slave =rlb_next_rx_slave (bond); + assigned_slave =__rlb_next_rx_slave (bond);if (assigned_slave && (client_info->slave != assigned_slave)) { client_info->slave = assigned_slave; client_info->ntt =1 ;@@ -1495,9 +1495,10 @@ void bond_alb_monitor(struct work_struct *work)struct list_head *iter;struct slave *slave;
drivers/net/bonding/bond_alb.c void bond_alb_monitor(struct work_struct *work) · line 1504
Quoted 10 lines
drivers/net/bonding/bond_alb.c +3
-2-read_lock (&bond->lock); +rcu_read_lock (); -if (!bond_has_slaves (bond)) { +if (!bond_has_slaves_rcu (bond)) { +rcu_read_unlock (); bond_info->tx_rebalance_counter =0 ; bond_info->lp_counter =0 ; goto re_arm;
If I'm not mistaken there's one more bond_for_each_slave() inside this function which should be converted to RCU.
Quoted 46 lines
drivers/net/bonding/bond_alb.c +5
-9@@ -1528,7 +1529,7 @@ void bond_alb_monitor(struct work_struct *work)read_lock (&bond->curr_slave_lock); -bond_for_each_slave (bond, slave, iter) { +bond_for_each_slave_rcu (bond, slave, iter) {tlb_clear_slave (bond, slave,1 );if (slave == bond->curr_active_slave) { SLAVE_TLB_INFO(slave).load =@@ -1552,11 +1553,9 @@ void bond_alb_monitor(struct work_struct *work)* dev_set_promiscuity requires rtnl and * nothing else. Avoid race with bond_close. */ -read_unlock (&bond->lock); -if (!rtnl_trylock ()) { -read_lock (&bond->lock); +rcu_read_unlock (); +if (!rtnl_trylock ()) goto re_arm; - } bond_info->rlb_promisc_timeout_counter =0 ;@@ -1568,7 +1567,7 @@ void bond_alb_monitor(struct work_struct *work) bond_info->primary_is_promisc =0 ;rtnl_unlock (); -read_lock (&bond->lock); +rcu_read_lock (); }if (bond_info->rlb_rebalance) {@@ -1590,11 +1589,9 @@ void bond_alb_monitor(struct work_struct *work) } } } - +rcu_read_unlock (); re_arm:queue_delayed_work (bond->wq, &bond->alb_work, alb_delta_in_ticks); - -read_unlock (&bond->lock); }/* assumption: called before the slave is attached to the bond
Quoted 35 lines · Nikolay Aleksandrov
Quoted 34 lines · Ding Tianhong
The
bond_alb_monitoruse bond lock to protect the bond slave list, it is no effect here, we need to use RTNL or RCU to replace bond lock, thebond_alb_monitorwill called 10 times one second, RTNL may loss performance here, so the bond lock replace with RCU to protect the bond slave list, also the RTNL is preserved, the logic of the monitor did not changed.Suggested-by Jay Vosburgh <[email protected]>Suggested-by Veaceslav Falico <[email protected]>Signed-off-by Ding Tianhong <[email protected]>--- drivers/net/bonding/bond_alb.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-)drivers/net/bonding/bond_alb.c +2
-2@@ -816,7 +816,7 @@ static void rlb_rebalance(struct bonding *bond)for (; hash_index !=RLB_NULL_INDEX ; hash_index = client_info->used_next) { client_info = &(bond_info->rx_hashtbl[hash_index]); - assigned_slave =rlb_next_rx_slave (bond); + assigned_slave =__rlb_next_rx_slave (bond);if (assigned_slave && (client_info->slave != assigned_slave)) { client_info->slave = assigned_slave; client_info->ntt =1 ;@@ -1495,9 +1495,10 @@ void bond_alb_monitor(struct work_struct *work)struct list_head *iter;struct slave *slave; -read_lock (&bond->lock); +rcu_read_lock ();
Quoted 8 lines
drivers/net/bonding/bond_alb.c void bond_alb_monitor(struct work_struct *work) · line 1504
Quoted 6 lines
drivers/net/bonding/bond_alb.c +2
-1-if (!bond_has_slaves (bond)) { +if (!bond_has_slaves_rcu (bond)) { +rcu_read_unlock (); bond_info->tx_rebalance_counter =0 ; bond_info->lp_counter =0 ; goto re_arm;If I'm not mistaken there's one more
bond_for_each_slave()inside this function which should be converted to RCU.
But I really could not find any place should converted to RCU,
__rlb_next_rx_slave() is in RCU yet.
pls remind me if I miss something.
Regards
Ding
Quoted 51 lines
Quoted 36 lines
drivers/net/bonding/bond_alb.c +4
-6@@ -1528,7 +1529,7 @@ void bond_alb_monitor(struct work_struct *work)read_lock (&bond->curr_slave_lock); -bond_for_each_slave (bond, slave, iter) { +bond_for_each_slave_rcu (bond, slave, iter) {tlb_clear_slave (bond, slave,1 );if (slave == bond->curr_active_slave) { SLAVE_TLB_INFO(slave).load =@@ -1552,11 +1553,9 @@ void bond_alb_monitor(struct work_struct *work)* dev_set_promiscuity requires rtnl and * nothing else. Avoid race with bond_close. */ -read_unlock (&bond->lock); -if (!rtnl_trylock ()) { -read_lock (&bond->lock); +rcu_read_unlock (); +if (!rtnl_trylock ()) goto re_arm; - } bond_info->rlb_promisc_timeout_counter =0 ;@@ -1568,7 +1567,7 @@ void bond_alb_monitor(struct work_struct *work) bond_info->primary_is_promisc =0 ;rtnl_unlock (); -read_lock (&bond->lock); +rcu_read_lock (); }if (bond_info->rlb_rebalance) {@@ -1590,11 +1589,9 @@ void bond_alb_monitor(struct work_struct *work) } }drivers/net/bonding/bond_alb.c void bond_alb_monitor(struct work_struct *work) · line 1597
Quoted 10 lines
drivers/net/bonding/bond_alb.c +1
-3} - +rcu_read_unlock (); re_arm:queue_delayed_work (bond->wq, &bond->alb_work, alb_delta_in_ticks); - -read_unlock (&bond->lock); }/* assumption: called before the slave is attached to the bond --
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 45 lines · Ding Tianhong
Quoted 35 lines · Nikolay Aleksandrov
Quoted 34 lines · Ding Tianhong
The
bond_alb_monitoruse bond lock to protect the bond slave list, it is no effect here, we need to use RTNL or RCU to replace bond lock, thebond_alb_monitorwill called 10 times one second, RTNL may loss performance here, so the bond lock replace with RCU to protect the bond slave list, also the RTNL is preserved, the logic of the monitor did not changed.Suggested-by Jay Vosburgh <[email protected]>Suggested-by Veaceslav Falico <[email protected]>Signed-off-by Ding Tianhong <[email protected]>--- drivers/net/bonding/bond_alb.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-)drivers/net/bonding/bond_alb.c +2
-1@@ -816,7 +816,7 @@ static void rlb_rebalance(struct bonding *bond)for (; hash_index !=RLB_NULL_INDEX ; hash_index = client_info->used_next) { client_info = &(bond_info->rx_hashtbl[hash_index]); - assigned_slave =rlb_next_rx_slave (bond); + assigned_slave =__rlb_next_rx_slave (bond);if (assigned_slave && (client_info->slave != assigned_slave)) { client_info->slave = assigned_slave; client_info->ntt =1 ;@@ -1495,9 +1495,10 @@ void bond_alb_monitor(struct work_struct *work)struct list_head *iter;struct slave *slave; -read_lock (&bond->lock); +rcu_read_lock (); - if (!bond_has_slaves(bond)) {Quoted 8 lines
drivers/net/bonding/bond_alb.c void bond_alb_monitor(struct work_struct *work) · line 1504
Quoted 5 lines
drivers/net/bonding/bond_alb.c +2
-0+if (!bond_has_slaves_rcu (bond)) { +rcu_read_unlock (); bond_info->tx_rebalance_counter =0 ; bond_info->lp_counter =0 ; goto re_arm;If I'm not mistaken there's one more
bond_for_each_slave()inside this function
which should be converted to RCU.
Quoted 10 lines
But I really could not find any place should converted to RCU,
__rlb_next_rx_slave()is in RCU yet.pls remind me if I miss something.
Regards
Ding
I was talking about this piece of code inside bond_alb_monitor():
/* send learning packets */
if (bond_info->lp_counter >= BOND_ALB_LP_TICKS(bond)) {
/* change of curr_active_slave involves swapping of macaddresses.
* in order to avoid this swapping from happening while
* sending the learning packets, the curr_slave_lock must
be held for
* read.
*/
read_lock(&bond->curr_slave_lock);
bond_for_each_slave(bond, slave, iter)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
alb_send_learning_packets(slave, slave->dev->dev_addr);
read_unlock(&bond->curr_slave_lock);
bond_info->lp_counter = 0;
}This is copied after your patch-set was applied.
Cheers,
Nik
Quoted 49 lines
Quoted 44 lines
Quoted 39 lines
drivers/net/bonding/bond_alb.c +3
-4@@ -1528,7 +1529,7 @@ void bond_alb_monitor(struct work_struct *work)read_lock (&bond->curr_slave_lock); - bond_for_each_slave(bond, slave, iter) { +bond_for_each_slave_rcu (bond, slave, iter) {tlb_clear_slave (bond, slave,1 );if (slave == bond->curr_active_slave) { SLAVE_TLB_INFO(slave).load =@@ -1552,11 +1553,9 @@ void bond_alb_monitor(struct work_struct *work)* dev_set_promiscuity requires rtnl and * nothing else. Avoid race with bond_close. */ -read_unlock (&bond->lock); -if (!rtnl_trylock ()) { -read_lock (&bond->lock); +rcu_read_unlock (); +if (!rtnl_trylock ()) goto re_arm; - } bond_info->rlb_promisc_timeout_counter =0 ; @@ -1568,7 +1567,7 @@ void bond_alb_monitor(struct work_struct *work) bond_info->primary_is_promisc = 0;rtnl_unlock();drivers/net/bonding/bond_alb.c +2
-4-read_lock (&bond->lock); +rcu_read_lock (); }if (bond_info->rlb_rebalance) {@@ -1590,11 +1589,9 @@ void bond_alb_monitor(struct work_struct *work) } } } - +rcu_read_unlock (); re_arm:queue_delayed_work (bond->wq, &bond->alb_work, alb_delta_in_ticks); - -read_unlock (&bond->lock); }/* assumption: called before the slave is attached to the bond Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.htmlSignature · 3 lines
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 76 lines · Nikolay Aleksandrov
Quoted 44 lines · Ding Tianhong
Quoted 35 lines · Nikolay Aleksandrov
Quoted 34 lines · Ding Tianhong
The
bond_alb_monitoruse bond lock to protect the bond slave list, it is no effect here, we need to use RTNL or RCU to replace bond lock, thebond_alb_monitorwill called 10 times one second, RTNL may loss performance here, so the bond lock replace with RCU to protect the bond slave list, also the RTNL is preserved, the logic of the monitor did not changed.Suggested-by Jay Vosburgh <[email protected]>Suggested-by Veaceslav Falico <[email protected]>Signed-off-by Ding Tianhong <[email protected]>--- drivers/net/bonding/bond_alb.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-)drivers/net/bonding/bond_alb.c +2
-1@@ -816,7 +816,7 @@ static void rlb_rebalance(struct bonding *bond)for (; hash_index !=RLB_NULL_INDEX ; hash_index = client_info->used_next) { client_info = &(bond_info->rx_hashtbl[hash_index]); - assigned_slave =rlb_next_rx_slave (bond); + assigned_slave =__rlb_next_rx_slave (bond);if (assigned_slave && (client_info->slave != assigned_slave)) { client_info->slave = assigned_slave; client_info->ntt =1 ;@@ -1495,9 +1495,10 @@ void bond_alb_monitor(struct work_struct *work)struct list_head *iter;struct slave *slave; -read_lock (&bond->lock); +rcu_read_lock (); - if (!bond_has_slaves(bond)) {Quoted 8 lines
drivers/net/bonding/bond_alb.c void bond_alb_monitor(struct work_struct *work) · line 1504
Quoted 5 lines
drivers/net/bonding/bond_alb.c +2
-0+if (!bond_has_slaves_rcu (bond)) { +rcu_read_unlock (); bond_info->tx_rebalance_counter =0 ; bond_info->lp_counter =0 ; goto re_arm;If I'm not mistaken there's one more
bond_for_each_slave()inside this function
which should be converted to RCU.Quoted 10 lines
But I really could not find any place should converted to RCU,
__rlb_next_rx_slave()is in RCU yet.pls remind me if I miss something.
Regards
DingI was talking about this piece of code inside
bond_alb_monitor():/* send learning packets */ if (bond_info->lp_counter >= BOND_ALB_LP_TICKS(bond)) { /* change of curr_active_slave involves swapping of macaddresses.
* in order to avoid this swapping from happening while
* sending the learning packets, thecurr_slave_lockmust
be held for
* read.
*/
read_lock(&bond->curr_slave_lock);
bond_for_each_slave(bond, slave, iter)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^alb_send_learning_packets(slave, slave->dev->dev_addr); read_unlock(&bond->curr_slave_lock); bond_info->lp_counter = 0; }
Quoted 4 lines
This is copied after your patch-set was applied.
Cheers,
Nik
oh, yes, thanks, I take the focus on the wrong place.
Regards
Ding