On Tue, Mar 22, 2016 at 1:20 PM, Alexander Duyck
<alexander.du...@gmail.com> wrote:
> On Tue, Mar 22, 2016 at 12:19 PM, Tom Herbert <t...@herbertland.com> wrote:
>> In skb_segment the check of whether or not to perform the checksum on
>> host was changed to not consider rather remote checksum offload is
>> in use. In the case that can_checksum_protocol fails the checksum
>> is computed regardless. __skb_udp_tunnel_segment was modified in a
>> related patch to add NETIF_F_HW_CSUM into features when grabbing
>> the enc_features and remote checksum offload is being done. The
>> problem is that this bit can be cleared in lower GSO layers that
>> are also doing tunneling (e.g. ipip, GRE when used with GUE),
>> so when we get to skb_segment that intent has been lost and
>> can_checksum_protocol fails.
>
> So what you are describing sounds like a tunnel in tunnel scenario.
> It might work better to just skip masking the features if
> skb->remcsum_offload is set rather than trying to change how we
> perform the offload.
>
To be clear, my patch is restoring the old behavior not implementing a new one.
> I'm pretty sure this will cause data corruption and maybe a kernel
> panic if Tx checksum offload is disabled.
>
Nope, working fine for me.
The outer checksum would be computed in
>> This patch:
>>
>> - Restores the check in skb_segment to look at remote checksum offload.
>> - Removes the code in __skb_udp_tunnel_segment to force the
>> NETIF_F_HW_CSUM feature since this is no longer useful with above
>> change.
>> - Removes check for remote checksum offload in gso_reset_checksum.
>> A special case should not be needed here.
>>
>> Tested: Single netperf STREAM over GUE-ipip
>>
>> Before fix:
>> 5625 Mbps
>> After fix:
>> 6410 Mbps
>>
>> Fixes: 76443456227097179c1482 ("net: Move GSO csum into SKB_GSO_CB")
>> Signed-off-by: Tom Herbert <t...@herbertland.com>
>> ---
>> include/linux/skbuff.h | 4 ----
>> net/core/skbuff.c | 5 ++---
>> net/ipv4/udp_offload.c | 10 ----------
>> 3 files changed, 2 insertions(+), 17 deletions(-)
>>
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index 15d0df9..f6fe8a5 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -3615,10 +3615,6 @@ static inline int gso_pskb_expand_head(struct sk_buff
>> *skb, int extra)
>>
>> static inline void gso_reset_checksum(struct sk_buff *skb, __wsum res)
>> {
>> - /* Do not update partial checksums if remote checksum is enabled. */
>> - if (skb->remcsum_offload)
>> - return;
>> -
>> SKB_GSO_CB(skb)->csum = res;
>> SKB_GSO_CB(skb)->csum_start = skb_checksum_start(skb) - skb->head;
>> }
>
> I'm pretty sure this part here will break things when you don't have
> an outer offload enabled. What NIC did you test this on? Did you try
> disabling the Tx checksum support in the hardware to see what would
> happen?
>
Mellanox.
When TX checksum is disabled the outer checksum is computed in
__skb_udp_tunnel_segment.
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index f044f97..e4eb78d 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -3259,14 +3259,13 @@ skip_fraglist:
>> nskb->truesize += nskb->data_len;
>>
>> perform_csum_check:
>> - if (!csum) {
>> + if (!csum && !nskb->remcsum_offload) {
>> if (skb_has_shared_frag(nskb)) {
>> err = __skb_linearize(nskb);
>> if (err)
>> goto err;
>> }
>> - if (!nskb->remcsum_offload)
>> - nskb->ip_summed = CHECKSUM_NONE;
>> + nskb->ip_summed = CHECKSUM_NONE;
>> SKB_GSO_CB(nskb)->csum =
>> skb_checksum(nskb, doffset,
>> nskb->len - doffset, 0);
>
> I'm pretty sure this is going to cause a huge mess if you are
> requesting remote checksum but cannot perform an outer checksum. One
> of the reasons I merged these features together the way I did was
> because we needed to perform the initial checksum to avoid causing a
> kernel panic later on. Otherwise we don't have the SKB_GSO_CB()->csum
> and SKB_GSO_CB()->csum_start fields populated.
>
Nope, if we can't perform outer checksum it is done in the host. I'm
not seeing any problem.
>> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>> index 8a3405a..f86f1e1 100644
>> --- a/net/ipv4/udp_offload.c
>> +++ b/net/ipv4/udp_offload.c
>> @@ -78,16 +78,6 @@ static struct sk_buff *__skb_udp_tunnel_segment(struct
>> sk_buff *skb,
>>
>> features &= skb->dev->hw_enc_features;
>>
>> - /* The only checksum offload we care about from here on out is the
>> - * outer one so strip the existing checksum feature flags and
>> - * instead set the flag based on our outer checksum offload value.
>> - */
>> - if (remcsum || ufo) {
>> - features &= ~NETIF_F_CSUM_MASK;
>> - if (!need_csum || offload_csum)
>> - features |= NETIF_F_HW_CSUM;
>> - }
>> -
>> /* segment inner packet. */
>> segs = gso_inner_segment(skb, features);
>> if (IS_ERR_OR_NULL(segs)) {
>
> This part breaks UDP fragmentation I am pretty sure. We need this bit
> for UFO to avoid having to perform a checksum over the data twice if
> we are offloading the outer UDP checksum.
The maybe leave this for ufo if it is a problem (please test your
suppositions!), but this is not good for remcsum offload.
Tom
