| Message ID | 20160425183133.11331.54774.stgit@ahduyck-xeon-server |
|---|---|
| State | Deferred, archived |
| Delegated to: | David Miller |
| Headers | show |
On 4/25/2016 9:31 PM, Alexander Duyck wrote: > >From what I can tell the ConnectX-3 will support an inner IPv6 checksum and > segmentation offload, however it cannot support outer IPv6 headers. For > this reason I am adding the feature to the hw_enc_features and adding an > extra check to the features_check call that will disable GSO and checksum > offload in the case that the encapsulated frame has an outer IP version of > that is not 4. Hi Alex, Can you share the testing commands of running vxlan over IPv6 and what exactly didn't work for you ? we would like to test this in house and understand what went wrong, theoretically there shouldn't be a difference between IPv6/IPv4 outer checksum offloading in ConnectX-3. Anyway, I suspect it might be related to a driver bug most likely in get_real_size function @en_tx.c specifically in : *lso_header_size = (skb_inner_transport_header(skb) - skb->data) + inner_tcp_hdrlen(skb); will check this and get back to you. for the mlx5 patches I will also go through them later today. > Signed-off-by: Alexander Duyck <aduyck@mirantis.com> > --- > drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 25 +++++++++++++++++++----- > drivers/net/ethernet/mellanox/mlx4/en_tx.c | 15 ++++++++++++-- > 2 files changed, 33 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c > index bce37cbfde24..6f28ac58251c 100644 > --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c > +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c > @@ -2357,8 +2357,10 @@ out: > } > > /* set offloads */ > - priv->dev->hw_enc_features |= NETIF_F_IP_CSUM | NETIF_F_RXCSUM | > - NETIF_F_TSO | NETIF_F_GSO_UDP_TUNNEL | > + priv->dev->hw_enc_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | > + NETIF_F_RXCSUM | > + NETIF_F_TSO | NETIF_F_TSO6 | > + NETIF_F_GSO_UDP_TUNNEL | > NETIF_F_GSO_UDP_TUNNEL_CSUM | > NETIF_F_GSO_PARTIAL; > } > @@ -2369,8 +2371,10 @@ static void mlx4_en_del_vxlan_offloads(struct work_struct *work) > struct mlx4_en_priv *priv = container_of(work, struct mlx4_en_priv, > vxlan_del_task); > /* unset offloads */ > - priv->dev->hw_enc_features &= ~(NETIF_F_IP_CSUM | NETIF_F_RXCSUM | > - NETIF_F_TSO | NETIF_F_GSO_UDP_TUNNEL | > + priv->dev->hw_enc_features &= ~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | > + NETIF_F_RXCSUM | > + NETIF_F_TSO | NETIF_F_TSO6 | > + NETIF_F_GSO_UDP_TUNNEL | > NETIF_F_GSO_UDP_TUNNEL_CSUM | > NETIF_F_GSO_PARTIAL); > > @@ -2431,7 +2435,18 @@ static netdev_features_t mlx4_en_features_check(struct sk_buff *skb, > netdev_features_t features) > { > features = vlan_features_check(skb, features); > - return vxlan_features_check(skb, features); > + features = vxlan_features_check(skb, features); > + > + /* The ConnectX-3 doesn't support outer IPv6 checksums but it does > + * support inner IPv6 checksums and segmentation so we need to > + * strip that feature if this is an IPv6 encapsulated frame. > + */ > + if (skb->encapsulation && > + (skb->ip_summed == CHECKSUM_PARTIAL) && > + (ip_hdr(skb)->version != 4)) > + features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK); Dejavu, didn't you fix this already in harmonize_features, in i.e, it is enough to do here: if (skb->encapsulation && (skb->ip_summed == CHECKSUM_PARTIAL)) features &= ~NETIF_F_IPV6_CSUM; > + > + return features; > } > #endif > > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c > index c0d7b7296236..c9f5388ea22a 100644 > --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c > +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c > @@ -41,6 +41,7 @@ > #include <linux/vmalloc.h> > #include <linux/tcp.h> > #include <linux/ip.h> > +#include <linux/ipv6.h> > #include <linux/moduleparam.h> > > #include "mlx4_en.h" > @@ -918,8 +919,18 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev) > tx_ind, fragptr); > > if (skb->encapsulation) { > - struct iphdr *ipv4 = (struct iphdr *)skb_inner_network_header(skb); > - if (ipv4->protocol == IPPROTO_TCP || ipv4->protocol == IPPROTO_UDP) > + union { > + struct iphdr *v4; > + struct ipv6hdr *v6; > + unsigned char *hdr; > + } ip; > + u8 proto; > + > + ip.hdr = skb_inner_network_header(skb); > + proto = (ip.v4->version == 4) ? ip.v4->protocol : > + ip.v6->nexthdr; > + > + if (proto == IPPROTO_TCP || proto == IPPROTO_UDP) > op_own |= cpu_to_be32(MLX4_WQE_CTRL_IIP | MLX4_WQE_CTRL_ILP); > else > op_own |= cpu_to_be32(MLX4_WQE_CTRL_IIP); basically this is a bug fix, I don't know why the original author assumed it will be ipv4 !
On Tue, Apr 26, 2016 at 7:37 AM, Saeed Mahameed <saeedm@dev.mellanox.co.il> wrote: > > > On 4/25/2016 9:31 PM, Alexander Duyck wrote: >> >> >From what I can tell the ConnectX-3 will support an inner IPv6 checksum >> and >> segmentation offload, however it cannot support outer IPv6 headers. For >> this reason I am adding the feature to the hw_enc_features and adding an >> extra check to the features_check call that will disable GSO and checksum >> offload in the case that the encapsulated frame has an outer IP version of >> that is not 4. > > > Hi Alex, > > Can you share the testing commands of running vxlan over IPv6 and what > exactly didn't work for you ? > we would like to test this in house and understand what went wrong, > theoretically there shouldn't be a difference between IPv6/IPv4 outer > checksum offloading in ConnectX-3. The setup is pretty straight forward. Basically I left the first port in the default namespace and moved the second int a secondary namespace referred to below as $netns. I then assigned the IPv6 addresses fec0::10:1 and fec0::10:2. After that I ran the following: VXLAN=vx$net echo $VXLAN ${test_options[$i]} ip link add $VXLAN type vxlan id $net \ local fec0::10:1 remote $addr6 dev $PF0 \ ${test_options[$i]} dstport `expr 8800 + $net` ip netns exec $netns ip link add $VXLAN type vxlan id $net \ local $addr6 remote fec0::10:1 dev $port \ ${test_options[$i]} dstport `expr 8800 + $net` ifconfig $VXLAN 192.168.${net}.1/24 ip netns exec $netns ifconfig $VXLAN 192.168.${net}.2/24 > Anyway, I suspect it might be related to a driver bug most likely in > get_real_size function @en_tx.c > specifically in : *lso_header_size = (skb_inner_transport_header(skb) - > skb->data) + inner_tcp_hdrlen(skb); > > will check this and get back to you. I'm not entirely convinced. What I was seeing is t hat the hardware itself was performing Rx checksum offload only on tunnels with an outer IPv4 header and ignoring tunnels with an outer IPv6 header. > for the mlx5 patches I will also go through them later today. Thanks. > >> Signed-off-by: Alexander Duyck <aduyck@mirantis.com> >> --- >> drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 25 >> +++++++++++++++++++----- >> drivers/net/ethernet/mellanox/mlx4/en_tx.c | 15 ++++++++++++-- >> 2 files changed, 33 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c >> b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c >> index bce37cbfde24..6f28ac58251c 100644 >> --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c >> +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c >> @@ -2357,8 +2357,10 @@ out: >> } >> /* set offloads */ >> - priv->dev->hw_enc_features |= NETIF_F_IP_CSUM | NETIF_F_RXCSUM | >> - NETIF_F_TSO | NETIF_F_GSO_UDP_TUNNEL >> | >> + priv->dev->hw_enc_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM >> | >> + NETIF_F_RXCSUM | >> + NETIF_F_TSO | NETIF_F_TSO6 | >> + NETIF_F_GSO_UDP_TUNNEL | >> NETIF_F_GSO_UDP_TUNNEL_CSUM | >> NETIF_F_GSO_PARTIAL; >> } >> @@ -2369,8 +2371,10 @@ static void mlx4_en_del_vxlan_offloads(struct >> work_struct *work) >> struct mlx4_en_priv *priv = container_of(work, struct >> mlx4_en_priv, >> vxlan_del_task); >> /* unset offloads */ >> - priv->dev->hw_enc_features &= ~(NETIF_F_IP_CSUM | NETIF_F_RXCSUM | >> - NETIF_F_TSO | >> NETIF_F_GSO_UDP_TUNNEL | >> + priv->dev->hw_enc_features &= ~(NETIF_F_IP_CSUM | >> NETIF_F_IPV6_CSUM | >> + NETIF_F_RXCSUM | >> + NETIF_F_TSO | NETIF_F_TSO6 | >> + NETIF_F_GSO_UDP_TUNNEL | >> NETIF_F_GSO_UDP_TUNNEL_CSUM | >> NETIF_F_GSO_PARTIAL); >> @@ -2431,7 +2435,18 @@ static netdev_features_t >> mlx4_en_features_check(struct sk_buff *skb, >> netdev_features_t >> features) >> { >> features = vlan_features_check(skb, features); >> - return vxlan_features_check(skb, features); >> + features = vxlan_features_check(skb, features); >> + >> + /* The ConnectX-3 doesn't support outer IPv6 checksums but it does >> + * support inner IPv6 checksums and segmentation so we need to >> + * strip that feature if this is an IPv6 encapsulated frame. >> + */ >> + if (skb->encapsulation && >> + (skb->ip_summed == CHECKSUM_PARTIAL) && >> + (ip_hdr(skb)->version != 4)) >> + features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK); > > Dejavu, didn't you fix this already in harmonize_features, in > i.e, it is enough to do here: > > if (skb->encapsulation && (skb->ip_summed == CHECKSUM_PARTIAL)) > features &= ~NETIF_F_IPV6_CSUM; > So what this patch is doing is enabling an inner IPv6 header offloads. Up above we set the NETIF_F_IPV6_CSUM bit and we want it to stay set unless we have an outer IPv6 header because the inner headers may still need that bit set. If I did what you suggest it strips IPv6 checksum support for inner headers and if we have to use GSO partial I ended up encountering some of the other bugs that I have fixed for GSO partial where either sg or csum are not defined. > >> + >> + return features; >> } >> #endif >> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c >> b/drivers/net/ethernet/mellanox/mlx4/en_tx.c >> index c0d7b7296236..c9f5388ea22a 100644 >> --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c >> +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c >> @@ -41,6 +41,7 @@ >> #include <linux/vmalloc.h> >> #include <linux/tcp.h> >> #include <linux/ip.h> >> +#include <linux/ipv6.h> >> #include <linux/moduleparam.h> >> #include "mlx4_en.h" >> @@ -918,8 +919,18 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct >> net_device *dev) >> tx_ind, fragptr); >> if (skb->encapsulation) { >> - struct iphdr *ipv4 = (struct iphdr >> *)skb_inner_network_header(skb); >> - if (ipv4->protocol == IPPROTO_TCP || ipv4->protocol == >> IPPROTO_UDP) >> + union { >> + struct iphdr *v4; >> + struct ipv6hdr *v6; >> + unsigned char *hdr; >> + } ip; >> + u8 proto; >> + >> + ip.hdr = skb_inner_network_header(skb); >> + proto = (ip.v4->version == 4) ? ip.v4->protocol : >> + ip.v6->nexthdr; >> + >> + if (proto == IPPROTO_TCP || proto == IPPROTO_UDP) >> op_own |= cpu_to_be32(MLX4_WQE_CTRL_IIP | >> MLX4_WQE_CTRL_ILP); >> else >> op_own |= cpu_to_be32(MLX4_WQE_CTRL_IIP); > > > basically this is a bug fix, I don't know why the original author assumed it > will be ipv4 ! Because the feature flags didn't allow it any other way. I am adding the NETIF_F_TSO6 and NETIF_F_IPV6_CSUM flags in hw_enc_features and so situations such as this couldn't be encountered until you start adding those flags. - Alex
On Tue, Apr 26, 2016 at 6:50 PM, Alex Duyck <aduyck@mirantis.com> wrote: > > The setup is pretty straight forward. Basically I left the first port > in the default namespace and moved the second int a secondary > namespace referred to below as $netns. I then assigned the IPv6 > addresses fec0::10:1 and fec0::10:2. After that I ran the following: > > VXLAN=vx$net > echo $VXLAN ${test_options[$i]} > ip link add $VXLAN type vxlan id $net \ > local fec0::10:1 remote $addr6 dev $PF0 \ > ${test_options[$i]} dstport `expr 8800 + $net` > ip netns exec $netns ip link add $VXLAN type vxlan id $net \ > local $addr6 remote fec0::10:1 dev $port \ > ${test_options[$i]} dstport `expr 8800 + $net` > ifconfig $VXLAN 192.168.${net}.1/24 > ip netns exec $netns ifconfig $VXLAN 192.168.${net}.2/24 > Thanks, indeed i see that GSO is not working with vxlan over IPv6 over mlx5 device. We will test out those patches on both mlx4 and mlx5, and debug mlx4 IPv6 issue you see. > >> Anyway, I suspect it might be related to a driver bug most likely in >> get_real_size function @en_tx.c >> specifically in : *lso_header_size = (skb_inner_transport_header(skb) - >> skb->data) + inner_tcp_hdrlen(skb); >> >> will check this and get back to you. > > I'm not entirely convinced. What I was seeing is t hat the hardware > itself was performing Rx checksum offload only on tunnels with an > outer IPv4 header and ignoring tunnels with an outer IPv6 header. I don't get it, are you trying to say that the issue is in the RX side ? what do you mean by ignoring ? Dropping ? or just not validating checksum ? if so why would you disable GSO and IPv6 checksumming on TX ? >>> @@ -2431,7 +2435,18 @@ static netdev_features_t >>> mlx4_en_features_check(struct sk_buff *skb, >>> netdev_features_t >>> features) >>> { >>> features = vlan_features_check(skb, features); >>> - return vxlan_features_check(skb, features); >>> + features = vxlan_features_check(skb, features); >>> + >>> + /* The ConnectX-3 doesn't support outer IPv6 checksums but it does >>> + * support inner IPv6 checksums and segmentation so we need to >>> + * strip that feature if this is an IPv6 encapsulated frame. >>> + */ >>> + if (skb->encapsulation && >>> + (skb->ip_summed == CHECKSUM_PARTIAL) && >>> + (ip_hdr(skb)->version != 4)) >>> + features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK); >> >> Dejavu, didn't you fix this already in harmonize_features, in >> i.e, it is enough to do here: >> >> if (skb->encapsulation && (skb->ip_summed == CHECKSUM_PARTIAL)) >> features &= ~NETIF_F_IPV6_CSUM; >> > > So what this patch is doing is enabling an inner IPv6 header offloads. > Up above we set the NETIF_F_IPV6_CSUM bit and we want it to stay set > unless we have an outer IPv6 header because the inner headers may > still need that bit set. If I did what you suggest it strips IPv6 > checksum support for inner headers and if we have to use GSO partial I > ended up encountering some of the other bugs that I have fixed for GSO > partial where either sg or csum are not defined. > I see, you mean that you want to disable checksumming and GSO only for packets with Outer(IPv6):Inner(X) and keep it in case for Outer(IPv4):Inner(IPv6) but i think it is weird that the driver decides to disable features it didn't declare in first place (NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK) Retry: if (skb->encapsulation && (skb->ip_summed == CHECKSUM_PARTIAL) && (ip_hdr(skb)->version != 4)) features &= ~NETIF_F_IPV6_CSUM; will this work ? Anyway i prefer to debug the mlx4 issue first before we discuss the best approach to disable checksumming & GSO for outer IPv6 in mlx4. >> >>> + >>> + return features; >>> } >>> #endif >>> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c >>> b/drivers/net/ethernet/mellanox/mlx4/en_tx.c >>> index c0d7b7296236..c9f5388ea22a 100644 >>> --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c >>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c >>> @@ -41,6 +41,7 @@ >>> #include <linux/vmalloc.h> >>> #include <linux/tcp.h> >>> #include <linux/ip.h> >>> +#include <linux/ipv6.h> >>> #include <linux/moduleparam.h> >>> #include "mlx4_en.h" >>> @@ -918,8 +919,18 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct >>> net_device *dev) >>> tx_ind, fragptr); >>> if (skb->encapsulation) { >>> - struct iphdr *ipv4 = (struct iphdr >>> *)skb_inner_network_header(skb); >>> - if (ipv4->protocol == IPPROTO_TCP || ipv4->protocol == >>> IPPROTO_UDP) >>> + union { >>> + struct iphdr *v4; >>> + struct ipv6hdr *v6; >>> + unsigned char *hdr; >>> + } ip; >>> + u8 proto; >>> + >>> + ip.hdr = skb_inner_network_header(skb); >>> + proto = (ip.v4->version == 4) ? ip.v4->protocol : >>> + ip.v6->nexthdr; >>> + >>> + if (proto == IPPROTO_TCP || proto == IPPROTO_UDP) >>> op_own |= cpu_to_be32(MLX4_WQE_CTRL_IIP | >>> MLX4_WQE_CTRL_ILP); >>> else >>> op_own |= cpu_to_be32(MLX4_WQE_CTRL_IIP); >> >> >> basically this is a bug fix, I don't know why the original author assumed it >> will be ipv4 ! > > Because the feature flags didn't allow it any other way. I am adding > the NETIF_F_TSO6 and NETIF_F_IPV6_CSUM flags in hw_enc_features and so > situations such as this couldn't be encountered until you start adding > those flags. > True. Thanks, - Saeed
On Tue, Apr 26, 2016 at 1:23 PM, Saeed Mahameed <saeedm@dev.mellanox.co.il> wrote: > On Tue, Apr 26, 2016 at 6:50 PM, Alex Duyck <aduyck@mirantis.com> wrote: >> >> The setup is pretty straight forward. Basically I left the first port >> in the default namespace and moved the second int a secondary >> namespace referred to below as $netns. I then assigned the IPv6 >> addresses fec0::10:1 and fec0::10:2. After that I ran the following: >> >> VXLAN=vx$net >> echo $VXLAN ${test_options[$i]} >> ip link add $VXLAN type vxlan id $net \ >> local fec0::10:1 remote $addr6 dev $PF0 \ >> ${test_options[$i]} dstport `expr 8800 + $net` >> ip netns exec $netns ip link add $VXLAN type vxlan id $net \ >> local $addr6 remote fec0::10:1 dev $port \ >> ${test_options[$i]} dstport `expr 8800 + $net` >> ifconfig $VXLAN 192.168.${net}.1/24 >> ip netns exec $netns ifconfig $VXLAN 192.168.${net}.2/24 >> > > Thanks, indeed i see that GSO is not working with vxlan over IPv6 over > mlx5 device. > We will test out those patches on both mlx4 and mlx5, and debug mlx4 > IPv6 issue you see. > >> >>> Anyway, I suspect it might be related to a driver bug most likely in >>> get_real_size function @en_tx.c >>> specifically in : *lso_header_size = (skb_inner_transport_header(skb) - >>> skb->data) + inner_tcp_hdrlen(skb); >>> >>> will check this and get back to you. >> >> I'm not entirely convinced. What I was seeing is t hat the hardware >> itself was performing Rx checksum offload only on tunnels with an >> outer IPv4 header and ignoring tunnels with an outer IPv6 header. > > I don't get it, are you trying to say that the issue is in the RX side ? > what do you mean by ignoring ? Dropping ? or just not validating checksum ? > if so why would you disable GSO and IPv6 checksumming on TX ? I'm suspecting that whatever parsing logic exists in either the hardware or firmware may not be configured to parse tunnels with outer IPv6 headers. The tell-tale sign is what occurs with an IPv6 based tunnel with no outer checksum. The hardware is not performing a checksum on the inner headers so it reports it as a UDP frame with no checksum to the stack which ends up preventing us from doing GRO. That tells me that the hardware is not parsing IPv6 based tunnels on Rx. I am assuming that if the Rx side doesn't work then there is a good chance that the Tx won't. >>>> @@ -2431,7 +2435,18 @@ static netdev_features_t >>>> mlx4_en_features_check(struct sk_buff *skb, >>>> netdev_features_t >>>> features) >>>> { >>>> features = vlan_features_check(skb, features); >>>> - return vxlan_features_check(skb, features); >>>> + features = vxlan_features_check(skb, features); >>>> + >>>> + /* The ConnectX-3 doesn't support outer IPv6 checksums but it does >>>> + * support inner IPv6 checksums and segmentation so we need to >>>> + * strip that feature if this is an IPv6 encapsulated frame. >>>> + */ >>>> + if (skb->encapsulation && >>>> + (skb->ip_summed == CHECKSUM_PARTIAL) && >>>> + (ip_hdr(skb)->version != 4)) >>>> + features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK); >>> >>> Dejavu, didn't you fix this already in harmonize_features, in >>> i.e, it is enough to do here: >>> >>> if (skb->encapsulation && (skb->ip_summed == CHECKSUM_PARTIAL)) >>> features &= ~NETIF_F_IPV6_CSUM; >>> >> >> So what this patch is doing is enabling an inner IPv6 header offloads. >> Up above we set the NETIF_F_IPV6_CSUM bit and we want it to stay set >> unless we have an outer IPv6 header because the inner headers may >> still need that bit set. If I did what you suggest it strips IPv6 >> checksum support for inner headers and if we have to use GSO partial I >> ended up encountering some of the other bugs that I have fixed for GSO >> partial where either sg or csum are not defined. >> > > I see, you mean that you want to disable checksumming and GSO only for > packets with Outer(IPv6):Inner(X) and keep it in case for > Outer(IPv4):Inner(IPv6) > but i think it is weird that the driver decides to disable features it > didn't declare in first place (NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK) > > Retry: > > if (skb->encapsulation && (skb->ip_summed == CHECKSUM_PARTIAL) && > (ip_hdr(skb)->version != 4)) > features &= ~NETIF_F_IPV6_CSUM; > > will this work ? Sort of. All that would happen is that you would fall through to harmonize_features where NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK gets cleared. I just figured I would short-cut things since we cannot support inner checksum or any GSO offloads if the tunnel has an outer IPv6 header. In addition this happens to effectively be the same code I am using in vxlan_features_check to disable things if we cannot checksum a protocol so it should help to keep the code size smaller for the function if the compiler is smart enough to coalesce similar code. > Anyway i prefer to debug the mlx4 issue first before we discuss the > best approach to disable checksumming & GSO for outer IPv6 in mlx4. The current code as-is already has it disabled. All I am doing is enabling IPv6 checksums for inner headers as it seems like it doesn't work for outer headers. >>> >>>> + >>>> + return features; >>>> } >>>> #endif >>>> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c >>>> b/drivers/net/ethernet/mellanox/mlx4/en_tx.c >>>> index c0d7b7296236..c9f5388ea22a 100644 >>>> --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c >>>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c >>>> @@ -41,6 +41,7 @@ >>>> #include <linux/vmalloc.h> >>>> #include <linux/tcp.h> >>>> #include <linux/ip.h> >>>> +#include <linux/ipv6.h> >>>> #include <linux/moduleparam.h> >>>> #include "mlx4_en.h" >>>> @@ -918,8 +919,18 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct >>>> net_device *dev) >>>> tx_ind, fragptr); >>>> if (skb->encapsulation) { >>>> - struct iphdr *ipv4 = (struct iphdr >>>> *)skb_inner_network_header(skb); >>>> - if (ipv4->protocol == IPPROTO_TCP || ipv4->protocol == >>>> IPPROTO_UDP) >>>> + union { >>>> + struct iphdr *v4; >>>> + struct ipv6hdr *v6; >>>> + unsigned char *hdr; >>>> + } ip; >>>> + u8 proto; >>>> + >>>> + ip.hdr = skb_inner_network_header(skb); >>>> + proto = (ip.v4->version == 4) ? ip.v4->protocol : >>>> + ip.v6->nexthdr; >>>> + >>>> + if (proto == IPPROTO_TCP || proto == IPPROTO_UDP) >>>> op_own |= cpu_to_be32(MLX4_WQE_CTRL_IIP | >>>> MLX4_WQE_CTRL_ILP); >>>> else >>>> op_own |= cpu_to_be32(MLX4_WQE_CTRL_IIP); >>> >>> >>> basically this is a bug fix, I don't know why the original author assumed it >>> will be ipv4 ! >> >> Because the feature flags didn't allow it any other way. I am adding >> the NETIF_F_TSO6 and NETIF_F_IPV6_CSUM flags in hw_enc_features and so >> situations such as this couldn't be encountered until you start adding >> those flags. >> > > True. > > Thanks, > - Saeed
On 27/04/2016 12:01 AM, Alexander Duyck wrote: > On Tue, Apr 26, 2016 at 1:23 PM, Saeed Mahameed > <saeedm@dev.mellanox.co.il> wrote: >> On Tue, Apr 26, 2016 at 6:50 PM, Alex Duyck <aduyck@mirantis.com> wrote: >>> The setup is pretty straight forward. Basically I left the first port >>> in the default namespace and moved the second int a secondary >>> namespace referred to below as $netns. I then assigned the IPv6 >>> addresses fec0::10:1 and fec0::10:2. After that I ran the following: >>> >>> VXLAN=vx$net >>> echo $VXLAN ${test_options[$i]} >>> ip link add $VXLAN type vxlan id $net \ >>> local fec0::10:1 remote $addr6 dev $PF0 \ >>> ${test_options[$i]} dstport `expr 8800 + $net` >>> ip netns exec $netns ip link add $VXLAN type vxlan id $net \ >>> local $addr6 remote fec0::10:1 dev $port \ >>> ${test_options[$i]} dstport `expr 8800 + $net` >>> ifconfig $VXLAN 192.168.${net}.1/24 >>> ip netns exec $netns ifconfig $VXLAN 192.168.${net}.2/24 >>> >> Thanks, indeed i see that GSO is not working with vxlan over IPv6 over >> mlx5 device. >> We will test out those patches on both mlx4 and mlx5, and debug mlx4 >> IPv6 issue you see. >> >>>> Anyway, I suspect it might be related to a driver bug most likely in >>>> get_real_size function @en_tx.c >>>> specifically in : *lso_header_size = (skb_inner_transport_header(skb) - >>>> skb->data) + inner_tcp_hdrlen(skb); >>>> >>>> will check this and get back to you. >>> I'm not entirely convinced. What I was seeing is t hat the hardware >>> itself was performing Rx checksum offload only on tunnels with an >>> outer IPv4 header and ignoring tunnels with an outer IPv6 header. >> I don't get it, are you trying to say that the issue is in the RX side ? >> what do you mean by ignoring ? Dropping ? or just not validating checksum ? >> if so why would you disable GSO and IPv6 checksumming on TX ? > I'm suspecting that whatever parsing logic exists in either the > hardware or firmware may not be configured to parse tunnels with outer > IPv6 headers. The tell-tale sign is what occurs with an IPv6 based > tunnel with no outer checksum. The hardware is not performing a > checksum on the inner headers so it reports it as a UDP frame with no > checksum to the stack which ends up preventing us from doing GRO. > That tells me that the hardware is not parsing IPv6 based tunnels on > Rx. I am assuming that if the Rx side doesn't work then there is a > good chance that the Tx won't. > >>>>> @@ -2431,7 +2435,18 @@ static netdev_features_t >>>>> mlx4_en_features_check(struct sk_buff *skb, >>>>> netdev_features_t >>>>> features) >>>>> { >>>>> features = vlan_features_check(skb, features); >>>>> - return vxlan_features_check(skb, features); >>>>> + features = vxlan_features_check(skb, features); >>>>> + >>>>> + /* The ConnectX-3 doesn't support outer IPv6 checksums but it does >>>>> + * support inner IPv6 checksums and segmentation so we need to >>>>> + * strip that feature if this is an IPv6 encapsulated frame. >>>>> + */ >>>>> + if (skb->encapsulation && >>>>> + (skb->ip_summed == CHECKSUM_PARTIAL) && >>>>> + (ip_hdr(skb)->version != 4)) >>>>> + features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK); >>>> Dejavu, didn't you fix this already in harmonize_features, in >>>> i.e, it is enough to do here: >>>> >>>> if (skb->encapsulation && (skb->ip_summed == CHECKSUM_PARTIAL)) >>>> features &= ~NETIF_F_IPV6_CSUM; >>>> >>> So what this patch is doing is enabling an inner IPv6 header offloads. >>> Up above we set the NETIF_F_IPV6_CSUM bit and we want it to stay set >>> unless we have an outer IPv6 header because the inner headers may >>> still need that bit set. If I did what you suggest it strips IPv6 >>> checksum support for inner headers and if we have to use GSO partial I >>> ended up encountering some of the other bugs that I have fixed for GSO >>> partial where either sg or csum are not defined. >>> >> I see, you mean that you want to disable checksumming and GSO only for >> packets with Outer(IPv6):Inner(X) and keep it in case for >> Outer(IPv4):Inner(IPv6) >> but i think it is weird that the driver decides to disable features it >> didn't declare in first place (NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK) >> >> Retry: >> >> if (skb->encapsulation && (skb->ip_summed == CHECKSUM_PARTIAL) && >> (ip_hdr(skb)->version != 4)) >> features &= ~NETIF_F_IPV6_CSUM; >> >> will this work ? > Sort of. All that would happen is that you would fall through to > harmonize_features where NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK gets > cleared. I just figured I would short-cut things since we cannot > support inner checksum or any GSO offloads if the tunnel has an outer > IPv6 header. In addition this happens to effectively be the same code > I am using in vxlan_features_check to disable things if we cannot > checksum a protocol so it should help to keep the code size smaller > for the function if the compiler is smart enough to coalesce similar > code. > >> Anyway i prefer to debug the mlx4 issue first before we discuss the >> best approach to disable checksumming & GSO for outer IPv6 in mlx4. > The current code as-is already has it disabled. All I am doing is > enabling IPv6 checksums for inner headers as it seems like it doesn't > work for outer headers. Hi Alex, I will be working on the mlx4 issue next week after the holidays. I will check this offline in-house, without blocking the series. Regards, Tariq
On 04/27/2016 08:39 AM, Tariq Toukan wrote: > > > On 27/04/2016 12:01 AM, Alexander Duyck wrote: >> On Tue, Apr 26, 2016 at 1:23 PM, Saeed Mahameed >> <saeedm@dev.mellanox.co.il> wrote: >>> On Tue, Apr 26, 2016 at 6:50 PM, Alex Duyck <aduyck@mirantis.com> wrote: >>>> The setup is pretty straight forward. Basically I left the first port >>>> in the default namespace and moved the second int a secondary >>>> namespace referred to below as $netns. I then assigned the IPv6 >>>> addresses fec0::10:1 and fec0::10:2. After that I ran the following: >>>> >>>> VXLAN=vx$net >>>> echo $VXLAN ${test_options[$i]} >>>> ip link add $VXLAN type vxlan id $net \ >>>> local fec0::10:1 remote $addr6 dev $PF0 \ >>>> ${test_options[$i]} dstport `expr 8800 + $net` >>>> ip netns exec $netns ip link add $VXLAN type vxlan id $net \ >>>> local $addr6 remote fec0::10:1 >>>> dev $port \ >>>> ${test_options[$i]} dstport `expr >>>> 8800 + $net` >>>> ifconfig $VXLAN 192.168.${net}.1/24 >>>> ip netns exec $netns ifconfig $VXLAN 192.168.${net}.2/24 >>>> >>> Thanks, indeed i see that GSO is not working with vxlan over IPv6 over >>> mlx5 device. >>> We will test out those patches on both mlx4 and mlx5, and debug mlx4 >>> IPv6 issue you see. >>> >>>>> Anyway, I suspect it might be related to a driver bug most likely in >>>>> get_real_size function @en_tx.c >>>>> specifically in : *lso_header_size = >>>>> (skb_inner_transport_header(skb) - >>>>> skb->data) + inner_tcp_hdrlen(skb); >>>>> >>>>> will check this and get back to you. >>>> I'm not entirely convinced. What I was seeing is t hat the hardware >>>> itself was performing Rx checksum offload only on tunnels with an >>>> outer IPv4 header and ignoring tunnels with an outer IPv6 header. >>> I don't get it, are you trying to say that the issue is in the RX side ? >>> what do you mean by ignoring ? Dropping ? or just not validating >>> checksum ? >>> if so why would you disable GSO and IPv6 checksumming on TX ? >> I'm suspecting that whatever parsing logic exists in either the >> hardware or firmware may not be configured to parse tunnels with outer >> IPv6 headers. The tell-tale sign is what occurs with an IPv6 based >> tunnel with no outer checksum. The hardware is not performing a >> checksum on the inner headers so it reports it as a UDP frame with no >> checksum to the stack which ends up preventing us from doing GRO. >> That tells me that the hardware is not parsing IPv6 based tunnels on >> Rx. I am assuming that if the Rx side doesn't work then there is a >> good chance that the Tx won't. >> >>>>>> @@ -2431,7 +2435,18 @@ static netdev_features_t >>>>>> mlx4_en_features_check(struct sk_buff *skb, >>>>>> netdev_features_t >>>>>> features) >>>>>> { >>>>>> features = vlan_features_check(skb, features); >>>>>> - return vxlan_features_check(skb, features); >>>>>> + features = vxlan_features_check(skb, features); >>>>>> + >>>>>> + /* The ConnectX-3 doesn't support outer IPv6 checksums but >>>>>> it does >>>>>> + * support inner IPv6 checksums and segmentation so we >>>>>> need to >>>>>> + * strip that feature if this is an IPv6 encapsulated frame. >>>>>> + */ >>>>>> + if (skb->encapsulation && >>>>>> + (skb->ip_summed == CHECKSUM_PARTIAL) && >>>>>> + (ip_hdr(skb)->version != 4)) >>>>>> + features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK); >>>>> Dejavu, didn't you fix this already in harmonize_features, in >>>>> i.e, it is enough to do here: >>>>> >>>>> if (skb->encapsulation && (skb->ip_summed == CHECKSUM_PARTIAL)) >>>>> features &= ~NETIF_F_IPV6_CSUM; >>>>> >>>> So what this patch is doing is enabling an inner IPv6 header offloads. >>>> Up above we set the NETIF_F_IPV6_CSUM bit and we want it to stay set >>>> unless we have an outer IPv6 header because the inner headers may >>>> still need that bit set. If I did what you suggest it strips IPv6 >>>> checksum support for inner headers and if we have to use GSO partial I >>>> ended up encountering some of the other bugs that I have fixed for GSO >>>> partial where either sg or csum are not defined. >>>> >>> I see, you mean that you want to disable checksumming and GSO only for >>> packets with Outer(IPv6):Inner(X) and keep it in case for >>> Outer(IPv4):Inner(IPv6) >>> but i think it is weird that the driver decides to disable features it >>> didn't declare in first place (NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK) >>> >>> Retry: >>> >>> if (skb->encapsulation && (skb->ip_summed == CHECKSUM_PARTIAL) && >>> (ip_hdr(skb)->version != 4)) >>> features &= ~NETIF_F_IPV6_CSUM; >>> >>> will this work ? >> Sort of. All that would happen is that you would fall through to >> harmonize_features where NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK gets >> cleared. I just figured I would short-cut things since we cannot >> support inner checksum or any GSO offloads if the tunnel has an outer >> IPv6 header. In addition this happens to effectively be the same code >> I am using in vxlan_features_check to disable things if we cannot >> checksum a protocol so it should help to keep the code size smaller >> for the function if the compiler is smart enough to coalesce similar >> code. >> >>> Anyway i prefer to debug the mlx4 issue first before we discuss the >>> best approach to disable checksumming & GSO for outer IPv6 in mlx4. >> The current code as-is already has it disabled. All I am doing is >> enabling IPv6 checksums for inner headers as it seems like it doesn't >> work for outer headers. > > Hi Alex, > I will be working on the mlx4 issue next week after the holidays. > I will check this offline in-house, without blocking the series. > > Regards, > Tariq > If it helps below are the kind of results I am seeing with the patch series applied versus what is currently there. The big problem areas are IPv6 over IPv4 tunnels, and IPv4 over IPv6 tunnels without checksums. The breakdown below is bare-ip-version without tunnel, outer-inner with tunnel, or outer-csum-inner if a checksum is present on the outer UDP header. After the series is applied you can see the v6 over v4 issues are addressed, and GSO partial has improved the performance for traffic over v4 tunnels with outer UDP checksums. Throughput Throughput Local Local Result Units CPU Service Tag Util Demand % mlx4 - Before ------------------------------------------------- 26616.45 10^6bits/s 3.62 0.357 "v4" 26101.18 10^6bits/s 6.72 0.675 "v6" 22289.41 10^6bits/s 6.49 0.764 "v4-v4" N/A - could not connect "v4-v6" 12743.91 10^6bits/s 4.25 0.874 "v4-csum-v4" N/A - could not connect "v4-csum-v6" 0.69 10^6bits/s 0.66 2519.1 "v6-v4" 5924.47 10^6bits/s 4.23 1.871 "v6-v6" 10369.95 10^6bits/s 4.33 1.096 "v6-csum-v4" 10648.51 10^6bits/s 4.10 1.010 "v6-csum-v6" mlx4 - After ------------------------------------------------- 26585.36 10^6bits/s 3.60 0.355 "v4" 26342.86 10^6bits/s 6.67 0.664 "v6" 22295.93 10^6bits/s 6.34 0.746 "v4-v4" 19977.24 10^6bits/s 6.04 0.793 "v4-v6" 22164.71 10^6bits/s 6.46 0.763 "v4-csum-v4" 19685.22 10^6bits/s 6.12 0.815 "v4-csum-v6" 6126.80 10^6bits/s 4.29 1.835 "v6-v4" 5793.53 10^6bits/s 4.24 1.917 "v6-v6" 10278.52 10^6bits/s 4.07 1.039 "v6-csum-v4" 10526.68 10^6bits/s 4.11 1.024 "v6-csum-v6"
On 27/04/2016 9:05 PM, Alexander Duyck wrote: > On 04/27/2016 08:39 AM, Tariq Toukan wrote: >> >> >> On 27/04/2016 12:01 AM, Alexander Duyck wrote: >>> On Tue, Apr 26, 2016 at 1:23 PM, Saeed Mahameed >>> <saeedm@dev.mellanox.co.il> wrote: >>>> On Tue, Apr 26, 2016 at 6:50 PM, Alex Duyck <aduyck@mirantis.com> >>>> wrote: >>>>> The setup is pretty straight forward. Basically I left the first >>>>> port >>>>> in the default namespace and moved the second int a secondary >>>>> namespace referred to below as $netns. I then assigned the IPv6 >>>>> addresses fec0::10:1 and fec0::10:2. After that I ran the following: >>>>> >>>>> VXLAN=vx$net >>>>> echo $VXLAN ${test_options[$i]} >>>>> ip link add $VXLAN type vxlan id $net \ >>>>> local fec0::10:1 remote $addr6 dev $PF0 \ >>>>> ${test_options[$i]} dstport `expr 8800 + $net` >>>>> ip netns exec $netns ip link add $VXLAN type vxlan id $net \ >>>>> local $addr6 remote fec0::10:1 >>>>> dev $port \ >>>>> ${test_options[$i]} dstport `expr >>>>> 8800 + $net` >>>>> ifconfig $VXLAN 192.168.${net}.1/24 >>>>> ip netns exec $netns ifconfig $VXLAN 192.168.${net}.2/24 >>>>> >>>> Thanks, indeed i see that GSO is not working with vxlan over IPv6 over >>>> mlx5 device. >>>> We will test out those patches on both mlx4 and mlx5, and debug mlx4 >>>> IPv6 issue you see. >>>> >>>>>> Anyway, I suspect it might be related to a driver bug most likely in >>>>>> get_real_size function @en_tx.c >>>>>> specifically in : *lso_header_size = >>>>>> (skb_inner_transport_header(skb) - >>>>>> skb->data) + inner_tcp_hdrlen(skb); >>>>>> >>>>>> will check this and get back to you. >>>>> I'm not entirely convinced. What I was seeing is t hat the hardware >>>>> itself was performing Rx checksum offload only on tunnels with an >>>>> outer IPv4 header and ignoring tunnels with an outer IPv6 header. >>>> I don't get it, are you trying to say that the issue is in the RX >>>> side ? >>>> what do you mean by ignoring ? Dropping ? or just not validating >>>> checksum ? >>>> if so why would you disable GSO and IPv6 checksumming on TX ? >>> I'm suspecting that whatever parsing logic exists in either the >>> hardware or firmware may not be configured to parse tunnels with outer >>> IPv6 headers. The tell-tale sign is what occurs with an IPv6 based >>> tunnel with no outer checksum. The hardware is not performing a >>> checksum on the inner headers so it reports it as a UDP frame with no >>> checksum to the stack which ends up preventing us from doing GRO. >>> That tells me that the hardware is not parsing IPv6 based tunnels on >>> Rx. I am assuming that if the Rx side doesn't work then there is a >>> good chance that the Tx won't. >>> >>>>>>> @@ -2431,7 +2435,18 @@ static netdev_features_t >>>>>>> mlx4_en_features_check(struct sk_buff *skb, >>>>>>> netdev_features_t >>>>>>> features) >>>>>>> { >>>>>>> features = vlan_features_check(skb, features); >>>>>>> - return vxlan_features_check(skb, features); >>>>>>> + features = vxlan_features_check(skb, features); >>>>>>> + >>>>>>> + /* The ConnectX-3 doesn't support outer IPv6 checksums but >>>>>>> it does >>>>>>> + * support inner IPv6 checksums and segmentation so we >>>>>>> need to >>>>>>> + * strip that feature if this is an IPv6 encapsulated >>>>>>> frame. >>>>>>> + */ >>>>>>> + if (skb->encapsulation && >>>>>>> + (skb->ip_summed == CHECKSUM_PARTIAL) && >>>>>>> + (ip_hdr(skb)->version != 4)) >>>>>>> + features &= ~(NETIF_F_CSUM_MASK | >>>>>>> NETIF_F_GSO_MASK); >>>>>> Dejavu, didn't you fix this already in harmonize_features, in >>>>>> i.e, it is enough to do here: >>>>>> >>>>>> if (skb->encapsulation && (skb->ip_summed == CHECKSUM_PARTIAL)) >>>>>> features &= ~NETIF_F_IPV6_CSUM; >>>>>> >>>>> So what this patch is doing is enabling an inner IPv6 header >>>>> offloads. >>>>> Up above we set the NETIF_F_IPV6_CSUM bit and we want it to stay set >>>>> unless we have an outer IPv6 header because the inner headers may >>>>> still need that bit set. If I did what you suggest it strips IPv6 >>>>> checksum support for inner headers and if we have to use GSO >>>>> partial I >>>>> ended up encountering some of the other bugs that I have fixed for >>>>> GSO >>>>> partial where either sg or csum are not defined. >>>>> >>>> I see, you mean that you want to disable checksumming and GSO only for >>>> packets with Outer(IPv6):Inner(X) and keep it in case for >>>> Outer(IPv4):Inner(IPv6) >>>> but i think it is weird that the driver decides to disable features it >>>> didn't declare in first place (NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK) >>>> >>>> Retry: >>>> >>>> if (skb->encapsulation && (skb->ip_summed == CHECKSUM_PARTIAL) && >>>> (ip_hdr(skb)->version != 4)) >>>> features &= ~NETIF_F_IPV6_CSUM; >>>> >>>> will this work ? >>> Sort of. All that would happen is that you would fall through to >>> harmonize_features where NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK gets >>> cleared. I just figured I would short-cut things since we cannot >>> support inner checksum or any GSO offloads if the tunnel has an outer >>> IPv6 header. In addition this happens to effectively be the same code >>> I am using in vxlan_features_check to disable things if we cannot >>> checksum a protocol so it should help to keep the code size smaller >>> for the function if the compiler is smart enough to coalesce similar >>> code. >>> >>>> Anyway i prefer to debug the mlx4 issue first before we discuss the >>>> best approach to disable checksumming & GSO for outer IPv6 in mlx4. >>> The current code as-is already has it disabled. All I am doing is >>> enabling IPv6 checksums for inner headers as it seems like it doesn't >>> work for outer headers. >> >> Hi Alex, >> I will be working on the mlx4 issue next week after the holidays. >> I will check this offline in-house, without blocking the series. >> >> Regards, >> Tariq >> > > If it helps below are the kind of results I am seeing with the patch > series applied versus what is currently there. The big problem areas > are IPv6 over IPv4 tunnels, and IPv4 over IPv6 tunnels without checksums. > > The breakdown below is bare-ip-version without tunnel, outer-inner > with tunnel, or outer-csum-inner if a checksum is present on the outer > UDP header. > > After the series is applied you can see the v6 over v4 issues are > addressed, and GSO partial has improved the performance for traffic > over v4 tunnels with outer UDP checksums. > > Throughput Throughput Local Local Result > Units CPU Service Tag > Util Demand > % > mlx4 - Before > ------------------------------------------------- > 26616.45 10^6bits/s 3.62 0.357 "v4" > 26101.18 10^6bits/s 6.72 0.675 "v6" > 22289.41 10^6bits/s 6.49 0.764 "v4-v4" > N/A - could not connect "v4-v6" > 12743.91 10^6bits/s 4.25 0.874 "v4-csum-v4" > N/A - could not connect "v4-csum-v6" > 0.69 10^6bits/s 0.66 2519.1 "v6-v4" > 5924.47 10^6bits/s 4.23 1.871 "v6-v6" > 10369.95 10^6bits/s 4.33 1.096 "v6-csum-v4" > 10648.51 10^6bits/s 4.10 1.010 "v6-csum-v6" > > mlx4 - After > ------------------------------------------------- > 26585.36 10^6bits/s 3.60 0.355 "v4" > 26342.86 10^6bits/s 6.67 0.664 "v6" > 22295.93 10^6bits/s 6.34 0.746 "v4-v4" > 19977.24 10^6bits/s 6.04 0.793 "v4-v6" > 22164.71 10^6bits/s 6.46 0.763 "v4-csum-v4" > 19685.22 10^6bits/s 6.12 0.815 "v4-csum-v6" > 6126.80 10^6bits/s 4.29 1.835 "v6-v4" > 5793.53 10^6bits/s 4.24 1.917 "v6-v6" > 10278.52 10^6bits/s 4.07 1.039 "v6-csum-v4" > 10526.68 10^6bits/s 4.11 1.024 "v6-csum-v6" Yeah that can help in comparing our results and getting aligned to address the issues. Thanks for sharing. Regards, Tariq
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c index bce37cbfde24..6f28ac58251c 100644 --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c @@ -2357,8 +2357,10 @@ out: } /* set offloads */ - priv->dev->hw_enc_features |= NETIF_F_IP_CSUM | NETIF_F_RXCSUM | - NETIF_F_TSO | NETIF_F_GSO_UDP_TUNNEL | + priv->dev->hw_enc_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | + NETIF_F_RXCSUM | + NETIF_F_TSO | NETIF_F_TSO6 | + NETIF_F_GSO_UDP_TUNNEL | NETIF_F_GSO_UDP_TUNNEL_CSUM | NETIF_F_GSO_PARTIAL; } @@ -2369,8 +2371,10 @@ static void mlx4_en_del_vxlan_offloads(struct work_struct *work) struct mlx4_en_priv *priv = container_of(work, struct mlx4_en_priv, vxlan_del_task); /* unset offloads */ - priv->dev->hw_enc_features &= ~(NETIF_F_IP_CSUM | NETIF_F_RXCSUM | - NETIF_F_TSO | NETIF_F_GSO_UDP_TUNNEL | + priv->dev->hw_enc_features &= ~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | + NETIF_F_RXCSUM | + NETIF_F_TSO | NETIF_F_TSO6 | + NETIF_F_GSO_UDP_TUNNEL | NETIF_F_GSO_UDP_TUNNEL_CSUM | NETIF_F_GSO_PARTIAL); @@ -2431,7 +2435,18 @@ static netdev_features_t mlx4_en_features_check(struct sk_buff *skb, netdev_features_t features) { features = vlan_features_check(skb, features); - return vxlan_features_check(skb, features); + features = vxlan_features_check(skb, features); + + /* The ConnectX-3 doesn't support outer IPv6 checksums but it does + * support inner IPv6 checksums and segmentation so we need to + * strip that feature if this is an IPv6 encapsulated frame. + */ + if (skb->encapsulation && + (skb->ip_summed == CHECKSUM_PARTIAL) && + (ip_hdr(skb)->version != 4)) + features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK); + + return features; } #endif diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c index c0d7b7296236..c9f5388ea22a 100644 --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c @@ -41,6 +41,7 @@ #include <linux/vmalloc.h> #include <linux/tcp.h> #include <linux/ip.h> +#include <linux/ipv6.h> #include <linux/moduleparam.h> #include "mlx4_en.h" @@ -918,8 +919,18 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev) tx_ind, fragptr); if (skb->encapsulation) { - struct iphdr *ipv4 = (struct iphdr *)skb_inner_network_header(skb); - if (ipv4->protocol == IPPROTO_TCP || ipv4->protocol == IPPROTO_UDP) + union { + struct iphdr *v4; + struct ipv6hdr *v6; + unsigned char *hdr; + } ip; + u8 proto; + + ip.hdr = skb_inner_network_header(skb); + proto = (ip.v4->version == 4) ? ip.v4->protocol : + ip.v6->nexthdr; + + if (proto == IPPROTO_TCP || proto == IPPROTO_UDP) op_own |= cpu_to_be32(MLX4_WQE_CTRL_IIP | MLX4_WQE_CTRL_ILP); else op_own |= cpu_to_be32(MLX4_WQE_CTRL_IIP);