From: Joseph Gasparakis <joseph.gasparakis@intel•com>
To: Jerry Chu <hkchu@google•com>
Cc: Joseph Gasparakis <joseph.gasparakis@intel•com>,
Or Gerlitz <ogerlitz@mellanox•com>,
David Miller <davem@davemloft•net>,
Pravin B Shelar <pshelar@nicira•com>,
Eric Dumazet <edumazet@google•com>,
"netdev@vger•kernel.org" <netdev@vger•kernel.org>,
Tom Herbert <therbert@google•com>
Subject: Re: [PATCH] net-gre-gro: Fix a bug that breaks the forwarding path
Date: Tue, 4 Mar 2014 17:27:43 -0800 (PST) [thread overview]
Message-ID: <alpine.LFD.2.03.1403041725370.19558@intel.com> (raw)
In-Reply-To: <CAPshTCi515brk2mR=im9CYJUHWEFT2=wcT=VCR_VwkS1RV2BbQ@mail.gmail.com>
On Tue, 4 Mar 2014, Jerry Chu wrote:
> On Tue, Mar 4, 2014 at 5:01 PM, Joseph Gasparakis
> <joseph.gasparakis@intel•com> wrote:
> >
> >
> > On Tue, 4 Mar 2014, Jerry Chu wrote:
> >
> >> On Tue, Mar 4, 2014 at 2:53 PM, Joseph Gasparakis
> >> <joseph.gasparakis@intel•com> wrote:
> >> >
> >> >
> >> > On Tue, 4 Mar 2014, Jerry Chu wrote:
> >> >
> >> >> Hi Or,
> >> >>
> >> >> Thanks for writing this up.
> >> >>
> >> >> On Tue, Mar 4, 2014 at 8:13 AM, Or Gerlitz <ogerlitz@mellanox•com> wrote:
> >> >> > On 28/02/2014 23:56, David Miller wrote:
> >> >> >>
> >> >> >> The topic of the skb->encapsulation semantics has come up several times in
> >> >> >> the past few weeks. We cannot move forward on any changes in this area until
> >> >> >> the semantics are well defined, and documented. Can someone work on a patch
> >> >> >> which documents skb->encapsulation properly, and then we can come back to
> >> >> >> fixing this bug? Thanks.
> >> >> >
> >> >> >
> >> >> > Lets try... the skb->encapsulation flag was introduced and used in 3.8 by
> >> >> > the
> >> >> > sequence of these three commits
> >> >> >
> >> >> > 0afb166 vxlan: Add capability of Rx checksum offload for inner packet
> >> >> > d6727fe vxlan: capture inner headers during encapsulation
> >> >> > 6a674e9 net: Add support for hardware-offloaded encapsulation
> >> >> >
> >> >> > When discussed earlier on the list in the context of the skb->ip_summed
> >> >> > field,
> >> >> > Tom Herbert came with the following interpretation for the semantics which
> >> >> > Joseph confirmed
> >> >> >
> >> >> > "when skb->encapsulation is set the ip_summed is valid for both the inner
> >> >> > and outer header
> >> >> > (e.g. CHECKSUM_COMPLETE is always assumed okay for both layers). If
> >> >> > skb->encapsulation is not set then ip_summed is only valid for outer header"
> >> >>
> >> >> For GRE encapped pkts is the following interpretation correct?
> >> >>
> >> >> 1) ip_summed == CHECKSUM_COMPLETE
> >> >> => csum covers IP payload csum of the outer IP datagram
> >> >>
> >> >> 2) ip_summed == CHECKSUM_UNNECESSARY
> >> >> 2.1. skb->encapsulation is on => both GRE csum (if one is present) and TCP/UDP
> >> >> csum have been validated (assuming inner is a TCP or UDP pkt)
> >> >
> >> > i40e also supports SCTP csumming for the inner packet, too.
> >> >
> >> >>
> >> >> 2.2. skb->encapsulation is off => only GRE csum (if one is present) is
> >> >> validated.
> >> >>
> >> >
> >> > Apart for the SCTP request for inclusion, it looks reasonable to me.
> >> >
> >> >> >
> >> >> > As for the TX side of things, the change-log of commit 6a674e9 states
> >> >> >
> >> >> > "For Tx encapsulation offload, the driver will need to set the right bits in
> >> >> > netdev->hw_enc_features. The protocol driver will have to set the
> >> >> > skb->encapsulation bit and populate the inner headers, so the NIC driver
> >> >> > will use those inner headers to calculate the csum in hardware."
> >> >>
> >> >> So we only support/care about csum offload for the inner pkts, which
> >> >> makes sense.
> >> >
> >> > Well, we care about outer too. It is just that the inner headers are for
> >> > the inner csum, the outer headers are for the outer csum. The outer
> >> > headers will be always there anyway, so the patch introduced the inner
> >> > ones. But I guess this is what you meant, right?
> >>
> >> Actually i was wondering that since there is only one set csum_start/csum_offset
> >> fields per skb, which would you support CHECKSUM_PARTIAL for both inner
> >> and outer? You can only support one, right? (I haven't checked the UDP encap
> >> code to see how this works though.)
> >>
> >> Jerry
> >
> > I am not sure I understand the question, Jerry... Are you asking in Tx
> > path if the ip_summed==CHECKSUM_PARTIAL, which headers will the
> > csum_start/csum_offset refer to?
>
> Yes (if the question makes sense since i haven't read any UDP encap
> code - will the outer (UDP) csum be enabled?).
>
> Thanks,
>
> Jerry
I believe they will refer to the inner headers as csum_start and
csum_offset do not get changed during encapsulation and they refer to the
original (inner) packet, but please verify this.
>
> >
> >> >
> >> >>
> >> >> >
> >> >> > So in higher level, it seems that the role of the skb->encapsulation field
> >> >> > is to mark the skb to carry encapsulated packet for the code path between
> >> >> > the time the packet is encapsulated by the protocol driver (e.g vxlan/ipip)
> >> >> > to the time driver xmit is called. Or from the time driver rx code runs till
> >> >> > the the time the packet is decapsulated.
> >> >> >
> >> >> > Further, my personal interpretation was that on the rx path, skb should
> >> >> > carry the encapsulation flag **only** if the HW was able to offload the
> >> >> > inner checksum.
> >> >>
> >> >> SGTM.
> >> >>
> >> >> Jerry
> >> >>
> >> >> >
> >> >> > Joseph, what's your thinking here?
> >> >> >
> >> >> > Or.
> >> >> >
> >> >> >
> >> >>
> >>
>
next prev parent reply other threads:[~2014-03-05 1:08 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-02-27 21:26 [PATCH] net-gre-gro: Fix a bug that breaks the forwarding path H.K. Jerry Chu
2014-02-27 22:25 ` Or Gerlitz
2014-02-27 23:39 ` Jerry Chu
2014-02-28 21:56 ` David Miller
2014-03-03 9:30 ` Or Gerlitz
2014-03-04 16:13 ` Or Gerlitz
2014-03-04 22:13 ` Jerry Chu
2014-03-04 22:53 ` Joseph Gasparakis
2014-03-04 23:11 ` Jerry Chu
2014-03-05 1:01 ` Joseph Gasparakis
2014-03-05 0:54 ` Jerry Chu
2014-03-05 1:27 ` Joseph Gasparakis [this message]
2014-03-05 1:14 ` Alexei Starovoitov
2014-03-04 22:36 ` Joseph Gasparakis
2014-03-05 0:50 ` Tom Herbert
2014-03-05 1:46 ` Joseph Gasparakis
2014-03-05 20:47 ` Or Gerlitz
2014-03-06 16:42 ` Joseph Gasparakis
2014-03-06 16:30 ` Hannes Frederic Sowa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.LFD.2.03.1403041725370.19558@intel.com \
--to=joseph.gasparakis@intel$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=edumazet@google$(echo .)com \
--cc=hkchu@google$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=ogerlitz@mellanox$(echo .)com \
--cc=pshelar@nicira$(echo .)com \
--cc=therbert@google$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox