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

On Tue, Jul 15, 2014 at 11:21 AM, Or Gerlitz [off-list ref] wrote:

On Tue, Jul 15, 2014 at 8:17 PM, Jerry Chu [off-list ref] wrote:

quoted

On Tue, Jul 15, 2014 at 7:06 AM, Or Gerlitz [off-list ref] wrote:

quoted

On Tue, Jul 15, 2014 at 12:30 AM, Jerry Chu [off-list ref] wrote:

quoted

On Mon, Jul 14, 2014 at 1:38 PM, Or Gerlitz [off-list ref]
wrote:

quoted

On Mon, Jul 14, 2014 at 10:16 PM, Jerry Chu [off-list ref] wrote:

quoted

On Mon, Jul 14, 2014 at 12:05 PM, Or Gerlitz [off-list ref]
wrote:

quoted

On Sun, Jul 13, 2014 at 5:46 PM, H.K. Jerry Chu [off-list ref]
wrote:

quoted

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.

quoted

quoted

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

quoted

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.

quoted

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.

quoted

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.

quoted

quoted

quoted

quoted

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.

quoted

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.

On Tue, Jul 15, 2014 at 11:22 PM, Jerry Chu [off-list ref] wrote:

On Tue, Jul 15, 2014 at 11:21 AM, Or Gerlitz [off-list ref] wrote:

quoted

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.