* [Patch RFC net-next] net: usb: r8152: Fix rx_bytes/tx_bytes to include FCS
@ 2017-05-17 20:23 Andrew Lunn
2017-05-17 22:28 ` Florian Fainelli
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Lunn @ 2017-05-17 20:23 UTC (permalink / raw)
To: David Miller; +Cc: netdev, hayeswang, Mario Limonciello, Andrew Lunn
The statistics counters rx_bytes and tx_bytes don't include the
Ethernet Frame Check Sequence. The hardware appears to calculate it on
send, so it is not part of the URB passed to the device, and on
receive it was not being copied into the skb.
Fix the rx_bytes/tx_bytes counters, and copy the FCS into the skb so
tools like wireshark can see it.
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
RFC for the following points:
I run automatic tests on Ethernet switches supported by DSA. I have a
test host with many Ethernet interfaces which i connect to switch
ports. Some are quad Ethernet PCIe cards, using the Intel e1000e
driver. Some are USB-Ethernet dongles using the asix driver. And i
have some USB-Ethernet dongles using the r8152. The e1000e, asix and
DSA itself all give consistent rx_bytes and tx_bytes statistics. The
r8152 consistently returns counters which are 4 bytes per frame lower,
which results in some test failures.
Is it defined somewhere what should be included in rx_bytes/tx_bytes?
Should the FCS be included?
This is considered a user API change? The meaning of the counters are
going to be slightly different after this patch. I could be breaking
somebody else's tests :-(
---
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
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Patch RFC net-next] net: usb: r8152: Fix rx_bytes/tx_bytes to include FCS
2017-05-17 20:23 [Patch RFC net-next] net: usb: r8152: Fix rx_bytes/tx_bytes to include FCS Andrew Lunn
@ 2017-05-17 22:28 ` Florian Fainelli
2017-05-18 15:09 ` Andrew Lunn
0 siblings, 1 reply; 6+ messages in thread
From: Florian Fainelli @ 2017-05-17 22:28 UTC (permalink / raw)
To: Andrew Lunn, David Miller; +Cc: netdev, hayeswang, Mario Limonciello
On 05/17/2017 01:23 PM, Andrew Lunn wrote:
> The statistics counters rx_bytes and tx_bytes don't include the
> Ethernet Frame Check Sequence. The hardware appears to calculate it on
> send, so it is not part of the URB passed to the device, and on
> receive it was not being copied into the skb.
>
> Fix the rx_bytes/tx_bytes counters, and copy the FCS into the skb so
> tools like wireshark can see it.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
> RFC for the following points:
>
> I run automatic tests on Ethernet switches supported by DSA. I have a
> test host with many Ethernet interfaces which i connect to switch
> ports. Some are quad Ethernet PCIe cards, using the Intel e1000e
> driver. Some are USB-Ethernet dongles using the asix driver. And i
> have some USB-Ethernet dongles using the r8152. The e1000e, asix and
> DSA itself all give consistent rx_bytes and tx_bytes statistics. The
> r8152 consistently returns counters which are 4 bytes per frame lower,
> which results in some test failures.
>
> Is it defined somewhere what should be included in rx_bytes/tx_bytes?
> Should the FCS be included?
>
> This is considered a user API change? The meaning of the counters are
> going to be slightly different after this patch. I could be breaking
> somebody else's tests :-(
I am afraid that we won't be able to enforce a consistent behavior,
because the HW itself is not consistent, both on the NIC and on the
switch side.
For instance, whether your switch's MIB counters do include FCS, Marvell
tags, or not is going to be highly dependent on how the HW is designed,
whether the MIB counter logic is before, or after in the packet ingress
path.
Some NICs also allow passing the CRC frame up the stack (e.g:
NETIF_F_RXFCS), in which case, we should probably report the FCS as part
of skb->len, because that's what the stack is going to be given.
So I am afraid that as far as your tests are going to work, you will
need to have some logic that knows what kind of switch driver/HW is
used, whether to expect FCS/tag lengths reported, just like you will
have to have the same thing on the initiator of the tests....
> ---
> 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
> ---
> 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);
>
--
Florian
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch RFC net-next] net: usb: r8152: Fix rx_bytes/tx_bytes to include FCS
2017-05-17 22:28 ` Florian Fainelli
@ 2017-05-18 15:09 ` Andrew Lunn
2017-05-18 15:22 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Lunn @ 2017-05-18 15:09 UTC (permalink / raw)
To: Florian Fainelli; +Cc: David Miller, netdev, hayeswang, Mario Limonciello
> I am afraid that we won't be able to enforce a consistent behavior,
> because the HW itself is not consistent, both on the NIC and on the
> switch side.
Hi Florian
I agree with that, for MIB counters. They tend to come direct from the
hardware.
However, rx_bytes and tx_bytes are not from the hardware. They are
software stats, kept by the drivers. Just grep in driver/net/ethernet
and you see:
broadcom/bcmsysport.c: ndev->stats.rx_bytes += len;
broadcom/sb1250-mac.c: dev->stats.rx_bytes += len;
mellanox/mlx5/core/en_main.c: s->rx_bytes += rq_stats->bytes;
microchip/encx24j600.c: dev->stats.rx_bytes += rsv->len;
neterion/vxge/vxge-main.c: net_stats->rx_bytes += bytes;
nuvoton/w90p910_ether.c: dev->stats.rx_bytes += length;
etc.
Since these are software counters, they can be consistent. From a
practical point of view, i doubt they ever will all be consistent,
there are simply too many drivers to test and change if
needed. However, for the ones somebody cares about, they can be made
consistent.
I care about r8152, and would like to make it consistent with asix,
dsa, e1000e.
Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch RFC net-next] net: usb: r8152: Fix rx_bytes/tx_bytes to include FCS
2017-05-18 15:09 ` Andrew Lunn
@ 2017-05-18 15:22 ` David Miller
2017-05-18 17:00 ` Florian Fainelli
0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2017-05-18 15:22 UTC (permalink / raw)
To: andrew; +Cc: f.fainelli, netdev, hayeswang, mario_limonciello
From: Andrew Lunn <andrew@lunn.ch>
Date: Thu, 18 May 2017 17:09:25 +0200
> Since these are software counters, they can be consistent. From a
> practical point of view, i doubt they ever will all be consistent,
> there are simply too many drivers to test and change if
> needed. However, for the ones somebody cares about, they can be made
> consistent.
>
> I care about r8152, and would like to make it consistent with asix,
> dsa, e1000e.
No objection from me for making software counters consistent.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch RFC net-next] net: usb: r8152: Fix rx_bytes/tx_bytes to include FCS
2017-05-18 15:22 ` David Miller
@ 2017-05-18 17:00 ` Florian Fainelli
2017-05-18 17:33 ` Andrew Lunn
0 siblings, 1 reply; 6+ messages in thread
From: Florian Fainelli @ 2017-05-18 17:00 UTC (permalink / raw)
To: David Miller, andrew; +Cc: netdev, hayeswang, mario_limonciello
On 05/18/2017 08:22 AM, David Miller wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> Date: Thu, 18 May 2017 17:09:25 +0200
>
>> Since these are software counters, they can be consistent. From a
>> practical point of view, i doubt they ever will all be consistent,
>> there are simply too many drivers to test and change if
>> needed. However, for the ones somebody cares about, they can be made
>> consistent.
>>
>> I care about r8152, and would like to make it consistent with asix,
>> dsa, e1000e.
>
> No objection from me for making software counters consistent.
>
No objection for me as well, but I think we need to agree on what these
software counters represent, since there are several cases:
- BQL cares about bytes sent on the wire, so that should not include
pre/appended descriptors nor the FCS (nor the Ethernet preamble),
tx_bytes should be equivalent to that
- if we don't include the FCS on transmit, why should we include it on
receive? rx_bytes should have the same rules as tx_bytes: no
status/descriptor bytes, no FCS etc.
--
Florian
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch RFC net-next] net: usb: r8152: Fix rx_bytes/tx_bytes to include FCS
2017-05-18 17:00 ` Florian Fainelli
@ 2017-05-18 17:33 ` Andrew Lunn
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Lunn @ 2017-05-18 17:33 UTC (permalink / raw)
To: Florian Fainelli; +Cc: David Miller, netdev, hayeswang, mario_limonciello
Hi Florian
I agree we should define this, and we can add it to
Documentation/ABI/testing/sysfs-class-net-statistics
> - BQL cares about bytes sent on the wire, so that should not include
> pre/appended descriptors nor the FCS (nor the Ethernet preamble),
> tx_bytes should be equivalent to that
Can you point me at some documentation/code which shows this?
pre/appended descriptors i can understand, since it does not make it
to the wire. FCS does. Preamble and inter-frame gap also does make it
to the wire, and contributes to the overall load on the medium. But i
would expect BQL is tolerant to this. We are talking about an error of
about 0.26% for a full MTU frame if FCS is included when it should not
be.
If BQL really does care about not including the FCS, we probably have
a lot less to do. People should of audited their code when they added
support for BQL :-)
Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-05-18 17:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-17 20:23 [Patch RFC net-next] net: usb: r8152: Fix rx_bytes/tx_bytes to include FCS Andrew Lunn
2017-05-17 22:28 ` Florian Fainelli
2017-05-18 15:09 ` Andrew Lunn
2017-05-18 15:22 ` David Miller
2017-05-18 17:00 ` Florian Fainelli
2017-05-18 17:33 ` Andrew Lunn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).