| 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: Thu, 9 May 2024 17:06:16 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Hans Ulli Kroll <ulli.kroll@...glemail.com>, "David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 1/2] net: ethernet: cortina: Restore TSO support
On Thu, May 9, 2024 at 4:49 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Thu, May 9, 2024 at 4:38 PM Linus Walleij <linus.walleij@...aro.org> wrote:
> >
> > On Thu, May 9, 2024 at 10:21 AM Eric Dumazet <edumazet@...gle.com> wrote:
> > > On Thu, May 9, 2024 at 9:48 AM Linus Walleij <linus.walleij@...aro.org> wrote:
> > > >
> > > > An earlier commit deleted the TSO support in the Cortina Gemini
> > > > driver because the driver was confusing gso_size and MTU,
> > > > probably because what the Linux kernel calls "gso_size" was
> > > > called "MTU" in the datasheet.
> > > >
> > > > Restore the functionality properly reading the gso_size from
> > > > the skbuff.
> > > >
> > > > Tested with iperf3, running a server on a different machine
> > > > and client on the device with the cortina gemini ethernet:
> > > >
> > > > Connecting to host 192.168.1.2, port 5201
> > > > 60008000.ethernet-port eth0: segment offloading mss = 05a8 len=1c8a
> > > > 60008000.ethernet-port eth0: segment offloading mss = 05a8 len=1c8a
> > > > 60008000.ethernet-port eth0: segment offloading mss = 05a8 len=27da
> > > > 60008000.ethernet-port eth0: segment offloading mss = 05a8 len=0b92
> > > > 60008000.ethernet-port eth0: segment offloading mss = 05a8 len=2bda
> > > > (...)
> > > >
> > > > It also performs well: ~268 MBit/s.
> > >
> > > This does not look very good to me ?
> >
> > Oh it's pretty typical. This is an ARMv4 router from 2007, end-of-lifed
> > in 2015, and it is not meant to be stressed by the software like
> > this, the idea is that packets get routed by the DSA switch
> > (RTL8366RB).
> >
> > > What number do you have when/if TSO is turned off ?
> >
> > Around 187 MBit/s.
> >
> > > > + /* Translate to link layer size */
> > > > + mss += ETH_HLEN;
> > > > + if (skb->protocol == htons(ETH_P_8021Q))
> > > > + mss += VLAN_HLEN;
> > >
> > > Are you sure this is needed at all ?
> > > Why not include IP and TCP header sizes as well, if the datasheet
> > > mentions 'link layer size' ?
> >
> > Actually that code is just reusing the mss variable for
> > skb->len in the case where TSO is not used, so I'll try to
> > be more elaborate in the code :/
> >
> > I guess I actually need to account for it if ->gso_size expand
> > to the MTU of the interface if I bump it up. But I don't
> > know if the the TSO code actually does this or if it is
> > more conservative?
> >
> > > To double check, please disable GRO on the receive side and verify the
> > > packet sizes with tcpdump.
> > >
> > > Typically, for MTU=1500, IPv4, and TCP timestamp enabled,
> > > skb_shinfo(skb)->gso_size is 1448
> > >
> > > (Because 20 (ipv4 header) + 32 (tcp header with TS option) + 1448 = 1500)
> >
> > I disabled all segment offloading on the receiving side:
> > ethtool -K enp2s0 gro off gso off tso off
>
> >
> > The iperf3 -c generates segmens like in the commit message:
> > gemini-ethernet-port 60008000.ethernet-port eth0: segment offloading
> > mss = 05a8 len=2bda
> > gemini-ethernet-port 60008000.ethernet-port eth0: segment offloading
> > mss = 05a8 len=27da
> > gemini-ethernet-port 60008000.ethernet-port eth0: segment offloading
> > mss = 05a8 len=0b92
> >
> > And 05a8 is 1448 so it is expected.
> >
> > tcpdump -e -X enp2s0 gives this on a single segment in a segmented
> > iperf3 -c transfer:
> >
> > 16:24:09.182095 14:d6:4d:a8:3c:4f (oui Unknown) > fc:34:97:01:a0:c6
> > (oui Unknown), ethertype IPv4 (0x0800), length 1448: OpenWrt.lan.56624
> > > Fecusia.targus-getdata1: Flags [.], seq 18664:20046, ack 1, win
> > 4198, options [nop,nop,TS val 2770370491 ecr 3490176978], length 1382
> > 0x0000: 4500 059a 8ff6 4000 4006 218d c0a8 0188 E.....@.@.......
> > 0x0010: c0a8 0102 dd30 1451 a701 4f9d e809 8788 .....0.Q..O.....
> > 0x0020: 8010 1066 0b60 0000 0101 080a a520 7fbb ...f.`..........
> > (...)
> > 0x0580: de60 2081 5678 4f8b 31b1 6f85 87fe ae63 .`..VxO.1.o....c
> > 0x0590: e2ca 8281 fa72 16aa 52e2 .....r..R.
> >
> > As can be seen in the header, it is indeed 1448 bytes when arriving
> > as well, so it seems to work!
>
> Not really.
>
> Try to disable TSO, and look at the resulting incoming packets, how
> they are different.
>
> If skb_shinfo(skb)->gso_size is 1448, you should receive something like
>
> seq 18664:20112 .... length 1448 (this is the payload len at this stage)
>
> If you receive instead ".... length 1382" this means you gave to the
> NIC a 'link layer MSS' too small by 66 bytes.
I would use something like this (modulo GMAC_OFFLOAD_FEATURES changes)
diff --git a/drivers/net/ethernet/cortina/gemini.c
b/drivers/net/ethernet/cortina/gemini.c
index 2f98f644b9d7b5e48c4983dd2450a8c10fe04008..88cddd73851215666c26a10da8223c0fa0ac5473
100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -1143,13 +1143,21 @@ static int gmac_map_tx_bufs(struct net_device
*netdev, struct sk_buff *skb,
skb_frag_t *skb_frag;
dma_addr_t mapping;
void *buffer;
+ u32 mss;
int ret;
- /* TODO: implement proper TSO using MTU in word3 */
- word1 = skb->len;
+ word1 = mss = skb->len;
word3 = SOF_BIT;
- if (skb->len >= ETH_FRAME_LEN) {
+ if (skb_is_gso(skb)) {
+ mss = skb_shinfo(skb)->gso_size + skb_tcp_all_headers(skb);
+ /* skb->len will be all segments in this case */
+ netdev_dbg(netdev, "segment offloading mss = %04x len=%04x\n",
+ mss, skb->len);
+ word1 |= TSS_MTU_ENABLE_BIT;
+ word3 |= mss;
+ }
+ if (mss >= ETH_FRAME_LEN) {
/* Hardware offloaded checksumming isn't working on frames
* bigger than 1514 bytes. A hypothesis about this is that the
* checksum buffer is only 1518 bytes, so when the frames get
Powered by blists - more mailing lists