RE: [PATCH v2 net-next 2/5] net: enetc: add Tx checksum offload for i.MX95 ENETC

[Claudiu Manoil]

Hi Wei,

> -----Original Message-----
> From: Wei Fang <wei.fang@xxxxxxx>
> Sent: Monday, November 11, 2024 3:52 AM
[...]
> Subject: [PATCH v2 net-next 2/5] net: enetc: add Tx checksum offload for
> i.MX95 ENETC
> 
> In addition to supporting Rx checksum offload, i.MX95 ENETC also supports
> Tx checksum offload. The transmit checksum offload is implemented through
> the Tx BD. To support Tx checksum offload, software needs to fill some
> auxiliary information in Tx BD, such as IP version, IP header offset and
> size, whether L4 is UDP or TCP, etc.
> 
> Same as Rx checksum offload, Tx checksum offload capability isn't defined
> in register, so tx_csum bit is added to struct enetc_drvdata to indicate
> whether the device supports Tx checksum offload.
> 
> Signed-off-by: Wei Fang <wei.fang@xxxxxxx>
> ---
> v2: refine enetc_tx_csum_offload_check().
> ---
>  drivers/net/ethernet/freescale/enetc/enetc.c  | 53 ++++++++++++++++---
>  drivers/net/ethernet/freescale/enetc/enetc.h  |  2 +
>  .../net/ethernet/freescale/enetc/enetc_hw.h   | 14 +++--
>  .../freescale/enetc/enetc_pf_common.c         |  3 ++
>  4 files changed, 62 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c
> b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 3137b6ee62d3..502194317a96 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -143,6 +143,27 @@ static int enetc_ptp_parse(struct sk_buff *skb, u8
> *udp,
>  	return 0;
>  }
> 
> +static bool enetc_tx_csum_offload_check(struct sk_buff *skb)
> +{
> +	if (ip_hdr(skb)->version == 4)

I would avoid using ip_hdr(), or any form of touching packed data and try
extract this kind of info directly from the skb metadata instead, see also comment below.

i.e., why not:
if (skb->protocol == htons(ETH_P_IPV6)) ..  etc. ?
or
switch (skb->csum_offset) {
case offsetof(struct tcphdr, check):
[...]
case offsetof(struct udphdr, check):
[...]

> +		return ip_hdr(skb)->protocol == IPPROTO_TCP ||
> +		       ip_hdr(skb)->protocol == IPPROTO_UDP;
> +
> +	if (ip_hdr(skb)->version == 6)
> +		return ipv6_hdr(skb)->nexthdr == NEXTHDR_TCP ||
> +		       ipv6_hdr(skb)->nexthdr == NEXTHDR_UDP;
> +
> +	return false;
> +}
> +
> +static bool enetc_skb_is_tcp(struct sk_buff *skb)
> +{

There is a more efficient way of checking if L4 is TCP, without touching
packet data, i.e. through the 'csum_offset' skb field:
return skb->csum_offset == offsetof(struct tcphdr, check);

Pls. have a look at these optimizations, I would expect visible improvements
in throughput. Thanks.

> +	if (ip_hdr(skb)->version == 4)
> +		return ip_hdr(skb)->protocol == IPPROTO_TCP;
> +	else
> +		return ipv6_hdr(skb)->nexthdr == NEXTHDR_TCP;
> +}
> +