netdev - Re: [PATCH net-next 2/9] bonding: remove bond read lock for bond_mii

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 06 Nov 2013 15:18:26 -0800
From:	Jay Vosburgh <fubar@...ibm.com>
To:	Ding Tianhong <dingtianhong@...wei.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 2/9] bonding: remove bond read lock for bond_mii_monitor()

Ding Tianhong <dingtianhong@...wei.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@...wei.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@...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