bonding: use RTNL instead of bond lock for bond_3ad_state_machine_handler()
Subsystems (3)
PATCH 6/9 bonding: use RTNL instead of bond lock for bond_3ad_state_machine_handler()
The bond_3ad_state_machine_handler() is a slow path, it use the bond lock to protect the bond slave list and slave port, the slave list could not be protect by bond lock anymore, so I need to use RTNL or RCU instead of bond lock, but if I remove the bond lock, the bond_3ad_state_machine_handler() may use the slave port when bond releasing the slave, it will occur problems.
As the bond_3ad_unbind_slave() only protected by bond lock or RTNL, so add RCU is not a good solution as it could not protect the slave port, so RTNL is fit here.
--- drivers/net/bonding/bond_3ad.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)
drivers/net/bonding/bond_3ad.c +7 -8
@@ -2068,18 +2068,19 @@ void bond_3ad_state_machine_handler(struct work_struct *work)struct slave *slave;struct port *port; -read_lock (&bond->lock); +if (!rtnl_trylock ()) { +queue_delayed_work (bond->wq, &bond->ad_work, ad_delta_in_ticks); +return ; + } -//check if there are any slaves if (!bond_has_slaves (bond)) goto re_arm; -// check if agg_select_timer timer after initialize is timed out +/* check if agg_select_timer timer after initialize is timed out */ if (BOND_AD_INFO (bond).agg_select_timer && !(--BOND_AD_INFO (bond).agg_select_timer)) { slave =bond_first_slave (bond); port = slave ? &(SLAVE_AD_INFO (slave).port) :NULL ; -// select the active aggregator for the bond if (port) {if (!port->slave) { pr_warning("%s: Warning: bond's first port is uninitialized\n",
@@ -2093,7 +2094,6 @@ void bond_3ad_state_machine_handler(struct work_struct *work)bond_3ad_set_carrier (bond); } -// for each port run the state machines bond_for_each_slave (bond, slave, iter) { port = &(SLAVE_AD_INFO (slave).port);if (!port->slave) {
@@ -2114,7 +2114,7 @@ void bond_3ad_state_machine_handler(struct work_struct *work)ad_mux_machine (port);ad_tx_machine (port); -// turn off the BEGIN bit, since we already handled it +/* turn off the BEGIN bit, since we already handled it */ if (port->sm_vars &AD_PORT_BEGIN ) port->sm_vars &= ~AD_PORT_BEGIN ;
@@ -2122,9 +2122,8 @@ void bond_3ad_state_machine_handler(struct work_struct *work) } re_arm: +rtnl_unlock ();queue_delayed_work (bond->wq, &bond->ad_work, ad_delta_in_ticks); - -read_unlock (&bond->lock); }/**
Signature · 1 line
1.8.2.1
Hello.
Quoted 21 lines · Ding Tianhong
The
bond_3ad_state_machine_handler()is a slow path, it use the bond lock to protect the bond slave list and slave port, the slave list could not be protect by bond lock anymore, so I need to use RTNL or RCU instead of bond lock, but if I remove the bond lock, thebond_3ad_state_machine_handler()may use the slave port when bond releasing the slave, it will occur problems.As the
bond_3ad_unbind_slave()only protected by bond lock or RTNL, so add RCU is not a good solution as it could not protect the slave port, so RTNL is fit here.Signed-off-by Ding Tianhong <[email protected]>--- drivers/net/bonding/bond_3ad.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)drivers/net/bonding/bond_3ad.c +0
-0@@ -2068,18 +2068,19 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
[...]
Quoted 23 lines
drivers/net/bonding/bond_3ad.c +1
-4-//check if there are any slaves if (!bond_has_slaves (bond)) goto re_arm; -// check if agg_select_timer timer after initialize is timed out +/* check if agg_select_timer timer after initialize is timed out */ if (BOND_AD_INFO (bond).agg_select_timer && !(--BOND_AD_INFO (bond).agg_select_timer)) { slave =bond_first_slave (bond); port = slave ? &(SLAVE_AD_INFO (slave).port) :NULL ; -// select the active aggregator for the bond if (port) {if (!port->slave) { pr_warning("%s: Warning: bond's first port is uninitialized\n",@@ -2093,7 +2094,6 @@ void bond_3ad_state_machine_handler(struct work_struct *work)bond_3ad_set_carrier (bond); } -// for each port run the state machines bond_for_each_slave (bond, slave, iter) { port = &(SLAVE_AD_INFO (slave).port);if (!port->slave) {
drivers/net/bonding/bond_3ad.c void bond_3ad_state_machine_handler(struct work_struct *work) · line 2120
Quoted 8 lines
drivers/net/bonding/bond_3ad.c +1
-1@@ -2114,7 +2114,7 @@ void bond_3ad_state_machine_handler(struct work_struct *work)ad_mux_machine (port);ad_tx_machine (port); -// turn off the BEGIN bit, since we already handled it +/* turn off the BEGIN bit, since we already handled it */ if (port->sm_vars &AD_PORT_BEGIN ) port->sm_vars &= ~AD_PORT_BEGIN ;
These comment changes are not documented in the changelog and should most
probably be in a separate patch.
WBR, Sergei
Quoted 60 lines · Sergei Shtylyov
Hello.
Quoted 21 lines · Ding Tianhong
The
bond_3ad_state_machine_handler()is a slow path, it use the bond lock to protect the bond slave list and slave port, the slave list could not be protect by bond lock anymore, so I need to use RTNL or RCU instead of bond lock, but if I remove the bond lock, thebond_3ad_state_machine_handler()may use the slave port when bond releasing the slave, it will occur problems.As the
bond_3ad_unbind_slave()only protected by bond lock or RTNL, so add RCU is not a good solution as it could not protect the slave port, so RTNL is fit here.Signed-off-by Ding Tianhong <[email protected]>--- drivers/net/bonding/bond_3ad.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)drivers/net/bonding/bond_3ad.c +0
-0@@ -2068,18 +2068,19 @@ void bond_3ad_state_machine_handler(struct work_struct *work)[...]
Quoted 23 lines
drivers/net/bonding/bond_3ad.c +1
-4-//check if there are any slaves if (!bond_has_slaves (bond)) goto re_arm; -// check if agg_select_timer timer after initialize is timed out +/* check if agg_select_timer timer after initialize is timed out */ if (BOND_AD_INFO (bond).agg_select_timer && !(--BOND_AD_INFO (bond).agg_select_timer)) { slave =bond_first_slave (bond); port = slave ? &(SLAVE_AD_INFO (slave).port) :NULL ; -// select the active aggregator for the bond if (port) {if (!port->slave) { pr_warning("%s: Warning: bond's first port is uninitialized\n",@@ -2093,7 +2094,6 @@ void bond_3ad_state_machine_handler(struct work_struct *work)bond_3ad_set_carrier (bond); } -// for each port run the state machines bond_for_each_slave (bond, slave, iter) { port = &(SLAVE_AD_INFO (slave).port);if (!port->slave) {drivers/net/bonding/bond_3ad.c void bond_3ad_state_machine_handler(struct work_struct *work) · line 2120
Quoted 8 lines
drivers/net/bonding/bond_3ad.c +1
-1@@ -2114,7 +2114,7 @@ void bond_3ad_state_machine_handler(struct work_struct *work)ad_mux_machine (port);ad_tx_machine (port); -// turn off the BEGIN bit, since we already handled it +/* turn off the BEGIN bit, since we already handled it */ if (port->sm_vars &AD_PORT_BEGIN ) port->sm_vars &= ~AD_PORT_BEGIN ;These comment changes are not documented in the changelog and should most probably be in a separate patch.
Quoted 2 lines
WBR, Sergei
thanks for your reply, I miss it in the changelog, it is already a big patch set, so I really do not want to add patch any more, I will add it in changelog.
Tnanks
Ding
Quoted 3 lines
.