[PATCH v2 net-next 0/2] Start making stats consistent

[PATCH v2 net-next 0/2] Start making stats consistent

[Andrew Lunn]

It does not appear that the sysfs class net stats have been clearly
defined, resulting in different network interfaces implementing them
slightly different. Define that the rx_bytes and tx_bytes counters
should include the Ethernet header, data and FEC. Modify the r8152 USB
Ethernet driver to include the FEC in its counters.

Andrew Lunn (2):
  Documentation: sysfs-class-net-statistics: Clarify rx_bytes and
    tx_bytes
  net: usb: r8152: Fix rx_bytes/tx_bytes to include FCS

 Documentation/ABI/testing/sysfs-class-net-statistics | 6 ++++--
 drivers/net/usb/r8152.c                              | 5 ++---
 2 files changed, 6 insertions(+), 5 deletions(-)

-- 
2.11.0

[PATCH v2 net-next 1/2] Documentation: sysfs-class-net-statistics: Clarify rx_bytes and tx_bytes

[Andrew Lunn]

Document what is expected for the rx_bytes and tx_bytes statistics in
/sys/class/net/<device>/statistics. The FCS should be included in the
statistics. However, since this has been unclear until now, it is
expected a number of drivers don't. But maybe with time they will.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 Documentation/ABI/testing/sysfs-class-net-statistics | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-net-statistics b/Documentation/ABI/testing/sysfs-class-net-statistics
index 397118de7b5e..a487cbb79458 100644
--- a/Documentation/ABI/testing/sysfs-class-net-statistics
+++ b/Documentation/ABI/testing/sysfs-class-net-statistics
@@ -21,7 +21,8 @@ Contact:	netdev@vger.kernel.org
 Description:
 		Indicates the number of bytes received by this network device.
 		See the network driver for the exact meaning of when this
-		value is incremented.
+		value is incremented. However, for an Ethernet frame, it should
+		include the Ethernet header, data, and frame check sum.
 
 What:		/sys/class/<iface>/statistics/rx_compressed
 Date:		April 2005
@@ -125,7 +126,8 @@ Description:
 		device. See the network driver for the exact meaning of this
 		value, in particular whether this accounts for all successfully
 		transmitted packets or all packets that have been queued for
-		transmission.
+		transmission. For an Ethernet frame, it should include the
+		Ethernet header, data, and frame check sum.
 
 What:		/sys/class/<iface>/statistics/tx_carrier_errors
 Date:		April 2005
-- 
2.11.0

Re: [PATCH v2 net-next 1/2] Documentation: sysfs-class-net-statistics: Clarify rx_bytes and tx_bytes

[Stephen Hemminger]

On Tue, 23 May 2017 23:10:57 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> Document what is expected for the rx_bytes and tx_bytes statistics in
> /sys/class/net/<device>/statistics. The FCS should be included in the
> statistics. However, since this has been unclear until now, it is
> expected a number of drivers don't. But maybe with time they will.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  Documentation/ABI/testing/sysfs-class-net-statistics | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-net-statistics b/Documentation/ABI/testing/sysfs-class-net-statistics
> index 397118de7b5e..a487cbb79458 100644
> --- a/Documentation/ABI/testing/sysfs-class-net-statistics
> +++ b/Documentation/ABI/testing/sysfs-class-net-statistics
> @@ -21,7 +21,8 @@ Contact:	netdev@vger.kernel.org
>  Description:
>  		Indicates the number of bytes received by this network device.
>  		See the network driver for the exact meaning of when this
> -		value is incremented.
> +		value is incremented. However, for an Ethernet frame, it should
> +		include the Ethernet header, data, and frame check sum.
>  
>  What:		/sys/class/<iface>/statistics/rx_compressed
>  Date:		April 2005
> @@ -125,7 +126,8 @@ Description:
>  		device. See the network driver for the exact meaning of this
>  		value, in particular whether this accounts for all successfully
>  		transmitted packets or all packets that have been queued for
> -		transmission.
> +		transmission. For an Ethernet frame, it should include the
> +		Ethernet header, data, and frame check sum.
>  
>  What:		/sys/class/<iface>/statistics/tx_carrier_errors
>  Date:		April 2005

Sysfs statistics must match the netlink statistics. What matters is the
values reported in netlink, proc, and sysfs are the same.
Documenting the sysfs value here is nice but less important.
I worry that some naive implementer or tester might expect sysfs values
to follow different standard than other places.

For devices the important thing is that rx and tx values match. I.e if
FCS is counted on RX then it needs to get counted on Tx. I don't think
FCS should be part of data  statistics since many devices are virtual
and have no visible FCS.  For example if a packet is sent of virtio (no FCS)
to host vhost (no FCS) and then through bridge to physical hardware
which with your proposal does have FCS; all the data counts all matched.

With the advent of overlay networks this gets even more confusing.
Does overlay header count?

Linux in general has followed the BSD model of NOT including FCS
in device statistics. Some hardware vendors do include the FCS in
their stats but that is really a porting bug.

Unfortunately, even the router vendors can't agree on this.
RFC 2665 says that FCS should be included, but BSD derived systems
like Juniper do not. This is a mess.

Re: [PATCH v2 net-next 1/2] Documentation: sysfs-class-net-statistics: Clarify rx_bytes and tx_bytes

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Tue, 23 May 2017 23:10:57 +0200

> Document what is expected for the rx_bytes and tx_bytes statistics in
> /sys/class/net/<device>/statistics. The FCS should be included in the
> statistics. However, since this has been unclear until now, it is
> expected a number of drivers don't. But maybe with time they will.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Precedence has not been very kind to us here.

Also, we really I want drivers doing the simplest thing possible,
which on transmit is:

	tx_bytes += skb->len;

Not:

	tx_bytes += skb->len + ETH_FCS_LEN;

I understand the problem you're trying to tackle, but software stats
should be for software state, and that state is emphatically skb->len

So if we are to strive for software statistic "consistency" it should
be against the software part of the packet, not the software part
"plus X and Y link layer stuff no visible in the software packet".

Thanks.

[PATCH v2 net-next 2/2] net: usb: r8152: Fix rx_bytes/tx_bytes to include FCS

[Andrew Lunn]

The statistics counters rx_bytes and tx_bytes don't include the
Ethernet Frame Check Sequence. Fix the rx_bytes/tx_bytes counters to
include the FCS, and include the received FCS in the skb.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/usb/r8152.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index ddc62cb69be8..4081a2cd8b1b 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1228,7 +1228,7 @@ static void write_bulk_callback(struct urb *urb)
 		stats->tx_errors += agg->skb_num;
 	} else {
 		stats->tx_packets += agg->skb_num;
-		stats->tx_bytes += agg->skb_len;
+		stats->tx_bytes += agg->skb_len + CRC_SIZE;
 	}
 
 	spin_lock(&tp->tx_lock);
@@ -1826,7 +1826,6 @@ static int rx_bottom(struct r8152 *tp, int budget)
 			if (urb->actual_length < len_used)
 				break;
 
-			pkt_len -= CRC_SIZE;
 			rx_data += sizeof(struct rx_desc);
 
 			skb = napi_alloc_skb(napi, pkt_len);
@@ -1850,7 +1849,7 @@ static int rx_bottom(struct r8152 *tp, int budget)
 			}
 
 find_next_rx:
-			rx_data = rx_agg_align(rx_data + pkt_len + CRC_SIZE);
+			rx_data = rx_agg_align(rx_data + pkt_len);
 			rx_desc = (struct rx_desc *)rx_data;
 			len_used = (int)(rx_data - (u8 *)agg->head);
 			len_used += sizeof(struct rx_desc);
-- 
2.11.0