lkml.org 
[lkml]   [2022]   [Nov]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [EXT] Re: [PATCH v3 1/1] net: fec: add xdp and page pool statistics
Date


> -----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

\
 
 \ /
  Last update: 2022-11-14 22:18    [W:0.457 / U:0.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site