From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Hemminger Date: Tue, 25 Apr 2017 10:54:05 -0700 Subject: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return uninitialized stats In-Reply-To: <1493111272.68612.2.camel@intel.com> References: <20170421212012.25950-1-bpoirier@suse.com> <630A6B92B7EDEB45A87E20D3D286660171182EED@hasmsx109.ger.corp.intel.com> <20170424191026.wpnpyhdaurfjixl7@f1.synalogic.ca> <309B89C4C689E141A5FF6A0C5FB2118B8C5EF4F7@ORSMSX101.amr.corp.intel.com> <1493111272.68612.2.camel@intel.com> Message-ID: <20170425105405.01541742@xeon-e3> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: intel-wired-lan@osuosl.org List-ID: On Tue, 25 Apr 2017 02:07:52 -0700 Jeff Kirsher wrote: > On Tue, 2017-04-25 at 07:10 +0000, Brown, Aaron F wrote: > > > From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl. > > > org] On > > > Behalf Of Benjamin Poirier > > > Sent: Monday, April 24, 2017 12:10 PM > > > To: Neftin, Sasha > > > Cc: Kirsher at f1.synalogic.ca; Stefan Priebe ; > > > netdev at vger.kernel.org; intel-wired-lan at lists.osuosl.org > > > Subject: Re: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return > > > uninitialized > > > stats > > > > > > Sasha, please use reply-all to keep everyone in cc (including > > > me...). > > > > > > On 2017/04/24 11:17, Neftin, Sasha wrote: > > > > On 4/23/2017 15:53, Neftin, Sasha wrote: > > > > > -----Original Message----- > > > > > From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osu > > > > > osl.org] > > > > > > On Behalf Of Benjamin Poirier > > > > > Sent: Saturday, April 22, 2017 00:20 > > > > > To: Kirsher, Jeffrey T > > > > > Cc: netdev at vger.kernel.org; intel-wired-lan at lists.osuosl.org; > > > > > Stefan > > > > > > Priebe > > > > > Subject: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return > > > > > uninitialized > > > > > > stats > > > > > > > > > > Some statistics passed to ethtool are garbage because > > > > > > e1000e_get_stats64() doesn't write them, for example: > > > tx_heartbeat_errors. > > > This leaks kernel memory to userspace and confuses users. > > > > > > > > > > Do like ixgbe and use dev_get_stats() which first zeroes out > > > > > > rtnl_link_stats64. > > > > > > > > > > Reported-by: Stefan Priebe > > > > > Signed-off-by: Benjamin Poirier > > > > > --- > > > > > ?? drivers/net/ethernet/intel/e1000e/ethtool.c | 2 +- > > > > > ?? 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c > > > > > > b/drivers/net/ethernet/intel/e1000e/ethtool.c > > > > > index 7aff68a4a4df..f117b90cdc2f 100644 > > > > > --- a/drivers/net/ethernet/intel/e1000e/ethtool.c > > > > > +++ b/drivers/net/ethernet/intel/e1000e/ethtool.c > > > > > @@ -2063,7 +2063,7 @@ static void > > > > > e1000_get_ethtool_stats(struct > > > > > > net_device *netdev, > > > > > ???????????pm_runtime_get_sync(netdev->dev.parent); > > > > > -?e1000e_get_stats64(netdev, &net_stats); > > > > > +?dev_get_stats(netdev, &net_stats); > > > > > ???????????pm_runtime_put_sync(netdev->dev.parent); > > > > > -- > > > > > 2.12.2 > > > > > > > > > > _______________________________________________ > > > > > Intel-wired-lan mailing list > > > > > Intel-wired-lan at lists.osuosl.org > > > > > http://lists.osuosl.org/mailman/listinfo/intel-wired-lan > > > > > > > > Hello, > > > > > > > > We would like to not accept this patch. Suggested generic method > > > > '*dev_get_stats' (net/core/dev.c) calls 'ops->ndo_get_stats64' > > > > method > > > > > > which > > > > eventually calls e1000e_get_stats64 (netdev.c) - so there is same > > > > functionality. Also, see that 'e1000e_get_stats64' method in > > > > netdev.c (line > > > > > > No, it's not the same functionality because dev_get_stats() does a > > > memset on the rtnl_link_stats64 struct. > > > > > > > 5928) calls 'memset' with 0's before update statistics.? Local > > > > sanity check > > > > > > I don't see any memset in e1000e_get_stats64(). What kernel version > > > are > > > you looking at? > > > > The call to memset was removed from the upstream kernel with: > > ------------------------------------------------------------------- > > ----------------- > > commit 5944701df90d9577658e2354cc27c4ceaeca30fe > > Author: stephen hemminger > > Date:?? Fri Jan 6 19:12:53 2017 -0800 > > > > ??? net: remove useless memset's in drivers get_stats64 > > > > ??? In dev_get_stats() the statistic structure storage has already > > been > > ??? zeroed. Therefore network drivers do not need to call memset() > > again. > > ... > > < changes to other drivers snipped out > > > ... > > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c > > b/drivers/net/ethernet/int > > index 723025b..79651eb 100644 > > --- a/drivers/net/ethernet/intel/e1000e/netdev.c > > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c > > @@ -5925,7 +5925,6 @@ void e1000e_get_stats64(struct net_device > > *netdev, > > ?{ > > ??????? struct e1000_adapter *adapter = netdev_priv(netdev); > > > > -?????? memset(stats, 0, sizeof(struct rtnl_link_stats64)); > > ??????? spin_lock(&adapter->stats64_lock); > > ??????? e1000e_update_stats(adapter); > > ??????? /* Fill out the OS statistics structure */ > > ------------------------------------------------------------------- > > ----------------- > > > > This also is where the bad counters start to show up for e1000e for > > my test systems.? From this driver on I see (very) large values for > > tx_dropped, rx_over_errors and tx_fifo_errors on driver load (even > > before bringing the interface up.? It seems the memset is not so > > useless for this driver after all.? Would simply reverting the e1000e > > portion of this patch resolve the issue? > > Looks like Aaron beat me to the punch on pointing out that we had this > very code in there before. It appears that Stephen's > assertion/assumption was incorrect about the stats structure being > zero'd out, which is why we are seeing the issue. > > I have no issue reverting Stephen's earlier patch, or do we want to > pursue why the stats structure is not zero'd out and resolve that > instead. Either way, just want to make sure we are all on the same > page as to the right solution so that we do not end up repeating this > in the future. Lets's fix this in the base code. From: Stephen Hemminger Date: Tue, 25 Apr 2017 10:50:19 -0700 Subject: [PATCH net] net: always zero statistics Drivers with 32 bit statistics API also should get zeroed statistics. Fixes: 5944701df90d ("net: remove useless memset's in drivers get_stats64") Signed-off-by: Stephen Hemminger --- net/core/dev.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/net/core/dev.c b/net/core/dev.c index 533a6d6f6092..07d27d3feb37 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -7607,14 +7607,15 @@ struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev, { const struct net_device_ops *ops = dev->netdev_ops; - if (ops->ndo_get_stats64) { - memset(storage, 0, sizeof(*storage)); + memset(storage, 0, sizeof(*storage)); + + if (ops->ndo_get_stats64) ops->ndo_get_stats64(dev, storage); - } else if (ops->ndo_get_stats) { + else if (ops->ndo_get_stats) netdev_stats_to_stats64(storage, ops->ndo_get_stats(dev)); - } else { + else netdev_stats_to_stats64(storage, &dev->stats); - } + storage->rx_dropped += atomic_long_read(&dev->rx_dropped); storage->tx_dropped += atomic_long_read(&dev->tx_dropped); storage->rx_nohandler += atomic_long_read(&dev->rx_nohandler); -- 2.11.0 -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 833 bytes Desc: OpenPGP digital signature URL: