[PATCH net-next] net-gre-gro: Fix a bug that breaks the forwarding path

[PATCH net-next] net-gre-gro: Fix a bug that breaks the forwarding path

[H.K. Jerry Chu]

From: Jerry Chu <hkchu@google.com>

Fixed a bug that was introduced by my GRE-GRO patch
(bf5a755f5e9186406bbf50f4087100af5bd68e40 net-gre-gro: Add GRE
support to the GRO stack) that breaks the forwarding path
because various GSO related fields were not set. The bug will
cause on the egress path either the GSO code to fail, or a
GRE-TSO capable (NETIF_F_GSO_GRE) NICs to choke. The following
fix has been tested for both cases.

Signed-off-by: H.K. Jerry Chu <hkchu@google.com>
---
 net/core/dev.c           | 2 ++
 net/ipv4/af_inet.c       | 3 +++
 net/ipv4/gre_offload.c   | 3 +++
 net/ipv4/tcp_offload.c   | 2 +-
 net/ipv6/tcpv6_offload.c | 2 +-
 5 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 6e2a2cd..d603c14 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4089,6 +4089,8 @@ static void napi_reuse_skb(struct napi_struct *napi, struct sk_buff *skb)
 	skb->vlan_tci = 0;
 	skb->dev = napi->dev;
 	skb->skb_iif = 0;
+	skb->encapsulation = 0;
+	skb_shinfo(skb)->gso_type = 0;
 	skb->truesize = SKB_TRUESIZE(skb_end_offset(skb));
 
 	napi->skb = skb;
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index d5e6836..d156b3c 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1429,6 +1429,9 @@ static int inet_gro_complete(struct sk_buff *skb, int nhoff)
 	int proto = iph->protocol;
 	int err = -ENOSYS;
 
+	if (skb->encapsulation)
+		skb_set_inner_network_header(skb, nhoff);
+
 	csum_replace2(&iph->check, iph->tot_len, newlen);
 	iph->tot_len = newlen;
 
diff --git a/net/ipv4/gre_offload.c b/net/ipv4/gre_offload.c
index eb92deb..f0bdd47 100644
--- a/net/ipv4/gre_offload.c
+++ b/net/ipv4/gre_offload.c
@@ -263,6 +263,9 @@ static int gre_gro_complete(struct sk_buff *skb, int nhoff)
 	int err = -ENOENT;
 	__be16 type;
 
+	skb->encapsulation = 1;
+	skb_shinfo(skb)->gso_type = SKB_GSO_GRE;
+
 	type = greh->protocol;
 	if (greh->flags & GRE_KEY)
 		grehlen += GRE_HEADER_SECTION;
diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
index 4e86c59..55046ec 100644
--- a/net/ipv4/tcp_offload.c
+++ b/net/ipv4/tcp_offload.c
@@ -309,7 +309,7 @@ static int tcp4_gro_complete(struct sk_buff *skb, int thoff)
 
 	th->check = ~tcp_v4_check(skb->len - thoff, iph->saddr,
 				  iph->daddr, 0);
-	skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
+	skb_shinfo(skb)->gso_type |= SKB_GSO_TCPV4;
 
 	return tcp_gro_complete(skb);
 }
diff --git a/net/ipv6/tcpv6_offload.c b/net/ipv6/tcpv6_offload.c
index 8517d3c..01b0ff9 100644
--- a/net/ipv6/tcpv6_offload.c
+++ b/net/ipv6/tcpv6_offload.c
@@ -73,7 +73,7 @@ static int tcp6_gro_complete(struct sk_buff *skb, int thoff)
 
 	th->check = ~tcp_v6_check(skb->len - thoff, &iph->saddr,
 				  &iph->daddr, 0);
-	skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
+	skb_shinfo(skb)->gso_type |= SKB_GSO_TCPV6;
 
 	return tcp_gro_complete(skb);
 }
-- 
2.0.0.526.g5318336

Re: [PATCH net-next] net-gre-gro: Fix a bug that breaks the forwarding path

[Or Gerlitz]

On Sun, Jul 13, 2014 at 5:46 PM, H.K. Jerry Chu <hkchu@google.com> wrote:
>
> From: Jerry Chu <hkchu@google.com>
>
> Fixed a bug that was introduced by my GRE-GRO patch
> (bf5a755f5e9186406bbf50f4087100af5bd68e40 net-gre-gro: Add GRE
> support to the GRO stack) that breaks the forwarding path
> because various GSO related fields were not set. The bug will
> cause on the egress path either the GSO code to fail, or a
> GRE-TSO capable (NETIF_F_GSO_GRE) NICs to choke. The following
> fix has been tested for both cases.


Hi Jerry,

Anything different in this version vs. the one you posted earlier on
February or this is a plain re-post?

Or.

Re: [PATCH net-next] net-gre-gro: Fix a bug that breaks the forwarding path

[Jerry Chu]

Hi Or,

On Mon, Jul 14, 2014 at 12:05 PM, Or Gerlitz <or.gerlitz@gmail.com> wrote:
>
> On Sun, Jul 13, 2014 at 5:46 PM, H.K. Jerry Chu <hkchu@google.com> wrote:
> >
> > From: Jerry Chu <hkchu@google.com>
> >
> > Fixed a bug that was introduced by my GRE-GRO patch
> > (bf5a755f5e9186406bbf50f4087100af5bd68e40 net-gre-gro: Add GRE
> > support to the GRO stack) that breaks the forwarding path
> > because various GSO related fields were not set. The bug will
> > cause on the egress path either the GSO code to fail, or a
> > GRE-TSO capable (NETIF_F_GSO_GRE) NICs to choke. The following
> > fix has been tested for both cases.
>
>
> Hi Jerry,
>
> Anything different in this version vs. the one you posted earlier on
> February or this is a plain re-post?

I simply moved the patch against the latest net-next and resolved some small
conflict so yes it's pretty much the same. Also I don't see the subsequent
discussion on skb->encapsulation affects the validity of this patch so
i'm resubmitting
it. Also the patch has been confirmed to address the problem Wolfgang reported
last week. Feel free to test against the configuration (VXLAN?) you
had some question about earlier.

Thanks,

Jerry

>
>
> Or.

Re: [PATCH net-next] net-gre-gro: Fix a bug that breaks the forwarding path

[Or Gerlitz]

On Mon, Jul 14, 2014 at 10:16 PM, Jerry Chu <hkchu@google.com> wrote:
> On Mon, Jul 14, 2014 at 12:05 PM, Or Gerlitz <or.gerlitz@gmail.com> wrote:
>> On Sun, Jul 13, 2014 at 5:46 PM, H.K. Jerry Chu <hkchu@google.com> wrote:
>>> Fixed a bug that was introduced by my GRE-GRO patch
>>> (bf5a755f5e9186406bbf50f4087100af5bd68e40 net-gre-gro: Add GRE
>>> support to the GRO stack) that breaks the forwarding path
>>> because various GSO related fields were not set. The bug will
>>> cause on the egress path either the GSO code to fail, or a
>>> GRE-TSO capable (NETIF_F_GSO_GRE) NICs to choke. The following
>>> fix has been tested for both cases.

>> Anything different in this version vs. the one you posted earlier on
>> February or this is a plain re-post?

> I simply moved the patch against the latest net-next and resolved some small
> conflict so yes it's pretty much the same.

OK, got it.

> Also I don't see the subsequent
> discussion on skb->encapsulation affects the validity of this patch so
> i'm resubmitting it. Also the patch has been confirmed to address the problem
> Wolfgang reported last week. Feel free to test against the configuration (VXLAN?)
> you had some question about earlier.

Well, re-reading that thread, we were not very decisive there... my
comment of setting the inner network header twice in inet_gro_complete
doesn't apply only to vxlan but to other tunneling protocols. Also, if
we really need (why do we? or explained it on the Feb thread) to set
skb->encapsulation for the sake of TX in a protocol (GRE) gro
complete, looks a bit fishy to me... but saying this I think brings us
back to that incomplete discussion [1]  sounds as this needs some
plumbering... Still, this way or another I understand a regression was
introduced here and should be fixed.

Or.

[1] http://marc.info/?t=139353642700003&r=1&w=2

Re: [PATCH net-next] net-gre-gro: Fix a bug that breaks the forwarding path

[Jerry Chu]

On Mon, Jul 14, 2014 at 1:38 PM, Or Gerlitz <or.gerlitz@gmail.com> wrote:
> On Mon, Jul 14, 2014 at 10:16 PM, Jerry Chu <hkchu@google.com> wrote:
>> On Mon, Jul 14, 2014 at 12:05 PM, Or Gerlitz <or.gerlitz@gmail.com> wrote:
>>> On Sun, Jul 13, 2014 at 5:46 PM, H.K. Jerry Chu <hkchu@google.com> wrote:
>>>> Fixed a bug that was introduced by my GRE-GRO patch
>>>> (bf5a755f5e9186406bbf50f4087100af5bd68e40 net-gre-gro: Add GRE
>>>> support to the GRO stack) that breaks the forwarding path
>>>> because various GSO related fields were not set. The bug will
>>>> cause on the egress path either the GSO code to fail, or a
>>>> GRE-TSO capable (NETIF_F_GSO_GRE) NICs to choke. The following
>>>> fix has been tested for both cases.
>
>>> Anything different in this version vs. the one you posted earlier on
>>> February or this is a plain re-post?
>
>> I simply moved the patch against the latest net-next and resolved some small
>> conflict so yes it's pretty much the same.
>
> OK, got it.
>
>> Also I don't see the subsequent
>> discussion on skb->encapsulation affects the validity of this patch so
>> i'm resubmitting it. Also the patch has been confirmed to address the problem
>> Wolfgang reported last week. Feel free to test against the configuration (VXLAN?)
>> you had some question about earlier.
>
> Well, re-reading that thread, we were not very decisive there... my
> comment of setting the inner network header twice in inet_gro_complete
> doesn't apply only to vxlan but to other tunneling protocols.

Understood, but my reply about it applying to the inner most hdr in the end
is not limited to vxlan either.

> Also, if
> we really need (why do we? or explained it on the Feb thread) to set
> skb->encapsulation for the sake of TX in a protocol (GRE) gro
> complete, looks a bit fishy to me...

Why does it look fishy? When h/w support sLRO on tunneled pkts
driver will set that bit. So it seems very reasonable for the GRO stack
that supports tunneled pkts to set that bit too.

> but saying this I think brings us
> back to that incomplete discussion [1]  sounds as this needs some
> plumbering... Still, this way or another I understand a regression was
> introduced here and should be fixed.

Yep. I thought we've made reasonable progress on the skb->encapsualtion
even though we can't close all the questions/issues (but we have a real
bug that needs to be fixed here).

Jerry

>
> Or.
>
> [1] http://marc.info/?t=139353642700003&r=1&w=2

Re: [PATCH net-next] net-gre-gro: Fix a bug that breaks the forwarding path

[David Miller]

From: "H.K. Jerry Chu" <hkchu@google.com>
Date: Sun, 13 Jul 2014 07:46:20 -0700

> From: Jerry Chu <hkchu@google.com>
> 
> Fixed a bug that was introduced by my GRE-GRO patch
> (bf5a755f5e9186406bbf50f4087100af5bd68e40 net-gre-gro: Add GRE
> support to the GRO stack) that breaks the forwarding path
> because various GSO related fields were not set. The bug will
> cause on the egress path either the GSO code to fail, or a
> GRE-TSO capable (NETIF_F_GSO_GRE) NICs to choke. The following
> fix has been tested for both cases.
> 
> Signed-off-by: H.K. Jerry Chu <hkchu@google.com>

As a regression fix, this should be targetted at 'net' rather than
'net-next' so I can queue it up for -stable too.

Thanks.

Re: [PATCH net-next] net-gre-gro: Fix a bug that breaks the forwarding path

[Jerry Chu]

On Mon, Jul 14, 2014 at 3:05 PM, David Miller <davem@davemloft.net> wrote:
>
> From: "H.K. Jerry Chu" <hkchu@google.com>
> Date: Sun, 13 Jul 2014 07:46:20 -0700
>
> > From: Jerry Chu <hkchu@google.com>
> >
> > Fixed a bug that was introduced by my GRE-GRO patch
> > (bf5a755f5e9186406bbf50f4087100af5bd68e40 net-gre-gro: Add GRE
> > support to the GRO stack) that breaks the forwarding path
> > because various GSO related fields were not set. The bug will
> > cause on the egress path either the GSO code to fail, or a
> > GRE-TSO capable (NETIF_F_GSO_GRE) NICs to choke. The following
> > fix has been tested for both cases.
> >
> > Signed-off-by: H.K. Jerry Chu <hkchu@google.com>
>
> As a regression fix, this should be targetted at 'net' rather than
> 'net-next' so I can queue it up for -stable too.

Ok, will submit a separate patch for 'net' soon.

Thanks,

Jerry

>
> Thanks.

Re: [PATCH net-next] net-gre-gro: Fix a bug that breaks the forwarding path

[Or Gerlitz]

On Tue, Jul 15, 2014 at 12:30 AM, Jerry Chu <hkchu@google.com> wrote:
>
> On Mon, Jul 14, 2014 at 1:38 PM, Or Gerlitz <or.gerlitz@gmail.com> wrote:
> > On Mon, Jul 14, 2014 at 10:16 PM, Jerry Chu <hkchu@google.com> wrote:
> >> On Mon, Jul 14, 2014 at 12:05 PM, Or Gerlitz <or.gerlitz@gmail.com> wrote:
> >>> On Sun, Jul 13, 2014 at 5:46 PM, H.K. Jerry Chu <hkchu@google.com> wrote:
> >>>> Fixed a bug that was introduced by my GRE-GRO patch
> >>>> (bf5a755f5e9186406bbf50f4087100af5bd68e40 net-gre-gro: Add GRE
> >>>> support to the GRO stack) that breaks the forwarding path
> >>>> because various GSO related fields were not set. The bug will
> >>>> cause on the egress path either the GSO code to fail, or a
> >>>> GRE-TSO capable (NETIF_F_GSO_GRE) NICs to choke. The following
> >>>> fix has been tested for both cases.
> >
> >>> Anything different in this version vs. the one you posted earlier on
> >>> February or this is a plain re-post?
> >
> >> I simply moved the patch against the latest net-next and resolved some small
> >> conflict so yes it's pretty much the same.
> >
> > OK, got it.
> >
> >> Also I don't see the subsequent
> >> discussion on skb->encapsulation affects the validity of this patch so
> >> i'm resubmitting it. Also the patch has been confirmed to address the problem
> >> Wolfgang reported last week. Feel free to test against the configuration (VXLAN?)
> >> you had some question about earlier.
> >
> > Well, re-reading that thread, we were not very decisive there... my
> > comment of setting the inner network header twice in inet_gro_complete
> > doesn't apply only to vxlan but to other tunneling protocols.
>
> Understood, but my reply about it applying to the inner most hdr in the end
> is not limited to vxlan either.
>
> > Also, if
> > we really need (why do we? or explained it on the Feb thread) to set
> > skb->encapsulation for the sake of TX in a protocol (GRE) gro
> > complete, looks a bit fishy to me...
>
> Why does it look fishy? When h/w support sLRO on tunneled pkts
> driver will set that bit. So it seems very reasonable for the GRO stack
> that supports tunneled pkts to set that bit too.


what do you mean by "h/w support sLRO on tunneled pkts" here? as far
as I understand, the driver should set the bit when they know it's an
encapsulated
packet for which the HW offloaded checksum verification.

So my fishiness comment was referring to the fact that we see here
that the stack
is setting this bit too sometimes.

> > but saying this I think brings us
> > back to that incomplete discussion [1]  sounds as this needs some
> > plumbering... Still, this way or another I understand a regression was
> > introduced here and should be fixed.

> Yep. I thought we've made reasonable progress on the skb->encapsualtion
> even though we can't close all the questions/issues

where/how exactly (or more or less) this progress comes into play, is it
by the description exchanged in the emails sent over the Feb/Mar thread
or in Tom's 3.15/16/17 patches?

> (but we have a real bug that needs to be fixed here).

agree...

Re: [PATCH net-next] net-gre-gro: Fix a bug that breaks the forwarding path

[Jerry Chu]

On Tue, Jul 15, 2014 at 7:06 AM, Or Gerlitz <or.gerlitz@gmail.com> wrote:
> On Tue, Jul 15, 2014 at 12:30 AM, Jerry Chu <hkchu@google.com> wrote:
>>
>> On Mon, Jul 14, 2014 at 1:38 PM, Or Gerlitz <or.gerlitz@gmail.com> wrote:
>> > On Mon, Jul 14, 2014 at 10:16 PM, Jerry Chu <hkchu@google.com> wrote:
>> >> On Mon, Jul 14, 2014 at 12:05 PM, Or Gerlitz <or.gerlitz@gmail.com> wrote:
>> >>> On Sun, Jul 13, 2014 at 5:46 PM, H.K. Jerry Chu <hkchu@google.com> wrote:
>> >>>> Fixed a bug that was introduced by my GRE-GRO patch
>> >>>> (bf5a755f5e9186406bbf50f4087100af5bd68e40 net-gre-gro: Add GRE
>> >>>> support to the GRO stack) that breaks the forwarding path
>> >>>> because various GSO related fields were not set. The bug will
>> >>>> cause on the egress path either the GSO code to fail, or a
>> >>>> GRE-TSO capable (NETIF_F_GSO_GRE) NICs to choke. The following
>> >>>> fix has been tested for both cases.
>> >
>> >>> Anything different in this version vs. the one you posted earlier on
>> >>> February or this is a plain re-post?
>> >
>> >> I simply moved the patch against the latest net-next and resolved some small
>> >> conflict so yes it's pretty much the same.
>> >
>> > OK, got it.
>> >
>> >> Also I don't see the subsequent
>> >> discussion on skb->encapsulation affects the validity of this patch so
>> >> i'm resubmitting it. Also the patch has been confirmed to address the problem
>> >> Wolfgang reported last week. Feel free to test against the configuration (VXLAN?)
>> >> you had some question about earlier.
>> >
>> > Well, re-reading that thread, we were not very decisive there... my
>> > comment of setting the inner network header twice in inet_gro_complete
>> > doesn't apply only to vxlan but to other tunneling protocols.
>>
>> Understood, but my reply about it applying to the inner most hdr in the end
>> is not limited to vxlan either.
>>
>> > Also, if
>> > we really need (why do we? or explained it on the Feb thread) to set
>> > skb->encapsulation for the sake of TX in a protocol (GRE) gro
>> > complete, looks a bit fishy to me...
>>
>> Why does it look fishy? When h/w support sLRO on tunneled pkts
>> driver will set that bit. So it seems very reasonable for the GRO stack
>> that supports tunneled pkts to set that bit too.
>
>
> what do you mean by "h/w support sLRO on tunneled pkts" here? as far
> as I understand, the driver should set the bit when they know it's an
> encapsulated
> packet for which the HW offloaded checksum verification.
>
> So my fishiness comment was referring to the fact that we see here
> that the stack
> is setting this bit too sometimes.

There are many other places in the "stack" that will set that bit if you
grep it. I think many of them are there to make GSO work on tunneled pkts.
The code here is no different.

>
>> > but saying this I think brings us
>> > back to that incomplete discussion [1]  sounds as this needs some
>> > plumbering... Still, this way or another I understand a regression was
>> > introduced here and should be fixed.
>
>> Yep. I thought we've made reasonable progress on the skb->encapsualtion
>> even though we can't close all the questions/issues
>
> where/how exactly (or more or less) this progress comes into play, is it
> by the description exchanged in the emails sent over the Feb/Mar thread
> or in Tom's 3.15/16/17 patches?

Davem wanted a more precise definition of skb->encapsulation and I thought
some worthy discussion has happened as part of the thread of the
original patch (even though it left with some more questions but
that's often the case
with complex kernel code, right?)

>
>> (but we have a real bug that needs to be fixed here).
>
> agree...

Re: [PATCH net-next] net-gre-gro: Fix a bug that breaks the forwarding path

[Or Gerlitz]

On Tue, Jul 15, 2014 at 8:17 PM, Jerry Chu <hkchu@google.com> wrote:
> On Tue, Jul 15, 2014 at 7:06 AM, Or Gerlitz <or.gerlitz@gmail.com> wrote:

>> what do you mean by "h/w support sLRO on tunneled pkts" here? as far
>> as I understand, the driver should set the bit when they know it's an
>> encapsulated  packet for which the HW offloaded checksum verification.

can you clarify the sLRO comment?

>> So my fishiness comment was referring to the fact that we see here
>> that the stack is setting this bit too sometimes.

> There are many other places in the "stack" that will set that bit if you
> grep it. I think many of them are there to make GSO work on tunneled pkts.
> The code here is no different.

I am not near the code now, but AFAIK, the "stack" sets it in the TX
path and the driver sets it in the RX path, any deviation you see
there for this convension except for the change introduced by this
patch.

> Davem wanted a more precise definition of skb->encapsulation and I thought
> some worthy discussion has happened as part of the thread of the
> original patch (even though it left with some more questions but
> that's often the case with complex kernel code, right?)


complex as it may be, if the design is valid (e.g doesn't contradict
itself, etc...) it should be subject to proper documentation and
implementation. So in that repspect, I read Dave's comment as saying
-- guys, this isn't the case with that skb encapsulation bit.

For a nearby example, see the documentation of the semantic of the
different checksum values (e.g none/unnecessary/complete/
partial) that the stack and drivers are setting, it wasn't very
clear/percise since maybe kernel 2.4 and only over one of the last
kernel cycles on Dec 2013 this patch made it clear "net: skbuff:
improve comment on checksumming"

Re: [PATCH net-next] net-gre-gro: Fix a bug that breaks the forwarding path

[Tom Herbert]

On Tue, Jul 15, 2014 at 10:17 AM, Jerry Chu <hkchu@google.com> wrote:
> On Tue, Jul 15, 2014 at 7:06 AM, Or Gerlitz <or.gerlitz@gmail.com> wrote:
>> On Tue, Jul 15, 2014 at 12:30 AM, Jerry Chu <hkchu@google.com> wrote:
>>>
>>> On Mon, Jul 14, 2014 at 1:38 PM, Or Gerlitz <or.gerlitz@gmail.com> wrote:
>>> > On Mon, Jul 14, 2014 at 10:16 PM, Jerry Chu <hkchu@google.com> wrote:
>>> >> On Mon, Jul 14, 2014 at 12:05 PM, Or Gerlitz <or.gerlitz@gmail.com> wrote:
>>> >>> On Sun, Jul 13, 2014 at 5:46 PM, H.K. Jerry Chu <hkchu@google.com> wrote:
>>> >>>> Fixed a bug that was introduced by my GRE-GRO patch
>>> >>>> (bf5a755f5e9186406bbf50f4087100af5bd68e40 net-gre-gro: Add GRE
>>> >>>> support to the GRO stack) that breaks the forwarding path
>>> >>>> because various GSO related fields were not set. The bug will
>>> >>>> cause on the egress path either the GSO code to fail, or a
>>> >>>> GRE-TSO capable (NETIF_F_GSO_GRE) NICs to choke. The following
>>> >>>> fix has been tested for both cases.
>>> >
>>> >>> Anything different in this version vs. the one you posted earlier on
>>> >>> February or this is a plain re-post?
>>> >
>>> >> I simply moved the patch against the latest net-next and resolved some small
>>> >> conflict so yes it's pretty much the same.
>>> >
>>> > OK, got it.
>>> >
>>> >> Also I don't see the subsequent
>>> >> discussion on skb->encapsulation affects the validity of this patch so
>>> >> i'm resubmitting it. Also the patch has been confirmed to address the problem
>>> >> Wolfgang reported last week. Feel free to test against the configuration (VXLAN?)
>>> >> you had some question about earlier.
>>> >
>>> > Well, re-reading that thread, we were not very decisive there... my
>>> > comment of setting the inner network header twice in inet_gro_complete
>>> > doesn't apply only to vxlan but to other tunneling protocols.
>>>
>>> Understood, but my reply about it applying to the inner most hdr in the end
>>> is not limited to vxlan either.
>>>
>>> > Also, if
>>> > we really need (why do we? or explained it on the Feb thread) to set
>>> > skb->encapsulation for the sake of TX in a protocol (GRE) gro
>>> > complete, looks a bit fishy to me...
>>>
>>> Why does it look fishy? When h/w support sLRO on tunneled pkts
>>> driver will set that bit. So it seems very reasonable for the GRO stack
>>> that supports tunneled pkts to set that bit too.
>>
>>
>> what do you mean by "h/w support sLRO on tunneled pkts" here? as far
>> as I understand, the driver should set the bit when they know it's an
>> encapsulated
>> packet for which the HW offloaded checksum verification.
>>
>> So my fishiness comment was referring to the fact that we see here
>> that the stack
>> is setting this bit too sometimes.
>
> There are many other places in the "stack" that will set that bit if you
> grep it. I think many of them are there to make GSO work on tunneled pkts.
> The code here is no different.
>
>>
>>> > but saying this I think brings us
>>> > back to that incomplete discussion [1]  sounds as this needs some
>>> > plumbering... Still, this way or another I understand a regression was
>>> > introduced here and should be fixed.
>>
>>> Yep. I thought we've made reasonable progress on the skb->encapsualtion
>>> even though we can't close all the questions/issues
>>
>> where/how exactly (or more or less) this progress comes into play, is it
>> by the description exchanged in the emails sent over the Feb/Mar thread
>> or in Tom's 3.15/16/17 patches?
>
> Davem wanted a more precise definition of skb->encapsulation and I thought
> some worthy discussion has happened as part of the thread of the
> original patch (even though it left with some more questions but
> that's often the case
> with complex kernel code, right?)
>
Unfortunately, skb->encapsulation is overloaded with different
meanings in TX and RX path. I think the invariant should be that if it
is set, the inner headers are valid, so this is currently only useful
on TX. For Rx, where it's used to indicate inner checksum was
validated, I intend to a add a separate field in skbuf (probably two
bits to allow for multiple level encapsulations).

>>
>>> (but we have a real bug that needs to be fixed here).
>>
>> agree...

Re: [PATCH net-next] net-gre-gro: Fix a bug that breaks the forwarding path

[Jerry Chu]

On Tue, Jul 15, 2014 at 11:21 AM, Or Gerlitz <or.gerlitz@gmail.com> wrote:
> On Tue, Jul 15, 2014 at 8:17 PM, Jerry Chu <hkchu@google.com> wrote:
>>
>> On Tue, Jul 15, 2014 at 7:06 AM, Or Gerlitz <or.gerlitz@gmail.com> wrote:
>> > On Tue, Jul 15, 2014 at 12:30 AM, Jerry Chu <hkchu@google.com> wrote:
>> >>
>> >> On Mon, Jul 14, 2014 at 1:38 PM, Or Gerlitz <or.gerlitz@gmail.com>
>> >> wrote:
>> >> > On Mon, Jul 14, 2014 at 10:16 PM, Jerry Chu <hkchu@google.com> wrote:
>> >> >> On Mon, Jul 14, 2014 at 12:05 PM, Or Gerlitz <or.gerlitz@gmail.com>
>> >> >> wrote:
>> >> >>> On Sun, Jul 13, 2014 at 5:46 PM, H.K. Jerry Chu <hkchu@google.com>
>> >> >>> wrote:
>> >> >>>> Fixed a bug that was introduced by my GRE-GRO patch
>> >> >>>> (bf5a755f5e9186406bbf50f4087100af5bd68e40 net-gre-gro: Add GRE
>> >> >>>> support to the GRO stack) that breaks the forwarding path
>> >> >>>> because various GSO related fields were not set. The bug will
>> >> >>>> cause on the egress path either the GSO code to fail, or a
>> >> >>>> GRE-TSO capable (NETIF_F_GSO_GRE) NICs to choke. The following
>> >> >>>> fix has been tested for both cases.
>> >> >
>> >> >>> Anything different in this version vs. the one you posted earlier
>> >> >>> on
>> >> >>> February or this is a plain re-post?
>> >> >
>> >> >> I simply moved the patch against the latest net-next and resolved
>> >> >> some small
>> >> >> conflict so yes it's pretty much the same.
>> >> >
>> >> > OK, got it.
>> >> >
>> >> >> Also I don't see the subsequent
>> >> >> discussion on skb->encapsulation affects the validity of this patch
>> >> >> so
>> >> >> i'm resubmitting it. Also the patch has been confirmed to address
>> >> >> the problem
>> >> >> Wolfgang reported last week. Feel free to test against the
>> >> >> configuration (VXLAN?)
>> >> >> you had some question about earlier.
>> >> >
>> >> > Well, re-reading that thread, we were not very decisive there... my
>> >> > comment of setting the inner network header twice in
>> >> > inet_gro_complete
>> >> > doesn't apply only to vxlan but to other tunneling protocols.
>> >>
>> >> Understood, but my reply about it applying to the inner most hdr in the
>> >> end
>> >> is not limited to vxlan either.
>> >>
>> >> > Also, if
>> >> > we really need (why do we? or explained it on the Feb thread) to set
>> >> > skb->encapsulation for the sake of TX in a protocol (GRE) gro
>> >> > complete, looks a bit fishy to me...
>> >>
>> >> Why does it look fishy? When h/w support sLRO on tunneled pkts
>> >> driver will set that bit. So it seems very reasonable for the GRO stack
>> >> that supports tunneled pkts to set that bit too.
>> >
>> >
>> > what do you mean by "h/w support sLRO on tunneled pkts" here? as far
>> > as I understand, the driver should set the bit when they know it's an
>> > encapsulated
>> > packet for which the HW offloaded checksum verification.
>> >
>> > So my fishiness comment was referring to the fact that we see here
>> > that the stack
>> > is setting this bit too sometimes.
>>
>> There are many other places in the "stack" that will set that bit if you
>> grep it. I think many of them are there to make GSO work on tunneled pkts.
>> The code here is no different.
>
>
> I am not near the code now, but AFAIK, the "stack" sets it in the TX path
> and the driver sets it in the RX path, any deviation you see there for this
> convension except for the change introduced by this patch.

Where does the forwarding path (e.g., the case at hand) belong then?

AFAICT the current dev_hard_start_xmit() code pretty much requires
skb->encapsulation to be set on all tunneled pkts in order for the proper
TX offload to be possible.

>
>
>>
>>
>> >
>> >> > but saying this I think brings us
>> >> > back to that incomplete discussion [1]  sounds as this needs some
>> >> > plumbering... Still, this way or another I understand a regression
>> >> > was
>> >> > introduced here and should be fixed.
>> >
>> >> Yep. I thought we've made reasonable progress on the skb->encapsualtion
>> >> even though we can't close all the questions/issues
>> >
>> > where/how exactly (or more or less) this progress comes into play, is it
>> > by the description exchanged in the emails sent over the Feb/Mar thread
>> > or in Tom's 3.15/16/17 patches?
>>
>> Davem wanted a more precise definition of skb->encapsulation and I thought
>> some worthy discussion has happened as part of the thread of the
>> original patch (even though it left with some more questions but
>> that's often the case with complex kernel code, right?)
>
>
> complex as it may be, if the design is valid (e.g doesn't contradict itself,
> etc...) it should be subject to proper documentation and implementation. So
> in that repspect, I read Dave's comment as saying -- guys, this isn't the
> case with that skb encapsulation bit.
>
> For a nearby example, see the documentation of the semantic of the different
> checksum values (e.g none/unnecessary/complete/partial) that the stack and
> drivers are setting, it wasn't very clear/percise since maybe kernel 2.4 and
> only over one of the last kernel cycles on Dec 2013 this patch made it clear
> "net: skbuff: improve comment on checksumming"
>
> Or.

Re: [PATCH net-next] net-gre-gro: Fix a bug that breaks the forwarding path

[Or Gerlitz]

On Tue, Jul 15, 2014 at 9:44 PM, Tom Herbert <therbert@google.com> wrote:
> Unfortunately, skb->encapsulation is overloaded with different
> meanings in TX and RX path. I think the invariant should be that if it
> is set, the inner headers are valid, so this is currently only useful
> on TX. For Rx, where it's used to indicate inner checksum was validated

I think what Jerry is trying to say here is that the TX part of the
forwarding path uses some pre-knowledge
from the RX part which sets some fields

> I intend to a add a separate field in skbuf (probably two
> bits to allow for multiple level encapsulations).

Re: [PATCH net-next] net-gre-gro: Fix a bug that breaks the forwarding path

[Or Gerlitz]

On Tue, Jul 15, 2014 at 11:22 PM, Jerry Chu <hkchu@google.com> wrote:
> On Tue, Jul 15, 2014 at 11:21 AM, Or Gerlitz <or.gerlitz@gmail.com> wrote:

>> I am not near the code now, but AFAIK, the "stack" sets it in the TX path
>> and the driver sets it in the RX path, any deviation you see there for this
>> convension except for the change introduced by this patch.

> Where does the forwarding path (e.g., the case at hand) belong then?

yep, so forwarding flow is something like

HW --> driver RX --> stack RX --> some stack processing --> stack TX
--> driver TX --> HW

and this is different from a path of

application --> stack TX --> driver TX --> HW

It seems as of even before we throw in the tunneling thing, the GRO stack
which is part of that "stack RX" code I mentioned above sets the
gso_type of SKBs
for the sake of forwarding  so these things already happen.

> AFAICT the current dev_hard_start_xmit() code pretty much requires
> skb->encapsulation to be set on all tunneled pkts in order for the proper
> TX offload to be possible.