| Message ID | 20230602094659.965523-2-wei.fang@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7190d0ff0e17690a9b1279d84a06473600ba2060 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: enetc: correct the statistics of rx bytes | expand |
Hi Wei, On Fri, Jun 02, 2023 at 05:46:58PM +0800, wei.fang@nxp.com wrote: > From: Wei Fang <wei.fang@nxp.com> > > The rx_bytes of struct net_device_stats should count the length of > ethernet frames excluding the FCS. However, there are two problems > with the rx_bytes statistics of the current enetc driver. one is > that the length of VLAN header is not counted if the VLAN extraction > feature is enabled. The other is that the length of L2 header is not > counted, because eth_type_trans() is invoked before updating rx_bytes > which will subtract the length of L2 header from skb->len. Thanks for noticing the issue and for sending a patch. > BTW, the rx_bytes statistics of XDP path also have similar problem, > I will fix it in another patch. > > Fixes: a800abd3ecb9 ("net: enetc: move skb creation into enetc_build_skb") > Signed-off-by: Wei Fang <wei.fang@nxp.com> > --- > drivers/net/ethernet/freescale/enetc/enetc.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c > index 3c4fa26f0f9b..d6c0f3f46c2a 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > @@ -1229,7 +1229,13 @@ static int enetc_clean_rx_ring(struct enetc_bdr *rx_ring, > if (!skb) > break; > > - rx_byte_cnt += skb->len; > + /* When set, the outer VLAN header is extracted and reported > + * in the receive buffer descriptor. So rx_byte_cnt should > + * add the length of the extracted VLAN header. > + */ > + if (bd_status & ENETC_RXBD_FLAG_VLAN) > + rx_byte_cnt += VLAN_HLEN; > + rx_byte_cnt += skb->len + ETH_HLEN; Hmm, to avoid the conditional, have you considered adding an "int *rx_byte_cnt" argument to enetc_build_skb()? It can be updated with "(*rx_byte_cnt) += size" from all places where we already have "(*cleaned_cnt)++". > rx_frm_cnt++; > > napi_gro_receive(napi, skb); > -- > 2.25.1 >
On Fri, Jun 02, 2023 at 01:27:10PM +0300, Vladimir Oltean wrote: > Hmm, to avoid the conditional, have you considered adding an "int *rx_byte_cnt" > argument to enetc_build_skb()? It can be updated with "(*rx_byte_cnt) += size" > from all places where we already have "(*cleaned_cnt)++". Ah, don't mind me. We can't avoid the conditional, since the VLAN header is offloaded as you said. It's not in the buffer.
On Fri, Jun 02, 2023 at 05:46:58PM +0800, wei.fang@nxp.com wrote: > From: Wei Fang <wei.fang@nxp.com> > > The rx_bytes of struct net_device_stats should count the length of > ethernet frames excluding the FCS. However, there are two problems > with the rx_bytes statistics of the current enetc driver. one is > that the length of VLAN header is not counted if the VLAN extraction > feature is enabled. The other is that the length of L2 header is not > counted, because eth_type_trans() is invoked before updating rx_bytes > which will subtract the length of L2 header from skb->len. > BTW, the rx_bytes statistics of XDP path also have similar problem, > I will fix it in another patch. > > Fixes: a800abd3ecb9 ("net: enetc: move skb creation into enetc_build_skb") > Signed-off-by: Wei Fang <wei.fang@nxp.com> > --- Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com> By the way, you mangled Jakub's email (kuba@kernel.or) - I've fixed it up.
> -----Original Message----- > From: Vladimir Oltean <vladimir.oltean@nxp.com> > Sent: 2023年6月2日 18:32 > To: Wei Fang <wei.fang@nxp.com> > Cc: Claudiu Manoil <claudiu.manoil@nxp.com>; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; > ast@kernel.org; daniel@iogearbox.net; hawk@kernel.org; > john.fastabend@gmail.com; netdev@vger.kernel.org; > linux-kernel@vger.kernel.org > Subject: Re: [PATCH net 1/2] net: enetc: correct the statistics of rx bytes > > On Fri, Jun 02, 2023 at 05:46:58PM +0800, wei.fang@nxp.com wrote: > > From: Wei Fang <wei.fang@nxp.com> > > > > The rx_bytes of struct net_device_stats should count the length of > > ethernet frames excluding the FCS. However, there are two problems > > with the rx_bytes statistics of the current enetc driver. one is that > > the length of VLAN header is not counted if the VLAN extraction > > feature is enabled. The other is that the length of L2 header is not > > counted, because eth_type_trans() is invoked before updating rx_bytes > > which will subtract the length of L2 header from skb->len. > > BTW, the rx_bytes statistics of XDP path also have similar problem, I > > will fix it in another patch. > > > > Fixes: a800abd3ecb9 ("net: enetc: move skb creation into > > enetc_build_skb") > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > > --- > > Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > By the way, you mangled Jakub's email (kuba@kernel.or) - I've fixed it up. Yes, I realized this after sending the email. Thank you so much!
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c index 3c4fa26f0f9b..d6c0f3f46c2a 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc.c +++ b/drivers/net/ethernet/freescale/enetc/enetc.c @@ -1229,7 +1229,13 @@ static int enetc_clean_rx_ring(struct enetc_bdr *rx_ring, if (!skb) break; - rx_byte_cnt += skb->len; + /* When set, the outer VLAN header is extracted and reported + * in the receive buffer descriptor. So rx_byte_cnt should + * add the length of the extracted VLAN header. + */ + if (bd_status & ENETC_RXBD_FLAG_VLAN) + rx_byte_cnt += VLAN_HLEN; + rx_byte_cnt += skb->len + ETH_HLEN; rx_frm_cnt++; napi_gro_receive(napi, skb);