[PATCH net-next 2/9] bonding: remove bond read lock for bond_mii_monitor()

[PATCH net-next 2/9] bonding: remove bond read lock for bond_mii_monitor()

[Ding Tianhong]

The bond_mii_monitor() still use bond lock read to protect
bond_has_slaves() and bond_miimon_inspect(), it is no effect,
so I move the RTNL to the top of the function to protect the
whole monitor, of course, if the bond state did not changed,
the monitor still calling RTNL, it may bring more performance
loss, but as a slow path, it is negligible.

also in bond_miimon_commit(), I remove the unwanted curr_slave_lock
when calling the bond_select_active_slave(), the RTNL is enough.

Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
---
 drivers/net/bonding/bond_main.c | 44 ++++++++++++-----------------------------
 1 file changed, 13 insertions(+), 31 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 9c9803c..98171eb 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2074,9 +2074,7 @@ static void bond_miimon_commit(struct bonding *bond)
 do_failover:
 		ASSERT_RTNL();
 		block_netpoll_tx();
-		write_lock_bh(&bond->curr_slave_lock);
 		bond_select_active_slave(bond);
-		write_unlock_bh(&bond->curr_slave_lock);
 		unblock_netpoll_tx();
 	}
 
@@ -2098,47 +2096,31 @@ void bond_mii_monitor(struct work_struct *work)
 	bool should_notify_peers = false;
 	unsigned long delay;
 
-	read_lock(&bond->lock);
-
 	delay = msecs_to_jiffies(bond->params.miimon);
 
-	if (!bond_has_slaves(bond))
+	if (!rtnl_trylock()) {
+		delay = 1;
 		goto re_arm;
+	}
 
-	should_notify_peers = bond_should_notify_peers(bond);
-
-	if (bond_miimon_inspect(bond)) {
-		read_unlock(&bond->lock);
-
-		/* Race avoidance with bond_close cancel of workqueue */
-		if (!rtnl_trylock()) {
-			read_lock(&bond->lock);
-			delay = 1;
-			should_notify_peers = false;
-			goto re_arm;
-		}
+	if (!bond_has_slaves(bond)) {
+		rtnl_unlock();
+		goto re_arm;
+	}
 
-		read_lock(&bond->lock);
+	should_notify_peers = bond_should_notify_peers(bond);
 
+	if (bond_miimon_inspect(bond))
 		bond_miimon_commit(bond);
 
-		read_unlock(&bond->lock);
-		rtnl_unlock();	/* might sleep, hold no other locks */
-		read_lock(&bond->lock);
-	}
+	if (should_notify_peers)
+		call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, bond->dev);
+
+	rtnl_unlock();
 
 re_arm:
 	if (bond->params.miimon)
 		queue_delayed_work(bond->wq, &bond->mii_work, delay);
-
-	read_unlock(&bond->lock);
-
-	if (should_notify_peers) {
-		if (!rtnl_trylock())
-			return;
-		call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, bond->dev);
-		rtnl_unlock();
-	}
 }
 
 static bool bond_has_this_ip(struct bonding *bond, __be32 ip)
-- 
1.8.2.1

Re: [PATCH net-next 2/9] bonding: remove bond read lock for bond_mii_monitor()

[Jay Vosburgh]

Ding Tianhong <dingtianhong@huawei.com> wrote:

>The bond_mii_monitor() still use bond lock read to protect
>bond_has_slaves() and bond_miimon_inspect(), it is no effect,
>so I move the RTNL to the top of the function to protect the
>whole monitor, of course, if the bond state did not changed,
>the monitor still calling RTNL, it may bring more performance
>loss, but as a slow path, it is negligible.

	I'm not sure this last part is true (about it being ok to
acquire RTNL every pass).  The reason the bond_miimon_* functions are
arranged in the way they are is specifically to avoid taking RTNL
unnecessarily.  A common setting is miimon=100, which will acquire and
release RTNL ten times per second.

	The inspect function can be make RCU safe, and then the function
will operate pretty much as it does now (with the multiple phases).  If
a slave disappears between the phases, that's ok; one extra cycle on
RTNL isn't a big deal, but 10 per second arguably is.

	My comment also applies to the later patches in the series that
make similar "always acquire RTNL" changes to the ARP monitor, ALB
monitoring function, and the 802.3ad state machine.  That would be
patches:

Subject: [PATCH net-next 3/9] bonding: rebuild the lock use for
 bond_alb_monitor()
Subject: [PATCH net-next 4/9] bonding: rebuild the lock use for
 bond_loadbalance_arp_mon()
Subject: [PATCH net-next 5/9] bonding: rebuild the lock use for
 bond_activebackup_arp_mon()
Subject: [PATCH net-next 6/9] bonding: use RTNL instead of bond lock for
 bond_3ad_state_machine_handler()

	The 802.3ad state machine patch or the balance-alb patch,
combined with the miimon patch, will acquire and release the RTNL lock
20 times per second (10 for the 3ad state machine or alb monitor, and 10
more for a typical miimon configuration).  I don't believe this is a
reasonable implementation.

	-J

>also in bond_miimon_commit(), I remove the unwanted curr_slave_lock
>when calling the bond_select_active_slave(), the RTNL is enough.
>
>Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>---
> drivers/net/bonding/bond_main.c | 44 ++++++++++++-----------------------------
> 1 file changed, 13 insertions(+), 31 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 9c9803c..98171eb 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -2074,9 +2074,7 @@ static void bond_miimon_commit(struct bonding *bond)
> do_failover:
> 		ASSERT_RTNL();
> 		block_netpoll_tx();
>-		write_lock_bh(&bond->curr_slave_lock);
> 		bond_select_active_slave(bond);
>-		write_unlock_bh(&bond->curr_slave_lock);
> 		unblock_netpoll_tx();
> 	}
>
>@@ -2098,47 +2096,31 @@ void bond_mii_monitor(struct work_struct *work)
> 	bool should_notify_peers = false;
> 	unsigned long delay;
>
>-	read_lock(&bond->lock);
>-
> 	delay = msecs_to_jiffies(bond->params.miimon);
>
>-	if (!bond_has_slaves(bond))
>+	if (!rtnl_trylock()) {
>+		delay = 1;
> 		goto re_arm;
>+	}
>
>-	should_notify_peers = bond_should_notify_peers(bond);
>-
>-	if (bond_miimon_inspect(bond)) {
>-		read_unlock(&bond->lock);
>-
>-		/* Race avoidance with bond_close cancel of workqueue */
>-		if (!rtnl_trylock()) {
>-			read_lock(&bond->lock);
>-			delay = 1;
>-			should_notify_peers = false;
>-			goto re_arm;
>-		}
>+	if (!bond_has_slaves(bond)) {
>+		rtnl_unlock();
>+		goto re_arm;
>+	}
>
>-		read_lock(&bond->lock);
>+	should_notify_peers = bond_should_notify_peers(bond);
>
>+	if (bond_miimon_inspect(bond))
> 		bond_miimon_commit(bond);
>
>-		read_unlock(&bond->lock);
>-		rtnl_unlock();	/* might sleep, hold no other locks */
>-		read_lock(&bond->lock);
>-	}
>+	if (should_notify_peers)
>+		call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, bond->dev);
>+
>+	rtnl_unlock();
>
> re_arm:
> 	if (bond->params.miimon)
> 		queue_delayed_work(bond->wq, &bond->mii_work, delay);
>-
>-	read_unlock(&bond->lock);
>-
>-	if (should_notify_peers) {
>-		if (!rtnl_trylock())
>-			return;
>-		call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, bond->dev);
>-		rtnl_unlock();
>-	}
> }
>
> static bool bond_has_this_ip(struct bonding *bond, __be32 ip)
>-- 
>1.8.2.1

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH net-next 2/9] bonding: remove bond read lock for bond_mii_monitor()

[Ding Tianhong]

On 2013/11/7 7:18, Jay Vosburgh wrote:
> Ding Tianhong <dingtianhong@huawei.com> wrote:
> 
>> The bond_mii_monitor() still use bond lock read to protect
>> bond_has_slaves() and bond_miimon_inspect(), it is no effect,
>> so I move the RTNL to the top of the function to protect the
>> whole monitor, of course, if the bond state did not changed,
>> the monitor still calling RTNL, it may bring more performance
>> loss, but as a slow path, it is negligible.
> 
> 	I'm not sure this last part is true (about it being ok to
> acquire RTNL every pass).  The reason the bond_miimon_* functions are
> arranged in the way they are is specifically to avoid taking RTNL
> unnecessarily.  A common setting is miimon=100, which will acquire and
> release RTNL ten times per second.
> 
> 	The inspect function can be make RCU safe, and then the function
> will operate pretty much as it does now (with the multiple phases).  If
> a slave disappears between the phases, that's ok; one extra cycle on
> RTNL isn't a big deal, but 10 per second arguably is.
> 
> 	My comment also applies to the later patches in the series that
> make similar "always acquire RTNL" changes to the ARP monitor, ALB
> monitoring function, and the 802.3ad state machine.  That would be
> patches:
> 
> Subject: [PATCH net-next 3/9] bonding: rebuild the lock use for
>  bond_alb_monitor()
> Subject: [PATCH net-next 4/9] bonding: rebuild the lock use for
>  bond_loadbalance_arp_mon()
> Subject: [PATCH net-next 5/9] bonding: rebuild the lock use for
>  bond_activebackup_arp_mon()
> Subject: [PATCH net-next 6/9] bonding: use RTNL instead of bond lock for
>  bond_3ad_state_machine_handler()
> 
> 	The 802.3ad state machine patch or the balance-alb patch,
> combined with the miimon patch, will acquire and release the RTNL lock
> 20 times per second (10 for the 3ad state machine or alb monitor, and 10
> more for a typical miimon configuration).  I don't believe this is a
> reasonable implementation.
> 
> 	-J
> 

Thansk for your reply. your opinion is reasonable and clear, I will
optimization the lock for these patch, the RTNL should be replace by 
RCU in some place.

for better performance :)

Ding

>> also in bond_miimon_commit(), I remove the unwanted curr_slave_lock
>> when calling the bond_select_active_slave(), the RTNL is enough.
>>
>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>> ---
>> drivers/net/bonding/bond_main.c | 44 ++++++++++++-----------------------------
>> 1 file changed, 13 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 9c9803c..98171eb 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -2074,9 +2074,7 @@ static void bond_miimon_commit(struct bonding *bond)
>> do_failover:
>> 		ASSERT_RTNL();
>> 		block_netpoll_tx();
>> -		write_lock_bh(&bond->curr_slave_lock);
>> 		bond_select_active_slave(bond);
>> -		write_unlock_bh(&bond->curr_slave_lock);
>> 		unblock_netpoll_tx();
>> 	}
>>
>> @@ -2098,47 +2096,31 @@ void bond_mii_monitor(struct work_struct *work)
>> 	bool should_notify_peers = false;
>> 	unsigned long delay;
>>
>> -	read_lock(&bond->lock);
>> -
>> 	delay = msecs_to_jiffies(bond->params.miimon);
>>
>> -	if (!bond_has_slaves(bond))
>> +	if (!rtnl_trylock()) {
>> +		delay = 1;
>> 		goto re_arm;
>> +	}
>>
>> -	should_notify_peers = bond_should_notify_peers(bond);
>> -
>> -	if (bond_miimon_inspect(bond)) {
>> -		read_unlock(&bond->lock);
>> -
>> -		/* Race avoidance with bond_close cancel of workqueue */
>> -		if (!rtnl_trylock()) {
>> -			read_lock(&bond->lock);
>> -			delay = 1;
>> -			should_notify_peers = false;
>> -			goto re_arm;
>> -		}
>> +	if (!bond_has_slaves(bond)) {
>> +		rtnl_unlock();
>> +		goto re_arm;
>> +	}
>>
>> -		read_lock(&bond->lock);
>> +	should_notify_peers = bond_should_notify_peers(bond);
>>
>> +	if (bond_miimon_inspect(bond))
>> 		bond_miimon_commit(bond);
>>
>> -		read_unlock(&bond->lock);
>> -		rtnl_unlock();	/* might sleep, hold no other locks */
>> -		read_lock(&bond->lock);
>> -	}
>> +	if (should_notify_peers)
>> +		call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, bond->dev);
>> +
>> +	rtnl_unlock();
>>
>> re_arm:
>> 	if (bond->params.miimon)
>> 		queue_delayed_work(bond->wq, &bond->mii_work, delay);
>> -
>> -	read_unlock(&bond->lock);
>> -
>> -	if (should_notify_peers) {
>> -		if (!rtnl_trylock())
>> -			return;
>> -		call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, bond->dev);
>> -		rtnl_unlock();
>> -	}
>> }
>>
>> static bool bond_has_this_ip(struct bonding *bond, __be32 ip)
>> -- 
>> 1.8.2.1
> 
> ---
> 	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
> 
> 
> .
>