* [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).