From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jay Vosburgh Subject: Re: [PATCH v2 net] bonding: don't use stale speed and duplex information Date: Thu, 25 Feb 2016 05:33:55 -0800 Message-ID: <11163.1456407235@nyx> References: <25869.1454962202@famine> <56CEBCC6.3040008@gmail.com> Cc: netdev@vger.kernel.org, "Tantilov, Emil S" , Veaceslav Falico , dingtianhong , Andy Gospodarek , "David S. Miller" To: zhuyj Return-path: Received: from youngberry.canonical.com ([91.189.89.112]:37462 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759644AbcBYNeZ (ORCPT ); Thu, 25 Feb 2016 08:34:25 -0500 In-reply-to: <56CEBCC6.3040008@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: zhuyj wrote: [...] >I delved into the source code and Emil's tests. I think that the problem >that this patch expects to fix occurs very unusually. > >Do you agree with me? > >If so, maybe the following patch can reduce the performance loss. >Please comment on it. Thanks a lot. > > >diff --git a/drivers/net/bonding/bond_main.c >b/drivers/net/bonding/bond_main.c >index b7f1a99..c4c511a 100644 >--- a/drivers/net/bonding/bond_main.c >+++ b/drivers/net/bonding/bond_main.c >@@ -2129,7 +2129,9 @@ static void bond_miimon_commit(struct bonding *bond) > continue; > > case BOND_LINK_UP: >- bond_update_speed_duplex(slave); >+ if (slave->speed == SPEED_UNKNOWN) >+ bond_update_speed_duplex(slave); >+ > bond_set_slave_link_state(slave, BOND_LINK_UP, >BOND_SLAVE_NOTIFY_NOW); > slave->last_link_up = jiffies; I don't believe the speed is necessarily SPEED_UNKNOWN coming in here. If the race occurs at a time later than the initial enslavement, speed may already be set (and the race manifests if the new speed changes, i.e., the link changes from 1 Gb/sec to 10 Gb/sec), so I don't think this is functionally correct. Also, the call to bond_miimon_commit itself is already gated by bond_miimon_inspect finding a link state change. The performance impact here should be minimal. -J --- -Jay Vosburgh, jay.vosburgh@canonical.com