RE: [EXT] Re: [PATCH v5 2/3] net: fec: add xdp statistics

[Shenwei Wang]

> -----Original Message-----
> From: Alexander Lobakin <alexandr.lobakin@xxxxxxxxx>
> Sent: Wednesday, November 16, 2022 10:02 AM
> To: Shenwei Wang <shenwei.wang@xxxxxxx>
> Cc: Alexander Lobakin <alexandr.lobakin@xxxxxxxxx>; David S. Miller
> <davem@xxxxxxxxxxxxx>; Eric Dumazet <edumazet@xxxxxxxxxx>; Jakub
> Kicinski <kuba@xxxxxxxxxx>; Paolo Abeni <pabeni@xxxxxxxxxx>; Jesper
> Dangaard Brouer <hawk@xxxxxxxxxx>; Ilias Apalodimas
> <ilias.apalodimas@xxxxxxxxxx>; Alexei Starovoitov <ast@xxxxxxxxxx>; Daniel
> Borkmann <daniel@xxxxxxxxxxxxx>; John Fastabend
> <john.fastabend@xxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx; kernel test robot <lkp@xxxxxxxxx>
> Subject: [EXT] Re: [PATCH v5 2/3] net: fec: add xdp statistics
> 
> Caution: EXT Email
> 
> From: Shenwei Wang <shenwei.wang@xxxxxxx>
> Date: Tue, 15 Nov 2022 14:49:50 -0600
> 
> > Add xdp statistics for ethtool stats and using u64 to record the xdp counters.
> >
> > Signed-off-by: Shenwei Wang <shenwei.wang@xxxxxxx>
> > Reported-by: kernel test robot <lkp@xxxxxxxxx>
> 
> Nit: would be nice if you Cc me for the next submissions as I was commenting on
> the previous ones. Just to make sure reviewers won't miss anything.

Sure of course.

> 
> > ---
> >  drivers/net/ethernet/freescale/fec.h      | 15 +++++
> >  drivers/net/ethernet/freescale/fec_main.c | 74
> > +++++++++++++++++++++--
> >  2 files changed, 83 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/fec.h
> > b/drivers/net/ethernet/freescale/fec.h
> > index 61e847b18343..adbe661552be 100644
> > --- a/drivers/net/ethernet/freescale/fec.h
> > +++ b/drivers/net/ethernet/freescale/fec.h
> > @@ -526,6 +526,19 @@ struct fec_enet_priv_txrx_info {
> >       struct  sk_buff *skb;
> >  };
> >
> > +enum {
> > +     RX_XDP_REDIRECT = 0,
> > +     RX_XDP_PASS,
> > +     RX_XDP_DROP,
> > +     RX_XDP_TX,
> > +     RX_XDP_TX_ERRORS,
> > +     TX_XDP_XMIT,
> > +     TX_XDP_XMIT_ERRORS,
> > +
> > +     /* The following must be the last one */
> > +     XDP_STATS_TOTAL,
> > +};
> > +
> >  struct fec_enet_priv_tx_q {
> >       struct bufdesc_prop bd;
> >       unsigned char *tx_bounce[TX_RING_SIZE]; @@ -546,6 +559,8 @@
> > struct fec_enet_priv_rx_q {
> >       /* page_pool */
> >       struct page_pool *page_pool;
> >       struct xdp_rxq_info xdp_rxq;
> > +     struct u64_stats_sync syncp;
> > +     u64 stats[XDP_STATS_TOTAL];
> 
> Why `u64`? u64_stats infra declares `u64_stat_t` type and a bunch of helpers like
> u64_stats_add(), u64_stats_read() and so on, they will be solved then by the
> compiler to the most appropriate ops for the architecture. So they're more
> "generic" if you prefer.
> Sure, if you show some numbers where `u64_stat_t` is slower than `u64` on your
> machine, then okay, but otherwise...

I will take a leave next week. Will do a compare testing when I come back. Then
we can decide which way to go.

Thanks,
Shenwei 

> 
> >
> >       /* rx queue number, in the range 0-7 */
> >       u8 id;
> 
> [...]
> 
> > --
> > 2.34.1
> 
> Thanks,
> Olek