| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 14 Nov 2022 21:17:48 +0000
From: Shenwei Wang <shenwei.wang@....com>
To: Alexander Lobakin <alexandr.lobakin@...el.com>
CC: Andrew Lunn <andrew@...n.ch>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Jesper Dangaard Brouer <hawk@...nel.org>,
John Fastabend <john.fastabend@...il.com>,
Wei Fang <wei.fang@....com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"imx@...ts.linux.dev" <imx@...ts.linux.dev>,
kernel test robot <lkp@...el.com>
Subject: RE: [EXT] Re: [PATCH v3 1/1] net: fec: add xdp and page pool
statistics
> -----Original Message-----
> From: Alexander Lobakin <alexandr.lobakin@...el.com>
> Sent: Monday, November 14, 2022 9:23 AM
> To: Shenwei Wang <shenwei.wang@....com>
> Cc: Alexander Lobakin <alexandr.lobakin@...el.com>; Andrew Lunn
> <andrew@...n.ch>; David S. Miller <davem@...emloft.net>; Eric Dumazet
> <edumazet@...gle.com>; Jakub Kicinski <kuba@...nel.org>; Paolo Abeni
> <pabeni@...hat.com>; Alexei Starovoitov <ast@...nel.org>; Daniel Borkmann
> <daniel@...earbox.net>; Jesper Dangaard Brouer <hawk@...nel.org>; John
> Fastabend <john.fastabend@...il.com>; Wei Fang <wei.fang@....com>;
> netdev@...r.kernel.org; linux-kernel@...r.kernel.org; imx@...ts.linux.dev;
> kernel test robot <lkp@...el.com>
> Subject: [EXT] Re: [PATCH v3 1/1] net: fec: add xdp and page pool statistics
>
> Caution: EXT Email
>
> From: Shenwei Wang <shenwei.wang@....com>
> Date: Mon, 14 Nov 2022 15:06:04 +0000
>
> > > -----Original Message-----
> > > From: Andrew Lunn <andrew@...n.ch>
> > > Sent: Monday, November 14, 2022 8:08 AM
> > > To: Alexander Lobakin <alexandr.lobakin@...el.com>
> > > Cc: Shenwei Wang <shenwei.wang@....com>; David S. Miller
> > > <davem@...emloft.net>; Eric Dumazet <edumazet@...gle.com>; Jakub
> > > Kicinski <kuba@...nel.org>; Paolo Abeni <pabeni@...hat.com>; Alexei
> > > Starovoitov <ast@...nel.org>; Daniel Borkmann
> > > <daniel@...earbox.net>; Jesper Dangaard Brouer <hawk@...nel.org>;
> > > John Fastabend <john.fastabend@...il.com>; Wei Fang
> > > <wei.fang@....com>; netdev@...r.kernel.org;
> > > linux-kernel@...r.kernel.org; imx@...ts.linux.dev; kernel test robot
> > > <lkp@...el.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@...8qxpc0mek:~/bpf# ./xdpsock -i eth0
sock0@...0:0 rxdrop xdp-drv
pps pkts 1.00
rx 349399 1035008
tx 0 0
sock0@...0: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@...8qxpc0mek:~/bpf# ./xdpsock -i eth0
sock0@...0:0 rxdrop xdp-drv
pps pkts 1.00
rx 350109 1989130
tx 0 0
sock0@...0: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@...8qxpc0mek:~/bpf# ./xdpsock -i eth0
sock0@...0: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
Powered by blists - more mailing lists