From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joseph Gasparakis Subject: Re: [PATCH] net-gre-gro: Fix a bug that breaks the forwarding path Date: Tue, 4 Mar 2014 17:01:06 -0800 (PST) Message-ID: References: <1393536377-32243-1-git-send-email-hkchu@google.com> <20140228.165614.1112789771887381245.davem@davemloft.net> <5315FBAA.3030809@mellanox.com> Mime-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Cc: Joseph Gasparakis , Or Gerlitz , David Miller , Pravin B Shelar , Eric Dumazet , "netdev@vger.kernel.org" , Tom Herbert To: Jerry Chu Return-path: Received: from mga14.intel.com ([143.182.124.37]:3167 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755753AbaCEAla (ORCPT ); Tue, 4 Mar 2014 19:41:30 -0500 In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Tue, 4 Mar 2014, Jerry Chu wrote: > On Tue, Mar 4, 2014 at 2:53 PM, Joseph Gasparakis > 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 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? > > > >> > >> > > >> > 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. > >> > > >> > > >> >