Message-ID: <836bc0f5-004b-3b7d-d0d7-b70b030977c6@intel.com>
Date:   Mon, 30 Jan 2023 16:40:48 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Subject: Re: [PATCH 2/2] gro: optimise redundant parsing of packets
Content-Language: en-US
To:     Richard Gobert <richardbgobert@gmail.com>
References: <20230130130047.GA7913@debian> <20230130130752.GA8015@debian>
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>
From:   Alexander Lobakin <alexandr.lobakin@intel.com>
In-Reply-To: <20230130130752.GA8015@debian>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?c0lvVzd1ZFV1WGdEVG1TTjhYUFFPT2VSTndrbGdmVDJOeFJXUGsyNWVqckk3?=
 =?utf-8?B?Y003QzNqRWM3dmsxZVFxQ1ZmY2lJcW95dmNuQ1hiZnRjb1diTGJIclBXd29W?=
 =?utf-8?B?NittNllJOCtLaWlkVHpPUDNXQUtIQ25tcDhZUGdGNFFwRVRpbXFOZVhKanUw?=
 =?utf-8?B?cGIzN0x0bVNoczhXdGpuOGViM0dlYnJ0TzhYei85ekZUNFR2TmRVWlZuNkZT?=
 =?utf-8?B?ZzNNWEk1T0xpVEVyd3l4eXlCa1dSOWhnK3c1NUpXTk16OWpoWEwyeE5UOEI4?=
 =?utf-8?B?Y05DcFNoZlNhcTFqaFNOUkpYV251SUZHT2dNRzFUY0tIamMxV3l6eE1iak1T?=
 =?utf-8?B?LzM0R0paRzU1OEhWeTJoS280czJXc0NyK2tnRDNVODJ1blpXdHZMMi8zN3g5?=
 =?utf-8?B?UFhaajhuQzM0d2ZPbHcyT2I2NEEzb3BlaXJSTjFLejV3VWRBMHpPb3lGRVFN?=
 =?utf-8?B?R3g1aEJOV0txVDVBRVh0elJUM3pVemFUWXc3MVFyMVhVNklud1dqR3NqTUZO?=
 =?utf-8?B?VFRaSG8zK2NXejZKSVlva1k0VkpsSzVQRUh4S0FUTjVDMytNNmg5b2Z5b3Fm?=
 =?utf-8?B?Mlh0T042SExMNzdIaFNVenNWWlRVWnMrS3dIYjNQSVJ3YUNIQzE3bjhCcWdu?=
 =?utf-8?B?akFYeHdzV2xoanl5aVl4a05UMmJheGxvRHgvYUlKTEJsdzBJMkNHMGN6aHdW?=
 =?utf-8?B?V2lXS1BHUi9JMDNIVERzTkdsUzNjVVR5Zy9OZ2E3eWphTWtwT21udHJ2ZHd6?=
 =?utf-8?B?cy8yUmZ3RE9uaGZHUUtRSzBlcjVFOXY5QXBncUtqTjJwbTJXTmxFSGxwYkNM?=
 =?utf-8?B?VVNZWmd4akRwT1EyREZDaTJqbXoxSzFuS21nS1dHSSsyeDVYSDBHVzhqMng0?=
 =?utf-8?B?WlMxaE9Vak1lcnRmNHVCMzBzV0VkZWJvUWplc1dMSWtwbjJ1N0U1eW9RZXpH?=
 =?utf-8?B?R3kvSFlnWGRPcml5bHk4Q1N4THhDRm41YmFXbmNtcWhCanVNMFBhWldtalVq?=
 =?utf-8?B?SHdGbEtqNVhPcFFFZDdJTHd3alVwYkc5QTMvSTFmZU5mdytBdWxuTjcxeUsr?=
 =?utf-8?B?RDRwdlJIODlUOWRpaTI5d0kweWV3RHZUMnNIakJYQUNGNTkvSEtrQWZRTkpj?=
 =?utf-8?B?YUpHYVpFSERyWkluWFBSU3oybUV1eDVyZkRZaTd0OXd1azh3SWpLck1sTDY0?=
 =?utf-8?B?bmxTeVQ3aExTQjh2N2JPRThSem5Id05rdUF4UUE1aGNiZ3FPMXFONkVhbVZz?=
 =?utf-8?B?d0ZIdStvdmU4eUM5NnlRZDhEanRzVnhMcVJxaFFpbkk1Ti9jSlZtdEdnQ25Q?=
 =?utf-8?B?M3oyRytZR29vZVRhY2FFcEZITDdtcVFhZ2RXWXZKS0NNNkxGOWUvRzBsRlkr?=
 =?utf-8?B?ZU9wclhlY29nV3d2SzBDSkhiVFN0Unp2bUxLclE0c1NwMzRJQmJrcndaRWJ5?=
 =?utf-8?B?UURBV2lrV0pjVThUNFFPdEdPbHBOUGdaeklnajl4QWxtWFpvYXcvOS9OQzJr?=
 =?utf-8?B?clNYb2R4bXNRTUFhYWhxUXk1MDVyNUxudHVvQkRjY0llcUxUeDc0TzByU05N?=
 =?utf-8?B?NG9vQmVlZExFQzcvaTVtTkt3YjRBbEppZ2JucDFycm9vZEg2TnVscG5sdFVm?=
 =?utf-8?B?cUR1Sit5NVBQM3kzelhPOU5zK3RJM3NCdmtPRGN3Zi9WSjRPc1BPY1ZyQlNz?=
 =?utf-8?B?YXFGZWp5MDJHT0xFcmUzOVVPUzJkeFh1d2U4eEEzY2tLd1laNERBNDBqdDQw?=
 =?utf-8?B?TWZISWFPUWVNZVRPN0RpWHpaaS9rckp4K1MvSXFPWFJPbmJUMTZOZVJGRlh0?=
 =?utf-8?B?SDdlQ1k0MElHdlc5aGtLd0hZdGp2cXBReEw3ang2ckJCdk42aUwrUE02aVZM?=
 =?utf-8?B?K3F1TW00UEEwSlFNQmFqUnhzMUdOWVdKZkJ0UFJNbWdvT2V4dGVkWmF2ZzFr?=
 =?utf-8?B?NVBYcjhnaXdtbGxoYndTb01sRTNTaFpmVzg3eGs5bUlBT2QrajB2U09vc1FE?=
 =?utf-8?B?eWR1VU9ZVkx0WU1SS1NEc216N1RFbVBaQmYvZjQxdXd6YzFwS2lyZjloUlpG?=
 =?utf-8?B?SHljTnp1Rzdlb1plTlRjOGkwY1A2a3FZRktydGlFZEtISjNLVWFxSXJqdCs1?=
 =?utf-8?B?TWJGMXB6VzQzUVA4U3pldjVkcjRrT2xEZmV2dXd4dlIzU0pYZTNLcnB5VFNn?=
 =?utf-8?B?Smc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 8095acf4-245e-4aec-3cb9-08db02d8605f
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Jan 2023 15:40:55.9383
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: +3Irjfs/oZxm9xQWbrlkFlUQu8Tp6TdFSNKuJuUL3m08Oqlzm7XmH8LxMzVXtmO47vfE/SUkErzSD52k/mxN4ABRiXaKI7vztOiB5Lqq4Pk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR11MB6613
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

From: Richard Gobert <richardbgobert@gmail.com>
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@gmail.com>
> ---
>  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