Messages in this thread | | | From | Shenwei Wang <> | Subject | RE: [EXT] Re: [PATCH v3 1/1] net: fec: add xdp and page pool statistics | Date | Mon, 14 Nov 2022 21:17:48 +0000 |
| |
> -----Original Message----- > From: Alexander Lobakin <alexandr.lobakin@intel.com> > Sent: Monday, November 14, 2022 9:23 AM > To: Shenwei Wang <shenwei.wang@nxp.com> > Cc: Alexander Lobakin <alexandr.lobakin@intel.com>; Andrew Lunn > <andrew@lunn.ch>; David S. Miller <davem@davemloft.net>; Eric Dumazet > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni > <pabeni@redhat.com>; Alexei Starovoitov <ast@kernel.org>; Daniel Borkmann > <daniel@iogearbox.net>; Jesper Dangaard Brouer <hawk@kernel.org>; John > Fastabend <john.fastabend@gmail.com>; Wei Fang <wei.fang@nxp.com>; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; imx@lists.linux.dev; > kernel test robot <lkp@intel.com> > Subject: [EXT] Re: [PATCH v3 1/1] net: fec: add xdp and page pool statistics > > Caution: EXT Email > > From: Shenwei Wang <shenwei.wang@nxp.com> > Date: Mon, 14 Nov 2022 15:06:04 +0000 > > > > -----Original Message----- > > > From: Andrew Lunn <andrew@lunn.ch> > > > Sent: Monday, November 14, 2022 8:08 AM > > > To: Alexander Lobakin <alexandr.lobakin@intel.com> > > > Cc: Shenwei Wang <shenwei.wang@nxp.com>; David S. Miller > > > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub > > > Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Alexei > > > Starovoitov <ast@kernel.org>; Daniel Borkmann > > > <daniel@iogearbox.net>; Jesper Dangaard Brouer <hawk@kernel.org>; > > > John Fastabend <john.fastabend@gmail.com>; Wei Fang > > > <wei.fang@nxp.com>; netdev@vger.kernel.org; > > > linux-kernel@vger.kernel.org; imx@lists.linux.dev; kernel test robot > > > <lkp@intel.com> > > > Subject: [EXT] Re: [PATCH v3 1/1] net: fec: add xdp and page pool > > > statistics > > > > > > Caution: EXT Email > > > > > >> Drivers should never select PAGE_POOL_STATS. This Kconfig option > > >> was made to allow user to choose whether he wants stats or better > > >> performance on slower systems. It's pure user choice, if something > > >> doesn't build or link, it must be guarded with > > >> IS_ENABLED(CONFIG_PAGE_POOL_STATS). > > > > > > Given how simple the API is, and the stubs for when > > > CONFIG_PAGE_POOL_STATS is disabled, i doubt there is any need for the > driver to do anything. > > > > > >>> struct page_pool *page_pool; > > >>> struct xdp_rxq_info xdp_rxq; > > >>> + u32 stats[XDP_STATS_TOTAL]; > > >> > > >> Still not convinced it is okay to deliberately provoke overflows > > >> here, maybe we need some more reviewers to help us agree on what is > better? > > > > > > You will find that many embedded drivers only have 32 bit hardware > > > stats and do wrap around. And the hardware does not have atomic read > > > and clear so you can accumulate into a u64. The FEC is from the > > > times of MIB 2 ifTable, which only requires 32 bit counters. ifXtable is > modern compared to the FEC. > > > > > > Software counters like this are a different matter. The overhead of > > > a > > > u64 on a 32 bit system is probably in the noise, so i think there is > > > strong argument for using u64. > > > > If it is required to support u64 counters, the code logic need to > > change to record the counter locally per packet, and then update the > > counters for the fec instance when the napi receive loop is complete. > > In this way we can reduce the performance overhead. > > That's how it is usually done in the drivers. You put u32 counters on the stack, > it's impossible to overflow them in just one NAPI poll cycle. Then, after you're > done with processing descriptors, you just increment the 64-bit on-ring counters > at once. >
Did some testing with the atomic64_t counter, with the following codes to update the u64 counter in the end of every NAPI poll cycle.
@@ -1764,7 +1768,13 @@ 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 1 + if (xdp_prog) { + int i; + for(i = 0; i < XDP_STATS_TOTAL; i++) + atomic64_add(xdp_stats[i], &rxq->stats[i]); + } +#endif return pkt_received; }
With the codes above, the testing result is below: root@imx8qxpc0mek:~/bpf# ./xdpsock -i eth0 sock0@eth0:0 rxdrop xdp-drv pps pkts 1.00 rx 349399 1035008 tx 0 0
sock0@eth0:0 rxdrop xdp-drv pps pkts 1.00 rx 349407 1384640 tx 0 0
Without the atomic_add codes above, the testing result is below: root@imx8qxpc0mek:~/bpf# ./xdpsock -i eth0 sock0@eth0:0 rxdrop xdp-drv pps pkts 1.00 rx 350109 1989130 tx 0 0
sock0@eth0:0 rxdrop xdp-drv pps pkts 1.00 rx 350425 2339786 tx 0 0
And regarding the u32 counter solution, the testing result is below: root@imx8qxpc0mek:~/bpf# ./xdpsock -i eth0 sock0@eth0:0 rxdrop xdp-drv pps pkts 1.00 rx 361347 2637796 tx 0 0
There are about 10K pkts/s difference here. Do we really want the u64 counters?
Regards, Shenwei
> > > > Thanks, > > Shenwei > > > > > > > > Andrew > > Thanks, > Olek
| |