[PATCH net-next] ipv6: gro: do not use slow memcmp() in ipv6_gro_receive()

[PATCH net-next] ipv6: gro: do not use slow memcmp() in ipv6_gro_receive()

[Eric Dumazet]

ipv6_gro_receive() compares 34 bytes using slow memcmp(),
while handcoding with a couple of ipv6_addr_equal() is much faster.

Before this patch, "perf top -e cycles:pp -C <cpu>" would
see memcmp() using ~10% of cpu cycles on a 40Gbit NIC
receiving IPv6 TCP traffic.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv6/ip6_offload.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c
index c7e495f1201105f1ac1724a7b8fd82399efcce32..70f525c33cb6c1f375919b94a7afc45cc6bdcd5f 100644
--- a/net/ipv6/ip6_offload.c
+++ b/net/ipv6/ip6_offload.c
@@ -229,14 +229,21 @@ static struct sk_buff *ipv6_gro_receive(struct list_head *head,
 		 * XXX skbs on the gro_list have all been parsed and pulled
 		 * already so we don't need to compare nlen
 		 * (nlen != (sizeof(*iph2) + ipv6_exthdrs_len(iph2, &ops)))
-		 * memcmp() alone below is suffcient, right?
+		 * memcmp() alone below is sufficient, right?
 		 */
 		 if ((first_word & htonl(0xF00FFFFF)) ||
-		    memcmp(&iph->nexthdr, &iph2->nexthdr,
-			   nlen - offsetof(struct ipv6hdr, nexthdr))) {
+		    !ipv6_addr_equal(&iph->saddr, &iph2->saddr) ||
+		    !ipv6_addr_equal(&iph->daddr, &iph2->daddr) ||
+		    *(u16 *)&iph->nexthdr != *(u16 *)&iph2->nexthdr) {
+not_same_flow:
 			NAPI_GRO_CB(p)->same_flow = 0;
 			continue;
 		}
+		if (unlikely(nlen > sizeof(struct ipv6hdr))) {
+			if (memcmp(iph + 1, iph2 + 1,
+				   nlen - sizeof(struct ipv6hdr)))
+				goto not_same_flow;
+		}
 		/* flush if Traffic Class fields are different */
 		NAPI_GRO_CB(p)->flush |= !!(first_word & htonl(0x0FF00000));
 		NAPI_GRO_CB(p)->flush |= flush;
-- 
2.19.1.930.g4563a0d9d0-goog

Re: [PATCH net-next] ipv6: gro: do not use slow memcmp() in ipv6_gro_receive()

[David Miller]

From: Eric Dumazet <edumazet@google.com>
Date: Tue,  6 Nov 2018 14:25:52 -0800

> +		if (unlikely(nlen > sizeof(struct ipv6hdr))) {
> +			if (memcmp(iph + 1, iph2 + 1,
> +				   nlen - sizeof(struct ipv6hdr)))
> +				goto not_same_flow;
> +		}

Is this even possible?

	off = skb_gro_offset(skb);
	hlen = off + sizeof(*iph);
	iph = skb_gro_header_fast(skb, off);

off is some offset to the ipv6hdr in skb.  This is GRO's CB data_offset.

	skb_set_network_header(skb, off);
	skb_gro_pull(skb, sizeof(*iph));
	skb_set_transport_header(skb, skb_gro_offset(skb));

Set network header to location of iph in SKB.

GRO pull causes an incremebt of data_offset by sizeof(*iph) bytes.

Set transport header to new data_offset value.

	nlen = skb_network_header_len(skb);

This is transport_header - network_header.

>From what I can see, it is impossible for this to take on any value
other than sizeof(*ipv6hdr).

If you agree, please let's get rid of nlen and this useless code, and
replace with sizeof(*ipv6hdr) as needed.

Thanks.

Re: [PATCH net-next] ipv6: gro: do not use slow memcmp() in ipv6_gro_receive()

[Eric Dumazet]

On Tue, Nov 6, 2018 at 2:41 PM David Miller <davem@davemloft.net> wrote:
>
> From: Eric Dumazet <edumazet@google.com>
> Date: Tue,  6 Nov 2018 14:25:52 -0800
>
> > +             if (unlikely(nlen > sizeof(struct ipv6hdr))) {
> > +                     if (memcmp(iph + 1, iph2 + 1,
> > +                                nlen - sizeof(struct ipv6hdr)))
> > +                             goto not_same_flow;
> > +             }
>
> Is this even possible?

I believe that nlen can be indeed > sizeof(struct ipv6hdr) in presence
of exthdrs,
eg if ipv6_gso_pull_exthdrs() had to be called (line 201)

 I admit I have not checked if this was actually possible.


>
>         off = skb_gro_offset(skb);
>         hlen = off + sizeof(*iph);
>         iph = skb_gro_header_fast(skb, off);
>
> off is some offset to the ipv6hdr in skb.  This is GRO's CB data_offset.
>
>         skb_set_network_header(skb, off);
>         skb_gro_pull(skb, sizeof(*iph));
>         skb_set_transport_header(skb, skb_gro_offset(skb));
>
> Set network header to location of iph in SKB.
>
> GRO pull causes an incremebt of data_offset by sizeof(*iph) bytes.
>
> Set transport header to new data_offset value.
>
>         nlen = skb_network_header_len(skb);
>
> This is transport_header - network_header.
>
> From what I can see, it is impossible for this to take on any value
> other than sizeof(*ipv6hdr).
>
> If you agree, please let's get rid of nlen and this useless code, and
> replace with sizeof(*ipv6hdr) as needed.
>
> Thanks.

Re: [PATCH net-next] ipv6: gro: do not use slow memcmp() in ipv6_gro_receive()

[David Miller]

From: Eric Dumazet <edumazet@google.com>
Date: Tue, 6 Nov 2018 14:51:15 -0800

> On Tue, Nov 6, 2018 at 2:41 PM David Miller <davem@davemloft.net> wrote:
>>
>> From: Eric Dumazet <edumazet@google.com>
>> Date: Tue,  6 Nov 2018 14:25:52 -0800
>>
>> > +             if (unlikely(nlen > sizeof(struct ipv6hdr))) {
>> > +                     if (memcmp(iph + 1, iph2 + 1,
>> > +                                nlen - sizeof(struct ipv6hdr)))
>> > +                             goto not_same_flow;
>> > +             }
>>
>> Is this even possible?
> 
> I believe that nlen can be indeed > sizeof(struct ipv6hdr) in presence
> of exthdrs,
> eg if ipv6_gso_pull_exthdrs() had to be called (line 201)
> 
>  I admit I have not checked if this was actually possible.

Indeed, that does make it possible.

Patch applied, thanks!