[PATCH net v3 0/1] Prevent DSA tags from breaking COE

[PATCH net v3 0/1] Prevent DSA tags from breaking COE

[Romain Gantois]

Hello everyone,

This is the third version of my proposed fix for the stmmac checksum
offloading issue that has recently been reported.

significant changes in v3:
- Use __vlan_get_protocol to make sure that 8021Q-encapsulated
  traffic is checked correctly.

significant changes in v2:
- Replaced the stmmac_link_up-based fix with an ethertype check in the TX
  and RX hotpaths.

The Checksum Offloading Engine of some stmmac cores (e.g. DWMAC1000)
computes an incorrect checksum when presented with DSA-tagged packets. This
causes all TCP/UDP transfers to break when the stmmac device is connected
to the CPU port of a DSA switch.

I ran some tests using different tagging protocols with DSA_LOOP, and all
of the protocols that set a custom ethertype field in the MAC header caused
the checksum offload engine to ignore the tagged packets. On TX, this
caused packets to egress with incorrect checksums. On RX, these packets
were similarly ignored by the COE, yet the stmmac driver set
CHECKSUM_UNNECESSARY, wrongly assuming that their checksums had been
verified in hardware.

Version 2 of this patch series fixes this issue by checking ethertype
fields in both the TX and RX hotpaths of the stmmac driver. On TX, if a
non-IP ethertype is detected, the packet is checksummed in software.  On
RX, the same condition causes stmmac to avoid setting CHECKSUM_UNNECESSARY.

To measure the performance degradation to the TX/RX hotpaths, I did some
iperf3 runs with 512-byte unfragmented UDP packets.

measured degradation on TX: -466 pps (-0.2%) on RX: -338 pps (-1.2%)
original performances on TX: 22kpps on RX: 27kpps

The performance hit on the RX path can be partly explained by the fact that
the stmmac driver doesn't set CHECKSUM_UNNECESSARY anymore.

The TX performance degradation observed in v2 seems to have improved.
It's not entirely clear to me why that is.

Best Regards,

Romain

Romain Gantois (1):
  net: stmmac: Prevent DSA tags from breaking COE

 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 23 ++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

-- 
2.43.0

[PATCH net v3 1/1] net: stmmac: Prevent DSA tags from breaking COE

[Romain Gantois]

Some DSA tagging protocols change the EtherType field in the MAC header
e.g.  DSA_TAG_PROTO_(DSA/EDSA/BRCM/MTK/RTL4C_A/SJA1105). On TX these tagged
frames are ignored by the checksum offload engine and IP header checker of
some stmmac cores.

On RX, the stmmac driver wrongly assumes that checksums have been computed
for these tagged packets, and sets CHECKSUM_UNNECESSARY.

Add an additional check in the stmmac TX and RX hotpaths so that COE is
deactivated for packets with ethertypes that will not trigger the COE and
IP header checks.

Fixes: 6b2c6e4a938f ("net: stmmac: propagate feature flags to vlan")
Cc: stable@vger.kernel.org
Reported-by: Richard Tresidder <rtresidd@electromag.com.au>
Link: https://lore.kernel.org/netdev/e5c6c75f-2dfa-4e50-a1fb-6bf4cdb617c2@electromag.com.au/
Reported-by: Romain Gantois <romain.gantois@bootlin.com>
Link: https://lore.kernel.org/netdev/c57283ed-6b9b-b0e6-ee12-5655c1c54495@bootlin.com/
Signed-off-by: Romain Gantois <romain.gantois@bootlin.com>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 23 ++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index a9b6b383e863..6797c944a2ac 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -4371,6 +4371,19 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 	return NETDEV_TX_OK;
 }
 
+/* Check if ethertype will trigger IP
+ * header checks/COE in hardware
+ */
+static inline bool stmmac_has_ip_ethertype(struct sk_buff *skb)
+{
+	int depth = 0;
+	__be16 proto;
+
+	proto = __vlan_get_protocol(skb, eth_header_parse_protocol(skb), &depth);
+
+	return depth <= ETH_HLEN && (proto == htons(ETH_P_IP) || proto == htons(ETH_P_IPV6));
+}
+
 /**
  *  stmmac_xmit - Tx entry point of the driver
  *  @skb : the socket buffer
@@ -4435,9 +4448,13 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 	/* DWMAC IPs can be synthesized to support tx coe only for a few tx
 	 * queues. In that case, checksum offloading for those queues that don't
 	 * support tx coe needs to fallback to software checksum calculation.
+	 *
+	 * Packets that won't trigger the COE e.g. most DSA-tagged packets will
+	 * also have to be checksummed in software.
 	 */
 	if (csum_insertion &&
-	    priv->plat->tx_queues_cfg[queue].coe_unsupported) {
+	    (priv->plat->tx_queues_cfg[queue].coe_unsupported ||
+	    !stmmac_has_ip_ethertype(skb))) {
 		if (unlikely(skb_checksum_help(skb)))
 			goto dma_map_err;
 		csum_insertion = !csum_insertion;
@@ -4997,7 +5014,7 @@ static void stmmac_dispatch_skb_zc(struct stmmac_priv *priv, u32 queue,
 	stmmac_rx_vlan(priv->dev, skb);
 	skb->protocol = eth_type_trans(skb, priv->dev);
 
-	if (unlikely(!coe))
+	if (unlikely(!coe) || !stmmac_has_ip_ethertype(skb))
 		skb_checksum_none_assert(skb);
 	else
 		skb->ip_summed = CHECKSUM_UNNECESSARY;
@@ -5513,7 +5530,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 		stmmac_rx_vlan(priv->dev, skb);
 		skb->protocol = eth_type_trans(skb, priv->dev);
 
-		if (unlikely(!coe))
+		if (unlikely(!coe) || !stmmac_has_ip_ethertype(skb))
 			skb_checksum_none_assert(skb);
 		else
 			skb->ip_summed = CHECKSUM_UNNECESSARY;
-- 
2.43.0

Re: [PATCH net v3 1/1] net: stmmac: Prevent DSA tags from breaking COE

[Vladimir Oltean]

On Mon, Jan 08, 2024 at 12:17:45PM +0100, Romain Gantois wrote:
> Some DSA tagging protocols change the EtherType field in the MAC header
> e.g.  DSA_TAG_PROTO_(DSA/EDSA/BRCM/MTK/RTL4C_A/SJA1105). On TX these tagged
> frames are ignored by the checksum offload engine and IP header checker of
> some stmmac cores.
> 
> On RX, the stmmac driver wrongly assumes that checksums have been computed
> for these tagged packets, and sets CHECKSUM_UNNECESSARY.
> 
> Add an additional check in the stmmac TX and RX hotpaths so that COE is
> deactivated for packets with ethertypes that will not trigger the COE and
> IP header checks.
> 
> Fixes: 6b2c6e4a938f ("net: stmmac: propagate feature flags to vlan")
> Cc: stable@vger.kernel.org
> Reported-by: Richard Tresidder <rtresidd@electromag.com.au>
> Link: https://lore.kernel.org/netdev/e5c6c75f-2dfa-4e50-a1fb-6bf4cdb617c2@electromag.com.au/
> Reported-by: Romain Gantois <romain.gantois@bootlin.com>
> Link: https://lore.kernel.org/netdev/c57283ed-6b9b-b0e6-ee12-5655c1c54495@bootlin.com/
> Signed-off-by: Romain Gantois <romain.gantois@bootlin.com>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

>  .../net/ethernet/stmicro/stmmac/stmmac_main.c | 23 ++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index a9b6b383e863..6797c944a2ac 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4371,6 +4371,19 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>  	return NETDEV_TX_OK;
>  }
>  
> +/* Check if ethertype will trigger IP
> + * header checks/COE in hardware
> + */

Nitpick: you could render this in kernel-doc format.
https://docs.kernel.org/doc-guide/kernel-doc.html

> +static inline bool stmmac_has_ip_ethertype(struct sk_buff *skb)

Nitpick: in netdev it is preferred not to use the "inline" keyword at
all in C files, only "static inline" in headers, and to let the compiler
decide by itself when it is appropriate to inline the code (which it
does by itself even without the "inline" keyword). For a bit more
background why, you can view Documentation/process/4.Coding.rst, section
"Inline functions".

> +{
> +	int depth = 0;
> +	__be16 proto;
> +
> +	proto = __vlan_get_protocol(skb, eth_header_parse_protocol(skb), &depth);
> +
> +	return depth <= ETH_HLEN && (proto == htons(ETH_P_IP) || proto == htons(ETH_P_IPV6));
> +}

Re: [PATCH net v3 1/1] net: stmmac: Prevent DSA tags from breaking COE

[Miquel Raynal]

Hi Romain,

> > +/* Check if ethertype will trigger IP
> > + * header checks/COE in hardware
> > + */  
> 
> Nitpick: you could render this in kernel-doc format.
> https://docs.kernel.org/doc-guide/kernel-doc.html
> 
> > +static inline bool stmmac_has_ip_ethertype(struct sk_buff *skb)  
> 
> Nitpick: in netdev it is preferred not to use the "inline" keyword at
> all in C files, only "static inline" in headers, and to let the compiler
> decide by itself when it is appropriate to inline the code (which it
> does by itself even without the "inline" keyword). For a bit more
> background why, you can view Documentation/process/4.Coding.rst, section
> "Inline functions".
> 
> > +{
> > +	int depth = 0;
> > +	__be16 proto;
> > +
> > +	proto = __vlan_get_protocol(skb, eth_header_parse_protocol(skb), &depth);
> > +
> > +	return depth <= ETH_HLEN && (proto == htons(ETH_P_IP) || proto == htons(ETH_P_IPV6));

I also want to nitpick a bit :) If you are to send a v4, maybe you can
enclose the first condition within parenthesis to further clarify the
return logic.

Cheers,
Miquèl

Re: [PATCH net v3 1/1] net: stmmac: Prevent DSA tags from breaking C

[Romain Gantois]

On Mon, 8 Jan 2024, Vladimir Oltean wrote:

...

> Nitpick: you could render this in kernel-doc format.
> https://docs.kernel.org/doc-guide/kernel-doc.html
> 
> > +static inline bool stmmac_has_ip_ethertype(struct sk_buff *skb)
> 
> Nitpick: in netdev it is preferred not to use the "inline" keyword at
> all in C files, only "static inline" in headers, and to let the compiler
> decide by itself when it is appropriate to inline the code (which it
> does by itself even without the "inline" keyword). For a bit more
> background why, you can view Documentation/process/4.Coding.rst, section
> "Inline functions".

I see, the kernel docs were indeed enlightening on this point. As a side note, 
I've just benchmarked both the "with-inline" and "without-inline" versions. 
First of all, objdump seems to confirm that GCC does indeed follow this pragma 
in this particular case. Also, RX perfs are better with stmmac_has_ip_ethertype 
inlined, but TX perfs are actually consistently worse with this function 
inlined, which could very well be caused by cache effects.

In any case, I think it is better to remove the "inline" pragma as you said. 
I'll do that in v4.

Best Regards,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net v3 1/1] net: stmmac: Prevent DSA tags from breaking C

[Vladimir Oltean]

On Mon, Jan 08, 2024 at 03:23:38PM +0100, Romain Gantois wrote:
> I see, the kernel docs were indeed enlightening on this point. As a side note, 
> I've just benchmarked both the "with-inline" and "without-inline" versions. 
> First of all, objdump seems to confirm that GCC does indeed follow this pragma 
> in this particular case. Also, RX perfs are better with stmmac_has_ip_ethertype 
> inlined, but TX perfs are actually consistently worse with this function 
> inlined, which could very well be caused by cache effects.
> 
> In any case, I think it is better to remove the "inline" pragma as you said. 
> I'll do that in v4.

Are you doing any code instrumentation, or just measuring the results
and deducing what might cause them?

It might be worth looking at the perf events and seeing what function
consumes the most amount of time.

CPU_CORE=0
perf record -e cycles -C $CPU_CORE sleep 10 && perf report
perf record -e cache-misses -C $CPU_CORE sleep 10 && perf report

Re: [PATCH net v3 1/1] net: stmmac: Prevent DSA tags from breaking C

[Romain Gantois]

On Mon, 8 Jan 2024, Vladimir Oltean wrote:

> On Mon, Jan 08, 2024 at 03:23:38PM +0100, Romain Gantois wrote:
> > I see, the kernel docs were indeed enlightening on this point. As a side note, 
> > I've just benchmarked both the "with-inline" and "without-inline" versions. 
> > First of all, objdump seems to confirm that GCC does indeed follow this pragma 
> > in this particular case. Also, RX perfs are better with stmmac_has_ip_ethertype 
> > inlined, but TX perfs are actually consistently worse with this function 
> > inlined, which could very well be caused by cache effects.
> > 
> > In any case, I think it is better to remove the "inline" pragma as you said. 
> > I'll do that in v4.
> 
> Are you doing any code instrumentation, or just measuring the results
> and deducing what might cause them?
> 
> It might be worth looking at the perf events and seeing what function
> consumes the most amount of time.
> 
> CPU_CORE=0
> perf record -e cycles -C $CPU_CORE sleep 10 && perf report
> perf record -e cache-misses -C $CPU_CORE sleep 10 && perf report
> 

Unfortunately my hardware doesn't support these performance metrics, but I did 
manage to do some instrumentation with the ftrace profiler:

Same test conditions as before, 10 second iperf3 runs with unfragmented UDP 
packets.

no inline TX
  average time per call for stmmac_xmit(): 85us
  average time per call for stmmac_has_ip_ethertype(): 2us

no inline RX
  average time per call for stmmac_napi_poll_rx(): 8142us
  average time per call for stmmac_has_ip_ethertype(): 2us

inline TX:
  average time per call for stmmac_xmit(): 85us

inline RX:
  average time per call for stmmac_napi_poll_rx(): 8410us

It seems like this time, RX performed slightly worse with the function inline. 
To be honest, I'm starting to doubt the reproducibility of these tests. In any 
case it seems better to just remove the "inline" and let gcc do the optimizing.

Best Regards,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com