| Message ID | 1421994224-2127-1-git-send-email-stephen@networkplumber.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 1D1255AD6; Fri, 23 Jan 2015 07:23:57 +0100 (CET) Received: from mail-pa0-f46.google.com (mail-pa0-f46.google.com [209.85.220.46]) by dpdk.org (Postfix) with ESMTP id C6B355AD4 for <dev@dpdk.org>; Fri, 23 Jan 2015 07:23:53 +0100 (CET) Received: by mail-pa0-f46.google.com with SMTP id lj1so2044740pab.5 for <dev@dpdk.org>; Thu, 22 Jan 2015 22:23:53 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=3URV9z4tHKdPCQZceOLz3fXPqfbfqi5g/zSUu1LKRY4=; b=IdifN/nPoOVlk0UFgNDgpw4qOQIfvr0eWogZQ8Ono7nlW0FbXI4M8ZS4j/sGHvPR4t VWynYdBVHrJK7vlxGchB4gQ5S8gWu5w3Lqi8C9Ya0oVf6wSoTsXKLN3dxNDJwmgznadg ebMCjpdZ0xINeY8G0duTNYxFnLSeTwAS1nbiuphGMZmUhTQ9bu3j778PNbGQGTRLTn/+ r/NNylnfBjDXJyaz26RulJ25KHc1+eTBqH77XEtUA1eW+EK3D4GVbf7s1Rya6ezm98Rg Bqsu/SS0PavfaeOA6sAl4nlLfwZFbuxKtQpkyKmC3c0IkV+UKc/xFqybzFfG0o78GKgi pl8A== X-Gm-Message-State: ALoCoQlOAgT2hVkqWIU/3BzqGsdBqXnfesXSFQGA77GVNZSZQtQOpTWGnWmBqNPKhmvvVOrd0gk9 X-Received: by 10.68.196.231 with SMTP id ip7mr8502098pbc.156.1421994232945; Thu, 22 Jan 2015 22:23:52 -0800 (PST) Received: from urahara.brocade.com (static-50-53-82-155.bvtn.or.frontiernet.net. [50.53.82.155]) by mx.google.com with ESMTPSA id nj2sm725755pbc.16.2015.01.22.22.23.50 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Thu, 22 Jan 2015 22:23:52 -0800 (PST) From: stephen@networkplumber.org To: dev@dpdk.org Date: Thu, 22 Jan 2015 22:23:44 -0800 Message-Id: <1421994224-2127-1-git-send-email-stephen@networkplumber.org> X-Mailer: git-send-email 2.1.4 Cc: Stephen Hemminger <shemming@brocade.com> Subject: [dpdk-dev] [PATCH] ixgbe: do not include CRC in Tx byte count X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Stephen Hemminger
Jan. 23, 2015, 6:23 a.m. UTC
From: Stephen Hemminger <shemming@brocade.com> The ixgbe driver was including CRC in the transmit packet byte count, but not for packets received. This was notice when forwarding and the number of bytes received was greater than the number of bytes transmitted for the same number of packets. Make the driver behave like other virtual devices and not include CRC in byte count. Use the same queue counters already computed and used for Rx. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> --- lib/librte_pmd_ixgbe/ixgbe_ethdev.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
Comments
Hi Stephen, 2015-01-22 22:23, stephen@networkplumber.org: > From: Stephen Hemminger <shemming@brocade.com> > > The ixgbe driver was including CRC in the transmit packet byte > count, but not for packets received. This was notice when forwarding and > the number of bytes received was greater than the number of bytes transmitted Tx includes CRC and Rx count is greater, really? > for the same number of packets. Make the driver behave like other > virtual devices and not include CRC in byte count. Use the same queue > counters already computed and used for Rx. Please could you describe the difference between gptc/gotc and qptc/qbtc? Thank you
On Tue, 27 Jan 2015 11:11:39 +0100 Thomas Monjalon <thomas.monjalon@6wind.com> wrote: > Hi Stephen, > > 2015-01-22 22:23, stephen@networkplumber.org: > > From: Stephen Hemminger <shemming@brocade.com> > > > > The ixgbe driver was including CRC in the transmit packet byte > > count, but not for packets received. This was notice when forwarding and > > the number of bytes received was greater than the number of bytes transmitted > > Tx includes CRC and Rx count is greater, really? > > > for the same number of packets. Make the driver behave like other > > virtual devices and not include CRC in byte count. Use the same queue > > counters already computed and used for Rx. > > Please could you describe the difference between gptc/gotc and qptc/qbtc? > > Thank you The byte counts for global registers include CRC in the byte count. This has been observed by experimentation and validated by QA and documented in Intel HW specs. The original code used queue counts for Rx because the global counters include missed packets (which must not be included in the ipacket counts). I suspect that is why the original developer used the queue counts, as a good side effect the Rx byte count was correct (no CRC). I just extended this for Tx.
Anyone to carefully review this patch? 2015-01-27 11:38, Stephen Hemminger: > On Tue, 27 Jan 2015 11:11:39 +0100 > Thomas Monjalon <thomas.monjalon@6wind.com> wrote: > > > Hi Stephen, > > > > 2015-01-22 22:23, stephen@networkplumber.org: > > > From: Stephen Hemminger <shemming@brocade.com> > > > > > > The ixgbe driver was including CRC in the transmit packet byte > > > count, but not for packets received. This was notice when forwarding and > > > the number of bytes received was greater than the number of bytes transmitted > > > > Tx includes CRC and Rx count is greater, really? ping > > > for the same number of packets. Make the driver behave like other > > > virtual devices and not include CRC in byte count. Use the same queue > > > counters already computed and used for Rx. > > > > Please could you describe the difference between gptc/gotc and qptc/qbtc? > > > > Thank you > > > The byte counts for global registers include CRC in the byte count. > This has been observed by experimentation and validated by QA and documented > in Intel HW specs. > > The original code used queue counts for Rx because the global counters > include missed packets (which must not be included in the ipacket counts). > I suspect that is why the original developer used the queue counts, as > a good side effect the Rx byte count was correct (no CRC). I just extended > this for Tx.
On Wed, 4 Mar 2015 20:55:05 +0000 Thomas Monjalon <thomas.monjalon@6wind.com> wrote: > Anyone to carefully review this patch? > > 2015-01-27 11:38, Stephen Hemminger: > > On Tue, 27 Jan 2015 11:11:39 +0100 > > Thomas Monjalon <thomas.monjalon@6wind.com> wrote: > > > > > Hi Stephen, > > > > > > 2015-01-22 22:23, stephen@networkplumber.org: > > > > From: Stephen Hemminger <shemming@brocade.com> > > > > > > > > The ixgbe driver was including CRC in the transmit packet byte > > > > count, but not for packets received. This was notice when forwarding and > > > > the number of bytes received was greater than the number of bytes transmitted > > > > > > Tx includes CRC and Rx count is greater, really? Yes. This is observable fact. The gotc register includes CRC and the per-queue register do not.
Konstantin, Helin, your review would be appreciate here. Thanks > From: Stephen Hemminger <shemming@brocade.com> > > The ixgbe driver was including CRC in the transmit packet byte > count, but not for packets received. This was notice when forwarding and > the number of bytes received was greater than the number of bytes transmitted > for the same number of packets. Make the driver behave like other > virtual devices and not include CRC in byte count. Use the same queue > counters already computed and used for Rx. > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of stephen@networkplumber.org > Sent: Friday, January 23, 2015 6:24 AM > To: dev@dpdk.org > Cc: Stephen Hemminger > Subject: [dpdk-dev] [PATCH] ixgbe: do not include CRC in Tx byte count > > From: Stephen Hemminger <shemming@brocade.com> > > The ixgbe driver was including CRC in the transmit packet byte > count, but not for packets received. > This was notice when forwarding and > the number of bytes received was greater than the number of bytes transmitted > for the same number of packets. Make the driver behave like other > virtual devices and not include CRC in byte count. Use the same queue > counters already computed and used for Rx. About RX side stats - as I remember it depends to what value hw_stip_crc is set at configure(). If hw_stip_crc==1, then, yes CRC bytes are not included into QBRC value. I If hw_stip_crc==0, then CRC bytes are included into QBRC. I.E: ./dpdk.org/x86_64-native-linuxapp-gcc/app/testpmd -c ff -n 4 -w 86:00.1 -- -i --tx-queue-stats-mapping='(0,0,0)' --rx-queue-stats-mapping='(0,0,0)' --crc-strip testpmd> start testpmd> show port stats all ######################## NIC statistics for port 0 ######################## RX-packets: 1 RX-errors: 0 RX-bytes: 60 // sum of QBRC[] RX-badcrc: 0 RX-badlen: 0 RX-errors: 0 RX-nombuf: 0 TX-packets: 1 TX-errors: 0 TX-bytes: 64 //GOTC Stats reg 0 RX-packets: 1 RX-errors: 0 RX-bytes: 60 //QBRC[0] ... Stats reg 0 TX-packets: 1 TX-bytes: 60 // QBTC[0] ############################################################################ ./dpdk.org/x86_64-native-linuxapp-gcc/app/testpmd -c ff -n 4 -w 86:00.1 -- -i --tx-queue-stats-mapping='(0,0,0)' --rx-queue-stats-mapping='(0,0,0)' testpmd> start testpmd> show port stats all ######################## NIC statistics for port 0 ######################## RX-packets: 1 RX-errors: 0 RX-bytes: 64 // sum of QBRC[] RX-badcrc: 0 RX-badlen: 0 RX-errors: 0 RX-nombuf: 0 TX-packets: 1 TX-errors: 0 TX-bytes: 64 //GOTC Stats reg 0 RX-packets: 1 RX-errors: 0 RX-bytes: 64 //QBRC[0] .... Stats reg 0 TX-packets: 1 TX-bytes: 60 // QBTC[0] ############################################################################ So now, number of RX/TX stat bytes for simple forwarding are equal when hw_stip_crc==0 and differ when hw_stip_crc==1, With your change, as I understand, it would be visa-versa. I don' t mind changing it yours way, but wonder why that is any better? Probably the proper patch, should also do something like that: stats->ibytes = total_qbrc - stats->ipackets * dev->data->dev_conf. hw_strip_crc * CRC_LEN; too? About using sum of QBRC[] for stats.ibytes instead of GORC, I think the initial reason was: http://www.intel.com/content/www/us/en/ethernet-controllers/82599-10-gbe-controller-spec-update.html: 7 GPRC and GORCL/H Also Count Missed Packets Problem: GPRC (Good Packets Received Count) and GORCL/H (Good Octets Received Count) count missed packets and missed packets bytes; this is not consistent with previous products. Implication: None. 331521-002 23 Errata-Intel(r) 82599 10 GbE Controller Workaround: Statistics are available indirectly for these registers. This workaround is included in Intel drivers. * For GPRC - Subtract MPC (Missed Packet Count) from GPRC. Alternatively, use QPRC. * For GORCL/H - use QBRCL/H (Quad Bytes Received). Konstantin > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> > --- > lib/librte_pmd_ixgbe/ixgbe_ethdev.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c > index b58ec45..27355eb 100644 > --- a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c > +++ b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c > @@ -1724,12 +1724,15 @@ ixgbe_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats) > struct ixgbe_hw_stats *hw_stats = > IXGBE_DEV_PRIVATE_TO_STATS(dev->data->dev_private); > uint32_t bprc, lxon, lxoff, total; > - uint64_t total_missed_rx, total_qbrc, total_qprc; > + uint64_t total_missed_rx; > + uint64_t total_qbrc, total_qprc, total_qbtc, total_qptc; > unsigned i; > > total_missed_rx = 0; > total_qbrc = 0; > total_qprc = 0; > + total_qbtc = 0; > + total_qptc = 0; > > hw_stats->crcerrs += IXGBE_READ_REG(hw, IXGBE_CRCERRS); > hw_stats->illerrc += IXGBE_READ_REG(hw, IXGBE_ILLERRC); > @@ -1770,6 +1773,8 @@ ixgbe_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats) > > total_qprc += hw_stats->qprc[i]; > total_qbrc += hw_stats->qbrc[i]; > + total_qptc += hw_stats->qptc[i]; > + total_qbtc += hw_stats->qbtc[i]; > } > hw_stats->mlfc += IXGBE_READ_REG(hw, IXGBE_MLFC); > hw_stats->mrfc += IXGBE_READ_REG(hw, IXGBE_MRFC); > @@ -1860,8 +1865,8 @@ ixgbe_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats) > /* Fill out the rte_eth_stats statistics structure */ > stats->ipackets = total_qprc; > stats->ibytes = total_qbrc; > - stats->opackets = hw_stats->gptc; > - stats->obytes = hw_stats->gotc; > + stats->opackets = total_qptc; > + stats->obytes = total_qbtc; > stats->imcasts = hw_stats->mprc; > > for (i = 0; i < IXGBE_QUEUE_STAT_COUNTERS; i++) { > -- > 2.1.4
On Mon, 23 Mar 2015 16:45:44 +0000 "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote: > > > > -----Original Message----- > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of stephen@networkplumber.org > > Sent: Friday, January 23, 2015 6:24 AM > > To: dev@dpdk.org > > Cc: Stephen Hemminger > > Subject: [dpdk-dev] [PATCH] ixgbe: do not include CRC in Tx byte count > > > > From: Stephen Hemminger <shemming@brocade.com> > > > > The ixgbe driver was including CRC in the transmit packet byte > > count, but not for packets received. > > This was notice when forwarding and > > the number of bytes received was greater than the number of bytes transmitted > > for the same number of packets. Make the driver behave like other > > virtual devices and not include CRC in byte count. Use the same queue > > counters already computed and used for Rx. > > About RX side stats - as I remember it depends to what value hw_stip_crc is set at configure(). > If hw_stip_crc==1, then, yes CRC bytes are not included into QBRC value. > I If hw_stip_crc==0, then CRC bytes are included into QBRC. That is an additional bug! * CRC should never be included in the byte count. This is not how Linux or other drivers report byte count. * the byte count must be symmetrical Rx == Tx The Brocade router always set strip_crc to 1. So did not see the additional bug of CRC being included in byte count.
> -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen > Hemminger > Sent: Tuesday, March 24, 2015 3:55 PM > To: Ananyev, Konstantin > Cc: dev@dpdk.org; Stephen Hemminger > Subject: Re: [dpdk-dev] [PATCH] ixgbe: do not include CRC in Tx byte count > > On Mon, 23 Mar 2015 16:45:44 +0000 > "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote: > > > > > > > > -----Original Message----- > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of > > > stephen@networkplumber.org > > > Sent: Friday, January 23, 2015 6:24 AM > > > To: dev@dpdk.org > > > Cc: Stephen Hemminger > > > Subject: [dpdk-dev] [PATCH] ixgbe: do not include CRC in Tx byte > > > count > > > > > > From: Stephen Hemminger <shemming@brocade.com> > > > > > > The ixgbe driver was including CRC in the transmit packet byte > > > count, but not for packets received. > > > This was notice when forwarding and > > > the number of bytes received was greater than the number of bytes > > > transmitted for the same number of packets. Make the driver behave > > > like other virtual devices and not include CRC in byte count. Use > > > the same queue counters already computed and used for Rx. > > > > About RX side stats - as I remember it depends to what value hw_stip_crc > is set at configure(). > > If hw_stip_crc==1, then, yes CRC bytes are not included into QBRC value. > > I If hw_stip_crc==0, then CRC bytes are included into QBRC. > > That is an additional bug! > * CRC should never be included in the byte count. > This is not how Linux or other drivers report byte count. > > * the byte count must be symmetrical Rx == Tx > > The Brocade router always set strip_crc to 1. So did not see the additional > bug of CRC being included in byte count. > > Great to see some discussion here all but can we hold on to the patch until after the 2.0 package is made so there is enough time to review and test it properly? Thanks Siobhan <siobhan.a.butler@intel.com>
diff --git a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c index b58ec45..27355eb 100644 --- a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c +++ b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c @@ -1724,12 +1724,15 @@ ixgbe_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats) struct ixgbe_hw_stats *hw_stats = IXGBE_DEV_PRIVATE_TO_STATS(dev->data->dev_private); uint32_t bprc, lxon, lxoff, total; - uint64_t total_missed_rx, total_qbrc, total_qprc; + uint64_t total_missed_rx; + uint64_t total_qbrc, total_qprc, total_qbtc, total_qptc; unsigned i; total_missed_rx = 0; total_qbrc = 0; total_qprc = 0; + total_qbtc = 0; + total_qptc = 0; hw_stats->crcerrs += IXGBE_READ_REG(hw, IXGBE_CRCERRS); hw_stats->illerrc += IXGBE_READ_REG(hw, IXGBE_ILLERRC); @@ -1770,6 +1773,8 @@ ixgbe_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats) total_qprc += hw_stats->qprc[i]; total_qbrc += hw_stats->qbrc[i]; + total_qptc += hw_stats->qptc[i]; + total_qbtc += hw_stats->qbtc[i]; } hw_stats->mlfc += IXGBE_READ_REG(hw, IXGBE_MLFC); hw_stats->mrfc += IXGBE_READ_REG(hw, IXGBE_MRFC); @@ -1860,8 +1865,8 @@ ixgbe_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats) /* Fill out the rte_eth_stats statistics structure */ stats->ipackets = total_qprc; stats->ibytes = total_qbrc; - stats->opackets = hw_stats->gptc; - stats->obytes = hw_stats->gotc; + stats->opackets = total_qptc; + stats->obytes = total_qbtc; stats->imcasts = hw_stats->mprc; for (i = 0; i < IXGBE_QUEUE_STAT_COUNTERS; i++) {