RE: [EXT] Re: [PATCH v4 2/2] net: fec: add xdp and page pool statistics

[Shenwei Wang]

> -----Original Message-----
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: Tuesday, November 15, 2022 11:29 AM
> To: Shenwei Wang <shenwei.wang@xxxxxxx>
> Cc: 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 v4 2/2] net: fec: add xdp and page pool statistics
> 
> Caution: EXT Email
> 
> > @@ -1582,6 +1586,7 @@ fec_enet_rx_queue(struct net_device *ndev, int
> budget, u16 queue_id)
> >       struct bpf_prog *xdp_prog = READ_ONCE(fep->xdp_prog);
> >       u32 ret, xdp_result = FEC_ENET_XDP_PASS;
> >       u32 data_start = FEC_ENET_XDP_HEADROOM;
> > +     u32 xdp_stats[XDP_STATS_TOTAL];
> >       struct xdp_buff xdp;
> >       struct page *page;
> >       u32 sub_len = 4;
> > @@ -1656,11 +1661,13 @@ fec_enet_rx_queue(struct net_device *ndev, int
> budget, u16 queue_id)
> >               fec_enet_update_cbd(rxq, bdp, index);
> >
> >               if (xdp_prog) {
> > +                     memset(xdp_stats, 0, sizeof(xdp_stats));
> >                       xdp_buff_clear_frags_flag(&xdp);
> >                       /* subtract 16bit shift and FCS */
> >                       xdp_prepare_buff(&xdp, page_address(page),
> >                                        data_start, pkt_len - sub_len, false);
> > -                     ret = fec_enet_run_xdp(fep, xdp_prog, &xdp, rxq, index);
> > +                     ret = fec_enet_run_xdp(fep, xdp_prog, &xdp, rxq,
> > +                                            xdp_stats, index);
> >                       xdp_result |= ret;
> >                       if (ret != FEC_ENET_XDP_PASS)
> >                               goto rx_processing_done; @@ -1762,6
> > +1769,15 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, u16
> queue_id)
> >       if (xdp_result & FEC_ENET_XDP_REDIR)
> >               xdp_do_flush_map();
> >
> > +     if (xdp_prog) {
> > +             int i;
> > +
> > +             u64_stats_update_begin(&rxq->syncp);
> > +             for (i = 0; i < XDP_STATS_TOTAL; i++)
> > +                     rxq->stats[i] += xdp_stats[i];
> > +             u64_stats_update_end(&rxq->syncp);
> > +     }
> > +
> 
> This looks wrong. You are processing upto the napi budget, 64 frames, in a loop.
> The memset to 0 happens inside the loop, but you do the accumulation outside
> the loop?
> 

My bad. That should be moved outside the loop.

Thanks,
Shenwei

> This patch is getting pretty big. Please break it up, at least into one patch for XDP
> stats and one for page pool stats.
> 
>     Andrew