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

[Wei Fang]

Hi Claudiu,

> -----Original Message-----
> From: Wei Fang
> Sent: 2024年11月11日 17:26
> To: Claudiu Manoil <claudiu.manoil@xxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx;
> Vladimir Oltean <vladimir.oltean@xxxxxxx>; Clark Wang
> <xiaoning.wang@xxxxxxx>; andrew+netdev@xxxxxxx; davem@xxxxxxxxxxxxx;
> edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; Frank Li
> <frank.li@xxxxxxx>
> Subject: RE: [PATCH v2 net-next 2/5] net: enetc: add Tx checksum offload for
> i.MX95 ENETC
> 
> > > --- 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. ?
> 
> skb->protocol may be VLAN protocol, such as ETH_P_8021Q, ETH_P_8021AD.
> If so, it is impossible to determine whether it is an IPv4 or IPv6 frames through
> protocol.
> 
> > or
> > switch (skb->csum_offset) {
> > case offsetof(struct tcphdr, check):
> > [...]
> > case offsetof(struct udphdr, check):
> > [...]
> 
> This seems to be able to be used to determine whether it is a UDP or TCP
> frame.
> Thanks.
> 
> >
> > > +		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.
> 
> For small packets this might improve performance, but I'm not sure if it would
> be
> a significant improvement. :)
> 

I didn't see any visible improvements in performance after using csum_offset.
For example, when using pktgen to send 10,000,000 packets, the time taken is
almost the same regardless of whether they are large or small packets, and the
CPU idle ratio seen through the top command is also basically the same. Also,
the UDP performance tested by iperf3 is basically the same.

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