Re: [PATCH net-next 2/3] net: stmmac: Use pagepool instead of streaming buffers for Tx

On Mon, Aug 3, 2026 at 11:39=E2=80=AFAM Mina Almasry <[email protected]=
m> wrote:
>
> On Mon, Aug 3, 2026 at 12:08=E2=80=AFAM
> <[email protected]> wrote:
> >
> > From: Nazim Amirul <[email protected]>
> >
> > Use page pool buffers instead of streaming skb buffers in stmmac_xmit
> > when snps,pagepool-tx-buf-quirk is enabled. As page pool buffers are
> > recycled, the overhead of dma map/unmap for each transmit can be
> > avoided. There is a trade-off of copying from the skb into the page
> > pool buffer, but this helps where the copy is cheaper than dma
> > map/unmap (for example with IOMMU invalidate cost).
> >
> > Currently page pool buffers are only used for single-fragment transmit
> > in the non-TSO path.
> >
> > Signed-off-by: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.=
com>
>
> Whoa!
>
> I haven't looked at the patch deeply to be honest, but using the
> page-pool for tx is a bit interesting. There is no other code that
> uses it like that I think. I can't think of any blockers off the top
> of my head, but I think there are places where we keep a global list
> of created page_pools on the system/netdev and the code parses those
> lists may be assuming it's a normal page-pool, which is a pp used for
> rx and attached to the an rx queue.
>
> Just Ccing the maintainers of pp to know about this use case. I can't
> think of issues again, but I wonder if a more-expert has thoughts.
>

Ah, yes, gemini reminded me of the most critical bit. The page_pool
actually doesn't do concurrency checks on allocation because it
assumes allocations only-ever come from the napi, and can't race with
allocations coming from another napi. Does your patch account for that
somehow? Or can allocations in tx come from multiple cpus in parallel?

Full feedback:

```
  ### 1. Concurrency & Memory Corruption (The Lockless Fast-Path)

  page_pool allocations (page_pool_alloc_pages()) are lockless in the
fast path (pool->alloc cache). They inherently assume exactly one
allocator context (which natively maps to a single
  RX NAPI softirq).
  If this driver allows multiple CPUs to concurrently enter the TX
routine and allocate from the same page_pool, the lockless cache will
immediately corrupt. To use it in TX, they must
  guarantee:

  =E2=80=A2 They allocate 1 dedicated page_pool per TX ring.
  =E2=80=A2 The TX ring is strictly protected by a spinlock (e.g.
__netif_tx_lock) during all allocations, or they are utilizing strict
CPU-bound XPS.

  ### 2. Slow-Path Contention (ptr_ring Thrashing)

  Even if the locking is safe, performance will likely tank due to
NAPI mismatches.
  page_pool keeps its speed because pages freed by the NAPI context
are dropped directly back into the lockless alloc array. If a page is
freed by a remote CPU or an unbound context, it
  falls back to a spinlock-protected ptr_ring.
  In a TX flow, if CPU A injects the packet (ndo_start_xmit) but CPU B
handles the hardware TX-completion IRQ, CPU B will constantly push
pages into the locked ptr_ring, and CPU A will
  constantly pull from it. You lose all the lockless benefits of
page_pool and just burn cycles on cross-CPU spinlocks.

  ### 3. Memory Waste (Page Splitting)

  TX injections are usually small (e.g., bouncing a fragmented
header). page_pool allocates strictly in PAGE_SIZE chunks unless you
heavily orchestrate page_pool_alloc_frag(). Standard
  SLUB kmalloc() or skb_alloc() is almost always cheaper, more memory
efficient, and more cache-friendly for TX bounce-buffers than spinning
up dedicated page pools.

  TL;DR: Unless the TX allocations and the TX completions are strictly
pinned to the identical NAPI context/CPU, and they are allocating
massive buffers, throwing page_pool at the TX
  path usually results in lock contention and memory inefficiency.
They are almost certainly better off using standard kernel allocators,
or at minimum mapping DMA_TO_DEVICE correctly
  over standard per-CPU locked pools.
```

--=20
Thanks,
Mina