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

From: Claudiu Manoil
Date: Mon Nov 11 2024 - 02:27:00 EST


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;
> +}
> +