netdev - Re: [PATCH net-next 1/2] net: ethernet: cortina: Restore TSO support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89i+JphFK4TCVjXxbxCicJwrxFC=+ngjnheZWK3KvCJ4Ocg@mail.gmail.com>
Date: Thu, 9 May 2024 16:49:30 +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: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.