| Message ID | 20240919084104.661180-2-wei.fang@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: enetc: fix some issues of XDP | expand |
On Thu, Sep 19, 2024 at 04:41:02PM +0800, Wei Fang wrote: > The xdp_drops statistic indicates the number of XDP frames dropped in > the Rx direction. However, enetc_xdp_drop() is also used in XDP_TX and > XDP_REDIRECT actions. If frame loss occurs in these two actions, the > frames loss count should not be included in xdp_drops, because there > are already xdp_tx_drops and xdp_redirect_failures to count the frame > loss of these two actions, so it's better to remove xdp_drops statistic > from enetc_xdp_drop() and increase xdp_drops in XDP_DROP action. > > Fixes: 7ed2bc80074e ("net: enetc: add support for XDP_TX") > Signed-off-by: Wei Fang <wei.fang@nxp.com> Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> > --- > drivers/net/ethernet/freescale/enetc/enetc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c > index 032d8eadd003..56e59721ec7d 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > @@ -1521,7 +1521,6 @@ static void enetc_xdp_drop(struct enetc_bdr *rx_ring, int rx_ring_first, > &rx_ring->rx_swbd[rx_ring_first]); > enetc_bdr_idx_inc(rx_ring, &rx_ring_first); > } > - rx_ring->stats.xdp_drops++; > } > > static int enetc_clean_rx_ring_xdp(struct enetc_bdr *rx_ring, > @@ -1586,6 +1585,7 @@ static int enetc_clean_rx_ring_xdp(struct enetc_bdr *rx_ring, > fallthrough; > case XDP_DROP: > enetc_xdp_drop(rx_ring, orig_i, i); > + rx_ring->stats.xdp_drops++; > break; > case XDP_PASS: > rxbd = orig_rxbd; > -- > 2.34.1 > >
On 2024-09-19 at 14:11:02, Wei Fang (wei.fang@nxp.com) wrote: > The xdp_drops statistic indicates the number of XDP frames dropped in > the Rx direction. However, enetc_xdp_drop() is also used in XDP_TX and > XDP_REDIRECT actions. If frame loss occurs in these two actions, the > frames loss count should not be included in xdp_drops, because there > are already xdp_tx_drops and xdp_redirect_failures to count the frame > loss of these two actions, so it's better to remove xdp_drops statistic > from enetc_xdp_drop() and increase xdp_drops in XDP_DROP action. nit: s/xdp_drops/xdp_rx_drops would be appropriate as you have xdp_tx_drops and xdp_redirect_failures.
> -----Original Message----- > From: Ratheesh Kannoth <rkannoth@marvell.com> > Sent: 2024年9月23日 13:03 > To: Wei Fang <wei.fang@nxp.com> > Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > pabeni@redhat.com; Claudiu Manoil <claudiu.manoil@nxp.com>; Vladimir > Oltean <vladimir.oltean@nxp.com>; ast@kernel.org; daniel@iogearbox.net; > hawk@kernel.org; john.fastabend@gmail.com; linux-kernel@vger.kernel.org; > netdev@vger.kernel.org; bpf@vger.kernel.org; stable@vger.kernel.org; > imx@lists.linux.dev > Subject: Re: [PATCH net 1/3] net: enetc: remove xdp_drops statistic from > enetc_xdp_drop() > > On 2024-09-19 at 14:11:02, Wei Fang (wei.fang@nxp.com) wrote: > > The xdp_drops statistic indicates the number of XDP frames dropped in > > the Rx direction. However, enetc_xdp_drop() is also used in XDP_TX and > > XDP_REDIRECT actions. If frame loss occurs in these two actions, the > > frames loss count should not be included in xdp_drops, because there > > are already xdp_tx_drops and xdp_redirect_failures to count the frame > > loss of these two actions, so it's better to remove xdp_drops statistic > > from enetc_xdp_drop() and increase xdp_drops in XDP_DROP action. > nit: s/xdp_drops/xdp_rx_drops would be appropriate as you have > xdp_tx_drops and > xdp_redirect_failures. Sorry, I don't quite understand what you mean.
On Mon, Sep 23, 2024 at 08:53:07AM +0300, Wei Fang wrote: > > -----Original Message----- > > From: Ratheesh Kannoth <rkannoth@marvell.com> > > Sent: 2024年9月23日 13:03 > > To: Wei Fang <wei.fang@nxp.com> > > Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > > pabeni@redhat.com; Claudiu Manoil <claudiu.manoil@nxp.com>; Vladimir > > Oltean <vladimir.oltean@nxp.com>; ast@kernel.org; daniel@iogearbox.net; > > hawk@kernel.org; john.fastabend@gmail.com; linux-kernel@vger.kernel.org; > > netdev@vger.kernel.org; bpf@vger.kernel.org; stable@vger.kernel.org; > > imx@lists.linux.dev > > Subject: Re: [PATCH net 1/3] net: enetc: remove xdp_drops statistic from > > enetc_xdp_drop() > > > > On 2024-09-19 at 14:11:02, Wei Fang (wei.fang@nxp.com) wrote: > > > The xdp_drops statistic indicates the number of XDP frames dropped in > > > the Rx direction. However, enetc_xdp_drop() is also used in XDP_TX and > > > XDP_REDIRECT actions. If frame loss occurs in these two actions, the > > > frames loss count should not be included in xdp_drops, because there > > > are already xdp_tx_drops and xdp_redirect_failures to count the frame > > > loss of these two actions, so it's better to remove xdp_drops statistic > > > from enetc_xdp_drop() and increase xdp_drops in XDP_DROP action. > > nit: s/xdp_drops/xdp_rx_drops would be appropriate as you have > > xdp_tx_drops and > > xdp_redirect_failures. > > Sorry, I don't quite understand what you mean. I don't understand what he means either. I guess he didn't realize you aren't proposing any new name, just working with existing concepts in the driver. Anyway, an ack about this from Ratheesh would be great, to not leave us hanging.
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c index 032d8eadd003..56e59721ec7d 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc.c +++ b/drivers/net/ethernet/freescale/enetc/enetc.c @@ -1521,7 +1521,6 @@ static void enetc_xdp_drop(struct enetc_bdr *rx_ring, int rx_ring_first, &rx_ring->rx_swbd[rx_ring_first]); enetc_bdr_idx_inc(rx_ring, &rx_ring_first); } - rx_ring->stats.xdp_drops++; } static int enetc_clean_rx_ring_xdp(struct enetc_bdr *rx_ring, @@ -1586,6 +1585,7 @@ static int enetc_clean_rx_ring_xdp(struct enetc_bdr *rx_ring, fallthrough; case XDP_DROP: enetc_xdp_drop(rx_ring, orig_i, i); + rx_ring->stats.xdp_drops++; break; case XDP_PASS: rxbd = orig_rxbd;
The xdp_drops statistic indicates the number of XDP frames dropped in the Rx direction. However, enetc_xdp_drop() is also used in XDP_TX and XDP_REDIRECT actions. If frame loss occurs in these two actions, the frames loss count should not be included in xdp_drops, because there are already xdp_tx_drops and xdp_redirect_failures to count the frame loss of these two actions, so it's better to remove xdp_drops statistic from enetc_xdp_drop() and increase xdp_drops in XDP_DROP action. Fixes: 7ed2bc80074e ("net: enetc: add support for XDP_TX") Signed-off-by: Wei Fang <wei.fang@nxp.com> --- drivers/net/ethernet/freescale/enetc/enetc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)