Date:	Thu, 12 Dec 2013 16:22:34 +0800
From:	Ding Tianhong <dingtianhong@...wei.com>
To:	Jay Vosburgh <fubar@...ibm.com>
CC:	Andy Gospodarek <andy@...yhouse.net>,
	"David S. Miller" <davem@...emloft.net>,
	Nikolay Aleksandrov <nikolay@...hat.com>,
	"Veaceslav Falico" <vfalico@...hat.com>,
	Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v5 7/11] bonding: rebuild the lock use for bond_3ad_state_machine_handler()

On 2013/12/12 10:41, Jay Vosburgh wrote:
> Ding Tianhong <dingtianhong@...wei.com> wrote:
> 
>> The bond_3ad_state_machine_handler() use the bond lock to protect
>> the bond slave list and slave port, so as the before patch said,
>> I remove bond lock and replace with RCU.
>>
>> There was a lot of function need RCU protect, I have two choice
>> to make the function in RCU-safe, (1) create new similar functions
>> and make the bond slave list in RCU. (2) modify the existed functions
>> and make them in read-side critical section, because the RCU
>> read-side critical sections may be nested.
>>
>> I choose (2) because it is no need to create more similar functions.
> 
> 	A few comments:
> 
> 	First, the large number of comment-only changes make the actual
> functional changes harder to follow (plus there's one indenting mistake,
> noted in the patch, below).
> 
> 	I looked through the locking for handling port->sm_vars and
> aggregator->lag_ports after the patches are applied.
> 
> 	I don't see anything that will mutex changes to
> aggregator->lag_ports between bond_3ad_state_machine_handler and
> bond_3ad_unbind_slave.  These functions will modify ->lag_ports either
> directly (unbind only) or via ad_agg_selection_logic (both functions).
> 
> 	Even though the slave has been removed from the list by the time
> the state machine runs, it appears to be possible for both functions to
> manipulate the same aggregator->lag_ports by finding the aggregator via
> two different ports that are both members of that aggregator (i.e., port
> A of the agg is being unbound, and port B of the agg is running its
> state machine).
> 
> 	The bond_3ad_state_machine_handler calls ad_agg_selection_logic
> with either just rcu_read_lock, or rcu_read_lock plus a a port's state
> machine lock (for the case that ad_port_selection_logic calls
> ad_agg_selection_logic).
> 
> 	The bond_3ad_unbind_slave calls ad_agg_selection_logic or
> modifies lag_ports directly while holding RTNL (inherited from its
> caller) and bond->lock for write.
> 
> 	Prior to this patch, state_machine_handler additionally held
> bond->lock for read, thus bond->lock provided mutexing between the two.
>

Excellent and detailed analysis, agreed.

> 	The bond_3ad_lacpdu_recv function still (after your patches are
> applied) calls bond_3ad_rx_indication with bond->lock held for read;
> this (along with __get_state_machine_lock) protects ->sm_vars in
> ad_rx_machine.  ad_rx_machine does not modify lag_ports, only sm_vars.
> I think this is safe, particularly against race with _unbind_slave, as
> the rx handler is cleared prior to unbind for exactly this reason.
> Unless its possible for the rx_indication to already be running before
> the removal of rx_handler and still be running when _unbind_slave is
> called; I'm not sure on this one, anybody have a definitive answer?
> 

I think it will not happen, the removal of rx_handle will not happen until
the RCU read-side critical section over, just see netdev_rx_handler_unregister(),
there is synchronize_net(), and the rx_handler already in read-side critical section,
so the when _unbind_slave is called, the rx_handler is finish and clean, nothing
will happen, if I miss something, pls tell me.

> 	One place that should have a comment updated is in
> __bond_release_one:
> 
> __bond_release_one(...)
> [...]
> 	/* Inform AD package of unbinding of slave. */
> 	if (bond->params.mode == BOND_MODE_8023AD) {
> 		/* must be called before the slave is
> 		 * detached from the list
> 		 */
> 		bond_3ad_unbind_slave(slave);
> 	}
> 
> 	This comment is no longer correct, as by this point, the slave
> has already been unlinked.  
yes, the comment is too old and need to modify, I will fix it.

> This unlinking appears to protect the
> ->sm_vars of the port from being modified concurrently by unbind_slave
> and state_machine (as in the above case) because ->sm_vars changes are
> limited to the port being operated on only.  I'm not 100% sure on this,
> though, if the state_machine could race there and be operating on the
> same slave (port) at the time _unbind is also doing so.

the state_machine is in read-side critical sector, when the _unbind start,
the slave already unlink from bond, and at this time, if the state_machine
is operating, the bond will not see the slave again, so nothing bad will occur.

> 
> 	Lastly, bond_3ad_adapter_speed_changed or _duplex_changed are
> called with RTNL only, and those functions will modify port->sm_vars
> with no further locking (they are not mutexed against 3ad_state_machine
> or incoming LACPDUs, which do not hold RTNL).  This one actually looks
> like a preexisting problem, not new to this patch.
> 

good catch, it need to be fixed this time, bond_3ad_handle_link_change still
has the problem and the problem should be fix in net, not net-next, I will
send it later.

>> The nots in the function is still too old, clean up the nots.
>>
>> Suggested-by: Nikolay Aleksandrov <nikolay@...hat.com>
>> Suggested-by: Jay Vosburgh <fubar@...ibm.com>
>> Suggested-by: Veaceslav Falico <vfalico@...hat.com>
>> Signed-off-by: Ding Tianhong <dingtianhong@...wei.com>
>> ---
>> drivers/net/bonding/bond_3ad.c | 53 ++++++++++++++++++++++++------------------
>> 1 file changed, 31 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>> index 187b1b7..d935da5 100644
>> --- a/drivers/net/bonding/bond_3ad.c
>> +++ b/drivers/net/bonding/bond_3ad.c
>> @@ -147,11 +147,12 @@ static inline struct aggregator *__get_first_agg(struct port *port)
>> 	struct bonding *bond = __get_bond_by_port(port);
>> 	struct slave *first_slave;
>>
>> -	// If there's no bond for this port, or bond has no slaves
>> +	/* If there's no bond for this port, or bond has no slaves */
>> 	if (bond == NULL)
>> 		return NULL;
>> -	first_slave = bond_first_slave(bond);
>> -
>> +	rcu_read_lock();
>> +	first_slave = bond_first_slave_rcu(bond);
>> +	rcu_read_unlock();
>> 	return first_slave ? &(SLAVE_AD_INFO(first_slave).aggregator) : NULL;
>> }
>>
>> @@ -702,9 +703,13 @@ static struct aggregator *__get_active_agg(struct aggregator *aggregator)
>> 	struct list_head *iter;
>> 	struct slave *slave;
>>
>> -	bond_for_each_slave(bond, slave, iter)
>> -		if (SLAVE_AD_INFO(slave).aggregator.is_active)
>> +	rcu_read_lock();
>> +	bond_for_each_slave_rcu(bond, slave, iter)
>> +		if (SLAVE_AD_INFO(slave).aggregator.is_active) {
>> +			rcu_read_unlock();
>> 			return &(SLAVE_AD_INFO(slave).aggregator);
>> +		}
>> +	rcu_read_unlock();
>>
>> 	return NULL;
>> }
>> @@ -1471,7 +1476,8 @@ static void ad_agg_selection_logic(struct aggregator *agg)
>> 	active = __get_active_agg(agg);
>> 	best = (active && agg_device_up(active)) ? active : NULL;
>>
>> -	bond_for_each_slave(bond, slave, iter) {
>> +	rcu_read_lock();
>> +	bond_for_each_slave_rcu(bond, slave, iter) {
>> 		agg = &(SLAVE_AD_INFO(slave).aggregator);
>>
>> 		agg->is_active = 0;
>> @@ -1505,7 +1511,7 @@ static void ad_agg_selection_logic(struct aggregator *agg)
>> 		active->is_active = 1;
>> 	}
>>
>> -	// if there is new best aggregator, activate it
>> +	/* if there is new best aggregator, activate it */
>> 	if (best) {
>> 		pr_debug("best Agg=%d; P=%d; a k=%d; p k=%d; Ind=%d; Act=%d\n",
>> 			 best->aggregator_identifier, best->num_of_ports,
>> @@ -1516,7 +1522,7 @@ static void ad_agg_selection_logic(struct aggregator *agg)
>> 			 best->lag_ports, best->slave,
>> 			 best->slave ? best->slave->dev->name : "NULL");
>>
>> -		bond_for_each_slave(bond, slave, iter) {
>> +		bond_for_each_slave_rcu(bond, slave, iter) {
>> 			agg = &(SLAVE_AD_INFO(slave).aggregator);
>>
>> 			pr_debug("Agg=%d; P=%d; a k=%d; p k=%d; Ind=%d; Act=%d\n",
>> @@ -1526,10 +1532,10 @@ static void ad_agg_selection_logic(struct aggregator *agg)
>> 				 agg->is_individual, agg->is_active);
>> 		}
>>
>> -		// check if any partner replys
>> +		/* check if any partner replys */
>> 		if (best->is_individual) {
>> 			pr_warning("%s: Warning: No 802.3ad response from the link partner for any adapters in the bond\n",
>> -				   best->slave ? best->slave->bond->dev->name : "NULL");
>> +			best->slave ? best->slave->bond->dev->name : "NULL");
> 
> 	This was not re-indented properly; the start of the line
> ("best->slave") was correctly placed; if you don't want it to wrap 80
> columns, then the line needs to be split, not shifted to the left.
> 
> 	-J
> 

yes, I miss it.

Best Regards
Ding

>> 		}
>>
>> 		best->is_active = 1;
>> @@ -1541,7 +1547,7 @@ static void ad_agg_selection_logic(struct aggregator *agg)
>> 			 best->partner_oper_aggregator_key,
>> 			 best->is_individual, best->is_active);
>>
>> -		// disable the ports that were related to the former active_aggregator
>> +		/* disable the ports that were related to the former active_aggregator */
>> 		if (active) {
>> 			for (port = active->lag_ports; port;
>> 			     port = port->next_port_in_aggregator) {
>> @@ -1565,6 +1571,8 @@ static void ad_agg_selection_logic(struct aggregator *agg)
>> 		}
>> 	}
>>
>> +	rcu_read_unlock();
>> +
>> 	bond_3ad_set_carrier(bond);
>> }
>>
>> @@ -2068,18 +2076,18 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
>> 	struct slave *slave;
>> 	struct port *port;
>>
>> -	read_lock(&bond->lock);
>> +	rcu_read_lock();
>>
>> -	//check if there are any slaves
>> +	/* 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);
>> +		slave = bond_first_slave_rcu(bond);
>> 		port = slave ? &(SLAVE_AD_INFO(slave).port) : NULL;
>>
>> -		// select the active aggregator for the bond
>> +		/* select the active aggregator for the bond */
>> 		if (port) {
>> 			if (!port->slave) {
>> 				pr_warning("%s: Warning: bond's first port is uninitialized\n",
>> @@ -2093,8 +2101,8 @@ 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) {
>> +	/* for each port run the state machines */
>> +	bond_for_each_slave_rcu(bond, slave, iter) {
>> 		port = &(SLAVE_AD_INFO(slave).port);
>> 		if (!port->slave) {
>> 			pr_warning("%s: Warning: Found an uninitialized port\n",
>> @@ -2114,7 +2122,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 +2130,8 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
>> 	}
>>
>> re_arm:
>> +	rcu_read_unlock();
>> 	queue_delayed_work(bond->wq, &bond->ad_work, ad_delta_in_ticks);
>> -
>> -	read_unlock(&bond->lock);
>> }
>>
>> /**
>> @@ -2303,7 +2310,9 @@ int bond_3ad_set_carrier(struct bonding *bond)
>> 	struct aggregator *active;
>> 	struct slave *first_slave;
>>
>> -	first_slave = bond_first_slave(bond);
>> +	rcu_read_lock();
>> +	first_slave = bond_first_slave_rcu(bond);
>> +	rcu_read_unlock();
>> 	if (!first_slave)
>> 		return 0;
>> 	active = __get_active_agg(&(SLAVE_AD_INFO(first_slave).aggregator));
>> -- 
>> 1.8.2.1
> 
> ---
> 	-Jay Vosburgh, IBM Linux Technology Center, fubar@...ibm.com
> 
> 
> .
> 


--
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