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: <CANn89iKgi6yEEenSy1M-PVRYWz=Ri9UorV7irCywOZ8xTbNk_A@mail.gmail.com>
Date: Thu, 9 May 2024 10:21:01 +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 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 ?

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@...aro.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
>