Message-ID: <5720FF84.9020404@gmail.com>
Date:	Wed, 27 Apr 2016 11:05:56 -0700
From:	Alexander Duyck <alexander.duyck@...il.com>
To:	Tariq Toukan <ttoukan.linux@...il.com>,
	Saeed Mahameed <saeedm@....mellanox.co.il>
Cc:	Alex Duyck <aduyck@...antis.com>, Tal Alon <talal@...lanox.com>,
	Linux Kernel Network Developers <netdev@...r.kernel.org>,
	David Miller <davem@...emloft.net>,
	Gal Pressman <galp@...lanox.com>,
	Or Gerlitz <ogerlitz@...lanox.com>,
	Eran Ben Elisha <eranbe@...lanox.com>
Subject: Re: [net-next PATCH 6/8] mlx4: Add support for inner IPv6 checksum
 offloads and TSO

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@....mellanox.co.il> wrote:
>>> On Tue, Apr 26, 2016 at 6:50 PM, Alex Duyck <aduyck@...antis.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"

Powered by blists - more mailing lists