Date:   Wed, 22 Feb 2023 15:47:46 +0100
From:   Richard Gobert <richardbgobert@gmail.com>
To:     Alexander Lobakin <alexandr.lobakin@intel.com>
Cc:     davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, yoshfuji@linux-ipv6.org, dsahern@kernel.org,
        steffen.klassert@secunet.com, lixiaoyan@google.com,
        alexanderduyck@fb.com, leon@kernel.org, ye.xingchen@zte.com.cn,
        iwienand@redhat.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] gro: optimise redundant parsing of packets
Message-ID: <20230222144743.GA12416@debian>
References: <20230130130047.GA7913@debian>
 <20230130130752.GA8015@debian>
 <836bc0f5-004b-3b7d-d0d7-b70b030977c6@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <836bc0f5-004b-3b7d-d0d7-b70b030977c6@intel.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

> > 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.