| Message ID | 20240509-gemini-ethernet-fix-tso-v1-1-10cd07b54d1c@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethernet: cortina: TSO and pause param | expand |
On Thu, May 9, 2024 at 9:48 AM Linus Walleij <linus.walleij@linaro.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 ? What number do you have when/if TSO is turned off ? > > Fixes: ac631873c9e7 ("net: ethernet: cortina: Drop TSO support") > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/net/ethernet/cortina/gemini.c | 27 +++++++++++++++++++++++---- > 1 file changed, 23 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c > index c569e5615ecf..599de7914122 100644 > --- a/drivers/net/ethernet/cortina/gemini.c > +++ b/drivers/net/ethernet/cortina/gemini.c > @@ -79,7 +79,8 @@ MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)"); > #define GMAC0_IRQ4_8 (GMAC0_MIB_INT_BIT | GMAC0_RX_OVERRUN_INT_BIT) > > #define GMAC_OFFLOAD_FEATURES (NETIF_F_SG | NETIF_F_IP_CSUM | \ > - NETIF_F_IPV6_CSUM | NETIF_F_RXCSUM) > + NETIF_F_IPV6_CSUM | NETIF_F_RXCSUM | \ > + NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6) > > /** > * struct gmac_queue_page - page buffer per-page info > @@ -1148,13 +1149,29 @@ 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; > + u16 mss; > int ret; > > - /* TODO: implement proper TSO using MTU in word3 */ > word1 = skb->len; > word3 = SOF_BIT; > > - if (skb->len >= ETH_FRAME_LEN) { > + mss = skb_shinfo(skb)->gso_size; > + if (mss) { > + /* 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; > + } else { > + mss = skb->len; > + } > + > + /* 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' ? 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) > + > + 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 > @@ -1169,7 +1186,9 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb, > return ret; > } > word1 |= TSS_BYPASS_BIT; > - } else if (skb->ip_summed == CHECKSUM_PARTIAL) { > + } > + > + if (skb->ip_summed == CHECKSUM_PARTIAL) { > int tcp = 0; > > /* We do not switch off the checksumming on non TCP/UDP > > -- > 2.45.0 >
On Thu, May 9, 2024 at 10:21 AM Eric Dumazet <edumazet@google.com> wrote: > On Thu, May 9, 2024 at 9:48 AM Linus Walleij <linus.walleij@linaro.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! Yours, Linus Walleij
On Thu, May 9, 2024 at 4:38 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Thu, May 9, 2024 at 10:21 AM Eric Dumazet <edumazet@google.com> wrote: > > On Thu, May 9, 2024 at 9:48 AM Linus Walleij <linus.walleij@linaro.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.
On Thu, May 9, 2024 at 4:49 PM Eric Dumazet <edumazet@google.com> wrote: > > On Thu, May 9, 2024 at 4:38 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > On Thu, May 9, 2024 at 10:21 AM Eric Dumazet <edumazet@google.com> wrote: > > > On Thu, May 9, 2024 at 9:48 AM Linus Walleij <linus.walleij@linaro.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
On Thu, May 9, 2024 at 5:06 PM Eric Dumazet <edumazet@google.com> wrote: > > On Thu, May 9, 2024 at 4:49 PM Eric Dumazet <edumazet@google.com> wrote: > > > > On Thu, May 9, 2024 at 4:38 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > > > On Thu, May 9, 2024 at 10:21 AM Eric Dumazet <edumazet@google.com> wrote: > > > > On Thu, May 9, 2024 at 9:48 AM Linus Walleij <linus.walleij@linaro.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) { Also this last check makes little sense, because normal MTU/MSS would trigger this condition. Forcing software checksumming would make TSO useless. I suspect this code is there because of the old buggy TSO implementation of this driver.
On Thu, May 9, 2024 at 4:49 PM Eric Dumazet <edumazet@google.com> wrote: > On Thu, May 9, 2024 at 4:38 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > 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 Notice ^^^ "length 1448, that's at the link layer. > > > Fecusia.targus-getdata1: Flags [.], seq 18664:20046, ack 1, win > > 4198, options [nop,nop,TS val 2770370491 ecr 3490176978], length 1382 That is on ... whatever tcpdump decide to show. OK I might not be the best with this tool. > > 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.`.......... Ethernet headers missing here. > > (...) > > 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) I think that is what I get? > If you receive instead ".... length 1382" this means you gave to the > NIC a 'link layer MSS' too small by 66 bytes. I think it's right, I just suck at using tcpdump switches :/ Yours, Linus Walleij
On Thu, May 9, 2024 at 5:10 PM Eric Dumazet <edumazet@google.com> wrote: > > + if (mss >= ETH_FRAME_LEN) { > > Also this last check makes little sense, because normal MTU/MSS would > trigger this condition. > > Forcing software checksumming would make TSO useless. > > I suspect this code is there because of the old buggy TSO > implementation of this driver. I think you're right, when the packets are "pure" TCP let's say. The TSO should just split those. I think the check is needed for e.g. UDP with large payload (such as ping -s 9000....) or those custom packets with funny extra bytes and ethertype that the DSA switch uses. We determined that the HW IP checksummer actually get hiccups when the frame is larger than 1518 bytes so that is why this bypass path is needed. I think I will try to make one "clean path" for just pure TCP and no funky business, and an exceptional path for any other package. Yours, Linus Walleij
diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c index c569e5615ecf..599de7914122 100644 --- a/drivers/net/ethernet/cortina/gemini.c +++ b/drivers/net/ethernet/cortina/gemini.c @@ -79,7 +79,8 @@ MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)"); #define GMAC0_IRQ4_8 (GMAC0_MIB_INT_BIT | GMAC0_RX_OVERRUN_INT_BIT) #define GMAC_OFFLOAD_FEATURES (NETIF_F_SG | NETIF_F_IP_CSUM | \ - NETIF_F_IPV6_CSUM | NETIF_F_RXCSUM) + NETIF_F_IPV6_CSUM | NETIF_F_RXCSUM | \ + NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6) /** * struct gmac_queue_page - page buffer per-page info @@ -1148,13 +1149,29 @@ 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; + u16 mss; int ret; - /* TODO: implement proper TSO using MTU in word3 */ word1 = skb->len; word3 = SOF_BIT; - if (skb->len >= ETH_FRAME_LEN) { + mss = skb_shinfo(skb)->gso_size; + if (mss) { + /* 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; + } else { + mss = skb->len; + } + + /* Translate to link layer size */ + mss += ETH_HLEN; + if (skb->protocol == htons(ETH_P_8021Q)) + mss += VLAN_HLEN; + + 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 @@ -1169,7 +1186,9 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb, return ret; } word1 |= TSS_BYPASS_BIT; - } else if (skb->ip_summed == CHECKSUM_PARTIAL) { + } + + if (skb->ip_summed == CHECKSUM_PARTIAL) { int tcp = 0; /* We do not switch off the checksumming on non TCP/UDP
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. Fixes: ac631873c9e7 ("net: ethernet: cortina: Drop TSO support") Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/net/ethernet/cortina/gemini.c | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-)