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 874FEA04C9; Wed, 16 Sep 2020 14:30:40 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 34A111C294; Wed, 16 Sep 2020 14:30:39 +0200 (CEST) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id 179231C239; Wed, 16 Sep 2020 14:30:36 +0200 (CEST) IronPort-SDR: Ba4SVlR0ubz6shyNQGAVcJho2iQUzAHHe/8bez7ryYS2m3XiiG4e/b7/w++7o5LOqombN3xDER Np3jVsXF+BQw== X-IronPort-AV: E=McAfee;i="6000,8403,9745"; a="244288743" X-IronPort-AV: E=Sophos;i="5.76,432,1592895600"; d="scan'208";a="244288743" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Sep 2020 05:30:36 -0700 IronPort-SDR: 1z5c7p8+ea+VYVZT3fYuN7UF0AWCQXYFDbn2nkuOSpTQYv4qiNYLcWAHuyXKw1lcwBPekl16Km 70Y6Je4X0q1w== X-IronPort-AV: E=Sophos;i="5.76,432,1592895600"; d="scan'208";a="451834768" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.248.202]) ([10.213.248.202]) by orsmga004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Sep 2020 05:30:34 -0700 To: Junyu Jiang , dev@dpdk.org Cc: Jeff Guo , Beilei Xing , stable@dpdk.org References: <20200910015426.3140-1-junyux.jiang@intel.com> <20200916015105.39815-1-junyux.jiang@intel.com> From: Ferruh Yigit Message-ID: <295e61ad-cbc5-3fbe-c996-c3f9a11b34d6@intel.com> Date: Wed, 16 Sep 2020 13:30:31 +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: <20200916015105.39815-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] [dpdk-stable] [PATCH v2] 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/16/2020 2:51 AM, Junyu Jiang wrote: > This patch fixed the issue that rx/tx bytes overflowed "Rx/Tx 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 > --- > drivers/net/i40e/i40e_ethdev.c | 47 ++++++++++++++++++++++++++++++++++ > drivers/net/i40e/i40e_ethdev.h | 9 +++++++ > 2 files changed, 56 insertions(+) > > diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c > index 563f21d9d..4d4ea9861 100644 > --- a/drivers/net/i40e/i40e_ethdev.c > +++ b/drivers/net/i40e/i40e_ethdev.c > @@ -3073,6 +3073,13 @@ i40e_update_vsi_stats(struct i40e_vsi *vsi) > i40e_stat_update_48(hw, I40E_GLV_BPRCH(idx), I40E_GLV_BPRCL(idx), > vsi->offset_loaded, &oes->rx_broadcast, > &nes->rx_broadcast); > + /* enlarge the limitation when rx_bytes overflowed */ > + if (vsi->offset_loaded) { > + if (I40E_RXTX_BYTES_LOW(vsi->old_rx_bytes) > nes->rx_bytes) > + nes->rx_bytes += (uint64_t)1 << I40E_48_BIT_WIDTH; > + nes->rx_bytes += I40E_RXTX_BYTES_HIGH(vsi->old_rx_bytes); > + } > + vsi->old_rx_bytes = nes->rx_bytes; Can you please describe this logic? (indeed better to describe it in the commit log) 'nes->rx_bytes' is diff in the stats register since last read. 'old_rx_bytes' is the previous stats diff. Why/how "I40E_RXTX_BYTES_LOW(vsi->old_rx_bytes) > nes->rx_bytes" has a meaning? Isn't this very depends on the read frequency? I guess I am missing something but please help me understand. Also can you please confirm the initial value of the "vsi->offset_loaded" is correct. <....> > @@ -282,6 +282,9 @@ struct rte_flow { > #define I40E_ETH_OVERHEAD \ > (RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE * 2) > > +#define I40E_RXTX_BYTES_HIGH(bytes) ((bytes) & ~I40E_48_BIT_MASK) > +#define I40E_RXTX_BYTES_LOW(bytes) ((bytes) & I40E_48_BIT_MASK) > + HIGH/LOW is a little misleading, for 64Bits it sounds like it is getting low 32 bits and high 32 bits, can you please clarify macro masks out 48/16 bits. > struct i40e_adapter; > struct rte_pci_driver; > > @@ -399,6 +402,8 @@ struct i40e_vsi { > uint8_t vlan_anti_spoof_on; /* The VLAN anti-spoofing enabled */ > uint8_t vlan_filter_on; /* The VLAN filter enabled */ > struct i40e_bw_info bw_info; /* VSI bandwidth information */ > + uint64_t old_rx_bytes; > + uint64_t old_tx_bytes; 'prev' seems better naming than 'old', what do you think renaming 'old_rx_bytes' -> 'prev_rx_bytes' (for all variables)?