Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6693bcdb-b5dc-2f5b-41fa-9a9bba909dc7@intel.com>
Date:   Mon, 22 May 2023 17:51:37 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH RESEND bpf-next 03/15] ice: make RX checksum checking code
 more reusable
Content-Language: en-US
To:     Larysa Zaremba <larysa.zaremba@intel.com>
CC:     <bpf@vger.kernel.org>, Stanislav Fomichev <sdf@google.com>,
        "Alexei Starovoitov" <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        "Andrii Nakryiko" <andrii@kernel.org>,
        Jakub Kicinski <kuba@kernel.org>,
        "Martin KaFai Lau" <martin.lau@linux.dev>,
        Song Liu <song@kernel.org>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>, Jiri Olsa <jolsa@kernel.org>,
        Jesse Brandeburg <jesse.brandeburg@intel.com>,
        Tony Nguyen <anthony.l.nguyen@intel.com>,
        Anatoly Burakov <anatoly.burakov@intel.com>,
        Jesper Dangaard Brouer <brouer@redhat.com>,
        Alexander Lobakin <alexandr.lobakin@intel.com>,
        "Magnus Karlsson" <magnus.karlsson@gmail.com>,
        Maryam Tahhan <mtahhan@redhat.com>,
        <xdp-hints@xdp-project.net>, <netdev@vger.kernel.org>,
        <intel-wired-lan@lists.osuosl.org>, <linux-kernel@vger.kernel.org>
References: <20230512152607.992209-1-larysa.zaremba@intel.com>
 <20230512152607.992209-4-larysa.zaremba@intel.com>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <20230512152607.992209-4-larysa.zaremba@intel.com>
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?NWpyNEQ2QVpGSmxhWUxpSlM5SzlTallRQUdKem1USHVqZHF3MGV0OW1CQzBt?=
 =?utf-8?B?NnFnUVdjWnB4aERtdFVoWFZCTHVodDZKOGlhN1NLVDByUVB1Y0VTQzJpOXRY?=
 =?utf-8?B?MTJubUdIMDltNDA0QnY2THNrUGhQYndrd1JtOEszNE0yM0ZyYzlHWnBlU3dF?=
 =?utf-8?B?OWQ4RWdYUVhuejNEQXR6ZlRhNFRWcmpRcE11YVRWeXFUWmhjQ0xEaUJIRGpj?=
 =?utf-8?B?S2l2d0JoSzBSai9vMXVlei8wckFPVGtJRXVnTkVmbVViZGVYY0JQVE5BL2dV?=
 =?utf-8?B?c2V5bWh6dHR3czUvWThNMktsbkozU0N6SHpONTdaTmpoaUdsaDh2WDBWVFp1?=
 =?utf-8?B?bDNZbDV4NlpQaHQvTll5eFhOSFFkRTdMKzlISFRXbEtwbS9nZkpJYkdlSkZw?=
 =?utf-8?B?RTI0QjZaK00rb0t5OCtPVlFCQW9MR3kxbmVaMVlMTjV6TVdNSStQY1JPQ1Ir?=
 =?utf-8?B?SWY3SnhuYXhSdTczWUN5MHdNeng2NlhnK1RNLzhuZ0F0ZTdicUtGN0NHMjZR?=
 =?utf-8?B?VkZNN09oQzZzdWdmcjZaTFNBS0d1VmJPWHoxalh2YUZBVEVOaE1pSHRmTENz?=
 =?utf-8?B?TnJvT2xEUElYVlRTbnViTHJHaG0zNWZBYm5MSlArRjBPKzZjN2JuVC9Way93?=
 =?utf-8?B?L2s2Vkd0V1JYQlNzSXd6YWd6QkFuK3NVOXIrbWFFcnVSb0lHc1FLL2lQVjZY?=
 =?utf-8?B?T0h2bSs0MnErMGoyaExmOWtXRkZXM0lwVTFJSWduKzBtR253c3ZaNnJJTzVz?=
 =?utf-8?B?RTJZOXFDWHRNTVZCdWxJcjdTemdrVnJleXg4MjJLREYreXN6ZXlDNXVsWGZP?=
 =?utf-8?B?WEZ5TGlwKzRTdkhWYXZnOUhxczV2SldHU3F6cGtoYmcxVktjUDh6dFZHb09Q?=
 =?utf-8?B?YisvMFZURGVocE5WUDh5VzVIcm5ESGx2R0d0ZXJ0eS9rMVUwR2YvMUI4aXJH?=
 =?utf-8?B?S1pvU0tnOUMxQTBKQUUzZzhtMElDa3UrdE90bDFhc3h6VStkWnFGLzg3UUZZ?=
 =?utf-8?B?OTVla3Y3ZjQ0NFJ6TEp4aDVNTXhMLzJKUUVRVS9URm9ETWxaNnZXVWZrdmUv?=
 =?utf-8?B?ZEpsR3RWckxqc3VUdjIzQXU1WlpHK3Nlb0NzbW4xS2ZKUUF3U3ZSMTV2MTF1?=
 =?utf-8?B?QmY1cWljTXBjSWV4ZlJHb1BuSWNETHJPejBMTDEzRWhpSUhOZXB0K0JROFh5?=
 =?utf-8?B?bXEycmFXWW14Nlo5aGFVSGJhMUpXNmU5WG5TYUpXVjBnbXo2SHI3TFVibExt?=
 =?utf-8?B?Zy91Zk9XaFk1Y1VjT0RmejN1Q0pLU291MHVTa1FQek1ad0lHdjlzTGtMa0po?=
 =?utf-8?B?U1NBaTJoV2tQdkd4NFpEY3NiZnlNNE9yNXBZcTloaXlwbTM4NHpiYXJFejRP?=
 =?utf-8?B?NGtKWmR1ZlUvVFVpMS9uRGd3VlRERFdIR2pRNmp3dW82VVUvVWZuanZrblFY?=
 =?utf-8?B?aTZscVRFN1RUTXMvdE9uck5VYlNsOWJOeEM0WldmZUlrRmdHTXVMTkNkSkx3?=
 =?utf-8?B?eDBxa0QvZ0prMXVlK0xha2MrRUJVaDEwSDRLeFVYelNzUDJLVHRicjFwdlpR?=
 =?utf-8?B?SHFZZVlWREpKMlRuWEFCaEtJYWhlbWdudkM2cG5RbFJRbEVlYVE0eWFTK056?=
 =?utf-8?B?d0Mwc2kzVjl2dHRGd0RyeGl1OUMvL3lGUUNZOTE1Tk4zUDhWMnM5UHJjNHZL?=
 =?utf-8?B?TFdOVFZEdWZBdld5bTBweDk1dWE1eXF3RllxQTlLTGo3RU5XSjRZbTBuekh2?=
 =?utf-8?B?V2hFRGFHczIwdlJNaVZoTWlUdHFuRC9YQXJBenBqdFIwVFJBOVBpZmtPUDAy?=
 =?utf-8?B?T2lYRG1sN3haMkl3bzZPczdPK2hVL0NNWDA0SnZZVFRqcmVnQTcwTUxmTGFB?=
 =?utf-8?B?ZFNZcU43SERyN0lpdHhUYWxjU3gxa0YzeGlUSGxLSno0L1pvODl2MUZyblkv?=
 =?utf-8?B?MG84eWdYUTJpZHB5N2hEM0F0T085MmNiNGJEeDIzUkNkSXluMU1LdVNtTFJm?=
 =?utf-8?B?NXMrNjRFNnh5dVhjK0IwdWQ0ZVJKZTk4OEMzUW05d1ovWmdLbDRaN0hwMXZx?=
 =?utf-8?B?QjlpTHVCU0xadFlMVDZxbW1qNS9tanR3UXVJR1lRRUlWS1pqdEFoK0ZRcEdu?=
 =?utf-8?B?d1lOL3VweTBIbjBpTFlBWnNZU0dHdWtjVHNJR1FTZ3VUUW1rUEt0UGU0MFdZ?=
 =?utf-8?B?alE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 7037259a-0b0a-4740-9c02-08db5adca4a1
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 May 2023 15:53:10.6899
 (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: VemEfZi/8UYUMezq/INRSZ2ZgOr5IAjPgqrf0IIQLrI67Fjn6+7AVWEcqXzjEklUHAzz8P+OngpIbFMD7k7ZzZgVLNLpkssLtVScR8QWne8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5893
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_PASS,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

From: Larysa Zaremba <larysa.zaremba@intel.com>
Date: Fri, 12 May 2023 17:25:55 +0200

> Previously, we only needed RX checksum flags in skb path,
> hence all related code was written with skb in mind.
> But with the addition of XDP hints via kfuncs to the ice driver,
> the same logic will be needed in .xmo_() callbacks.
> 
> Put generic process of determining checksum status into
> a separate function.
> 
> Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 71 ++++++++++++-------
>  1 file changed, 46 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
> index 1aab79dc8915..6a4fd3f3fc0a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
> @@ -104,17 +104,17 @@ ice_rx_hash_to_skb(struct ice_rx_ring *rx_ring,
>  }
>  
>  /**
> - * ice_rx_csum - Indicate in skb if checksum is good
> - * @ring: the ring we care about
> - * @skb: skb currently being received and modified
> + * ice_rx_csum_checked - Indicates, whether hardware has checked the checksum

%CHECKSUM_UNNECESSARY means that the csum is correct / frame is not
damaged. So "checked" is not enough I'd say, it's "verified" at least.
OTOH that's too long already, I'd go with classic "csum_ok" :D

>   * @rx_desc: the receive descriptor
>   * @ptype: the packet type decoded by hardware
> + * @csum_lvl_dst: address to put checksum level into
> + * @ring: ring for error stats, can be NULL
>   *
> - * skb->protocol must be set before this function is called
> + * Returns true, if hardware has checked the checksum.
>   */
> -static void
> -ice_rx_csum(struct ice_rx_ring *ring, struct sk_buff *skb,
> -	    union ice_32b_rx_flex_desc *rx_desc, u16 ptype)
> +static bool
> +ice_rx_csum_checked(union ice_32b_rx_flex_desc *rx_desc, u16 ptype,

(also const, but I guess you'll do that either way after the previous
 mails)

> +		    u8 *csum_lvl_dst, struct ice_rx_ring *ring)
>  {
>  	struct ice_rx_ptype_decoded decoded;
>  	u16 rx_status0, rx_status1;

[...]

> +/**
> + * ice_rx_csum_into_skb - Indicate in skb if checksum is good
> + * @ring: the ring we care about
> + * @skb: skb currently being received and modified
> + * @rx_desc: the receive descriptor
> + * @ptype: the packet type decoded by hardware
> + */
> +static void
> +ice_rx_csum_into_skb(struct ice_rx_ring *ring, struct sk_buff *skb,
> +		     union ice_32b_rx_flex_desc *rx_desc, u16 ptype)
> +{
> +	u8 csum_level = 0;

I'm not a fan of variables shorter than u32 on the stack. And since it
gets passed by a reference, I'm not sure the compiler will inline it =\

> +
> +	/* Start with CHECKSUM_NONE and by default csum_level = 0 */
> +	skb->ip_summed = CHECKSUM_NONE;
> +	skb_checksum_none_assert(skb);

Can we also remove this? Neither of these makes sense. ::ip_summed is
always zeroed after the memset() in __build_skb_around() (somewhere
there), while the assertion checks for `skb->ip_summed ==
CHECKSUM_NONE`, i.e. it's *always* true here (set and check :D). It's
some ancient pathetic rituals copied over and over again from e100
centuries or so...

...and BTW the comment is misleading, because the code doesn't zero
::csum_level as they claim :D

> +
> +	/* check if Rx checksum is enabled */
> +	if (!(ring->netdev->features & NETIF_F_RXCSUM))
> +		return;
> +
> +	if (!ice_rx_csum_checked(rx_desc, ptype, &csum_level, ring))
> +		return;
> +
> +	skb->ip_summed = CHECKSUM_UNNECESSARY;
> +	skb->csum_level = csum_level;

Since csum_level is useless when ip_summed is set to NONE, what do you
think about making the function return -1, 0, or 1 without writing
anything by reference?

	int csum_level;

	csum_level = ice_rx_csum_ok(rx_desc, ptype, ring);
	if (csum_level < 0)
		return;

	skb->ip_summed = CHECKSUM_UNNECESSARY;
	skb->csum_level = csum_level;

I'm not saying it's better (might be a bit at codegen), just proposing.

>  }
>  
>  /**
> @@ -232,7 +253,7 @@ ice_process_skb_fields(struct ice_rx_ring *rx_ring,
>  	/* modifies the skb - consumes the enet header */
>  	skb->protocol = eth_type_trans(skb, rx_ring->netdev);
>  
> -	ice_rx_csum(rx_ring, skb, rx_desc, ptype);
> +	ice_rx_csum_into_skb(rx_ring, skb, rx_desc, ptype);
>  
>  	if (rx_ring->ptp_rx)
>  		ice_ptp_rx_hwts_to_skb(rx_ring, rx_desc, skb);

Thanks,
Olek