| 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
| ||
|
Date: Thu, 7 Nov 2013 09:38:36 +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 2/9] bonding: remove bond read lock for bond_mii_monitor()
On 2013/11/7 7:18, Jay Vosburgh wrote:
> 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
>
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@...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
Powered by blists - more mailing lists