From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id C6946A04CC; Mon, 21 Sep 2020 13:41:46 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 0CD061C190; Mon, 21 Sep 2020 13:41:46 +0200 (CEST) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 3C8AF1C134; Mon, 21 Sep 2020 13:41:44 +0200 (CEST) IronPort-SDR: BaLAt8SO6mH0shJz2pNXqQehlLy1WmPOTbUv2xamZaI2pAQiEb8/zWTWKcx+iGmjhedhXMAar5 H/kURG/hSyWg== X-IronPort-AV: E=McAfee;i="6000,8403,9750"; a="221951991" X-IronPort-AV: E=Sophos;i="5.77,286,1596524400"; d="scan'208";a="221951991" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Sep 2020 04:41:39 -0700 IronPort-SDR: yB0WFcFyvw9apy6diZs8a1SmDgHFRcvqfASHi0wDBWweo4CT3nEa5nxibozR19jMeTgPJbQhXn Ynk9thsQJFzA== X-IronPort-AV: E=Sophos;i="5.77,286,1596524400"; d="scan'208";a="308996668" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.217.64]) ([10.213.217.64]) by orsmga006-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Sep 2020 04:41:37 -0700 To: Junyu Jiang , dev@dpdk.org Cc: Jeff Guo , Beilei Xing , stable@dpdk.org References: <20200910015426.3140-1-junyux.jiang@intel.com> <20200921095901.7089-1-junyux.jiang@intel.com> From: Ferruh Yigit Message-ID: Date: Mon, 21 Sep 2020 12:41:34 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.2.2 MIME-Version: 1.0 In-Reply-To: <20200921095901.7089-1-junyux.jiang@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v3] net/i40e: fix incorrect byte counters X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 9/21/2020 10:59 AM, Junyu Jiang wrote: > This patch fixed the issue that rx/tx bytes statistics counters > overflowed on 48 bit limitation by enlarging the limitation. > > Fixes: 4861cde46116 ("i40e: new poll mode driver") > Cc: stable@dpdk.org > > Signed-off-by: Junyu Jiang > --- > doc/guides/nics/i40e.rst | 7 +++++++ > drivers/net/i40e/i40e_ethdev.c | 32 ++++++++++++++++++++++++++++++++ > drivers/net/i40e/i40e_ethdev.h | 9 +++++++++ > 3 files changed, 48 insertions(+) > > diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst > index b7430f6c4..4baa58be6 100644 > --- a/doc/guides/nics/i40e.rst > +++ b/doc/guides/nics/i40e.rst > @@ -830,3 +830,10 @@ Tx bytes affected by the link status change > > For firmware versions prior to 6.01 for X710 series and 3.33 for X722 series, the tx_bytes statistics data is affected by > the link down event. Each time the link status changes to down, the tx_bytes decreases 110 bytes. > + > +RX/TX statistics may be incorrect when register overflowed > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + > +The rx_bytes/tx_bytes statistics register is 48 bit length. Although this limitation is enlarged to 64 bit length > +on the software side, but there is no way to detect if the overflow occurred more than once. So rx_bytes/tx_bytes > +statistics data is correct when statistics are updated at least once between two overflows. > diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c > index 563f21d9d..212338ef0 100644 > --- a/drivers/net/i40e/i40e_ethdev.c > +++ b/drivers/net/i40e/i40e_ethdev.c > @@ -3052,6 +3052,19 @@ i40e_dev_link_update(struct rte_eth_dev *dev, > return ret; > } > > +static void > +i40e_stat_update_48_in_64(uint64_t *new_bytes, > + uint64_t *prev_bytes, > + bool offset_loaded) > +{ > + if (offset_loaded) { > + if (I40E_RXTX_BYTES_L_48_BIT(*prev_bytes) > *new_bytes) > + *new_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH; > + *new_bytes += I40E_RXTX_BYTES_H_16_BIT(*prev_bytes); > + } > + *prev_bytes = *new_bytes; > +} > + I was more thinking reading stats and extending in same function, instead of extracting the extending part into its own function, like: static void i40e_stat_update_48_in_64(struct i40e_hw *hw, uint32_t hireg, uint32_t loreg, bool offset_loaded, uint64_t *offset, uint64_t *stat, uint64_t *prev_bytes) { i40e_stat_update_48(...) /* logic to convert 'stat' to 64 bits */ } Does it make sense?