[PATCH net-next] gro: relax ID check in inet_gro_receive()

[PATCH net-next] gro: relax ID check in inet_gro_receive()

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

GRE TSO support doesn't increment the ID in the inner IP header.

Remove the ID check in inet_gro_receive() so that GRO can properly
aggregate GRE encapsulated TCP packets, instead of forcing
a flush for every packet.

Testing the IP ID is not really needed anyway for proper GRO operation.

We can use more readable (and faster) code to access tot_len and
frag_off fields.

Tested on a bnx2x setup after commit a848ade408b6b
(bnx2x: add CSUM and TSO support for encapsulation protocols)

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Dmitry Kravkov <dmitry@broadcom.com>
Cc: Eilon Greenstein <eilong@broadcom.com>
Cc: Pravin B Shelar <pshelar@nicira.com>
Cc: H.K. Jerry Chu <hkchu@google.com>
Cc: Maciej Żenczykowski <maze@google.com>
---
 net/ipv4/af_inet.c |   11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 9e5882c..302a47e 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1355,7 +1355,6 @@ static struct sk_buff **inet_gro_receive(struct sk_buff **head,
 	const struct iphdr *iph;
 	unsigned int hlen;
 	unsigned int off;
-	unsigned int id;
 	int flush = 1;
 	int proto;
 
@@ -1381,9 +1380,9 @@ static struct sk_buff **inet_gro_receive(struct sk_buff **head,
 	if (unlikely(ip_fast_csum((u8 *)iph, 5)))
 		goto out_unlock;
 
-	id = ntohl(*(__be32 *)&iph->id);
-	flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id ^ IP_DF));
-	id >>= 16;
+	flush = ntohs(iph->tot_len) ^ skb_gro_len(skb);
+
+	flush |= (__force u16)iph->frag_off ^ htons(IP_DF);
 
 	for (p = *head; p; p = p->next) {
 		struct iphdr *iph2;
@@ -1400,11 +1399,9 @@ static struct sk_buff **inet_gro_receive(struct sk_buff **head,
 			continue;
 		}
 
-		/* All fields must match except length and checksum. */
 		NAPI_GRO_CB(p)->flush |=
 			(iph->ttl ^ iph2->ttl) |
-			(iph->tos ^ iph2->tos) |
-			((u16)(ntohs(iph2->id) + NAPI_GRO_CB(p)->count) ^ id);
+			(iph->tos ^ iph2->tos);
 
 		NAPI_GRO_CB(p)->flush |= flush;
 	}

Re: [PATCH net-next] gro: relax ID check in inet_gro_receive()

[Maciej Żenczykowski]

Ack.

I've never understood the usefulness of the 'IP ID increments by one'
check in the GRO TCP path anyway.

TCP packets are DF.
AFAICT, the IP identifier field does not really serve a useful purpose
for non-fragment-ed/able packets.

The only possible exception I can think of has to do with
broken/non-spec-compliant stuff which fragments DF packets, or removes
the DF flag.
But that stuff shouldn't really work (and often doesn't) anyway.

Maciej Żenczykowski, Kernel Networking Developer @ Google

On Wed, Mar 20, 2013 at 9:52 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> GRE TSO support doesn't increment the ID in the inner IP header.
>
> Remove the ID check in inet_gro_receive() so that GRO can properly
> aggregate GRE encapsulated TCP packets, instead of forcing
> a flush for every packet.
>
> Testing the IP ID is not really needed anyway for proper GRO operation.
>
> We can use more readable (and faster) code to access tot_len and
> frag_off fields.
>
> Tested on a bnx2x setup after commit a848ade408b6b
> (bnx2x: add CSUM and TSO support for encapsulation protocols)
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Dmitry Kravkov <dmitry@broadcom.com>
> Cc: Eilon Greenstein <eilong@broadcom.com>
> Cc: Pravin B Shelar <pshelar@nicira.com>
> Cc: H.K. Jerry Chu <hkchu@google.com>
> Cc: Maciej Żenczykowski <maze@google.com>
> ---
>  net/ipv4/af_inet.c |   11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index 9e5882c..302a47e 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -1355,7 +1355,6 @@ static struct sk_buff **inet_gro_receive(struct sk_buff **head,
>         const struct iphdr *iph;
>         unsigned int hlen;
>         unsigned int off;
> -       unsigned int id;
>         int flush = 1;
>         int proto;
>
> @@ -1381,9 +1380,9 @@ static struct sk_buff **inet_gro_receive(struct sk_buff **head,
>         if (unlikely(ip_fast_csum((u8 *)iph, 5)))
>                 goto out_unlock;
>
> -       id = ntohl(*(__be32 *)&iph->id);
> -       flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id ^ IP_DF));
> -       id >>= 16;
> +       flush = ntohs(iph->tot_len) ^ skb_gro_len(skb);
> +
> +       flush |= (__force u16)iph->frag_off ^ htons(IP_DF);
>
>         for (p = *head; p; p = p->next) {
>                 struct iphdr *iph2;
> @@ -1400,11 +1399,9 @@ static struct sk_buff **inet_gro_receive(struct sk_buff **head,
>                         continue;
>                 }
>
> -               /* All fields must match except length and checksum. */
>                 NAPI_GRO_CB(p)->flush |=
>                         (iph->ttl ^ iph2->ttl) |
> -                       (iph->tos ^ iph2->tos) |
> -                       ((u16)(ntohs(iph2->id) + NAPI_GRO_CB(p)->count) ^ id);
> +                       (iph->tos ^ iph2->tos);
>
>                 NAPI_GRO_CB(p)->flush |= flush;
>         }
>
>

Re: [PATCH net-next] gro: relax ID check in inet_gro_receive()

[Maciej Żenczykowski]

> I've never understood the usefulness of the 'IP ID increments by one'
check in the GRO TCP path anyway.

I think the above logic holds for non-TCP GRO in exactly the same way.
Furthermore, there's nothing like it in IPv6 (due to the outright lack
of an IPv6 ID field).

(And I have to learn to pay attention to those stupid 3 little
gray-on-white dots that hide the entire email I'm replying to)

Re: [PATCH net-next] gro: relax ID check in inet_gro_receive()

[David Miller]

From: Maciej Żenczykowski <maze@google.com>
Date: Thu, 21 Mar 2013 02:59:49 -0700

> TCP packets are DF.
> AFAICT, the IP identifier field does not really serve a useful purpose
> for non-fragment-ed/able packets.

Not incrementing it by one each frame breaks serial line TCP header
compression.

Re: [PATCH net-next] gro: relax ID check in inet_gro_receive()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 20 Mar 2013 21:52:33 -0700

> GRE TSO support doesn't increment the ID in the inner IP header.

Is this a fundamental limitation of doing TSO over GRO or
were the Broadcom folks just being lazy with their firmware
implementation?

I really don't want to apply this patch, because ipv4 frames
even with DF set should have an incrementing ID field, in
order to accomodate various header compression schemes.

We go out of our way to do this for normal unencapsulated TCP stream
packets, rather than set the ID field to zero (which we did for some
time until the compression issue was pointed out to us).

Re: [PATCH net-next] gro: relax ID check in inet_gro_receive()

[Eric Dumazet]

On Thu, 2013-03-21 at 11:46 -0400, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, 20 Mar 2013 21:52:33 -0700
> 
> > GRE TSO support doesn't increment the ID in the inner IP header.
> 
> Is this a fundamental limitation of doing TSO over GRO or
> were the Broadcom folks just being lazy with their firmware
> implementation?
> 

Well, I suspect this hardware is not capable of doing the proper ID
manipulation twice. (inner and outer header)

Still TSO support permits a single GRE flow going from 3Gbps to 9Gbps on
our hosts. So even if the inner IP id is 'broken', we are going to use
TSO.

Note we are limited by the receiver, as the receiver has to perform the
tcp checksum in software (bnx2x doesnt support CHECKSUM_COMPLETE yet)

Hopefully next firmware or NIC will do the right thing.

> I really don't want to apply this patch, because ipv4 frames
> even with DF set should have an incrementing ID field, in
> order to accomodate various header compression schemes.
> 
> We go out of our way to do this for normal unencapsulated TCP stream
> packets, rather than set the ID field to zero (which we did for some
> time until the compression issue was pointed out to us).

I understand your concern, but this check in GRO brings nothing at all.

Once we receive frames with 'bad IPv4 ID', should we accept them or drop
them ?

TCP stack doesn't care at receive (obviously as this ID is not a concern
for the transport layer), so GRO should do the same, as GRO is a best
effort to reduce cpu load.

I fully understand the 'tos' check because of proper ECN support, but
the ttl check or id check are totally useless and time consuming.

GRO aggregation should roughly work the same than TCP coalescing, and we
don't care of IP ID or ttl in TCP stack.

Re: [PATCH net-next] gro: relax ID check in inet_gro_receive()

[Ben Hutchings]

On Thu, 2013-03-21 at 11:46 -0400, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, 20 Mar 2013 21:52:33 -0700
> 
> > GRE TSO support doesn't increment the ID in the inner IP header.
> 
> Is this a fundamental limitation of doing TSO over GRO or
> were the Broadcom folks just being lazy with their firmware
> implementation?
> 
> I really don't want to apply this patch, because ipv4 frames
> even with DF set should have an incrementing ID field, in
> order to accomodate various header compression schemes.
> 
> We go out of our way to do this for normal unencapsulated TCP stream
> packets, rather than set the ID field to zero (which we did for some
> time until the compression issue was pointed out to us).

Besides which, GRO has been reliably reversible until now.  (gso_size is
available through packet sockets, even if tcpdump doesn't appear to use
it yet.)  Ignoring IPv4 IDs will break that guarantee.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next] gro: relax ID check in inet_gro_receive()

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Thu, 21 Mar 2013 16:20:25 +0000

> On Thu, 2013-03-21 at 11:46 -0400, David Miller wrote:
>> From: Eric Dumazet <eric.dumazet@gmail.com>
>> Date: Wed, 20 Mar 2013 21:52:33 -0700
>> 
>> > GRE TSO support doesn't increment the ID in the inner IP header.
>> 
>> Is this a fundamental limitation of doing TSO over GRO or
>> were the Broadcom folks just being lazy with their firmware
>> implementation?
>> 
>> I really don't want to apply this patch, because ipv4 frames
>> even with DF set should have an incrementing ID field, in
>> order to accomodate various header compression schemes.
>> 
>> We go out of our way to do this for normal unencapsulated TCP stream
>> packets, rather than set the ID field to zero (which we did for some
>> time until the compression issue was pointed out to us).
> 
> Besides which, GRO has been reliably reversible until now.  (gso_size is
> available through packet sockets, even if tcpdump doesn't appear to use
> it yet.)  Ignoring IPv4 IDs will break that guarantee.

Right, even ignoring the header compression issues, our segmentation
offloads must be perfectly reversible.

Re: [PATCH net-next] gro: relax ID check in inet_gro_receive()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 21 Mar 2013 09:08:11 -0700

> I understand your concern, but this check in GRO brings nothing at all.

It brings reversibility, a fundamental rule of our segmentation
offloads.

Re: [PATCH net-next] gro: relax ID check in inet_gro_receive()

[Dmitry Kravkov]

Resending for netdev in plantext.


On Thu, Mar 21, 2013 at 6:08 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> On Thu, 2013-03-21 at 11:46 -0400, David Miller wrote:
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Wed, 20 Mar 2013 21:52:33 -0700
> >
> > > GRE TSO support doesn't increment the ID in the inner IP header.
> >
> > Is this a fundamental limitation of doing TSO over GRO or
> > were the Broadcom folks just being lazy with their firmware
> > implementation?
> >
>
> Well, I suspect this hardware is not capable of doing the proper ID
> manipulation twice. (inner and outer header)

This is correct: ID only for one of the headers can be handled with
current FW/HW, for other DF is set.

> Still TSO support permits a single GRE flow going from 3Gbps to 9Gbps on
> our hosts. So even if the inner IP id is 'broken', we are going to use
> TSO.
>
> Note we are limited by the receiver, as the receiver has to perform the
> tcp checksum in software (bnx2x doesnt support CHECKSUM_COMPLETE yet)
>
> Hopefully next firmware or NIC will do the right thing.
>
> > I really don't want to apply this patch, because ipv4 frames
> > even with DF set should have an incrementing ID field, in
> > order to accomodate various header compression schemes.
> >
> > We go out of our way to do this for normal unencapsulated TCP stream
> > packets, rather than set the ID field to zero (which we did for some
> > time until the compression issue was pointed out to us).
>
> I understand your concern, but this check in GRO brings nothing at all.
>
> Once we receive frames with 'bad IPv4 ID', should we accept them or drop
> them ?
>
> TCP stack doesn't care at receive (obviously as this ID is not a concern
> for the transport layer), so GRO should do the same, as GRO is a best
> effort to reduce cpu load.
>
> I fully understand the 'tos' check because of proper ECN support, but
> the ttl check or id check are totally useless and time consuming.
>
> GRO aggregation should roughly work the same than TCP coalescing, and we
> don't care of IP ID or ttl in TCP stack.
>

Re: [PATCH net-next] gro: relax ID check in inet_gro_receive()

[David Miller]

From: Dmitry Kravkov <dkravkov@gmail.com>
Date: Thu, 21 Mar 2013 20:11:37 +0200

>> Well, I suspect this hardware is not capable of doing the proper ID
>> manipulation twice. (inner and outer header)
> 
> This is correct: ID only for one of the headers can be handled with
> current FW/HW, for other DF is set.

DF does not matter.

Regardless of DF, we must set the ID field correctly.

It is abundantly clear that the current GRE tunnel segmentation
is not generating packets according to our well documented
rules, in that we must be able to precisely create exactly
the original packet stream from the segmented frame.

Someone needs to send me patches to revert the bnx2x GRE segmentation
support, and any software implementation in our tree that has the same
bug.

If someone doesn't do it, I will revert all of this code myself.  You
simply will have to cope with not having this optimization until
your hardware can do it properly and according to our well established
rules for segmentation offloads.

Thanks.

Re: [PATCH net-next] gro: relax ID check in inet_gro_receive()

[Maciej Żenczykowski]

> This is correct: ID only for one of the headers can be handled with current FW/HW, for other DF is set.

I would prefer for IP ID to be correctly incremented on the inner IPv4
packet rather than the outer IPv4.
Could that be changed?

RE: [PATCH net-next] gro: relax ID check in inet_gro_receive()

[Dmitry Kravkov]

> -----Original Message-----
> From: Maciej Żenczykowski [mailto:maze@google.com]
> Sent: Thursday, March 21, 2013 10:08 PM
> To: Dmitry Kravkov
> Cc: Eric Dumazet; David Miller; netdev@vger.kernel.org; Dmitry Kravkov; Eilon Greenstein; Pravin B Shelar; Hsiao-keng Jerry Chu
> Subject: Re: [PATCH net-next] gro: relax ID check in inet_gro_receive()
> 
> > This is correct: ID only for one of the headers can be handled with current FW/HW, for other DF is set.
> 
> I would prefer for IP ID to be correctly incremented on the inner IPv4
> packet rather than the outer IPv4.
> Could that be changed?

Yes, I will check if it can be done in driver only ... 

Re: [PATCH net-next] gro: relax ID check in inet_gro_receive()

[Maciej Żenczykowski]

Reverting/deleting this code is going way too far.
Mark it experimental, but don't delete it.

This is (a) useful for continued development and experimentation and
(b) will likely be widely deployed (and here I don't mean by my
employer specifically, but mean in general by cloud/virtualization
companies and anyone else that cares about tunnel performance)
regardless of whether upstream linux decides to include it or not.

(Furthermore if the fw/driver is changed to increment inner IP ID
instead of outer IP ID, this will for the most part not be visible to
the end users, since the tunneling is usually internal to the
virtualization provider, and no-one besides their internal network
sees the external IP header.)

Re: [PATCH net-next] gro: relax ID check in inet_gro_receive()

[David Miller]

From: Maciej Żenczykowski <maze@google.com>
Date: Thu, 21 Mar 2013 13:14:44 -0700

> Reverting/deleting this code is going way too far.
> Mark it experimental, but don't delete it.

I disagree, people who want to play can apply the code
to their tree.

It's as simple as that.

We have very reasonable requirements for segmentation offloads, if
they are not met, the feature isn't working correctly and therefore
isn't ready for upstream.

Now if someone can provide a fix soon, that's fine too.

Re: [PATCH net-next] gro: relax ID check in inet_gro_receive()

[Maciej Żenczykowski]

> We have very reasonable requirements for segmentation offloads

I would tend to disagree with reversibility being a reasonable requirement.
It offers extremely little benefit, and thus it's a nice to have at best.
However, I do understand why you might want it, and thus I can
understand why you might not want to accept this patch (which would
make GRO less reversible).

This logic does not apply to the GRE segmentation offload patches.
They might create non incrementing IP ID packet sequences on the wire,
and this may prevent GRO from kicking in, but it does not make TSO
non-reversible
(if anything it makes it easy to detect that it happened).

Would you perhaps be willing to take a patch which simply removes the
flag from the driver features?
But leaves the code in?

Re: [PATCH net-next] gro: relax ID check in inet_gro_receive()

[David Miller]

From: Maciej Żenczykowski <maze@google.com>
Date: Thu, 21 Mar 2013 13:47:07 -0700

> They might create non incrementing IP ID packet sequences on the wire,
> and this may prevent GRO from kicking in, but it does not make TSO
> non-reversible
> (if anything it makes it easy to detect that it happened).

This is not an acceptable outcome for any form GSO, it should be
completely transparent.  The packets we would have output individually
and those which result from the segmented frame must be
indistinguishable.

Re: [PATCH net-next] gro: relax ID check in inet_gro_receive()

[Jesse Gross]

On Thu, Mar 21, 2013 at 2:05 PM, David Miller <davem@davemloft.net> wrote:
> From: Maciej Żenczykowski <maze@google.com>
> Date: Thu, 21 Mar 2013 13:47:07 -0700
>
>> They might create non incrementing IP ID packet sequences on the wire,
>> and this may prevent GRO from kicking in, but it does not make TSO
>> non-reversible
>> (if anything it makes it easy to detect that it happened).
>
> This is not an acceptable outcome for any form GSO, it should be
> completely transparent.  The packets we would have output individually
> and those which result from the segmented frame must be
> indistinguishable.

We actually don't set the outer IP ID for GRE packets with the DF bit
set in the non-GSO/TSO case either (and this is not new behavior).
Obviously TCP/IP header compression doesn't apply to the GRE packet
itself.

Therefore, I think if the driver is switched to increment the inner
ID, which it sounds like it can be, then everything should be
transparent.

Re: [PATCH net-next] gro: relax ID check in inet_gro_receive()

[David Miller]

From: Jesse Gross <jesse@nicira.com>
Date: Thu, 21 Mar 2013 14:37:38 -0700

> On Thu, Mar 21, 2013 at 2:05 PM, David Miller <davem@davemloft.net> wrote:
>> From: Maciej Żenczykowski <maze@google.com>
>> Date: Thu, 21 Mar 2013 13:47:07 -0700
>>
>>> They might create non incrementing IP ID packet sequences on the wire,
>>> and this may prevent GRO from kicking in, but it does not make TSO
>>> non-reversible
>>> (if anything it makes it easy to detect that it happened).
>>
>> This is not an acceptable outcome for any form GSO, it should be
>> completely transparent.  The packets we would have output individually
>> and those which result from the segmented frame must be
>> indistinguishable.
> 
> We actually don't set the outer IP ID for GRE packets with the DF bit
> set in the non-GSO/TSO case either (and this is not new behavior).
> Obviously TCP/IP header compression doesn't apply to the GRE packet
> itself.
> 
> Therefore, I think if the driver is switched to increment the inner
> ID, which it sounds like it can be, then everything should be
> transparent.

That's my impression as well.