[dpdk-dev] [PATCH] ixgbe: fix crc-strip enable changing rx bytes

[dpdk-dev] [PATCH] ixgbe: fix crc-strip enable changing rx bytes

[Harry van Haaren]

Fix a consistency issue in ixgbe, that when CRC stripping is enabled,
the CRC bytes are not added to the rx total byte count. When CRC strip
is disabled, these bytes are counted.

This patch reads the CRC strip register, and when enabled adds 4 bytes
to the total bytes recieved counter for each received packet.

E-mail discussion thread available:
http://dpdk.org/ml/archives/dev/2015-March/015685.html

To reproduce, run testpmd with no flags and with --crc-strip, sending a
single packet, and notice that the RX byte counter is 4 bytes smaller
when CRC stripping is enabled.

Fixes: 29a05247ebad ("ixgbe: configure CRC stripping behaviour of PF")

Reported-by: Stephen Hemminger <shemming@brocade.com>
Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 doc/guides/rel_notes/release_2_2.rst | 6 ++++++
 drivers/net/ixgbe/ixgbe_ethdev.c     | 7 +++++++
 2 files changed, 13 insertions(+)

diff --git a/doc/guides/rel_notes/release_2_2.rst b/doc/guides/rel_notes/release_2_2.rst
index 59dda59..6bab51b 100644
--- a/doc/guides/rel_notes/release_2_2.rst
+++ b/doc/guides/rel_notes/release_2_2.rst
@@ -134,6 +134,12 @@ Drivers
 
   VF needs the PF interrupt support initialized even if not started.
 
+* **ixgbe: Fixed CRC strip enable influencing RX byte counters.**
+
+  A workaround has been put in place to ensure consistent presentation
+  of total RX bytes. The CRC bytes are now always included in RX bytes,
+  providing consistency between RX and TX bytes.
+
 * **i40e: Fixed base driver allocation when not using first numa node.**
 
   Fixed i40e issue that occurred when a DPDK application didn't initialize
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 0b0bbcf..b311ffb 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -2361,6 +2361,13 @@ ixgbe_read_stats_registers(struct ixgbe_hw *hw, struct ixgbe_hw_stats
 	hw_stats->mrfc += IXGBE_READ_REG(hw, IXGBE_MRFC);
 	hw_stats->rlec += IXGBE_READ_REG(hw, IXGBE_RLEC);
 
+	/* Workaround for RX byte count not including CRC bytes when CRC
+	 * strip is enabled. Multiply rx_total_packets by CRC size and add
+	 * to adjust rx_bytes to the correct value
+	 */
+	if (IXGBE_READ_REG(hw, IXGBE_HLREG0) & IXGBE_HLREG0_RXCRCSTRP)
+		*total_qbrc += (*total_qprc * 4);
+
 	/* Note that gprc counts missed packets */
 	hw_stats->gprc += IXGBE_READ_REG(hw, IXGBE_GPRC);
 
-- 
1.9.1

Re: [dpdk-dev] [PATCH] ixgbe: fix crc-strip enable changing rx bytes

[Stephen Hemminger]

On Mon, 9 Nov 2015 14:55:04 +0000
Harry van Haaren <harry.van.haaren@intel.com> wrote:

> Fix a consistency issue in ixgbe, that when CRC stripping is enabled,
> the CRC bytes are not added to the rx total byte count. When CRC strip
> is disabled, these bytes are counted.
> 
> This patch reads the CRC strip register, and when enabled adds 4 bytes
> to the total bytes recieved counter for each received packet.
> 
> E-mail discussion thread available:
> http://dpdk.org/ml/archives/dev/2015-March/015685.html
> 
> To reproduce, run testpmd with no flags and with --crc-strip, sending a
> single packet, and notice that the RX byte counter is 4 bytes smaller
> when CRC stripping is enabled.
> 
> Fixes: 29a05247ebad ("ixgbe: configure CRC stripping behaviour of PF")
> 
> Reported-by: Stephen Hemminger <shemming@brocade.com>
> Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> ---
>  doc/guides/rel_notes/release_2_2.rst | 6 ++++++
>  drivers/net/ixgbe/ixgbe_ethdev.c     | 7 +++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/release_2_2.rst b/doc/guides/rel_notes/release_2_2.rst
> index 59dda59..6bab51b 100644
> --- a/doc/guides/rel_notes/release_2_2.rst
> +++ b/doc/guides/rel_notes/release_2_2.rst
> @@ -134,6 +134,12 @@ Drivers
>  
>    VF needs the PF interrupt support initialized even if not started.
>  
> +* **ixgbe: Fixed CRC strip enable influencing RX byte counters.**
> +
> +  A workaround has been put in place to ensure consistent presentation
> +  of total RX bytes. The CRC bytes are now always included in RX bytes,
> +  providing consistency between RX and TX bytes.
> +
>  * **i40e: Fixed base driver allocation when not using first numa node.**
>  
>    Fixed i40e issue that occurred when a DPDK application didn't initialize
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 0b0bbcf..b311ffb 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -2361,6 +2361,13 @@ ixgbe_read_stats_registers(struct ixgbe_hw *hw, struct ixgbe_hw_stats
>  	hw_stats->mrfc += IXGBE_READ_REG(hw, IXGBE_MRFC);
>  	hw_stats->rlec += IXGBE_READ_REG(hw, IXGBE_RLEC);
>  
> +	/* Workaround for RX byte count not including CRC bytes when CRC
> +	 * strip is enabled. Multiply rx_total_packets by CRC size and add
> +	 * to adjust rx_bytes to the correct value
> +	 */
> +	if (IXGBE_READ_REG(hw, IXGBE_HLREG0) & IXGBE_HLREG0_RXCRCSTRP)
> +		*total_qbrc += (*total_qprc * 4);
> +
>  	/* Note that gprc counts missed packets */
>  	hw_stats->gprc += IXGBE_READ_REG(hw, IXGBE_GPRC);
>  

NAK.

The CRC should never be part of rx/tx statistics.

The problem with putting CRC in statistics is the behavior introduces a
device and setting dependency. This is a disaster for generic programs (like vRouter)
that want to work across multiple hardware and virtual NIC's.

Also, Linux, BSD, and Unix do not include CRC in reported bytes for network
interfaces.

Please fix up my original patch to work for both settings, or I can do it.

Re: [dpdk-dev] [PATCH] ixgbe: fix crc-strip enable changing rx bytes

[Van Haaren, Harry]

Hi Stephen,

> From: Stephen Hemminger [mailto:shemming@brocade.com]
> Subject: Re: [PATCH] ixgbe: fix crc-strip enable changing rx bytes
> 
> Harry van Haaren <harry.van.haaren@intel.com> wrote:
> > <snip>
> > This patch reads the CRC strip register, and when enabled adds 4 bytes
> > to the total bytes recieved counter for each received packet.
> > <snip>
> 
> NAK.
> 
> The CRC should never be part of rx/tx statistics.
> 
> The problem with putting CRC in statistics is the behavior introduces a
> device and setting dependency. This is a disaster for generic programs (like vRouter)
> that want to work across multiple hardware and virtual NIC's.
> 
> Also, Linux, BSD, and Unix do not include CRC in reported bytes for network
> interfaces.
> 
> Please fix up my original patch to work for both settings, or I can do it.

I understand your point that generic programs should not require knowledge of if CRC bytes are included in the packet-size or not, and agree that this should be consistent in DPDK / Linux / BSD etc.

I will rework the PMDs to report byte counters without CRC bytes, and mark this patch as superseded.

-Harry