| Message ID | 1441156443-33381-6-git-send-email-jeffrey.t.kirsher@intel.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Sep 1, 2015 at 6:13 PM, Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote: > From: Mark Rustad <mark.d.rustad@intel.com> > > By using GSO for UDP-encapsulated packets, all ixgbe devices can > be directed to generate checksums for the inner headers because > the outer UDP checksum can be zero. So point the machinery at the > inner headers and have the hardware generate the checksum. > I suspect this is not UDP-encapsulation specific, will it work with TCP/IP/IP, TCP/IP/GRE etc.? Isn't there anyway the ixgbe could just be made to NETIF_HW_CSUM? That would be so much more straightforward and support nearly all use cases without needing to jump through all these hoops. Tom > Signed-off-by: Mark Rustad <mark.d.rustad@intel.com> > Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > --- > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 66 ++++++++++++++++++++++----- > 1 file changed, 54 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > index ab28dc2..900562e 100644 > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > @@ -6912,31 +6912,55 @@ static void ixgbe_tx_csum(struct ixgbe_ring *tx_ring, > if (!(first->tx_flags & IXGBE_TX_FLAGS_HW_VLAN) && > !(first->tx_flags & IXGBE_TX_FLAGS_CC)) > return; > + vlan_macip_lens = skb_network_offset(skb) << > + IXGBE_ADVTXD_MACLEN_SHIFT; > } else { > u8 l4_hdr = 0; > - switch (first->protocol) { > - case htons(ETH_P_IP): > - vlan_macip_lens |= skb_network_header_len(skb); > + union { > + struct iphdr *ipv4; > + struct ipv6hdr *ipv6; > + u8 *raw; > + } network_hdr; > + union { > + struct tcphdr *tcphdr; > + u8 *raw; > + } transport_hdr; > + > + if (skb->encapsulation) { > + network_hdr.raw = skb_inner_network_header(skb); > + transport_hdr.raw = skb_inner_transport_header(skb); > + vlan_macip_lens = skb_inner_network_offset(skb) << > + IXGBE_ADVTXD_MACLEN_SHIFT; > + } else { > + network_hdr.raw = skb_network_header(skb); > + transport_hdr.raw = skb_transport_header(skb); > + vlan_macip_lens = skb_network_offset(skb) << > + IXGBE_ADVTXD_MACLEN_SHIFT; > + } > + > + /* use first 4 bits to determine IP version */ > + switch (network_hdr.ipv4->version) { > + case IPVERSION: > + vlan_macip_lens |= transport_hdr.raw - network_hdr.raw; > type_tucmd |= IXGBE_ADVTXD_TUCMD_IPV4; > - l4_hdr = ip_hdr(skb)->protocol; > + l4_hdr = network_hdr.ipv4->protocol; > break; > - case htons(ETH_P_IPV6): > - vlan_macip_lens |= skb_network_header_len(skb); > - l4_hdr = ipv6_hdr(skb)->nexthdr; > + case 6: > + vlan_macip_lens |= transport_hdr.raw - network_hdr.raw; > + l4_hdr = network_hdr.ipv6->nexthdr; > break; > default: > if (unlikely(net_ratelimit())) { > dev_warn(tx_ring->dev, > - "partial checksum but proto=%x!\n", > - first->protocol); > + "partial checksum but version=%d\n", > + network_hdr.ipv4->version); > } > - break; > } > > switch (l4_hdr) { > case IPPROTO_TCP: > type_tucmd |= IXGBE_ADVTXD_TUCMD_L4T_TCP; > - mss_l4len_idx = tcp_hdrlen(skb) << > + mss_l4len_idx = (transport_hdr.tcphdr->doff * 4) << > IXGBE_ADVTXD_L4LEN_SHIFT; > break; > case IPPROTO_SCTP: > @@ -6962,7 +6986,6 @@ static void ixgbe_tx_csum(struct ixgbe_ring *tx_ring, > } > > /* vlan_macip_lens: MACLEN, VLAN tag */ > - vlan_macip_lens |= skb_network_offset(skb) << IXGBE_ADVTXD_MACLEN_SHIFT; > vlan_macip_lens |= first->tx_flags & IXGBE_TX_FLAGS_VLAN_MASK; > > ixgbe_tx_ctxtdesc(tx_ring, vlan_macip_lens, 0, > @@ -8207,6 +8230,21 @@ static void ixgbe_fwd_del(struct net_device *pdev, void *priv) > kfree(fwd_adapter); > } > > +#define IXGBE_MAX_TUNNEL_HDR_LEN 80 > +static netdev_features_t > +ixgbe_features_check(struct sk_buff *skb, struct net_device *dev, > + netdev_features_t features) > +{ > + if (!skb->encapsulation) > + return features; > + > + if (unlikely(skb_inner_mac_header(skb) - skb_transport_header(skb) > > + IXGBE_MAX_TUNNEL_HDR_LEN)) > + return features & ~NETIF_F_ALL_CSUM; > + > + return features; > +} > + > static const struct net_device_ops ixgbe_netdev_ops = { > .ndo_open = ixgbe_open, > .ndo_stop = ixgbe_close, > @@ -8254,6 +8292,7 @@ static const struct net_device_ops ixgbe_netdev_ops = { > .ndo_dfwd_del_station = ixgbe_fwd_del, > .ndo_add_vxlan_port = ixgbe_add_vxlan_port, > .ndo_del_vxlan_port = ixgbe_del_vxlan_port, > + .ndo_features_check = ixgbe_features_check, > }; > > /** > @@ -8613,6 +8652,9 @@ skip_sriov: > netdev->vlan_features |= NETIF_F_IPV6_CSUM; > netdev->vlan_features |= NETIF_F_SG; > > + netdev->hw_enc_features |= NETIF_F_SG | NETIF_F_IP_CSUM | > + NETIF_F_IPV6_CSUM; > + > netdev->priv_flags |= IFF_UNICAST_FLT; > netdev->priv_flags |= IFF_SUPP_NOFCS; > > -- > 2.4.3 > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Sep 1, 2015, at 8:17 PM, Tom Herbert <tom@herbertland.com> wrote: > > I suspect this is not UDP-encapsulation specific, will it work with > TCP/IP/IP, TCP/IP/GRE etc.? It could do more, but this is what has been tested up to this point. > Isn't there anyway the ixgbe could just be made to NETIF_HW_CSUM? That > would be so much more straightforward and support nearly all use cases > without needing to jump through all these hoops. Well, the description says: --- Note: NETIF_F_HW_CSUM is a superset of NETIF_F_IP_CSUM + NETIF_F_IPV6_CSUM. It means that device can fill TCP/UDP-like checksum anywhere in the packets whatever headers there might be. --- The device can't do whatever, wherever. There is always a limit to the offset to the inner headers that can be handled, for instance. -- Mark Rustad, Networking Division, Intel Corporation
On Wed, Sep 2, 2015 at 9:46 AM, Rustad, Mark D <mark.d.rustad@intel.com> wrote: >> On Sep 1, 2015, at 8:17 PM, Tom Herbert <tom@herbertland.com> wrote: >> >> I suspect this is not UDP-encapsulation specific, will it work with >> TCP/IP/IP, TCP/IP/GRE etc.? > > It could do more, but this is what has been tested up to this point. > Well, please test the those other encapsulations too! It's nice and all if they get the benefit, but it's really bad news if these changes were to screw them up (i.e. you don't want users of the GRE, IPIP to find out that they're now broken). >> Isn't there anyway the ixgbe could just be made to NETIF_HW_CSUM? That >> would be so much more straightforward and support nearly all use cases >> without needing to jump through all these hoops. > > Well, the description says: > > --- > Note: NETIF_F_HW_CSUM is a superset of NETIF_F_IP_CSUM + NETIF_F_IPV6_CSUM. > It means that device can fill TCP/UDP-like checksum anywhere in the packets > whatever headers there might be. > --- > > The device can't do whatever, wherever. There is always a limit to the offset to the inner headers that can be handled, for instance. > If the device does NETIF_F_HW_CSUM then inner/outer headers are irrelevant at least in the non-GSO case. All the device needs to do is compute the checksum from start and write the answer at the given offset. No protocol awareness needed in the device, no need to parse headers on transmit. I have the same complaint that ixgbe requires a bunch of driver logic to offload VXLAN checksum unnecessary instead of just providing CHECKSUM_COMPLETE which would work with any encapsulation protocol, require no encapsulation awareness in the device, and should be a much simpler driver implementation. So my input to NIC vendors will continue to be they provide general protocol agnostic solutions and *stop* perpetuating these narrow protocol specific and unnecessarily complicated solutions. If you don't believe me, see the similar longstanding comments in skbuff.h about NIC capabilities and checksums and what choices vendors make. Tom > -- > Mark Rustad, Networking Division, Intel Corporation > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 2, 2015 at 8:38 PM, Tom Herbert <tom@herbertland.com> wrote: > On Wed, Sep 2, 2015 at 9:46 AM, Rustad, Mark D <mark.d.rustad@intel.com> wrote: >>> On Sep 1, 2015, at 8:17 PM, Tom Herbert <tom@herbertland.com> wrote: >>> >>> I suspect this is not UDP-encapsulation specific, will it work with >>> TCP/IP/IP, TCP/IP/GRE etc.? >> >> It could do more, but this is what has been tested up to this point. >> > Well, please test the those other encapsulations too! It's nice and > all if they get the benefit, but it's really bad news if these changes > were to screw them up (i.e. you don't want users of the GRE, IPIP to > find out that they're now broken). > >>> Isn't there anyway the ixgbe could just be made to NETIF_HW_CSUM? That >>> would be so much more straightforward and support nearly all use cases >>> without needing to jump through all these hoops. >> >> Well, the description says: >> >> --- >> Note: NETIF_F_HW_CSUM is a superset of NETIF_F_IP_CSUM + NETIF_F_IPV6_CSUM. >> It means that device can fill TCP/UDP-like checksum anywhere in the packets >> whatever headers there might be. >> --- >> >> The device can't do whatever, wherever. There is always a limit to the offset to the inner headers that can be handled, for instance. >> > If the device does NETIF_F_HW_CSUM then inner/outer headers are > irrelevant at least in the non-GSO case. All the device needs to do is > compute the checksum from start and write the answer at the given > offset. No protocol awareness needed in the device, no need to parse > headers on transmit. Tom, could you elaborate a little further on the semantics/requirements for devices supporting NETIF_F_HW_CSUM, clearly (as mentioned in > I have the same complaint that ixgbe requires a bunch of driver logic > to offload VXLAN checksum unnecessary instead of just providing > CHECKSUM_COMPLETE which would work with any encapsulation protocol, > require no encapsulation awareness in the device, and should be a much > simpler driver implementation. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 2, 2015 at 8:38 PM, Tom Herbert <tom@herbertland.com> wrote: > On Wed, Sep 2, 2015 at 9:46 AM, Rustad, Mark D <mark.d.rustad@intel.com> wrote: >> Note: NETIF_F_HW_CSUM is a superset of NETIF_F_IP_CSUM + NETIF_F_IPV6_CSUM. >> It means that device can fill TCP/UDP-like checksum anywhere in the packets >> whatever headers there might be. >> The device can't do whatever, wherever. There is always a limit to the offset to the inner headers that can be handled, for instance. > If the device does NETIF_F_HW_CSUM then inner/outer headers are > irrelevant at least in the non-GSO case. All the device needs to do is > compute the checksum from start and write the answer at the given > offset. No protocol awareness needed in the device, no need to parse > headers on transmit. Tom, could you elaborate a little further on the semantics/requirements for devices supporting NETIF_F_HW_CSUM, specifically, AFAIU this isn't a TX equivalent of supporting checksum complete on RX, right? when you say "write the answer at the given offset" what non-common answers are you expecting devices to produce? how the kernel is hinting to the device on the nature on the expected answer beyond the offset? Or. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 2, 2015 at 2:07 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote: > On Wed, Sep 2, 2015 at 8:38 PM, Tom Herbert <tom@herbertland.com> wrote: >> On Wed, Sep 2, 2015 at 9:46 AM, Rustad, Mark D <mark.d.rustad@intel.com> wrote: > >>> Note: NETIF_F_HW_CSUM is a superset of NETIF_F_IP_CSUM + NETIF_F_IPV6_CSUM. >>> It means that device can fill TCP/UDP-like checksum anywhere in the packets >>> whatever headers there might be. > >>> The device can't do whatever, wherever. There is always a limit to the offset to the inner headers that can be handled, for instance. > >> If the device does NETIF_F_HW_CSUM then inner/outer headers are >> irrelevant at least in the non-GSO case. All the device needs to do is >> compute the checksum from start and write the answer at the given >> offset. No protocol awareness needed in the device, no need to parse >> headers on transmit. > > Tom, could you elaborate a little further on the > semantics/requirements for devices supporting NETIF_F_HW_CSUM, > specifically, AFAIU this isn't a TX equivalent of supporting checksum > complete on RX, right? when you say "write the answer at the given > offset" what non-common answers are you expecting devices to produce? > how the kernel is hinting to the device on the nature on the expected > answer beyond the offset? > NETIF_F_HW_CSUM indicates that the device/driver is will to implement CHECKSUM_PARTIAL on out for the general case. CHECKSUM_PARTIAL is described in skbuff.h as: The device is required to checksum the packet as seen by hard_start_xmit() from skb->csum_start up to the end, and to record/write the checksum at offset skb->csum_start + skb->csum_offset. For instance, if we want to offload an inner checksum the stack would set csum_start to the offset of the the inner transport packet and csum_offset to the relative offset of the checksum field. The stack takes care of priming the checksum field with the not of pseudo header if the transport protocol needs that. Tom > Or. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 2, 2015 at 9:46 AM, Rustad, Mark D <mark.d.rustad@intel.com> wrote: >> On Sep 1, 2015, at 8:17 PM, Tom Herbert <tom@herbertland.com> wrote: >> >> I suspect this is not UDP-encapsulation specific, will it work with >> TCP/IP/IP, TCP/IP/GRE etc.? > Mark, another question in this area of code. Looking at ixgbe_tx_csum, I'm wondering what happens with those default cases for the switch statements. If those are hit for whatever reason does that mean the checksum is never resolved? It seems like if the device couldn't handle these cases then skb_checksum_help should be called to set the checksum. In particular I am wondering what happens in the case that a TCP or UDP packet is sent in IPv6 with an extension header present (so default is taken in switch (l4_hdr)). Would the checksum be properly set in this case? Thanks, Tom > It could do more, but this is what has been tested up to this point. > >> Isn't there anyway the ixgbe could just be made to NETIF_HW_CSUM? That >> would be so much more straightforward and support nearly all use cases >> without needing to jump through all these hoops. > > Well, the description says: > > --- > Note: NETIF_F_HW_CSUM is a superset of NETIF_F_IP_CSUM + NETIF_F_IPV6_CSUM. > It means that device can fill TCP/UDP-like checksum anywhere in the packets > whatever headers there might be. > --- > > The device can't do whatever, wherever. There is always a limit to the offset to the inner headers that can be handled, for instance. > > -- > Mark Rustad, Networking Division, Intel Corporation > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Sep 2, 2015, at 4:21 PM, Tom Herbert <tom@herbertland.com> wrote: > > Mark, another question in this area of code. Looking at ixgbe_tx_csum, > I'm wondering what happens with those default cases for the switch > statements. If those are hit for whatever reason does that mean the > checksum is never resolved? It seems like if the device couldn't > handle these cases then skb_checksum_help should be called to set the > checksum. In particular I am wondering what happens in the case that a > TCP or UDP packet is sent in IPv6 with an extension header present (so > default is taken in switch (l4_hdr)). Would the checksum be properly > set in this case? I will look further into this, but in a first look it appears that you are right and that it has been this way for some time. -- Mark Rustad, Networking Division, Intel Corporation
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c index ab28dc2..900562e 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c @@ -6912,31 +6912,55 @@ static void ixgbe_tx_csum(struct ixgbe_ring *tx_ring, if (!(first->tx_flags & IXGBE_TX_FLAGS_HW_VLAN) && !(first->tx_flags & IXGBE_TX_FLAGS_CC)) return; + vlan_macip_lens = skb_network_offset(skb) << + IXGBE_ADVTXD_MACLEN_SHIFT; } else { u8 l4_hdr = 0; - switch (first->protocol) { - case htons(ETH_P_IP): - vlan_macip_lens |= skb_network_header_len(skb); + union { + struct iphdr *ipv4; + struct ipv6hdr *ipv6; + u8 *raw; + } network_hdr; + union { + struct tcphdr *tcphdr; + u8 *raw; + } transport_hdr; + + if (skb->encapsulation) { + network_hdr.raw = skb_inner_network_header(skb); + transport_hdr.raw = skb_inner_transport_header(skb); + vlan_macip_lens = skb_inner_network_offset(skb) << + IXGBE_ADVTXD_MACLEN_SHIFT; + } else { + network_hdr.raw = skb_network_header(skb); + transport_hdr.raw = skb_transport_header(skb); + vlan_macip_lens = skb_network_offset(skb) << + IXGBE_ADVTXD_MACLEN_SHIFT; + } + + /* use first 4 bits to determine IP version */ + switch (network_hdr.ipv4->version) { + case IPVERSION: + vlan_macip_lens |= transport_hdr.raw - network_hdr.raw; type_tucmd |= IXGBE_ADVTXD_TUCMD_IPV4; - l4_hdr = ip_hdr(skb)->protocol; + l4_hdr = network_hdr.ipv4->protocol; break; - case htons(ETH_P_IPV6): - vlan_macip_lens |= skb_network_header_len(skb); - l4_hdr = ipv6_hdr(skb)->nexthdr; + case 6: + vlan_macip_lens |= transport_hdr.raw - network_hdr.raw; + l4_hdr = network_hdr.ipv6->nexthdr; break; default: if (unlikely(net_ratelimit())) { dev_warn(tx_ring->dev, - "partial checksum but proto=%x!\n", - first->protocol); + "partial checksum but version=%d\n", + network_hdr.ipv4->version); } - break; } switch (l4_hdr) { case IPPROTO_TCP: type_tucmd |= IXGBE_ADVTXD_TUCMD_L4T_TCP; - mss_l4len_idx = tcp_hdrlen(skb) << + mss_l4len_idx = (transport_hdr.tcphdr->doff * 4) << IXGBE_ADVTXD_L4LEN_SHIFT; break; case IPPROTO_SCTP: @@ -6962,7 +6986,6 @@ static void ixgbe_tx_csum(struct ixgbe_ring *tx_ring, } /* vlan_macip_lens: MACLEN, VLAN tag */ - vlan_macip_lens |= skb_network_offset(skb) << IXGBE_ADVTXD_MACLEN_SHIFT; vlan_macip_lens |= first->tx_flags & IXGBE_TX_FLAGS_VLAN_MASK; ixgbe_tx_ctxtdesc(tx_ring, vlan_macip_lens, 0, @@ -8207,6 +8230,21 @@ static void ixgbe_fwd_del(struct net_device *pdev, void *priv) kfree(fwd_adapter); } +#define IXGBE_MAX_TUNNEL_HDR_LEN 80 +static netdev_features_t +ixgbe_features_check(struct sk_buff *skb, struct net_device *dev, + netdev_features_t features) +{ + if (!skb->encapsulation) + return features; + + if (unlikely(skb_inner_mac_header(skb) - skb_transport_header(skb) > + IXGBE_MAX_TUNNEL_HDR_LEN)) + return features & ~NETIF_F_ALL_CSUM; + + return features; +} + static const struct net_device_ops ixgbe_netdev_ops = { .ndo_open = ixgbe_open, .ndo_stop = ixgbe_close, @@ -8254,6 +8292,7 @@ static const struct net_device_ops ixgbe_netdev_ops = { .ndo_dfwd_del_station = ixgbe_fwd_del, .ndo_add_vxlan_port = ixgbe_add_vxlan_port, .ndo_del_vxlan_port = ixgbe_del_vxlan_port, + .ndo_features_check = ixgbe_features_check, }; /** @@ -8613,6 +8652,9 @@ skip_sriov: netdev->vlan_features |= NETIF_F_IPV6_CSUM; netdev->vlan_features |= NETIF_F_SG; + netdev->hw_enc_features |= NETIF_F_SG | NETIF_F_IP_CSUM | + NETIF_F_IPV6_CSUM; + netdev->priv_flags |= IFF_UNICAST_FLT; netdev->priv_flags |= IFF_SUPP_NOFCS;