Received-SPF: pass (google.com: domain of linux-kernel+bounces-19815-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <9e8e8b47-24ab-4947-8f22-f2a07c2549ca@intel.com>
Date: Mon, 8 Jan 2024 17:04:16 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH RFC net-next 01/34] idpf: reuse libie's definitions of
 parsed ptype structures
Content-Language: en-US
To: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
CC: "David S. Miller" <davem@davemloft.net>, Eric Dumazet
	<edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni
	<pabeni@redhat.com>, Maciej Fijalkowski <maciej.fijalkowski@intel.com>,
	Michal Kubiak <michal.kubiak@intel.com>, Larysa Zaremba
	<larysa.zaremba@intel.com>, Alexei Starovoitov <ast@kernel.org>, "Daniel
 Borkmann" <daniel@iogearbox.net>, <intel-wired-lan@lists.osuosl.org>,
	<netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20231223025554.2316836-1-aleksander.lobakin@intel.com>
 <20231223025554.2316836-2-aleksander.lobakin@intel.com>
 <658c46269aa52_a33e629442@willemb.c.googlers.com.notmuch>
From: Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <658c46269aa52_a33e629442@willemb.c.googlers.com.notmuch>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WXBTd2RKYnp6aWZSWEpjSHVHSVlMTjk5TXlJQW5TMEtZbGVDQjQzdXVaTjZs?=
 =?utf-8?B?MWdRNFNXTTB2S0lnSjI4YzFtRFVyM0phd3Y0QUVhbzY2MGl6c01ML1paUWdY?=
 =?utf-8?B?ZG9SZXBYeXZEZ1dSRzNmMDUxYWFQaDVXMW56SVFwa2E0ZlpXYnRINFlvYjQy?=
 =?utf-8?B?VzlXS3VGaWEyTmhiVnZyd3dIZGF5VkprSVh5ZEZQUVVaOFJQODA2V2g4THJk?=
 =?utf-8?B?TFhFN3Q2U0RrWEliNDY3UDFYOTVNT2lvYXg3UTFUbG8wblYvSXdZd01vOVZW?=
 =?utf-8?B?YTdFWGFEQmF6SkEzeW1OK2lnNnprVDBRYU4wMlVaSGVpS2Q4TklQRXdibU1N?=
 =?utf-8?B?Qkp1M1crMDhzOFZ3Uy9vcU9BL1Z3Y0JqMHNlZm5tN2Y0RDNYNVI1ZU9CQXJ0?=
 =?utf-8?B?WGIxc3grTGFIbDZSRHF3SFBPNmZHdE1HMlNRSC9WQkc3dkxlR09KWlY5N2s3?=
 =?utf-8?B?VzNCelpQN1NEbkpTODE4V3p4NUNIM2dXOE9BemZTaGY3Z3Q3emx0eTd4bGlo?=
 =?utf-8?B?MVZVOUhrTnZJVUhoRlpxSnFNSTJjaGRUMkJieFJ0aGk2VFBZVm92RlpkeWti?=
 =?utf-8?B?TncrOWpzc3o3TGRkTkt5MnZZRXcxMXFRYWhQL3VmMnRGOHVvQ1dkTldCMVJH?=
 =?utf-8?B?a3lMK1FnT2FRM1N1MzhrVFlVbm03a3ZuRW1qRHVkQlUrOHRzTEtueTdNanQz?=
 =?utf-8?B?NTFsVWczcnpmNEVDYlN0ck5SaGdxbzNmN2tsM2s1WlNFcGlOMzRvcWo3ZHpY?=
 =?utf-8?B?MVltMnJqa1ppT3FHTWJjRUxOdXovdlh5dE5Xb1kzM3A5aGxkaTFINkppdklh?=
 =?utf-8?B?bm5yZ2E2UjY3cmgzZTU3VWRwTDZkQ1p4QnlRNitpVk9pTEVNRXU3KzQweTYw?=
 =?utf-8?B?RkN5a0oyS1cyVnNCV1FDNkxJSE1uQWtvOEkzVnczNEgxUDUySDRhWEN3K0pj?=
 =?utf-8?B?RTNWL1Y5akk5TG44b0lXQXJCbVNKeVBBSklQQitTM1JMYWFoaFdqeHhQWXUw?=
 =?utf-8?B?bU13WUxZWVFxVGx2RG5EcDJyN256clY4TlZvNlI1cHJCbjlXQkY3dm5WdTBq?=
 =?utf-8?B?OHhma0NnRE5XUDZMTlZWZml3TlBPZ1ZTZzFYQlE0bDUyOWFwOEM2Nk9nZk9Q?=
 =?utf-8?B?VHdScTh2blFhaDRCcHRRS01KeE54YlAwbGRWUjJudmtVdFZZMVZ4bzg0SUxv?=
 =?utf-8?B?dE04ZHZHZHlzamF5aGpMcjN1alZ3bFZESWZXM1dpb3JwaWduN09sTVlqNk40?=
 =?utf-8?B?STd4bVA3b2dyMFZsVXdPU001TFF5Q0x1TnVrMXYrTXVrb3M2L3ZiTFVjaVhG?=
 =?utf-8?B?YlpNc1lycUJxa1hWclk4UmpmYmU2eVZtRnlVcVlVSTJ2N0tTZmVHY2F4Szkv?=
 =?utf-8?B?NytxWDRYbWhRT3l4QW82WW9hZktRdkJhcjdGMmp1V1EwT0xQYk5wUFJIYWo1?=
 =?utf-8?B?d1FUMnhXK2drUkxXanlSWU9ORzNOL1ZxUmZXL1hJQ2FodTRJRTNKZ2cwdXBQ?=
 =?utf-8?B?SXlhTjRVT2k2V05xYmxSRHJYcXBBa2F6Q051TlFSaHhLanprWDdwSlVEaXdT?=
 =?utf-8?B?YzlMRnM5UzNyM0htZnA2V1VLREsvanNhT0xZVEczVjloSVhZbWQzMkdsUjhT?=
 =?utf-8?B?THZ6cEg5S2ZQQTM5VnZBclZyWnZXck5wQXZkL21vZHE2ZWN5M1Erdyt2Y1Jp?=
 =?utf-8?B?WVRzVW5Oczd6R3dZSEo3d3phakFWVkRxYy9RMmVtcUprb0YwSjhLUFJETS9C?=
 =?utf-8?B?b0lnQ0tvREc3S2JjOVRRZndiaE9oQlVTNnZqM3U5N056WVZsT1cvVkxmODNn?=
 =?utf-8?B?T0VEUWZlamtFd24zbDNCS2NDV280Y3hnZkdUK2xMUHZDWmVvTUh5MERXR1Jr?=
 =?utf-8?B?djRITXJCOGkweUxPYmRWbHpjY0cxbXdobEZyRFJzV09ZbXFBMVFPYTUyTDdH?=
 =?utf-8?B?V2lVaUcvcTVkYWRaejF5aThqd3BNV3d0STFZcEJDbGNtTS91anFVVVByUnZ3?=
 =?utf-8?B?ZngyTlhOMnRKeTFENEh1YUs4L3NTVndIaGgxN3lMdWRYa2NVMmZyclcwY0s4?=
 =?utf-8?B?U245SCt3M0E2blYvZk5KanFXdlVZSmtMM056OG1HSUFXUWFtS2YxamI5Qmdi?=
 =?utf-8?B?NzJjdnBwWHVMTHRPaFBuaXRTa3REZVpZcHRTei9yS3ljZThTRUdpR3NhQmRn?=
 =?utf-8?B?VEE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: e43381b8-c76a-4dc9-c61c-08dc10637aaa
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB8718.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Jan 2024 16:04:22.7161
 (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: nTBXT1eoRtpObhRvtbogYt2ctG/Ywy/QYmEIcr9RX7gSIr76vWvTy4btH4dACS2lJXqVRYGgSIVQOxYosS4zHAbcP65GuNIsm0I9zh/bFB4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR11MB8594
X-OriginatorOrg: intel.com

From: Willem De Bruijn <willemdebruijn.kernel@gmail.com>
Date: Wed, 27 Dec 2023 10:43:34 -0500

> Alexander Lobakin wrote:
>> idpf's in-kernel parsed ptype structure is almost identical to the one
>> used in the previous Intel drivers, which means it can be converted to
>> use libie's definitions and even helpers. The only difference is that
>> it doesn't use a constant table, rather than one obtained from the
>> device.

[...]

>>  static void idpf_rx_singleq_csum(struct idpf_queue *rxq, struct sk_buff *skb,
>> -				 struct idpf_rx_csum_decoded *csum_bits,
>> -				 u16 ptype)
>> +				 struct idpf_rx_csum_decoded csum_bits,
>> +				 struct libie_rx_ptype_parsed parsed)
>>  {
>> -	struct idpf_rx_ptype_decoded decoded;
>>  	bool ipv4, ipv6;
>>  
>>  	/* check if Rx checksum is enabled */
>> -	if (unlikely(!(rxq->vport->netdev->features & NETIF_F_RXCSUM)))
>> +	if (!libie_has_rx_checksum(rxq->vport->netdev, parsed))
>>  		return;
>>  
>>  	/* check if HW has decoded the packet and checksum */
>> -	if (unlikely(!(csum_bits->l3l4p)))
>> +	if (unlikely(!csum_bits.l3l4p))
>>  		return;
>>  
>> -	decoded = rxq->vport->rx_ptype_lkup[ptype];
>> -	if (unlikely(!(decoded.known && decoded.outer_ip)))
>> +	if (unlikely(parsed.outer_ip == LIBIE_RX_PTYPE_OUTER_L2))
>>  		return;
>>  
>> -	ipv4 = IDPF_RX_PTYPE_TO_IPV(&decoded, IDPF_RX_PTYPE_OUTER_IPV4);
>> -	ipv6 = IDPF_RX_PTYPE_TO_IPV(&decoded, IDPF_RX_PTYPE_OUTER_IPV6);
>> +	ipv4 = parsed.outer_ip == LIBIE_RX_PTYPE_OUTER_IPV4;
>> +	ipv6 = parsed.outer_ip == LIBIE_RX_PTYPE_OUTER_IPV6;
>>  
>>  	/* Check if there were any checksum errors */
>> -	if (unlikely(ipv4 && (csum_bits->ipe || csum_bits->eipe)))
>> +	if (unlikely(ipv4 && (csum_bits.ipe || csum_bits.eipe)))
>>  		goto checksum_fail;
>>  
>>  	/* Device could not do any checksum offload for certain extension
>>  	 * headers as indicated by setting IPV6EXADD bit
>>  	 */
>> -	if (unlikely(ipv6 && csum_bits->ipv6exadd))
>> +	if (unlikely(ipv6 && csum_bits.ipv6exadd))
>>  		return;
>>  
>>  	/* check for L4 errors and handle packets that were not able to be
>>  	 * checksummed due to arrival speed
>>  	 */
>> -	if (unlikely(csum_bits->l4e))
>> +	if (unlikely(csum_bits.l4e))
>>  		goto checksum_fail;
>>  
>> -	if (unlikely(csum_bits->nat && csum_bits->eudpe))
>> +	if (unlikely(csum_bits.nat && csum_bits.eudpe))
>>  		goto checksum_fail;
>>  
>>  	/* Handle packets that were not able to be checksummed due to arrival
>>  	 * speed, in this case the stack can compute the csum.
>>  	 */
>> -	if (unlikely(csum_bits->pprs))
>> +	if (unlikely(csum_bits.pprs))
>>  		return;
>>  
>>  	/* If there is an outer header present that might contain a checksum
>>  	 * we need to bump the checksum level by 1 to reflect the fact that
>>  	 * we are indicating we validated the inner checksum.
>>  	 */
>> -	if (decoded.tunnel_type >= IDPF_RX_PTYPE_TUNNEL_IP_GRENAT)
>> +	if (parsed.tunnel_type >= LIBIE_RX_PTYPE_TUNNEL_IP_GRENAT)
>>  		skb->csum_level = 1;
>>  
>> -	/* Only report checksum unnecessary for ICMP, TCP, UDP, or SCTP */
>> -	switch (decoded.inner_prot) {
>> -	case IDPF_RX_PTYPE_INNER_PROT_ICMP:
>> -	case IDPF_RX_PTYPE_INNER_PROT_TCP:
>> -	case IDPF_RX_PTYPE_INNER_PROT_UDP:
>> -	case IDPF_RX_PTYPE_INNER_PROT_SCTP:
>> -		skb->ip_summed = CHECKSUM_UNNECESSARY;
>> -		return;
>> -	default:
>> -		return;
>> -	}
>> +	skb->ip_summed = CHECKSUM_UNNECESSARY;
>> +	return;
> 
> Is it intentional to change from CHECKSUM_NONE to CHECKSUM_UNNECESSARY
> in the default case?

The basic logic wasn't changed. libie_has_rx_checksum() checks if the
protocol can be checksummed by HW at the beginning of the function
instead of the end (why calculate and check all this if the proto is not
supported?).

> 
> I suppose so, as idpf_rx_csum (the splitq equivalent) does the same
> (bar CHECKSUM_COMPLETE depending on descriptor bit).

Thanks,
Olek