| Message ID | 20240513-gemini-ethernet-fix-tso-v3-2-b442540cc140@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethernet: cortina: TSO and pause param | expand |
On Mon, May 13, 2024 at 3:38 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > It is possible to push the segment offloader to also > process non-segmented frames: just pass the skb->len > or desired MSS to the offloader and it will handle them. > > This is especially good if the user sets up the MTU > and the frames get big, because the checksumming engine > cannot handle any frames bigger than 1518 bytes, so > segmenting them all to be at max that will be helpful > for the hardware, which only need to quirk odd frames > such as big UDP ping packets. > > The vendor driver always uses the TSO like this, and > the driver seems more stable after this, so apparently > the hardware may have been engineered to always use > the TSO on anything it can handle. We do not copy what vendor drivers do. Please send the first patch as a standalone one, instead of sending a series with controversial changes. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/net/ethernet/cortina/gemini.c | 31 ++++++++++++++++++++++++------- > 1 file changed, 24 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c > index b2ac9dfe1aae..3ba579550cdd 100644 > --- a/drivers/net/ethernet/cortina/gemini.c > +++ b/drivers/net/ethernet/cortina/gemini.c > @@ -1148,6 +1148,7 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb, > struct gmac_txdesc *txd; > skb_frag_t *skb_frag; > dma_addr_t mapping; > + bool tcp = false; > void *buffer; > u16 mss; > int ret; > @@ -1155,6 +1156,13 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb, > word1 = skb->len; > word3 = SOF_BIT; > > + /* Determine if we are doing TCP */ > + if (skb->protocol == htons(ETH_P_IP)) > + tcp = (ip_hdr(skb)->protocol == IPPROTO_TCP); > + else > + /* IPv6 */ > + tcp = (ipv6_hdr(skb)->nexthdr == IPPROTO_TCP); > + > mss = skb_shinfo(skb)->gso_size; > if (mss) { > /* This means we are dealing with TCP and skb->len is the > @@ -1167,6 +1175,20 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb, > mss, skb->len); > word1 |= TSS_MTU_ENABLE_BIT; > word3 |= mss; > + } else if (tcp) { Please do not do this. Let the packet be dropped as it should. Can you share how this path is hit and how you tested it ? > + /* Even if we are not using TSO, use the segment offloader > + * for transferring the TCP frame: the TSO engine will deal > + * with chopping up frames that exceed ETH_DATA_LEN which > + * the checksumming engine cannot handle (see below) into > + * manageable chunks. It flawlessly deals with quite big > + * frames and frames containing custom DSA EtherTypes. > + */ > + mss = netdev->mtu + skb_tcp_all_headers(skb); This is broken anyway. if netdev->mtu is 1500, and skb_tcp_all_headers(skb) is 94, we would ask the NIC to send packets of 1594 bytes... > + mss = min(mss, skb->len); > + netdev_dbg(netdev, "botched TSO len %04x mtu %04x mss %04x\n", > + skb->len, netdev->mtu, mss); > + word1 |= TSS_MTU_ENABLE_BIT; > + word3 |= mss; > } else if (skb->len >= ETH_FRAME_LEN) { > /* Hardware offloaded checksumming isn't working on frames > * bigger than 1514 bytes. A hypothesis about this is that the > @@ -1185,21 +1207,16 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb, > } > > if (skb->ip_summed == CHECKSUM_PARTIAL) { > - int tcp = 0; > - > /* We do not switch off the checksumming on non TCP/UDP > * frames: as is shown from tests, the checksumming engine > * is smart enough to see that a frame is not actually TCP > * or UDP and then just pass it through without any changes > * to the frame. > */ > - if (skb->protocol == htons(ETH_P_IP)) { > + if (skb->protocol == htons(ETH_P_IP)) > word1 |= TSS_IP_CHKSUM_BIT; > - tcp = ip_hdr(skb)->protocol == IPPROTO_TCP; > - } else { /* IPv6 */ > + else > word1 |= TSS_IPV6_ENABLE_BIT; > - tcp = ipv6_hdr(skb)->nexthdr == IPPROTO_TCP; > - } > > word1 |= tcp ? TSS_TCP_CHKSUM_BIT : TSS_UDP_CHKSUM_BIT; > } > > -- > 2.45.0 >
diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c index b2ac9dfe1aae..3ba579550cdd 100644 --- a/drivers/net/ethernet/cortina/gemini.c +++ b/drivers/net/ethernet/cortina/gemini.c @@ -1148,6 +1148,7 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb, struct gmac_txdesc *txd; skb_frag_t *skb_frag; dma_addr_t mapping; + bool tcp = false; void *buffer; u16 mss; int ret; @@ -1155,6 +1156,13 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb, word1 = skb->len; word3 = SOF_BIT; + /* Determine if we are doing TCP */ + if (skb->protocol == htons(ETH_P_IP)) + tcp = (ip_hdr(skb)->protocol == IPPROTO_TCP); + else + /* IPv6 */ + tcp = (ipv6_hdr(skb)->nexthdr == IPPROTO_TCP); + mss = skb_shinfo(skb)->gso_size; if (mss) { /* This means we are dealing with TCP and skb->len is the @@ -1167,6 +1175,20 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb, mss, skb->len); word1 |= TSS_MTU_ENABLE_BIT; word3 |= mss; + } else if (tcp) { + /* Even if we are not using TSO, use the segment offloader + * for transferring the TCP frame: the TSO engine will deal + * with chopping up frames that exceed ETH_DATA_LEN which + * the checksumming engine cannot handle (see below) into + * manageable chunks. It flawlessly deals with quite big + * frames and frames containing custom DSA EtherTypes. + */ + mss = netdev->mtu + skb_tcp_all_headers(skb); + mss = min(mss, skb->len); + netdev_dbg(netdev, "botched TSO len %04x mtu %04x mss %04x\n", + skb->len, netdev->mtu, mss); + word1 |= TSS_MTU_ENABLE_BIT; + word3 |= mss; } else if (skb->len >= ETH_FRAME_LEN) { /* Hardware offloaded checksumming isn't working on frames * bigger than 1514 bytes. A hypothesis about this is that the @@ -1185,21 +1207,16 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb, } if (skb->ip_summed == CHECKSUM_PARTIAL) { - int tcp = 0; - /* We do not switch off the checksumming on non TCP/UDP * frames: as is shown from tests, the checksumming engine * is smart enough to see that a frame is not actually TCP * or UDP and then just pass it through without any changes * to the frame. */ - if (skb->protocol == htons(ETH_P_IP)) { + if (skb->protocol == htons(ETH_P_IP)) word1 |= TSS_IP_CHKSUM_BIT; - tcp = ip_hdr(skb)->protocol == IPPROTO_TCP; - } else { /* IPv6 */ + else word1 |= TSS_IPV6_ENABLE_BIT; - tcp = ipv6_hdr(skb)->nexthdr == IPPROTO_TCP; - } word1 |= tcp ? TSS_TCP_CHKSUM_BIT : TSS_UDP_CHKSUM_BIT; }
It is possible to push the segment offloader to also process non-segmented frames: just pass the skb->len or desired MSS to the offloader and it will handle them. This is especially good if the user sets up the MTU and the frames get big, because the checksumming engine cannot handle any frames bigger than 1518 bytes, so segmenting them all to be at max that will be helpful for the hardware, which only need to quirk odd frames such as big UDP ping packets. The vendor driver always uses the TSO like this, and the driver seems more stable after this, so apparently the hardware may have been engineered to always use the TSO on anything it can handle. Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/net/ethernet/cortina/gemini.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-)