[PATCH net-next v2 7/10] bonding: rebuild the lock use for bond_3ad_state_machine_handler()

[PATCH net-next v2 7/10] bonding: rebuild the lock use for bond_3ad_state_machine_handler()

[Ding Tianhong]

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.

The nots in the function is still too old, clean up the nots.

Suggested-by: Jay Vosburgh <fubar@us.ibm.com>
Suggested-by: Veaceslav Falico <vfalico@redhat.com>
Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
---
 drivers/net/bonding/bond_3ad.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 187b1b7..09edccf61 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -2068,18 +2068,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
-	if (!bond_has_slaves(bond))
+	/* check if there are any slaves */
+	if (!bond_has_slaves_rcu(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 +2093,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 +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:
+	rcu_read_unlock();
 	queue_delayed_work(bond->wq, &bond->ad_work, ad_delta_in_ticks);
-
-	read_unlock(&bond->lock);
 }
 
 /**
-- 
1.8.2.1

Re: [PATCH net-next v2 7/10] bonding: rebuild the lock use for bond_3ad_state_machine_handler()

[Nikolay Aleksandrov]

On 11/08/2013 03:08 AM, Ding Tianhong 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.
> 
> The nots in the function is still too old, clean up the nots.
> 
> Suggested-by: Jay Vosburgh <fubar@us.ibm.com>
> Suggested-by: Veaceslav Falico <vfalico@redhat.com>
> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
> ---
>  drivers/net/bonding/bond_3ad.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> index 187b1b7..09edccf61 100644
> --- a/drivers/net/bonding/bond_3ad.c
> +++ b/drivers/net/bonding/bond_3ad.c
> @@ -2068,18 +2068,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
> -	if (!bond_has_slaves(bond))
> +	/* check if there are any slaves */
> +	if (!bond_has_slaves_rcu(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",
This continues to execute:
aggregator = __get_first_agg(port);
ad_agg_selection_logic(aggregator)
And neither ad_agg_selection_logic nor __get_first_agg are RCU-safe.

> @@ -2093,8 +2093,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 +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:
> +	rcu_read_unlock();
>  	queue_delayed_work(bond->wq, &bond->ad_work, ad_delta_in_ticks);
> -
> -	read_unlock(&bond->lock);
>  }
>  
>  /**
> 

Re: [PATCH net-next v2 7/10] bonding: rebuild the lock use for bond_3ad_state_machine_handler()

[Ding Tianhong]

于 2013/11/8 23:43, Nikolay Aleksandrov 写道:
> On 11/08/2013 03:08 AM, Ding Tianhong 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.
>>
>> The nots in the function is still too old, clean up the nots.
>>
>> Suggested-by: Jay Vosburgh <fubar@us.ibm.com>
>> Suggested-by: Veaceslav Falico <vfalico@redhat.com>
>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>> ---
>>   drivers/net/bonding/bond_3ad.c | 21 ++++++++++-----------
>>   1 file changed, 10 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>> index 187b1b7..09edccf61 100644
>> --- a/drivers/net/bonding/bond_3ad.c
>> +++ b/drivers/net/bonding/bond_3ad.c
>> @@ -2068,18 +2068,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
>> -	if (!bond_has_slaves(bond))
>> +	/* check if there are any slaves */
>> +	if (!bond_has_slaves_rcu(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",
> This continues to execute:
> aggregator = __get_first_agg(port);
> ad_agg_selection_logic(aggregator)
> And neither ad_agg_selection_logic nor __get_first_agg are RCU-safe.
>
yes, I miss it, thanks a lot.

Regards.
Ding


> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>