Re: [PATCH 2/2] gro: optimise redundant parsing of packets

[Richard Gobert <richardbgobert@gmail.com>]

> > Currently, the IPv6 extension headers are parsed twice: first in
> > ipv6_gro_receive, and then again in ipv6_gro_complete.
> > 
> > The field NAPI_GRO_CB(skb)->proto is used by GRO to hold the layer 4
> > protocol type that comes after the IPv6 layer. I noticed that it is set
> > in ipv6_gro_receive, but isn't used anywhere. By using this field, and
> > also storing the size of the network header, we can avoid parsing
> > extension headers a second time in ipv6_gro_complete.
> > 
> > The implementation had to handle both inner and outer layers in case of
> > encapsulation (as they can't use the same field).
> > 
> > I've applied this optimisation to all base protocols (IPv6, IPv4,
> > Ethernet). Then, I benchmarked this patch on my machine, using ftrace to
> > measure ipv6_gro_complete's performance, and there was an improvement.
> 
> Would be nice to see some perf numbers. "there was an improvement"
> doesn't say a lot TBH...
> 

I just posted raw performance numbers as a reply to Eric's message. Take a
look there.

> > @@ -456,12 +459,16 @@ EXPORT_SYMBOL(eth_gro_receive);
> >  int eth_gro_complete(struct sk_buff *skb, int nhoff)
> >  {
> >  	struct ethhdr *eh = (struct ethhdr *)(skb->data + nhoff);
> > -	__be16 type = eh->h_proto;
> > +	__be16 type;
> 
> Please don't break RCT style when shortening/expanding variable
> declaration lines.

Will be fixed in v2.

> > @@ -358,7 +361,13 @@ INDIRECT_CALLABLE_SCOPE int ipv6_gro_complete(struct sk_buff *skb, int nhoff)
> >  		iph->payload_len = htons(payload_len);
> >  	}
> >  
> > -	nhoff += sizeof(*iph) + ipv6_exthdrs_len(iph, &ops);
> > +	if (!skb->encapsulation) {
> > +		ops = rcu_dereference(inet6_offloads[NAPI_GRO_CB(skb)->transport_proto]);
> > +		nhoff += NAPI_GRO_CB(skb)->network_len;
> 
> Why not use the same skb_network_header_len() here? Both
> skb->network_header and skb->transport_header must be set and correct at
> this point (if not, you can always fix that).
> 

When processing packets with encapsulation the network_header field is
overwritten when processing the inner IP header, so skb_network_header_len won't
return the correct value.