| 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
| ||
|
Message-ID: <8d65f680-f5b9-411e-b71c-122d48240b6e@molgen.mpg.de>
Date: Fri, 13 Dec 2024 09:09:43 +0100
From: Paul Menzel <pmenzel@...gen.mpg.de>
To: Song Yoong Siang <yoong.siang.song@...el.com>
Cc: Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
"David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
Jesper Dangaard Brouer <hawk@...nel.org>,
John Fastabend <john.fastabend@...il.com>,
Tony Nguyen <anthony.l.nguyen@...el.com>,
Przemek Kitszel <przemyslaw.kitszel@...el.com>,
Andrew Lunn <andrew+netdev@...n.ch>, Eric Dumazet <edumazet@...gle.com>,
Paolo Abeni <pabeni@...hat.com>,
Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
Vinicius Costa Gomes <vinicius.gomes@...el.com>,
intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [Intel-wired-lan] [PATCH iwl-next v2 1/1] igc: Improve
XDP_SETUP_PROG process
Dear Song,
Thank you for your patch. Maybe for the summary/title you could be more
specific:
igc: Avoid unnecessary link down event in XDP_SETUP_PROG process
Am 11.12.24 um 14:45 schrieb Song Yoong Siang:
> Improve XDP_SETUP_PROG process by avoiding unnecessary link down event.
>
> This patch is tested by using ip link set xdpdrv command to attach a simple
> XDP program which always return XDP_PASS.
return*s*
> Before this patch, attaching xdp program will cause ptp4l to lost sync for
to lose
> few seconds, as shown in ptp4l log below:
> ptp4l[198.082]: rms 4 max 8 freq +906 +/- 2 delay 12 +/- 0
> ptp4l[199.082]: rms 3 max 4 freq +906 +/- 3 delay 12 +/- 0
> ptp4l[199.536]: port 1 (enp2s0): link down
> ptp4l[199.536]: port 1 (enp2s0): SLAVE to FAULTY on FAULT_DETECTED (FT_UNSPECIFIED)
> ptp4l[199.600]: selected local clock 22abbc.fffe.bb1234 as best master
> ptp4l[199.600]: port 1 (enp2s0): assuming the grand master role
> ptp4l[199.600]: port 1 (enp2s0): master state recommended in slave only mode
> ptp4l[199.600]: port 1 (enp2s0): defaultDS.priority1 probably misconfigured
> ptp4l[202.266]: port 1 (enp2s0): link up
> ptp4l[202.300]: port 1 (enp2s0): FAULTY to LISTENING on INIT_COMPLETE
> ptp4l[205.558]: port 1 (enp2s0): new foreign master 44abbc.fffe.bb2144-1
> ptp4l[207.558]: selected best master clock 44abbc.fffe.bb2144
> ptp4l[207.559]: port 1 (enp2s0): LISTENING to UNCALIBRATED on RS_SLAVE
> ptp4l[208.308]: port 1 (enp2s0): UNCALIBRATED to SLAVE on MASTER_CLOCK_SELECTED
> ptp4l[208.933]: rms 742 max 1303 freq -195 +/- 682 delay 12 +/- 0
> ptp4l[209.933]: rms 178 max 274 freq +387 +/- 243 delay 12 +/- 0
>
> After this patch, attaching xdp program no longer cause ptp4l to lost sync,
to lose
> as shown on ptp4l log below:
> ptp4l[201.183]: rms 1 max 3 freq +959 +/- 1 delay 8 +/- 0
> ptp4l[202.183]: rms 1 max 3 freq +961 +/- 2 delay 8 +/- 0
> ptp4l[203.183]: rms 2 max 3 freq +958 +/- 2 delay 8 +/- 0
> ptp4l[204.183]: rms 3 max 5 freq +961 +/- 3 delay 8 +/- 0
> ptp4l[205.183]: rms 2 max 4 freq +964 +/- 3 delay 8 +/- 0
>
> Besides, before this patch, attaching xdp program will cause flood ping to
> loss 10 packets, as shown in ping statistics below:
to lose
> --- 169.254.1.2 ping statistics ---
> 100000 packets transmitted, 99990 received, +6 errors, 0.01% packet loss, time 34001ms
> rtt min/avg/max/mdev = 0.028/0.301/3104.360/13.838 ms, pipe 10, ipg/ewma 0.340/0.243 ms
>
> After this patch, attaching xdp program no longer cause flood ping to loss
cause*s*, to lose
> any packets, as shown in ping statistics below:
> --- 169.254.1.2 ping statistics ---
> 100000 packets transmitted, 100000 received, 0% packet loss, time 32326ms
> rtt min/avg/max/mdev = 0.027/0.231/19.589/0.155 ms, pipe 2, ipg/ewma 0.323/0.322 ms
>
> On the other hand, this patch is also tested with tools/testing/selftests/
> bpf/xdp_hw_metadata app to make sure XDP zero-copy is working fine with
> XDP Tx and Rx metadata. Below is the result of last packet after received
> 10000 UDP packets with interval 1 ms:
> poll: 1 (0) skip=0 fail=0 redir=10000
> xsk_ring_cons__peek: 1
> 0x55881c7ef7a8: rx_desc[9999]->addr=8f110 addr=8f110 comp_addr=8f110 EoP
> rx_hash: 0xFB9BB6A3 with RSS type:0x1
> HW RX-time: 1733923136269470866 (sec:1733923136.2695) delta to User RX-time sec:0.0000 (43.280 usec)
> XDP RX-time: 1733923136269482482 (sec:1733923136.2695) delta to User RX-time sec:0.0000 (31.664 usec)
> No rx_vlan_tci or rx_vlan_proto, err=-95
> 0x55881c7ef7a8: ping-pong with csum=ab19 (want 315b) csum_start=34 csum_offset=6
> 0x55881c7ef7a8: complete tx idx=9999 addr=f010
> HW TX-complete-time: 1733923136269591637 (sec:1733923136.2696) delta to User TX-complete-time sec:0.0001 (108.571 usec)
> XDP RX-time: 1733923136269482482 (sec:1733923136.2695) delta to User TX-complete-time sec:0.0002 (217.726 usec)
> HW RX-time: 1733923136269470866 (sec:1733923136.2695) delta to HW TX-complete-time sec:0.0001 (120.771 usec)
> 0x55881c7ef7a8: complete rx idx=10127 addr=8f110
>
> Signed-off-by: Song Yoong Siang <yoong.siang.song@...el.com>
> ---
> V2 changelog:
> - show some examples of problem in commit msg. (Vinicius)
> - igc_close()/igc_open() are too big a hammer for installing a new XDP
> program. Only do we we really need. (Vinicius)
The first sentence of the second item could go into the commit message
with a note, why `igc_close()`/`igc_open()` are not needed.
> ---
> drivers/net/ethernet/intel/igc/igc_xdp.c | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igc/igc_xdp.c b/drivers/net/ethernet/intel/igc/igc_xdp.c
> index 869815f48ac1..64b04aad614c 100644
> --- a/drivers/net/ethernet/intel/igc/igc_xdp.c
> +++ b/drivers/net/ethernet/intel/igc/igc_xdp.c
> @@ -14,6 +14,7 @@ int igc_xdp_set_prog(struct igc_adapter *adapter, struct bpf_prog *prog,
> bool if_running = netif_running(dev);
> struct bpf_prog *old_prog;
> bool need_update;
> + int i;
I’d use unsigned int as it’s used to access array elements.
>
> if (dev->mtu > ETH_DATA_LEN) {
> /* For now, the driver doesn't support XDP functionality with
> @@ -24,8 +25,13 @@ int igc_xdp_set_prog(struct igc_adapter *adapter, struct bpf_prog *prog,
> }
>
> need_update = !!adapter->xdp_prog != !!prog;
> - if (if_running && need_update)
> - igc_close(dev);
> + if (if_running && need_update) {
> + for (i = 0; i < adapter->num_rx_queues; i++) {
> + igc_disable_rx_ring(adapter->rx_ring[i]);
> + igc_disable_tx_ring(adapter->tx_ring[i]);
> + napi_disable(&adapter->rx_ring[i]->q_vector->napi);
> + }
> + }
>
> old_prog = xchg(&adapter->xdp_prog, prog);
> if (old_prog)
> @@ -36,8 +42,13 @@ int igc_xdp_set_prog(struct igc_adapter *adapter, struct bpf_prog *prog,
> else
> xdp_features_clear_redirect_target(dev);
>
> - if (if_running && need_update)
> - igc_open(dev);
> + if (if_running && need_update) {
> + for (i = 0; i < adapter->num_rx_queues; i++) {
> + napi_enable(&adapter->rx_ring[i]->q_vector->napi);
> + igc_enable_tx_ring(adapter->tx_ring[i]);
> + igc_enable_rx_ring(adapter->rx_ring[i]);
> + }
> + }
>
> return 0;
> }
Kind regards,
Paul
Powered by blists - more mailing lists