[PATCH] dpaa2-eth: treat skb with exact headroom as scatter/gather frames

[PATCH] dpaa2-eth: treat skb with exact headroom as scatter/gather frames

[Mathew McBride]

In commit f422abe3f23d ("dpaa2-eth: increase the needed headroom to
account for alignment"), the required skb headroom of dpaa2-eth was
increased to exactly 128 bytes. The headroom increase was to ensure
frames on the Tx path were always aligned to 64 bytes.

This caused a regression when vhost-net was used to accelerate virtual
machine frames between a KVM guest and a dpaa2-eth interface over a bridge.
While the skb passed to the driver had the required headroom (128 bytes),
the skb->head pointer did not match the alignment expected by the driver
(aligned_start => skb->head in dpaa2_eth_build_single_fd).

Treating outbound skb's where skb_headroom() == net_dev->needed_headroom
the same as skb's with inadequate headroom resolves this issue.

Signed-off-by: Mathew McBride <matt@traverse.com.au>
Fixes: f422abe3f23d ("dpaa2-eth: increase the needed headroom to account for alignment")
Closes: https://lore.kernel.org/netdev/70f0dcd9-1906-4d13-82df-7bbbbe7194c6@app.fastmail.com/T/#u
---
A while ago, changes were made to the dpaa2-eth driver to workaround
an issue when TX frames were not aligned to 64 bytes.

As part of this change, the required skb headroom in dpaa2-eth
was increased to 128 bytes.

When frames originating from a virtual machine over vhost-net
were forwarded to the dpaa2-eth driver for transmission,
the vhost frames were being dropped as they failed an alignment check.

The skb's originating from vhost-net had exactly the required headroom
(128 bytes).

I have tested a fix to the issue which treats frames with the "exact"
headroom the same as frames with inadequate headroom. These are
transmitted using the scatter/gather (S/G) process.

Network drivers are not my area of expertise so I cannot be 100%
confident this is the correct solution, however, I've done extensive
reliability testing on this fix to confirm it resolves the regression
involving vhost-net without any other side effects.

What I can't answer (yet) is if there are performance or other ramifcations
from having all VM-originating frames handled as S/G.

As far as I am aware, the virtual machine / vhost-net workload is the
only workload that generates skb's that require the S/G handling in
vhost-net. I have not seen any variants of this issue without vhost-net.

My original analysis of the problem can be found in the message below.
The diagnosis of the issue is still correct at the time of writing
(circa 6.18-rc1)

https://lore.kernel.org/netdev/70f0dcd9-1906-4d13-82df-7bbbbe7194c6@app.fastmail.com/T/#u
---
 drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
index c96d1d6ba8fe9..4eaf7cbec558d 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
@@ -1439,7 +1439,7 @@ static netdev_tx_t __dpaa2_eth_tx(struct sk_buff *skb,
 		percpu_extras->tx_sg_frames++;
 		percpu_extras->tx_sg_bytes += skb->len;
 		fd_len = dpaa2_fd_get_len(fd);
-	} else if (skb_headroom(skb) < needed_headroom) {
+	} else if (skb_headroom(skb) <= needed_headroom) {
 		err = dpaa2_eth_build_sg_fd_single_buf(priv, skb, fd, &swa);
 		percpu_extras->tx_sg_frames++;
 		percpu_extras->tx_sg_bytes += skb->len;

---
base-commit: 9b332cece987ee1790b2ed4c989e28162fa47860
change-id: 20251015-fix-dpaa2-vhost-net-3477be4e3ac9

Best regards,
-- 
Mathew McBride <matt@traverse.com.au>

Re: [PATCH] dpaa2-eth: treat skb with exact headroom as scatter/gather frames

[Ioana Ciornei]

On Wed, Oct 15, 2025 at 03:01:24PM +1100, Mathew McBride wrote:
> In commit f422abe3f23d ("dpaa2-eth: increase the needed headroom to
> account for alignment"), the required skb headroom of dpaa2-eth was
> increased to exactly 128 bytes. The headroom increase was to ensure
> frames on the Tx path were always aligned to 64 bytes.
> 
> This caused a regression when vhost-net was used to accelerate virtual
> machine frames between a KVM guest and a dpaa2-eth interface over a bridge.
> While the skb passed to the driver had the required headroom (128 bytes),
> the skb->head pointer did not match the alignment expected by the driver
> (aligned_start => skb->head in dpaa2_eth_build_single_fd).
> 
> Treating outbound skb's where skb_headroom() == net_dev->needed_headroom
> the same as skb's with inadequate headroom resolves this issue.
> 
> Signed-off-by: Mathew McBride <matt@traverse.com.au>
> Fixes: f422abe3f23d ("dpaa2-eth: increase the needed headroom to account for alignment")
> Closes: https://lore.kernel.org/netdev/70f0dcd9-1906-4d13-82df-7bbbbe7194c6@app.fastmail.com/T/#u
> ---
> A while ago, changes were made to the dpaa2-eth driver to workaround
> an issue when TX frames were not aligned to 64 bytes.
> 
> As part of this change, the required skb headroom in dpaa2-eth
> was increased to 128 bytes.
> 
> When frames originating from a virtual machine over vhost-net
> were forwarded to the dpaa2-eth driver for transmission,
> the vhost frames were being dropped as they failed an alignment check.
> 
> The skb's originating from vhost-net had exactly the required headroom
> (128 bytes).
> 
> I have tested a fix to the issue which treats frames with the "exact"
> headroom the same as frames with inadequate headroom. These are
> transmitted using the scatter/gather (S/G) process.
> 
> Network drivers are not my area of expertise so I cannot be 100%
> confident this is the correct solution, however, I've done extensive
> reliability testing on this fix to confirm it resolves the regression
> involving vhost-net without any other side effects.
> 
> What I can't answer (yet) is if there are performance or other ramifcations
> from having all VM-originating frames handled as S/G.
> 
> As far as I am aware, the virtual machine / vhost-net workload is the
> only workload that generates skb's that require the S/G handling in
> vhost-net. I have not seen any variants of this issue without vhost-net.
> 
> My original analysis of the problem can be found in the message below.
> The diagnosis of the issue is still correct at the time of writing
> (circa 6.18-rc1)
> 
> https://lore.kernel.org/netdev/70f0dcd9-1906-4d13-82df-7bbbbe7194c6@app.fastmail.com/T/#u

Hi Mathew,

First of all, sorry for missing your initial message.

While I was trying to understand how the 'aligned_start >= skb->head'
check ends up failing while you have the necessary 128bytes of headroom,
I think I discovered that this is not some kind of off-by-one issue.

The below snippet is from your original message.
	fsl_dpaa2_eth dpni.9: dpaa2_eth_build_single_fd alignment issue, aligned_start=ffff008002e09140 skb->head=ffff008002e09180
	fsl_dpaa2_eth dpni.9: dpaa2_eth_build_single_fd data=ffff008002e09200
	fsl_dpaa2_eth dpni.9: dpaa2_eth_build_single_fd is cloned=0
	dpaa2_eth_build_single_fdskb len=150 headroom=128 headlen=150 tailroom=42

If my understanding is correct skb->data=ffff008002e09200.
Following the dpaa2_eth_build_single_fd() logic, this means that
	buffer_start = 0xffff008002e09200 - 0x80
	buffer_start = 0xFFFF008002E09180

Now buffer_start is already pointing to the start of the skb's memory
and I don't think the extra 'buffer_start - DPAA2_ETH_TX_BUF_ALIGN'
adjustment is correct.

What I think happened is that I did not take into consideration that by
adding the DPAA2_ETH_TX_BUF_ALIGN inside the dpaa2_eth_needed_headroom()
function I also need to remove it from the manual adjustment.

Could you please try with the following diff and let me know how it does
in your setup?

--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
@@ -1077,7 +1077,7 @@ static int dpaa2_eth_build_single_fd(struct dpaa2_eth_priv *priv,
        dma_addr_t addr;

        buffer_start = skb->data - dpaa2_eth_needed_headroom(skb);
-       aligned_start = PTR_ALIGN(buffer_start - DPAA2_ETH_TX_BUF_ALIGN,
+       aligned_start = PTR_ALIGN(buffer_start,
                                  DPAA2_ETH_TX_BUF_ALIGN);
        if (aligned_start >= skb->head)
                buffer_start = aligned_start;

Ioana

Re: [PATCH] dpaa2-eth: treat skb with exact headroom as scatter/gather frames

[Mathew McBride]

On Wed, Oct 15, 2025, at 9:18 PM, Ioana Ciornei wrote:
> On Wed, Oct 15, 2025 at 03:01:24PM +1100, Mathew McBride wrote:
[snip]
> Hi Mathew,
> 
> First of all, sorry for missing your initial message.
> 
No problem. I had a revert patch in my tree and mostly forgot about the problem until a customer of ours reminded me recently. The S/G "solution" looked easy enough to try.

> While I was trying to understand how the 'aligned_start >= skb->head'
> check ends up failing while you have the necessary 128bytes of headroom,
> I think I discovered that this is not some kind of off-by-one issue.
> 
> The below snippet is from your original message.
> fsl_dpaa2_eth dpni.9: dpaa2_eth_build_single_fd alignment issue, aligned_start=ffff008002e09140 skb->head=ffff008002e09180
> fsl_dpaa2_eth dpni.9: dpaa2_eth_build_single_fd data=ffff008002e09200
> fsl_dpaa2_eth dpni.9: dpaa2_eth_build_single_fd is cloned=0
> dpaa2_eth_build_single_fdskb len=150 headroom=128 headlen=150 tailroom=42
> 
> If my understanding is correct skb->data=ffff008002e09200.
> Following the dpaa2_eth_build_single_fd() logic, this means that
> buffer_start = 0xffff008002e09200 - 0x80
> buffer_start = 0xFFFF008002E09180
> 
> Now buffer_start is already pointing to the start of the skb's memory
> and I don't think the extra 'buffer_start - DPAA2_ETH_TX_BUF_ALIGN'
> adjustment is correct.
> 
> What I think happened is that I did not take into consideration that by
> adding the DPAA2_ETH_TX_BUF_ALIGN inside the dpaa2_eth_needed_headroom()
> function I also need to remove it from the manual adjustment.
> 
> Could you please try with the following diff and let me know how it does
> in your setup?
> 

It looks good to me! I've tested across two different kernel series (6.6 and 6.18) with two different host userlands (Debian and OpenWrt). Both VM (vhost-net) and normal system traffic are OK.

Many thanks,
Matt

> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> @@ -1077,7 +1077,7 @@ static int dpaa2_eth_build_single_fd(struct dpaa2_eth_priv *priv,
>         dma_addr_t addr;
> 
>         buffer_start = skb->data - dpaa2_eth_needed_headroom(skb);
> -       aligned_start = PTR_ALIGN(buffer_start - DPAA2_ETH_TX_BUF_ALIGN,
> +       aligned_start = PTR_ALIGN(buffer_start,
>                                   DPAA2_ETH_TX_BUF_ALIGN);
>         if (aligned_start >= skb->head)
>                 buffer_start = aligned_start;
> 
> Ioana
> 

Re: [PATCH] dpaa2-eth: treat skb with exact headroom as scatter/gather frames

[Ioana Ciornei]

On Thu, Oct 16, 2025 at 08:33:27PM +1100, Mathew McBride wrote:
> 
> 
> On Wed, Oct 15, 2025, at 9:18 PM, Ioana Ciornei wrote:
> > On Wed, Oct 15, 2025 at 03:01:24PM +1100, Mathew McBride wrote:
> [snip]
> > Hi Mathew,
> > 
> > First of all, sorry for missing your initial message.
> > 
> No problem. I had a revert patch in my tree and mostly forgot about
> the problem until a customer of ours reminded me recently. The S/G
> "solution" looked easy enough to try.
> 
> > While I was trying to understand how the 'aligned_start >= skb->head'
> > check ends up failing while you have the necessary 128bytes of headroom,
> > I think I discovered that this is not some kind of off-by-one issue.
> > 
> > The below snippet is from your original message.
> > fsl_dpaa2_eth dpni.9: dpaa2_eth_build_single_fd alignment issue, aligned_start=ffff008002e09140 skb->head=ffff008002e09180
> > fsl_dpaa2_eth dpni.9: dpaa2_eth_build_single_fd data=ffff008002e09200
> > fsl_dpaa2_eth dpni.9: dpaa2_eth_build_single_fd is cloned=0
> > dpaa2_eth_build_single_fdskb len=150 headroom=128 headlen=150 tailroom=42
> > 
> > If my understanding is correct skb->data=ffff008002e09200.
> > Following the dpaa2_eth_build_single_fd() logic, this means that
> > buffer_start = 0xffff008002e09200 - 0x80
> > buffer_start = 0xFFFF008002E09180
> > 
> > Now buffer_start is already pointing to the start of the skb's memory
> > and I don't think the extra 'buffer_start - DPAA2_ETH_TX_BUF_ALIGN'
> > adjustment is correct.
> > 
> > What I think happened is that I did not take into consideration that by
> > adding the DPAA2_ETH_TX_BUF_ALIGN inside the dpaa2_eth_needed_headroom()
> > function I also need to remove it from the manual adjustment.
> > 
> > Could you please try with the following diff and let me know how it does
> > in your setup?
> > 
> 
> It looks good to me! I've tested across two different kernel series
> (6.6 and 6.18) with two different host userlands (Debian and OpenWrt).
> Both VM (vhost-net) and normal system traffic are OK.
> 

Great to hear that.

I will prepare a patch targetting the net tree. If you don't mind I will
add a formal Tested-by tag when I am sending out the patch.

Ioana