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

[Alexander Lobakin]

From: Richard Gobert <richardbgobert@xxxxxxxxx>
Date: Mon, 30 Jan 2023 14:07:55 +0100

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

> 
> Signed-off-by: Richard Gobert <richardbgobert@xxxxxxxxx>
> ---
>  include/net/gro.h      |  8 ++++++--
>  net/ethernet/eth.c     | 11 +++++++++--
>  net/ipv4/af_inet.c     |  8 +++++++-
>  net/ipv6/ip6_offload.c | 15 ++++++++++++---
>  4 files changed, 34 insertions(+), 8 deletions(-)

[...]

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

>  	struct packet_offload *ptype;
>  	int err = -ENOSYS;
>  
> -	if (skb->encapsulation)
> +	if (skb->encapsulation) {
>  		skb_set_inner_mac_header(skb, nhoff);
> +		type = eh->h_proto;
> +	} else {
> +		type = NAPI_GRO_CB(skb)->network_proto;
> +	}
>  
>  	ptype = gro_find_complete_by_type(type);
>  	if (ptype != NULL)
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index 6c0ec2789943..4401af7b3a15 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -1551,6 +1551,9 @@ struct sk_buff *inet_gro_receive(struct list_head *head, struct sk_buff *skb)
>  	 * immediately following this IP hdr.
>  	 */
>  
> +	if (!NAPI_GRO_CB(skb)->encap_mark)
> +		NAPI_GRO_CB(skb)->transport_proto = proto;
> +
>  	/* Note : No need to call skb_gro_postpull_rcsum() here,
>  	 * as we already checked checksum over ipv4 header was 0
>  	 */
> @@ -1621,12 +1624,15 @@ int inet_gro_complete(struct sk_buff *skb, int nhoff)
>  	__be16 newlen = htons(skb->len - nhoff);
>  	struct iphdr *iph = (struct iphdr *)(skb->data + nhoff);
>  	const struct net_offload *ops;
> -	int proto = iph->protocol;
> +	int proto;

(same)

>  	int err = -ENOSYS;
>  
>  	if (skb->encapsulation) {
>  		skb_set_inner_protocol(skb, cpu_to_be16(ETH_P_IP));
>  		skb_set_inner_network_header(skb, nhoff);
> +		proto = iph->protocol;
> +	} else {
> +		proto = NAPI_GRO_CB(skb)->transport_proto;
>  	}
>  
>  	csum_replace2(&iph->check, iph->tot_len, newlen);

[...]

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

> +	} else {
> +		nhoff += sizeof(*iph) + ipv6_exthdrs_len(iph, &ops);
> +	}
> +
>  	if (WARN_ON(!ops || !ops->callbacks.gro_complete))
>  		goto out;
> 

Thanks,
Olek