[PATCH net-next v2 0/3] net: atlantic: convert RX path to page_pool

[PATCH net-next v2 0/3] net: atlantic: convert RX path to page_pool

[Yangyu Chen]

The first two patches are standalone fixes for long-standing ring
teardown leaks and carry Cc: stable tags with their affected ranges
(patch 1: v4.11+, patch 2: v5.2+). They apply and were build- and
runtime-tested independently of each other and of the conversion in
patch 3, which is net-next material only and not tagged for stable.
Backports of patch 1 to trees without XDP support (before v5.19) only
need the xdp_frame branch dropped.

On systems where the NIC sits behind an IOMMU, the atlantic RX path
may not reach line rate: every RX buffer is allocated with
dev_alloc_pages() and mapped with dma_map_page(), then unmapped and
freed once the stack has consumed the packet. Every map/unmap is an
IOTLB/pagetable operation, and depending on the IOMMU this can
dominate the RX path at 10G rates. On an AMD Strix Halo system with a
Thunderbolt-attached QNAP QNA-T310G1S (MTU 1500, TCP over IPv6,
iperf3 -R), RX tops out at about 2.2 Gbit/s.

An earlier patch [1] worked around this by making the RX page order
tunable via a module parameter, amortizing one map/unmap over eight
pages worth of frames. The review feedback was to convert the driver
to the page_pool API instead of adding a knob. This series does that
conversion.

Patches 1 and 2 fix two long-standing teardown leaks first: TX
buffers stranded beyond the budgeted single aq_ring_tx_clean() pass
or past the frozen hw_head, and RX pages parked in consumed but not
yet refilled slots that the bounded deinit walk never visited. Both
are silent leaks today; after the conversion each stranded buffer
would hold a page_pool fragment reference and turn every interface
down into a permanently stalled pool shutdown (reproduction logs are
in the notes of both patches), so they need fixing before the
conversion lands.

Patch 3 converts the RX path to page_pool with the fragment API.
Pages are DMA-mapped once when they enter the per-ring pool
(PP_FLAG_DMA_MAP) and stay mapped while they recycle between the
driver and the stack, so steady-state RX performs no IOMMU work.
page_pool_dev_alloc_frag() takes over the sub-page splitting the
driver's hand-rolled "page flip" scheme did based on page_ref_count(),
buffer ownership becomes transfer-based, and the XDP memory model
switches to MEM_TYPE_PAGE_POOL. The diff is generated with
--histogram so the removed per-page helpers show as whole-function
deletions and aq_get_rxpages() as an in-place rewrite.

Performance, QNA-T310G1S (AQC100) behind Thunderbolt/IOMMU, MTU 1500,
TCP over IPv6, iperf3 -R:

  before:  2.24 Gbit/s
  after:   9.14 Gbit/s

matching what previously required the rxpageorder=3 workaround, but
with order-0 pages and no tunable.

Tested on the same setup: line rate with plain RX and with XDP_PASS;
XDP_TX with a MAC-swap reflector under sustained traffic; repeated
ifdown/ifup and module unload cycles under both plain RX and XDP_TX
load complete without "stalled pool shutdown" warnings or other splats.

Changes in v2 (thanks to Mina Almasry for the review):
 - split the RX deinit gap leak out of the conversion into its own
   fix (patch 2) and generate the conversion diff with --histogram so
   it reads as whole-function removals plus an in-place
   aq_get_rxpages() rewrite
 - add Cc: stable tags with the affected ranges to patches 1 and 2
 - comment the ring-before-rxq-registration ordering and its error
   unwind in aq_vec_ring_alloc()
 - aq_ring_tx_deinit(): return early instead of goto, drop the
   likely()/unlikely() annotations
 - aq_ptp_ring_alloc(): use the err_exit_xdp_rxq label instead of
   open-coding the unregister in the memory model error path
 - rebase on current net-next

[1] https://lore.kernel.org/lkml/tencent_E71C2F71D9631843941A5DF87204D1B5B509@qq.com/

v1: https://lore.kernel.org/lkml/tencent_7DB01BE7F8FA056BB5F11D3570CF636C4309@qq.com/

Yangyu Chen (3):
  net: atlantic: free stranded TX buffers on ring deinit
  net: atlantic: free RX pages of consumed but not refilled buffers
  net: atlantic: convert RX path to page_pool

 drivers/net/ethernet/aquantia/Kconfig         |   1 +
 .../ethernet/aquantia/atlantic/aq_ethtool.c   |   3 -
 .../net/ethernet/aquantia/atlantic/aq_ptp.c   |  18 +-
 .../net/ethernet/aquantia/atlantic/aq_ring.c  | 261 +++++++++---------
 .../net/ethernet/aquantia/atlantic/aq_ring.h  |   7 +-
 .../net/ethernet/aquantia/atlantic/aq_vec.c   |  23 +-
 6 files changed, 169 insertions(+), 144 deletions(-)


base-commit: 89d8006259b81dd25c962f6cc8d7ab268d6ea426
-- 
2.47.3

[PATCH net-next v2 1/3] net: atlantic: free stranded TX buffers on ring deinit

[Yangyu Chen]

aq_vec_deinit() drains the TX rings with a single aq_ring_tx_clean()
call, which frees at most AQ_CFG_TX_CLEAN_BUDGET (256) descriptors and
stops at hw_head, which no longer moves once aq_vec_stop() has stopped
the hardware and NAPI. Completed descriptors beyond the budget and
everything still posted in [hw_head, sw_tail) keep their skb or
xdp_frame when the interface goes down: aq_vec_ring_free() then frees
the buffer ring and the references are lost for good.

Today this is a silent memory leak on every interface down under
TX/XDP_TX load. With the following conversion of the RX path to
page_pool it becomes much more visible: XDP_TX frames carry fragment
references on the RX ring's page_pool, so a single stranded frame
keeps the pool's inflight count above zero forever. page_pool_destroy()
then never completes, the pool is leaked together with its pages, and
"page_pool_release_retry() stalled pool shutdown" is warned every 60
seconds from that point on, on every ifdown, XDP detach or ring resize
under XDP_TX load.

Bring back aq_ring_tx_deinit() as it was before the removal and use it
for teardown again, with one extension: TX rings can hold xdp_frames
nowadays, so release those too. They are returned with
xdp_return_frame() since this runs in process context.

Fixes: eb36bedf28be ("net: aquantia: remove function aq_ring_tx_deinit")
Cc: stable@vger.kernel.org # v4.11+
Assisted-by: Claude:claude-fable-5
Signed-off-by: Yangyu Chen <cyy@cyyself.name>
---

Notes:
    Without this fix, converting the RX path to page_pool (last patch of
    this series) turns the stranded XDP_TX frames into leaked page_pool
    fragments, so page_pool_destroy() can never drain and the shutdown
    stalls forever.
    
    Reproduced on an AQC100 with this patch dropped from the series (i.e.
    page_pool applied without the tx-deinit fix):
    
      # reflect received frames back out through XDP_TX
      xdp-bench tx enp99s0                    # or any trivial XDP_TX prog
      # from a peer on the same link, flood RX so frames are in flight, then
      ip link set enp99s0 down
    
    The pool is destroyed with frames still stranded on the TX ring, and
    page_pool_release_retry() warns every 60s with the same id and inflight
    count and a growing age, indefinitely:
    
      [161110.753385] page_pool_release_retry() stalled pool shutdown: id 361, 12 inflight 60 sec
      [161171.170756] page_pool_release_retry() stalled pool shutdown: id 361, 12 inflight 120 sec
      [161231.588685] page_pool_release_retry() stalled pool shutdown: id 361, 12 inflight 181 sec
      [161292.005886] page_pool_release_retry() stalled pool shutdown: id 361, 12 inflight 241 sec
    
    With this patch the stranded buffers are freed at deinit, inflight
    drops to zero and the pool drains cleanly.
    
    v1 -> v2:
     - return early instead of goto to an end-of-function label (Mina)
     - drop the likely()/unlikely() annotations, this is a reconfig
       path (Mina)
     - add Cc: stable with the affected range

 .../net/ethernet/aquantia/atlantic/aq_ring.c  | 29 +++++++++++++++++++
 .../net/ethernet/aquantia/atlantic/aq_ring.h  |  1 +
 .../net/ethernet/aquantia/atlantic/aq_vec.c   |  2 +-
 3 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
index 8ff07de2bd52..81685a4dc5a6 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
@@ -360,6 +360,35 @@ bool aq_ring_tx_clean(struct aq_ring_s *self)
 	return !!budget;
 }
 
+void aq_ring_tx_deinit(struct aq_ring_s *self)
+{
+	if (!self)
+		return;
+
+	for (; self->sw_head != self->sw_tail;
+		self->sw_head = aq_ring_next_dx(self, self->sw_head)) {
+		struct aq_ring_buff_s *buff = &self->buff_ring[self->sw_head];
+		struct device *ndev = aq_nic_get_dev(self->aq_nic);
+
+		if (buff->is_mapped) {
+			if (buff->is_sop) {
+				dma_unmap_single(ndev, buff->pa, buff->len,
+						 DMA_TO_DEVICE);
+			} else {
+				dma_unmap_page(ndev, buff->pa, buff->len,
+					       DMA_TO_DEVICE);
+			}
+		}
+
+		if (buff->is_eop) {
+			if (buff->skb)
+				dev_kfree_skb_any(buff->skb);
+			else if (buff->xdpf)
+				xdp_return_frame(buff->xdpf);
+		}
+	}
+}
+
 static void aq_rx_checksum(struct aq_ring_s *self,
 			   struct aq_ring_buff_s *buff,
 			   struct sk_buff *skb)
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.h b/drivers/net/ethernet/aquantia/atlantic/aq_ring.h
index a70b880ada67..6431cc62962f 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.h
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.h
@@ -202,6 +202,7 @@ void aq_ring_update_queue_state(struct aq_ring_s *ring);
 void aq_ring_queue_wake(struct aq_ring_s *ring);
 void aq_ring_queue_stop(struct aq_ring_s *ring);
 bool aq_ring_tx_clean(struct aq_ring_s *self);
+void aq_ring_tx_deinit(struct aq_ring_s *self);
 int aq_xdp_xmit(struct net_device *dev, int num_frames,
 		struct xdp_frame **frames, u32 flags);
 int aq_ring_rx_clean(struct aq_ring_s *self,
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_vec.c b/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
index 2f9033ceed8c..05814fea0f5f 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
@@ -275,7 +275,7 @@ void aq_vec_deinit(struct aq_vec_s *self)
 
 	for (i = 0U; self->tx_rings > i; ++i) {
 		ring = self->ring[i];
-		aq_ring_tx_clean(&ring[AQ_VEC_TX_ID]);
+		aq_ring_tx_deinit(&ring[AQ_VEC_TX_ID]);
 		aq_ring_rx_deinit(&ring[AQ_VEC_RX_ID]);
 	}
 
-- 
2.47.3

RE: [EXTERNAL] [PATCH net-next v2 1/3] net: atlantic: free stranded TX buffers on ring deinit

[Sukhdeep Soni [C]]

On 24 July 2026, Yangyu Chen wrote:

> aq_vec_deinit() drains the TX rings with a single aq_ring_tx_clean()
> call, which frees at most AQ_CFG_TX_CLEAN_BUDGET (256) descriptors and
> stops at hw_head, which no longer moves once aq_vec_stop() has stopped
> the hardware and NAPI. Completed descriptors beyond the budget and
> everything still posted in [hw_head, sw_tail) keep their skb or
> xdp_frame when the interface goes down: aq_vec_ring_free() then frees
> the buffer ring and the references are lost for good.
>
> Today this is a silent memory leak on every interface down under
> TX/XDP_TX load. With the following conversion of the RX path to
> page_pool it becomes much more visible: XDP_TX frames carry fragment
> references on the RX ring's page_pool, so a single stranded frame
> keeps the pool's inflight count above zero forever. page_pool_destroy()
> then never completes, the pool is leaked together with its pages, and
> "page_pool_release_retry() stalled pool shutdown" is warned every 60
> seconds from that point on, on every ifdown, XDP detach or ring resize
> under XDP_TX load.
>
> Bring back aq_ring_tx_deinit() as it was before the removal and use it
> for teardown again, with one extension: TX rings can hold xdp_frames
> nowadays, so release those too. They are returned with
> xdp_return_frame() since this runs in process context.
>
> Fixes: eb36bedf28be ("net: aquantia: remove function aq_ring_tx_deinit")
> Cc: mailto:stable@vger.kernel.org # v4.11+
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Yangyu Chen <mailto:cyy@cyyself.name>
> ---
>
> Notes:
>    Without this fix, converting the RX path to page_pool (last patch of
>    this series) turns the stranded XDP_TX frames into leaked page_pool
>    fragments, so page_pool_destroy() can never drain and the shutdown
>    stalls forever.
>    
>    Reproduced on an AQC100 with this patch dropped from the series (i.e.
>    page_pool applied without the tx-deinit fix):
>    
>      # reflect received frames back out through XDP_TX
>      xdp-bench tx enp99s0                    # or any trivial XDP_TX prog
>      # from a peer on the same link, flood RX so frames are in flight, then
>      ip link set enp99s0 down
>    
>    The pool is destroyed with frames still stranded on the TX ring, and
>    page_pool_release_retry() warns every 60s with the same id and inflight
>    count and a growing age, indefinitely:
>    
>      [161110.753385] page_pool_release_retry() stalled pool shutdown: id 361, 12 inflight 60 sec
>      [161171.170756] page_pool_release_retry() stalled pool shutdown: id 361, 12 inflight 120 sec
>      [161231.588685] page_pool_release_retry() stalled pool shutdown: id 361, 12 inflight 181 sec
>      [161292.005886] page_pool_release_retry() stalled pool shutdown: id 361, 12 inflight 241 sec
>    
>    With this patch the stranded buffers are freed at deinit, inflight
>    drops to zero and the pool drains cleanly.
>    
>    v1 -> v2:
>     - return early instead of goto to an end-of-function label (Mina)
>     - drop the likely()/unlikely() annotations, this is a reconfig
>       path (Mina)
>     - add Cc: stable with the affected range
>
> .../net/ethernet/aquantia/atlantic/aq_ring.c  | 29 +++++++++++++++++++
> .../net/ethernet/aquantia/atlantic/aq_ring.h  |  1 +
> .../net/ethernet/aquantia/atlantic/aq_vec.c   |  2 +-
> 3 files changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> index 8ff07de2bd52..81685a4dc5a6 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> @@ -360,6 +360,35 @@ bool aq_ring_tx_clean(struct aq_ring_s *self)
> 	return !!budget;
> }
> 
> +void aq_ring_tx_deinit(struct aq_ring_s *self)
> +{
> +	if (!self)
> +		return;
> +
> +	for (; self->sw_head != self->sw_tail;
> +		self->sw_head = aq_ring_next_dx(self, self->sw_head)) {
> +		struct aq_ring_buff_s *buff = &self->buff_ring[self->sw_head];
> +		struct device *ndev = aq_nic_get_dev(self->aq_nic);
> +
> +		if (buff->is_mapped) {
> +			if (buff->is_sop) {
> +				dma_unmap_single(ndev, buff->pa, buff->len,
> +						 DMA_TO_DEVICE);
> +			} else {
> +				dma_unmap_page(ndev, buff->pa, buff->len,
> +					       DMA_TO_DEVICE);
> +			}
> +		}
> +
> +		if (buff->is_eop) {
> +			if (buff->skb)
> +				dev_kfree_skb_any(buff->skb);
> +			else if (buff->xdpf)
> +				xdp_return_frame(buff->xdpf);
> +		}
> +	}
> +}
> +
> static void aq_rx_checksum(struct aq_ring_s *self,
> 			   struct aq_ring_buff_s *buff,
> 			   struct sk_buff *skb)
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.h b/drivers/net/ethernet/aquantia/atlantic/aq_ring.h
> index a70b880ada67..6431cc62962f 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.h
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.h
> @@ -202,6 +202,7 @@ void aq_ring_update_queue_state(struct aq_ring_s *ring);
> void aq_ring_queue_wake(struct aq_ring_s *ring);
> void aq_ring_queue_stop(struct aq_ring_s *ring);
> bool aq_ring_tx_clean(struct aq_ring_s *self);
> +void aq_ring_tx_deinit(struct aq_ring_s *self);
>  int aq_xdp_xmit(struct net_device *dev, int num_frames,
> 		struct xdp_frame **frames, u32 flags);
>  int aq_ring_rx_clean(struct aq_ring_s *self,
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_vec.c b/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
> index 2f9033ceed8c..05814fea0f5f 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
> @@ -275,7 +275,7 @@ void aq_vec_deinit(struct aq_vec_s *self)
> 
> 	for (i = 0U; self->tx_rings > i; ++i) {
> 		ring = self->ring[i];
> -		aq_ring_tx_clean(&ring[AQ_VEC_TX_ID]);
> +		aq_ring_tx_deinit(&ring[AQ_VEC_TX_ID]);
> 		aq_ring_rx_deinit(&ring[AQ_VEC_RX_ID]);
>	}
> 
> -- 
> 2.47.3

Reviewed-by: Sukhdeep Singh <sukhdeeps@marvell.com>

Re: [PATCH net-next v2 1/3] net: atlantic: free stranded TX buffers on ring deinit

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete fix: PTP TX ring still uses `aq_ring_tx_clean()` on teardown, leading to memory/DMA leaks and a locking violation.
--

--- Patch [1]: [PATCH net-next v2 1/3] net: atlantic: free stranded TX buffers on ring deinit ---
commit d506a87f11eebfd3878d7ddde798e451d9fd8627
Author: Yangyu Chen <cyy@cyyself.name>

net: atlantic: free stranded TX buffers on ring deinit

This commit brings back aq_ring_tx_deinit() to properly clean up TX rings 
on teardown by iterating up to sw_tail. It replaces the call to 
aq_ring_tx_clean() in aq_vec_deinit() to fix a memory and DMA mapping leak 
that occurs during interface shutdown.

Cc: stable@vger.kernel.org # v4.11+

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_vec.c b/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
> index 2f9033ceed8ce..05814fea0f5f7 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
> @@ -275,7 +275,7 @@ void aq_vec_deinit(struct aq_vec_s *self)
>  
>  	for (i = 0U; self->tx_rings > i; ++i) {
>  		ring = self->ring[i];
> -		aq_ring_tx_clean(&ring[AQ_VEC_TX_ID]);
> +		aq_ring_tx_deinit(&ring[AQ_VEC_TX_ID]);
>  		aq_ring_rx_deinit(&ring[AQ_VEC_RX_ID]);
>  	}

[Severity: High]
This is a pre-existing issue, but should the PTP TX ring be updated to use 
aq_ring_tx_deinit() as well?

Looking at the PTP TX ring teardown path:

drivers/net/ethernet/aquantia/atlantic/aq_ptp.c:aq_ptp_ring_deinit() {
    ...
    aq_ring_tx_clean(&aq_ptp->ptp_tx);
    ...
}

Since aq_ring_tx_clean() only cleans up to hw_head, won't any PTP descriptors
between hw_head and sw_tail be leaked when the interface is stopped while
PTP TX packets are in flight? 

Also, since aq_ring_tx_clean() updates u64_stats without disabling preemption,
does keeping it here leave the teardown path vulnerable to a lockdep splat on
32-bit systems?

Does it make sense to update aq_ptp_ring_deinit() to use the newly restored
aq_ring_tx_deinit() for the PTP TX ring to fully close this leak?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_1F173E0FC1606D2AC704DC9C98AF10984607@qq.com?part=1

[PATCH net-next v2 2/3] net: atlantic: free RX pages of consumed but not refilled buffers

[Yangyu Chen]

aq_ring_rx_deinit() only walks [sw_head, sw_tail), the region posted to
hardware. Since the page reuse strategy was added, a cleaned RX buffer
keeps its page (and its DMA mapping) in the ring for reuse, and refill
is batched: aq_ring_rx_fill() returns early until AQ_CFG_RX_REFILL_THRES
slots are free. Slots that were consumed but not yet reposted therefore
sit in the complementary [sw_tail, sw_head) gap with a live page, and
the deinit walk never visits them: up to a refill batch worth of pages
and DMA mappings leak on every interface down.

Walk the whole ring instead and release whatever is still there. Also
bail out if the buffer ring is already gone: a partial
aq_ptp_ring_alloc() failure frees the ring but leaves aq_nic set, so
aq_ptp_ring_deinit() still gets here on the unwind path.

Fixes: 46f4c29d9de6 ("net: aquantia: optimize rx performance by page reuse strategy")
Cc: stable@vger.kernel.org # v5.2+
Assisted-by: Claude:claude-fable-5
Signed-off-by: Yangyu Chen <cyy@cyyself.name>
---

Notes:
    Without this fix, the following page_pool conversion turns the missed
    pages into fragments that page_pool_destroy() waits for forever.
    Reproduced on an AQC100 with the conversion applied and this fix
    reverted -- ordinary small received frames (<= 256 byte header-only
    packets, e.g. ping replies or pure TCP ACKs) are enough to populate
    the [sw_tail, sw_head) gap:
    
      ping -c 200 -i 0.005 <peer>%enp99s0
      ip link set enp99s0 down
    
    One short ping flow left three of the eight RX rings' pools with
    stranded fragments, and page_pool_release_retry() warns for each of
    them every 60 seconds, indefinitely:
    
      [278084.929092] page_pool_release_retry() stalled pool shutdown: id 123, 1 inflight 60 sec
      [278084.961064] page_pool_release_retry() stalled pool shutdown: id 126, 1 inflight 60 sec
      [278084.961087] page_pool_release_retry() stalled pool shutdown: id 125, 6 inflight 60 sec
      [278145.346737] page_pool_release_retry() stalled pool shutdown: id 123, 1 inflight 120 sec
      [278145.378745] page_pool_release_retry() stalled pool shutdown: id 125, 6 inflight 120 sec
      [278145.378759] page_pool_release_retry() stalled pool shutdown: id 126, 1 inflight 120 sec
    
    With this patch the whole ring is walked at deinit, the pages are
    released, and the pools drain immediately. On the current code the
    same gap leaks the pages and their DMA mappings silently.
    
    Applies and was build- and runtime-tested independently of the rest
    of this series, against the current page scheme.
    
    New in v2: split out of the page_pool conversion and made a
    standalone fix for the existing page reuse scheme (Mina); Cc stable
    with the affected range.

 .../net/ethernet/aquantia/atlantic/aq_ring.c  | 22 +++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
index 81685a4dc5a6..e1193c6719d9 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
@@ -950,15 +950,29 @@ int aq_ring_rx_fill(struct aq_ring_s *self)
 
 void aq_ring_rx_deinit(struct aq_ring_s *self)
 {
-	if (!self)
+	unsigned int i;
+
+	if (!self || !self->buff_ring)
 		return;
 
-	for (; self->sw_head != self->sw_tail;
-		self->sw_head = aq_ring_next_dx(self, self->sw_head)) {
-		struct aq_ring_buff_s *buff = &self->buff_ring[self->sw_head];
+	/* Release every page still owned by the ring.
+	 *
+	 * Walking [sw_head, sw_tail) is not enough: refill is batched
+	 * (aq_ring_rx_fill() waits for AQ_CFG_RX_REFILL_THRES free slots),
+	 * so slots that were cleaned but not yet reposted accumulate in the
+	 * [sw_tail, sw_head) gap, and they keep their page for reuse. Walk
+	 * the whole ring and release whatever is left.
+	 */
+	for (i = 0; i < self->size; i++) {
+		struct aq_ring_buff_s *buff = &self->buff_ring[i];
+
+		if (!buff->rxdata.page)
+			continue;
 
 		aq_free_rxpage(&buff->rxdata, aq_nic_get_dev(self->aq_nic));
 	}
+
+	self->sw_head = self->sw_tail;
 }
 
 void aq_ring_free(struct aq_ring_s *self)
-- 
2.47.3

RE: [EXTERNAL] [PATCH net-next v2 2/3] net: atlantic: free RX pages of consumed but not refilled buffers

[Sukhdeep Soni [C]]

On 24 july 2026, Yangyu Chen wrote:

> aq_ring_rx_deinit() only walks [sw_head, sw_tail), the region posted to
> hardware. Since the page reuse strategy was added, a cleaned RX buffer
> keeps its page (and its DMA mapping) in the ring for reuse, and refill
> is batched: aq_ring_rx_fill() returns early until AQ_CFG_RX_REFILL_THRES
> slots are free. Slots that were consumed but not yet reposted therefore
> sit in the complementary [sw_tail, sw_head) gap with a live page, and
> the deinit walk never visits them: up to a refill batch worth of pages
> and DMA mappings leak on every interface down.
> 
> Walk the whole ring instead and release whatever is still there. Also
> bail out if the buffer ring is already gone: a partial
> aq_ptp_ring_alloc() failure frees the ring but leaves aq_nic set, so
> aq_ptp_ring_deinit() still gets here on the unwind path.
> 
> Fixes: 46f4c29d9de6 ("net: aquantia: optimize rx performance by page reuse strategy")
> Cc: mailto:stable@vger.kernel.org # v5.2+
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Yangyu Chen <mailto:cyy@cyyself.name>
> ---
>
> Notes:
>    Without this fix, the following page_pool conversion turns the missed
>    pages into fragments that page_pool_destroy() waits for forever.
>    Reproduced on an AQC100 with the conversion applied and this fix
>    reverted -- ordinary small received frames (<= 256 byte header-only
>    packets, e.g. ping replies or pure TCP ACKs) are enough to populate
>    the [sw_tail, sw_head) gap:
>    
>      ping -c 200 -i 0.005 <peer>%enp99s0
>      ip link set enp99s0 down
>    
>    One short ping flow left three of the eight RX rings' pools with
>    stranded fragments, and page_pool_release_retry() warns for each of
>    them every 60 seconds, indefinitely:
>    
>      [278084.929092] page_pool_release_retry() stalled pool shutdown: id 123, 1 inflight 60 sec
>      [278084.961064] page_pool_release_retry() stalled pool shutdown: id 126, 1 inflight 60 sec
>      [278084.961087] page_pool_release_retry() stalled pool shutdown: id 125, 6 inflight 60 sec
>      [278145.346737] page_pool_release_retry() stalled pool shutdown: id 123, 1 inflight 120 sec
>      [278145.378745] page_pool_release_retry() stalled pool shutdown: id 125, 6 inflight 120 sec
>      [278145.378759] page_pool_release_retry() stalled pool shutdown: id 126, 1 inflight 120 sec
>    
>    With this patch the whole ring is walked at deinit, the pages are
>    released, and the pools drain immediately. On the current code the
>    same gap leaks the pages and their DMA mappings silently.
>    
>    Applies and was build- and runtime-tested independently of the rest
>    of this series, against the current page scheme.
>    
>    New in v2: split out of the page_pool conversion and made a
>    standalone fix for the existing page reuse scheme (Mina); Cc stable
>    with the affected range.
>
> .../net/ethernet/aquantia/atlantic/aq_ring.c  | 22 +++++++++++++++----
> 1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> index 81685a4dc5a6..e1193c6719d9 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> @@ -950,15 +950,29 @@ int aq_ring_rx_fill(struct aq_ring_s *self)
> 
> void aq_ring_rx_deinit(struct aq_ring_s *self)
> {
> -	if (!self)
> +	unsigned int i;
> +
> +	if (!self || !self->buff_ring)
> 		return;
> 
> -	for (; self->sw_head != self->sw_tail;
> -		self->sw_head = aq_ring_next_dx(self, self->sw_head)) {
> -		struct aq_ring_buff_s *buff = &self->buff_ring[self->sw_head];
> +	/* Release every page still owned by the ring.
> +	 *
> +	 * Walking [sw_head, sw_tail) is not enough: refill is batched
> +	 * (aq_ring_rx_fill() waits for AQ_CFG_RX_REFILL_THRES free slots),
> +	 * so slots that were cleaned but not yet reposted accumulate in the
> +	 * [sw_tail, sw_head) gap, and they keep their page for reuse. Walk
> +	 * the whole ring and release whatever is left.
> +	 */
> +	for (i = 0; i < self->size; i++) {
> +		struct aq_ring_buff_s *buff = &self->buff_ring[i];
> +
> +		if (!buff->rxdata.page)
> +			continue;
> 
> 		aq_free_rxpage(&buff->rxdata, aq_nic_get_dev(self->aq_nic));
> 	}
> +
> +	self->sw_head = self->sw_tail;
> }
> 
> void aq_ring_free(struct aq_ring_s *self)
> -- 
> 2.47.3

Reviewed-by: Sukhdeep Singh <sukhdeeps@marvell.com>

Re: [PATCH net-next v2 2/3] net: atlantic: free RX pages of consumed but not refilled buffers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Medium] The PTP TX ring leaks SKBs and their DMA mappings on shutdown or error unwind paths if the hardware has not completed transmission of all queued packets.
--

--- Patch [2]: [PATCH net-next v2 2/3] net: atlantic: free RX pages of consumed but not refilled buffers ---
commit 7a64d2b15db815d6730701e2ca08b6013f4f4ae8
Author: Yangyu Chen <cyy@cyyself.name>

net: atlantic: free RX pages of consumed but not refilled buffers
    
This commit fixes a memory and DMA mapping leak in the atlantic driver that
occurs when the interface is brought down. It modifies aq_ring_rx_deinit()
to exhaustively walk the entire RX ring and release any remaining pages,
rather than only iterating over the region posted to hardware.

> Walk the whole ring instead and release whatever is still there. Also
> bail out if the buffer ring is already gone: a partial
> aq_ptp_ring_alloc() failure frees the ring but leaves aq_nic set, so
> aq_ptp_ring_deinit() still gets here on the unwind path.

[Severity: Medium]
This is a pre-existing issue, but since the commit message mentions the
unwind path via aq_ptp_ring_deinit(), I noticed how the PTP TX ring is
cleaned up:

drivers/net/ethernet/aquantia/atlantic/aq_ptp.c:aq_ptp_ring_deinit() {
    ...
	aq_ring_tx_clean(&aq_ptp->ptp_tx);
	aq_ring_rx_deinit(&aq_ptp->ptp_rx);
}

Does this code leak SKBs and their DMA mappings on the shutdown or error
unwind paths if the hardware has not completed transmission of all queued
packets?

Since aq_ring_tx_clean() only iterates up to hw_head, it leaves pending
descriptors uncleaned. Should there be an exhaustive aq_ring_tx_deinit()
call instead?

> Fixes: 46f4c29d9de6 ("net: aquantia: optimize rx performance by page reuse strategy")
> Cc: stable@vger.kernel.org # v5.2+
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Yangyu Chen <cyy@cyyself.name>
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> index 81685a4dc5a6d..e1193c6719d9d 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_1F173E0FC1606D2AC704DC9C98AF10984607@qq.com?part=2

[PATCH net-next v2 3/3] net: atlantic: convert RX path to page_pool

[Yangyu Chen]

The driver currently allocates RX buffers with dev_alloc_pages(), maps
them with dma_map_page(), and uses a hand-rolled page-flip scheme to
subdivide high-order pages. Behind an IOMMU, the map/unmap churn is a
major RX cost: on a Thunderbolt-attached QNAP QNA-T310G1S, iperf3 -R
over IPv6 tops out at about 2.2 Gbit/s over MTU 1500.

Convert RX buffers to page_pool fragments. Pages are DMA-mapped once
when entering the pool and recycled through the stack or XDP via the
MEM_TYPE_PAGE_POOL memory model. This removes the custom page-flip
accounting, lets page_pool handle fragment reuse, and ensures every RX
path either keeps the ring's fragment reference for reposting or hands
it to the skb/xdp_buff for later recycling.

Register the PTP RX ring's xdp_rxq as well, since it shares the RX
clean paths. Drop the ethtool PageFlips/PageReuses/PageFrees counters
which only described the old scheme.

On the QNA-T310G1S, MTU 1500, TCP over IPv6, iperf3 -R improves from
2.24 Gbit/s to 9.14 Gbit/s. The module was also smoke-tested with native
XDP PASS, DROP, and ABORTED actions; carrier recovered after each
attach/detach cycle and dmesg showed no page_pool/DMA warnings.

Assisted-by: Claude:claude-fable-5
Signed-off-by: Yangyu Chen <cyy@cyyself.name>
---

Notes:
    v1 -> v2:
     - split the RX deinit gap fix into the previous patch and generate
       this diff with --histogram so it reads as whole-function removals
       plus an in-place aq_get_rxpages() rewrite (Mina)
     - comment the ring-before-rxq-registration ordering and its error
       unwind in aq_vec_ring_alloc()
     - use the err_exit_xdp_rxq label instead of open-coding the
       unregister in aq_ptp_ring_alloc()'s memory model error path (Mina)

 drivers/net/ethernet/aquantia/Kconfig         |   1 +
 .../ethernet/aquantia/atlantic/aq_ethtool.c   |   3 -
 .../net/ethernet/aquantia/atlantic/aq_ptp.c   |  18 +-
 .../net/ethernet/aquantia/atlantic/aq_ring.c  | 210 +++++++-----------
 .../net/ethernet/aquantia/atlantic/aq_ring.h  |   6 +-
 .../net/ethernet/aquantia/atlantic/aq_vec.c   |  21 +-
 6 files changed, 120 insertions(+), 139 deletions(-)

diff --git a/drivers/net/ethernet/aquantia/Kconfig b/drivers/net/ethernet/aquantia/Kconfig
index cec2018c84a9..c8fb7b33e5b7 100644
--- a/drivers/net/ethernet/aquantia/Kconfig
+++ b/drivers/net/ethernet/aquantia/Kconfig
@@ -20,6 +20,7 @@ config AQTION
 	tristate "aQuantia AQtion(tm) Support"
 	depends on PCI
 	depends on MACSEC || MACSEC=n
+	select PAGE_POOL
 	help
 	  This enables the support for the aQuantia AQtion(tm) Ethernet card.
 
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c b/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
index 420af958d486..0f5125bd2315 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
@@ -100,9 +100,6 @@ static const char * const aq_ethtool_queue_rx_stat_names[] = {
 	"%sQueue[%d] AllocFails",
 	"%sQueue[%d] SkbAllocFails",
 	"%sQueue[%d] Polls",
-	"%sQueue[%d] PageFlips",
-	"%sQueue[%d] PageReuses",
-	"%sQueue[%d] PageFrees",
 	"%sQueue[%d] XdpAbort",
 	"%sQueue[%d] XdpDrop",
 	"%sQueue[%d] XdpPass",
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c b/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
index 558ac9237f75..3a40d986cd67 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
@@ -13,6 +13,7 @@
 #include <linux/ptp_classify.h>
 #include <linux/interrupt.h>
 #include <linux/clocksource.h>
+#include <net/xdp.h>
 
 #include "aq_nic.h"
 #include "aq_ptp.h"
@@ -1192,12 +1193,23 @@ int aq_ptp_ring_alloc(struct aq_nic_s *aq_nic)
 	if (err)
 		goto err_exit_ptp_tx;
 
+	err = xdp_rxq_info_reg(&aq_ptp->ptp_rx.xdp_rxq, aq_nic->ndev,
+			       rx_ring_idx, aq_ptp->napi.napi_id);
+	if (err < 0)
+		goto err_exit_ptp_rx;
+
+	err = xdp_rxq_info_reg_mem_model(&aq_ptp->ptp_rx.xdp_rxq,
+					 MEM_TYPE_PAGE_POOL,
+					 aq_ptp->ptp_rx.pg_pool);
+	if (err < 0)
+		goto err_exit_xdp_rxq;
+
 	if (aq_ptp->a1_ptp) {
 		err = aq_ring_hwts_rx_alloc(&aq_ptp->hwts_rx, aq_nic, PTP_HWST_RING_IDX,
 					    aq_nic->aq_nic_cfg.rxds,
 					    aq_nic->aq_nic_cfg.aq_hw_caps->rxd_size);
 		if (err)
-			goto err_exit_ptp_rx;
+			goto err_exit_xdp_rxq;
 	}
 
 	err = aq_ptp_skb_ring_init(&aq_ptp->skb_ring, aq_nic->aq_nic_cfg.rxds);
@@ -1217,6 +1229,8 @@ int aq_ptp_ring_alloc(struct aq_nic_s *aq_nic)
 err_exit_hwts_rx:
 	if (aq_ptp->a1_ptp)
 		aq_ring_hwts_rx_free(&aq_ptp->hwts_rx);
+err_exit_xdp_rxq:
+	xdp_rxq_info_unreg(&aq_ptp->ptp_rx.xdp_rxq);
 err_exit_ptp_rx:
 	aq_ring_free(&aq_ptp->ptp_rx);
 err_exit_ptp_tx:
@@ -1233,6 +1247,8 @@ void aq_ptp_ring_free(struct aq_nic_s *aq_nic)
 		return;
 
 	aq_ring_free(&aq_ptp->ptp_tx);
+	if (xdp_rxq_info_is_reg(&aq_ptp->ptp_rx.xdp_rxq))
+		xdp_rxq_info_unreg(&aq_ptp->ptp_rx.xdp_rxq);
 	aq_ring_free(&aq_ptp->ptp_rx);
 	if (aq_ptp->a1_ptp)
 		aq_ring_hwts_rx_free(&aq_ptp->hwts_rx);
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
index e1193c6719d9..9dd881710594 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
@@ -14,120 +14,37 @@
 #include "aq_vec.h"
 #include "aq_main.h"
 
+#include <net/page_pool/helpers.h>
 #include <net/xdp.h>
 #include <linux/filter.h>
 #include <linux/bpf_trace.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 
-static void aq_get_rxpages_xdp(struct aq_ring_buff_s *buff,
-			       struct xdp_buff *xdp)
-{
-	struct skb_shared_info *sinfo;
-	int i;
-
-	if (xdp_buff_has_frags(xdp)) {
-		sinfo = xdp_get_shared_info_from_buff(xdp);
-
-		for (i = 0; i < sinfo->nr_frags; i++) {
-			skb_frag_t *frag = &sinfo->frags[i];
-
-			page_ref_inc(skb_frag_page(frag));
-		}
-	}
-	page_ref_inc(buff->rxdata.page);
-}
-
-static inline void aq_free_rxpage(struct aq_rxpage *rxpage, struct device *dev)
-{
-	unsigned int len = PAGE_SIZE << rxpage->order;
-
-	dma_unmap_page(dev, rxpage->daddr, len, DMA_FROM_DEVICE);
-
-	/* Drop the ref for being in the ring. */
-	__free_pages(rxpage->page, rxpage->order);
-	rxpage->page = NULL;
-}
-
-static int aq_alloc_rxpages(struct aq_rxpage *rxpage, struct aq_ring_s *rx_ring)
-{
-	struct device *dev = aq_nic_get_dev(rx_ring->aq_nic);
-	unsigned int order = rx_ring->page_order;
-	struct page *page;
-	int ret = -ENOMEM;
-	dma_addr_t daddr;
-
-	page = dev_alloc_pages(order);
-	if (unlikely(!page))
-		goto err_exit;
-
-	daddr = dma_map_page(dev, page, 0, PAGE_SIZE << order,
-			     DMA_FROM_DEVICE);
-
-	if (unlikely(dma_mapping_error(dev, daddr)))
-		goto free_page;
-
-	rxpage->page = page;
-	rxpage->daddr = daddr;
-	rxpage->order = order;
-	rxpage->pg_off = rx_ring->page_offset;
-
-	return 0;
-
-free_page:
-	__free_pages(page, order);
-
-err_exit:
-	return ret;
-}
-
 static int aq_get_rxpages(struct aq_ring_s *self, struct aq_ring_buff_s *rxbuf)
 {
-	unsigned int order = self->page_order;
-	u16 page_offset = self->page_offset;
-	u16 frame_max = self->frame_max;
-	u16 tail_size = self->tail_size;
-	int ret;
+	unsigned int size = self->page_offset + self->frame_max +
+			    self->tail_size;
+	unsigned int pg_off;
+	struct page *page;
 
-	if (rxbuf->rxdata.page) {
-		/* One means ring is the only user and can reuse */
-		if (page_ref_count(rxbuf->rxdata.page) > 1) {
-			/* Try reuse buffer */
-			rxbuf->rxdata.pg_off += frame_max + page_offset +
-						tail_size;
-			if (rxbuf->rxdata.pg_off + frame_max + tail_size <=
-			    (PAGE_SIZE << order)) {
-				u64_stats_update_begin(&self->stats.rx.syncp);
-				self->stats.rx.pg_flips++;
-				u64_stats_update_end(&self->stats.rx.syncp);
+	/* Buffers whose page was not passed up the stack are reposted
+	 * with the data they already carry discarded.
+	 */
+	if (rxbuf->rxdata.page)
+		return 0;
 
-			} else {
-				/* Buffer exhausted. We have other users and
-				 * should release this page and realloc
-				 */
-				aq_free_rxpage(&rxbuf->rxdata,
-					       aq_nic_get_dev(self->aq_nic));
-				u64_stats_update_begin(&self->stats.rx.syncp);
-				self->stats.rx.pg_losts++;
-				u64_stats_update_end(&self->stats.rx.syncp);
-			}
-		} else {
-			rxbuf->rxdata.pg_off = page_offset;
-			u64_stats_update_begin(&self->stats.rx.syncp);
-			self->stats.rx.pg_reuses++;
-			u64_stats_update_end(&self->stats.rx.syncp);
-		}
+	page = page_pool_dev_alloc_frag(self->pg_pool, &pg_off, size);
+	if (unlikely(!page)) {
+		u64_stats_update_begin(&self->stats.rx.syncp);
+		self->stats.rx.alloc_fails++;
+		u64_stats_update_end(&self->stats.rx.syncp);
+		return -ENOMEM;
 	}
 
-	if (!rxbuf->rxdata.page) {
-		ret = aq_alloc_rxpages(&rxbuf->rxdata, self);
-		if (ret) {
-			u64_stats_update_begin(&self->stats.rx.syncp);
-			self->stats.rx.alloc_fails++;
-			u64_stats_update_end(&self->stats.rx.syncp);
-		}
-		return ret;
-	}
+	rxbuf->rxdata.page = page;
+	rxbuf->rxdata.daddr = page_pool_get_dma_addr(page);
+	rxbuf->rxdata.pg_off = pg_off + self->page_offset;
 
 	return 0;
 }
@@ -179,6 +96,15 @@ int aq_ring_rx_alloc(struct aq_ring_s *self,
 		     unsigned int idx,
 		     struct aq_nic_cfg_s *aq_nic_cfg)
 {
+	struct page_pool_params pp_params = {
+		.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
+		.pool_size = aq_nic_cfg->rxds,
+		.nid = NUMA_NO_NODE,
+		.dev = aq_nic_get_dev(aq_nic),
+		.dma_dir = DMA_FROM_DEVICE,
+	};
+	struct page_pool *pool;
+
 	self->aq_nic = aq_nic;
 	self->idx = idx;
 	self->size = aq_nic_cfg->rxds;
@@ -200,6 +126,18 @@ int aq_ring_rx_alloc(struct aq_ring_s *self,
 		self->tail_size = 0;
 	}
 
+	pp_params.order = self->page_order;
+	pp_params.max_len = PAGE_SIZE << self->page_order;
+
+	pool = page_pool_create(&pp_params);
+	if (IS_ERR(pool))
+		return PTR_ERR(pool);
+
+	self->pg_pool = pool;
+
+	/* On failure aq_ring_alloc() calls aq_ring_free(), which also
+	 * destroys the page pool.
+	 */
 	return aq_ring_alloc(self, aq_nic);
 }
 
@@ -346,7 +284,11 @@ bool aq_ring_tx_clean(struct aq_ring_s *self)
 			++self->stats.tx.packets;
 			self->stats.tx.bytes += xdp_get_frame_len(buff->xdpf);
 			u64_stats_update_end(&self->stats.tx.syncp);
-			xdp_return_frame_rx_napi(buff->xdpf);
+			/* Frames queued via ndo_xdp_xmit() may come from a
+			 * page pool owned by another NAPI context: no direct
+			 * recycling.
+			 */
+			xdp_return_frame(buff->xdpf);
 		}
 
 out:
@@ -437,22 +379,15 @@ int aq_xdp_xmit(struct net_device *dev, int num_frames,
 }
 
 static struct sk_buff *aq_xdp_build_skb(struct xdp_buff *xdp,
-					struct net_device *dev,
-					struct aq_ring_buff_s *buff)
+					struct net_device *dev)
 {
 	struct xdp_frame *xdpf;
-	struct sk_buff *skb;
 
 	xdpf = xdp_convert_buff_to_frame(xdp);
 	if (unlikely(!xdpf))
 		return NULL;
 
-	skb = xdp_build_skb_from_frame(xdpf, dev);
-	if (!skb)
-		return NULL;
-
-	aq_get_rxpages_xdp(buff, xdp);
-	return skb;
+	return xdp_build_skb_from_frame(xdpf, dev);
 }
 
 static struct sk_buff *aq_xdp_run_prog(struct aq_nic_s *aq_nic,
@@ -473,8 +408,16 @@ static struct sk_buff *aq_xdp_run_prog(struct aq_nic_s *aq_nic,
 	u64_stats_update_end(&rx_ring->stats.rx.syncp);
 
 	prog = READ_ONCE(rx_ring->xdp_prog);
-	if (!prog)
-		return aq_xdp_build_skb(xdp, aq_nic->ndev, buff);
+	if (!prog) {
+		skb = aq_xdp_build_skb(xdp, aq_nic->ndev);
+		/* The ring has already handed its page pool reference to the
+		 * xdp_buff, so if the skb could not be built the buffer must
+		 * be returned to the pool here or its fragments would leak.
+		 */
+		if (!skb)
+			xdp_return_buff(xdp);
+		return skb;
+	}
 
 	prefetchw(xdp->data_hard_start); /* xdp_frame write */
 
@@ -485,7 +428,7 @@ static struct sk_buff *aq_xdp_run_prog(struct aq_nic_s *aq_nic,
 	act = bpf_prog_run_xdp(prog, xdp);
 	switch (act) {
 	case XDP_PASS:
-		skb = aq_xdp_build_skb(xdp, aq_nic->ndev, buff);
+		skb = aq_xdp_build_skb(xdp, aq_nic->ndev);
 		if (!skb)
 			goto out_aborted;
 		u64_stats_update_begin(&rx_ring->stats.rx.syncp);
@@ -503,7 +446,6 @@ static struct sk_buff *aq_xdp_run_prog(struct aq_nic_s *aq_nic,
 		u64_stats_update_begin(&rx_ring->stats.rx.syncp);
 		++rx_ring->stats.rx.xdp_tx;
 		u64_stats_update_end(&rx_ring->stats.rx.syncp);
-		aq_get_rxpages_xdp(buff, xdp);
 		break;
 	case XDP_REDIRECT:
 		if (xdp_do_redirect(aq_nic->ndev, xdp, prog) < 0)
@@ -512,7 +454,6 @@ static struct sk_buff *aq_xdp_run_prog(struct aq_nic_s *aq_nic,
 		u64_stats_update_begin(&rx_ring->stats.rx.syncp);
 		++rx_ring->stats.rx.xdp_redirect;
 		u64_stats_update_end(&rx_ring->stats.rx.syncp);
-		aq_get_rxpages_xdp(buff, xdp);
 		break;
 	default:
 		fallthrough;
@@ -523,11 +464,13 @@ static struct sk_buff *aq_xdp_run_prog(struct aq_nic_s *aq_nic,
 		u64_stats_update_end(&rx_ring->stats.rx.syncp);
 		trace_xdp_exception(aq_nic->ndev, prog, act);
 		bpf_warn_invalid_xdp_action(aq_nic->ndev, prog, act);
+		xdp_return_buff(xdp);
 		break;
 	case XDP_DROP:
 		u64_stats_update_begin(&rx_ring->stats.rx.syncp);
 		++rx_ring->stats.rx.xdp_drop;
 		u64_stats_update_end(&rx_ring->stats.rx.syncp);
+		xdp_return_buff(xdp);
 		break;
 	}
 
@@ -546,8 +489,11 @@ static bool aq_add_rx_fragment(struct device *dev,
 	do {
 		skb_frag_t *frag;
 
-		if (unlikely(sinfo->nr_frags >= MAX_SKB_FRAGS))
+		if (unlikely(sinfo->nr_frags >= MAX_SKB_FRAGS)) {
+			/* Attached frags must reach xdp_return_buff() */
+			xdp_buff_set_frags_flag(xdp);
 			return true;
+		}
 
 		frag = &sinfo->frags[sinfo->nr_frags++];
 		buff_ = &ring->buff_ring[buff_->next];
@@ -571,6 +517,11 @@ static bool aq_add_rx_fragment(struct device *dev,
 		if (page_is_pfmemalloc(buff_->rxdata.page))
 			xdp_buff_set_frag_pfmemalloc(xdp);
 
+		/* The frag's page pool reference is owned by the xdp_buff
+		 * from now on.
+		 */
+		buff_->rxdata.page = NULL;
+
 	} while (!buff_->is_eop);
 
 	xdp_buff_set_frags_flag(xdp);
@@ -674,6 +625,7 @@ static int __aq_ring_rx_clean(struct aq_ring_s *self, struct napi_struct *napi,
 			err = -ENOMEM;
 			goto err_exit;
 		}
+		skb_mark_for_recycle(skb);
 		if (is_ptp_ring)
 			buff->len -=
 				aq_ptp_extract_ts(self->aq_nic, skb_hwtstamps(skb),
@@ -694,7 +646,7 @@ static int __aq_ring_rx_clean(struct aq_ring_s *self, struct napi_struct *napi,
 					buff->rxdata.pg_off + hdr_len,
 					buff->len - hdr_len,
 					self->frame_max);
-			page_ref_inc(buff->rxdata.page);
+			buff->rxdata.page = NULL;
 		}
 
 		if (!buff->is_eop) {
@@ -713,7 +665,7 @@ static int __aq_ring_rx_clean(struct aq_ring_s *self, struct napi_struct *napi,
 						buff_->rxdata.pg_off,
 						buff_->len,
 						self->frame_max);
-				page_ref_inc(buff_->rxdata.page);
+				buff_->rxdata.page = NULL;
 				buff_->is_cleaned = 1;
 
 				buff->is_ip_cso &= buff_->is_ip_cso;
@@ -851,6 +803,11 @@ static int __aq_ring_xdp_clean(struct aq_ring_s *rx_ring,
 		xdp_init_buff(&xdp, frame_sz, &rx_ring->xdp_rxq);
 		xdp_prepare_buff(&xdp, hard_start, rx_ring->page_offset,
 				 buff->len, false);
+		/* The xdp_buff owns the buffer's page pool reference from
+		 * here on; it comes back through the MEM_TYPE_PAGE_POOL
+		 * memory model on every XDP verdict.
+		 */
+		buff->rxdata.page = NULL;
 		if (!buff->is_eop) {
 			if (aq_add_rx_fragment(dev, rx_ring, buff, &xdp)) {
 				u64_stats_update_begin(&rx_ring->stats.rx.syncp);
@@ -858,6 +815,7 @@ static int __aq_ring_xdp_clean(struct aq_ring_s *rx_ring,
 				rx_ring->stats.rx.bytes += xdp_get_buff_len(&xdp);
 				++rx_ring->stats.rx.xdp_aborted;
 				u64_stats_update_end(&rx_ring->stats.rx.syncp);
+				xdp_return_buff(&xdp);
 				continue;
 			}
 		}
@@ -969,7 +927,9 @@ void aq_ring_rx_deinit(struct aq_ring_s *self)
 		if (!buff->rxdata.page)
 			continue;
 
-		aq_free_rxpage(&buff->rxdata, aq_nic_get_dev(self->aq_nic));
+		page_pool_put_full_page(self->pg_pool, buff->rxdata.page,
+					false);
+		buff->rxdata.page = NULL;
 	}
 
 	self->sw_head = self->sw_tail;
@@ -983,6 +943,11 @@ void aq_ring_free(struct aq_ring_s *self)
 	kfree(self->buff_ring);
 	self->buff_ring = NULL;
 
+	if (self->pg_pool) {
+		page_pool_destroy(self->pg_pool);
+		self->pg_pool = NULL;
+	}
+
 	if (self->dx_ring) {
 		dma_free_coherent(aq_nic_get_dev(self->aq_nic),
 				  self->size * self->dx_size, self->dx_ring,
@@ -1021,9 +986,6 @@ unsigned int aq_ring_fill_stats_data(struct aq_ring_s *self, u64 *data)
 			data[++count] = self->stats.rx.alloc_fails;
 			data[++count] = self->stats.rx.skb_alloc_fails;
 			data[++count] = self->stats.rx.polls;
-			data[++count] = self->stats.rx.pg_flips;
-			data[++count] = self->stats.rx.pg_reuses;
-			data[++count] = self->stats.rx.pg_losts;
 			data[++count] = self->stats.rx.xdp_aborted;
 			data[++count] = self->stats.rx.xdp_drop;
 			data[++count] = self->stats.rx.xdp_pass;
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.h b/drivers/net/ethernet/aquantia/atlantic/aq_ring.h
index 6431cc62962f..58bcadb3e3cc 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.h
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.h
@@ -17,12 +17,12 @@
 #define AQ_XDP_TAILROOM		SKB_DATA_ALIGN(sizeof(struct skb_shared_info))
 
 struct page;
+struct page_pool;
 struct aq_nic_cfg_s;
 
 struct aq_rxpage {
 	struct page *page;
 	dma_addr_t daddr;
-	unsigned int order;
 	unsigned int pg_off;
 };
 
@@ -105,9 +105,6 @@ struct aq_ring_stats_rx_s {
 	u64 alloc_fails;
 	u64 skb_alloc_fails;
 	u64 polls;
-	u64 pg_losts;
-	u64 pg_flips;
-	u64 pg_reuses;
 	u64 xdp_aborted;
 	u64 xdp_drop;
 	u64 xdp_pass;
@@ -151,6 +148,7 @@ struct aq_ring_s {
 	u16 tail_size;
 	union aq_ring_stats_s stats;
 	dma_addr_t dx_ring_pa;
+	struct page_pool *pg_pool;
 	struct bpf_prog *xdp_prog;
 	enum atl_ring_type ring_type;
 	struct xdp_rxq_info xdp_rxq;
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_vec.c b/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
index 05814fea0f5f..8e15405d4941 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
@@ -146,25 +146,32 @@ int aq_vec_ring_alloc(struct aq_vec_s *self, struct aq_nic_s *aq_nic,
 		aq_nic_set_tx_ring(aq_nic, idx_ring, ring);
 
 		ring = &self->ring[i][AQ_VEC_RX_ID];
+		/* Registering the MEM_TYPE_PAGE_POOL memory model below needs
+		 * the page pool created by aq_ring_rx_alloc(), so the ring is
+		 * allocated first. If a registration fails, the ring has to be
+		 * freed explicitly: rx_rings is not incremented yet, so the
+		 * unwind through aq_vec_ring_free() would not cover it.
+		 */
+		err = aq_ring_rx_alloc(ring, aq_nic, idx_ring, aq_nic_cfg);
+		if (err)
+			goto err_exit;
+
 		if (xdp_rxq_info_reg(&ring->xdp_rxq,
 				     aq_nic->ndev, idx,
 				     self->napi.napi_id) < 0) {
+			aq_ring_free(ring);
 			err = -ENOMEM;
 			goto err_exit;
 		}
 		if (xdp_rxq_info_reg_mem_model(&ring->xdp_rxq,
-					       MEM_TYPE_PAGE_SHARED, NULL) < 0) {
+					       MEM_TYPE_PAGE_POOL,
+					       ring->pg_pool) < 0) {
 			xdp_rxq_info_unreg(&ring->xdp_rxq);
+			aq_ring_free(ring);
 			err = -ENOMEM;
 			goto err_exit;
 		}
 
-		err = aq_ring_rx_alloc(ring, aq_nic, idx_ring, aq_nic_cfg);
-		if (err) {
-			xdp_rxq_info_unreg(&ring->xdp_rxq);
-			goto err_exit;
-		}
-
 		++self->rx_rings;
 	}
 
-- 
2.47.3

RE: [EXTERNAL] [PATCH net-next v2 3/3] net: atlantic: convert RX path to page_pool

[Sukhdeep Soni [C]]

On 24 july 2026, Yangyu Chen wrote:

> The driver currently allocates RX buffers with dev_alloc_pages(), maps
> them with dma_map_page(), and uses a hand-rolled page-flip scheme to
> subdivide high-order pages. Behind an IOMMU, the map/unmap churn is a
> major RX cost: on a Thunderbolt-attached QNAP QNA-T310G1S, iperf3 -R
> over IPv6 tops out at about 2.2 Gbit/s over MTU 1500.
>
> Convert RX buffers to page_pool fragments. Pages are DMA-mapped once
> when entering the pool and recycled through the stack or XDP via the
> MEM_TYPE_PAGE_POOL memory model. This removes the custom page-flip
> accounting, lets page_pool handle fragment reuse, and ensures every RX
> path either keeps the ring's fragment reference for reposting or hands
> it to the skb/xdp_buff for later recycling.
>
> Register the PTP RX ring's xdp_rxq as well, since it shares the RX
> clean paths. Drop the ethtool PageFlips/PageReuses/PageFrees counters
> which only described the old scheme.
>
> On the QNA-T310G1S, MTU 1500, TCP over IPv6, iperf3 -R improves from
> 2.24 Gbit/s to 9.14 Gbit/s. The module was also smoke-tested with native
> XDP PASS, DROP, and ABORTED actions; carrier recovered after each
> attach/detach cycle and dmesg showed no page_pool/DMA warnings.
>
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Yangyu Chen <mailto:cyy@cyyself.name>
> ---
>
> Notes:
>    v1 -> v2:
>     - split the RX deinit gap fix into the previous patch and generate
>       this diff with --histogram so it reads as whole-function removals
>       plus an in-place aq_get_rxpages() rewrite (Mina)
>     - comment the ring-before-rxq-registration ordering and its error
>       unwind in aq_vec_ring_alloc()
>     - use the err_exit_xdp_rxq label instead of open-coding the
>       unregister in aq_ptp_ring_alloc()'s memory model error path (Mina)
>
> drivers/net/ethernet/aquantia/Kconfig         |   1 +
> .../ethernet/aquantia/atlantic/aq_ethtool.c   |   3 -
> .../net/ethernet/aquantia/atlantic/aq_ptp.c   |  18 +-
> .../net/ethernet/aquantia/atlantic/aq_ring.c  | 210 +++++++-----------
> .../net/ethernet/aquantia/atlantic/aq_ring.h  |   6 +-
> .../net/ethernet/aquantia/atlantic/aq_vec.c   |  21 +-
> 6 files changed, 120 insertions(+), 139 deletions(-)
>
> diff --git a/drivers/net/ethernet/aquantia/Kconfig b/drivers/net/ethernet/aquantia/Kconfig
> index cec2018c84a9..c8fb7b33e5b7 100644
> --- a/drivers/net/ethernet/aquantia/Kconfig
> +++ b/drivers/net/ethernet/aquantia/Kconfig
> @@ -20,6 +20,7 @@ config AQTION
> 	tristate "aQuantia AQtion(tm) Support"
> 	depends on PCI
> 	depends on MACSEC || MACSEC=n
> +	select PAGE_POOL
>  	help
>	  This enables the support for the aQuantia AQtion(tm) Ethernet card.
> 
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c b/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
> index 420af958d486..0f5125bd2315 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
> @@ -100,9 +100,6 @@ static const char * const aq_ethtool_queue_rx_stat_names[] = {
> 	"%sQueue[%d] AllocFails",
> 	"%sQueue[%d] SkbAllocFails",
> 	"%sQueue[%d] Polls",
> -	"%sQueue[%d] PageFlips",
> -	"%sQueue[%d] PageReuses",
> -	"%sQueue[%d] PageFrees",
> 	"%sQueue[%d] XdpAbort",
> 	"%sQueue[%d] XdpDrop",
> 	"%sQueue[%d] XdpPass",
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c b/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
> index 558ac9237f75..3a40d986cd67 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
> @@ -13,6 +13,7 @@
> #include <linux/ptp_classify.h>
> #include <linux/interrupt.h>
> #include <linux/clocksource.h>
> +#include <net/xdp.h>
> 
> #include "aq_nic.h"
> #include "aq_ptp.h"
> @@ -1192,12 +1193,23 @@ int aq_ptp_ring_alloc(struct aq_nic_s *aq_nic)
> 	if (err)
> 		goto err_exit_ptp_tx;
> 
> +	err = xdp_rxq_info_reg(&aq_ptp->ptp_rx.xdp_rxq, aq_nic->ndev,
> +			       rx_ring_idx, aq_ptp->napi.napi_id);
> +	if (err < 0)
> +		goto err_exit_ptp_rx;
> +
> +	err = xdp_rxq_info_reg_mem_model(&aq_ptp->ptp_rx.xdp_rxq,
> +					 MEM_TYPE_PAGE_POOL,
> +					 aq_ptp->ptp_rx.pg_pool);
> +	if (err < 0)
> +		goto err_exit_xdp_rxq;
> +
> 	if (aq_ptp->a1_ptp) {
> 		err = aq_ring_hwts_rx_alloc(&aq_ptp->hwts_rx, aq_nic, PTP_HWST_RING_IDX,
> 					    aq_nic->aq_nic_cfg.rxds,
> 					    aq_nic->aq_nic_cfg.aq_hw_caps->rxd_size);
> 		if (err)
> -			goto err_exit_ptp_rx;
> +			goto err_exit_xdp_rxq;
> 	}
> 
> 	err = aq_ptp_skb_ring_init(&aq_ptp->skb_ring, aq_nic->aq_nic_cfg.rxds);
> @@ -1217,6 +1229,8 @@ int aq_ptp_ring_alloc(struct aq_nic_s *aq_nic)
> err_exit_hwts_rx:
> 	if (aq_ptp->a1_ptp)
> 		aq_ring_hwts_rx_free(&aq_ptp->hwts_rx);
> +err_exit_xdp_rxq:
> +	xdp_rxq_info_unreg(&aq_ptp->ptp_rx.xdp_rxq);
> err_exit_ptp_rx:
> 	aq_ring_free(&aq_ptp->ptp_rx);
> err_exit_ptp_tx:
> @@ -1233,6 +1247,8 @@ void aq_ptp_ring_free(struct aq_nic_s *aq_nic)
> 		return;
>  
> 	aq_ring_free(&aq_ptp->ptp_tx);
> +	if (xdp_rxq_info_is_reg(&aq_ptp->ptp_rx.xdp_rxq))
> +		xdp_rxq_info_unreg(&aq_ptp->ptp_rx.xdp_rxq);
> 	aq_ring_free(&aq_ptp->ptp_rx);
> 	if (aq_ptp->a1_ptp)
> 		aq_ring_hwts_rx_free(&aq_ptp->hwts_rx);
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> index e1193c6719d9..9dd881710594 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> @@ -14,120 +14,37 @@
> #include "aq_vec.h"
> #include "aq_main.h"
> 
> +#include <net/page_pool/helpers.h>
> #include <net/xdp.h>
> #include <linux/filter.h>
> #include <linux/bpf_trace.h>
> #include <linux/netdevice.h>
> #include <linux/etherdevice.h>
> 
> -static void aq_get_rxpages_xdp(struct aq_ring_buff_s *buff,
> -			       struct xdp_buff *xdp)
> -{
> -	struct skb_shared_info *sinfo;
> -	int i;
> -
> -	if (xdp_buff_has_frags(xdp)) {
> -		sinfo = xdp_get_shared_info_from_buff(xdp);
> -
> -		for (i = 0; i < sinfo->nr_frags; i++) {
> -			skb_frag_t *frag = &sinfo->frags[i];
> -
> -			page_ref_inc(skb_frag_page(frag));
> -		}
> -	}
> -	page_ref_inc(buff->rxdata.page);
> -}
> -
> -static inline void aq_free_rxpage(struct aq_rxpage *rxpage, struct device *dev)
> -{
> -	unsigned int len = PAGE_SIZE << rxpage->order;
> -
> -	dma_unmap_page(dev, rxpage->daddr, len, DMA_FROM_DEVICE);
> -
> -	/* Drop the ref for being in the ring. */
> -	__free_pages(rxpage->page, rxpage->order);
> -	rxpage->page = NULL;
> -}
> -
> -static int aq_alloc_rxpages(struct aq_rxpage *rxpage, struct aq_ring_s *rx_ring)
> -{
> -	struct device *dev = aq_nic_get_dev(rx_ring->aq_nic);
> -	unsigned int order = rx_ring->page_order;
> -	struct page *page;
> -	int ret = -ENOMEM;
> -	dma_addr_t daddr;
> -
> -	page = dev_alloc_pages(order);
> -	if (unlikely(!page))
> -		goto err_exit;
> -
> -	daddr = dma_map_page(dev, page, 0, PAGE_SIZE << order,
> -			     DMA_FROM_DEVICE);
> -
> -	if (unlikely(dma_mapping_error(dev, daddr)))
> -		goto free_page;
> -
> -	rxpage->page = page;
> -	rxpage->daddr = daddr;
> -	rxpage->order = order;
> -	rxpage->pg_off = rx_ring->page_offset;
> -
> -	return 0;
> -
> -free_page:
> -	__free_pages(page, order);
> -
> -err_exit:
> -	return ret;
> -}
> -
> static int aq_get_rxpages(struct aq_ring_s *self, struct aq_ring_buff_s *rxbuf)
> {
> -	unsigned int order = self->page_order;
> -	u16 page_offset = self->page_offset;
> -	u16 frame_max = self->frame_max;
> -	u16 tail_size = self->tail_size;
> -	int ret;
> +	unsigned int size = self->page_offset + self->frame_max +
> +			    self->tail_size;
> +	unsigned int pg_off;
> +	struct page *page;
> 
> -	if (rxbuf->rxdata.page) {
> -		/* One means ring is the only user and can reuse */
> -		if (page_ref_count(rxbuf->rxdata.page) > 1) {
> -			/* Try reuse buffer */
> -			rxbuf->rxdata.pg_off += frame_max + page_offset +
> -						tail_size;
> -			if (rxbuf->rxdata.pg_off + frame_max + tail_size <=
> -			    (PAGE_SIZE << order)) {
> -				u64_stats_update_begin(&self->stats.rx.syncp);
> -				self->stats.rx.pg_flips++;
> -				u64_stats_update_end(&self->stats.rx.syncp);
> +	/* Buffers whose page was not passed up the stack are reposted
> +	 * with the data they already carry discarded.
> +	 */
> +	if (rxbuf->rxdata.page)
> +		return 0;
> 
> -			} else {
> -				/* Buffer exhausted. We have other users and
> -				 * should release this page and realloc
> -				 */
> -				aq_free_rxpage(&rxbuf->rxdata,
> -					       aq_nic_get_dev(self->aq_nic));
> -				u64_stats_update_begin(&self->stats.rx.syncp);
> -				self->stats.rx.pg_losts++;
> -				u64_stats_update_end(&self->stats.rx.syncp);
> -			}
> -		} else {
> -			rxbuf->rxdata.pg_off = page_offset;
> -			u64_stats_update_begin(&self->stats.rx.syncp);
> -			self->stats.rx.pg_reuses++;
> -			u64_stats_update_end(&self->stats.rx.syncp);
> -		}
> +	page = page_pool_dev_alloc_frag(self->pg_pool, &pg_off, size);
> +	if (unlikely(!page)) {
> +		u64_stats_update_begin(&self->stats.rx.syncp);
> +		self->stats.rx.alloc_fails++;
> +		u64_stats_update_end(&self->stats.rx.syncp);
> +		return -ENOMEM;
> 	}
> 
> -	if (!rxbuf->rxdata.page) {
> -		ret = aq_alloc_rxpages(&rxbuf->rxdata, self);
> -		if (ret) {
> -			u64_stats_update_begin(&self->stats.rx.syncp);
> -			self->stats.rx.alloc_fails++;
> -			u64_stats_update_end(&self->stats.rx.syncp);
> -		}
> -		return ret;
> -	}
> +	rxbuf->rxdata.page = page;
> +	rxbuf->rxdata.daddr = page_pool_get_dma_addr(page);
> +	rxbuf->rxdata.pg_off = pg_off + self->page_offset;
>  
> 	return 0;
>  }
> @@ -179,6 +96,15 @@ int aq_ring_rx_alloc(struct aq_ring_s *self,
> 		     unsigned int idx,
> 		     struct aq_nic_cfg_s *aq_nic_cfg)
> {
> +	struct page_pool_params pp_params = {
> +		.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
> +		.pool_size = aq_nic_cfg->rxds,
> +		.nid = NUMA_NO_NODE,
> +		.dev = aq_nic_get_dev(aq_nic),
> +		.dma_dir = DMA_FROM_DEVICE,
> +	};
> +	struct page_pool *pool;
> +
>  	self->aq_nic = aq_nic;
>  	self->idx = idx;
> 	self->size = aq_nic_cfg->rxds;
> @@ -200,6 +126,18 @@ int aq_ring_rx_alloc(struct aq_ring_s *self,
> 		self->tail_size = 0;
> 	}
> 
> +	pp_params.order = self->page_order;
> +	pp_params.max_len = PAGE_SIZE << self->page_order;
> +
> +	pool = page_pool_create(&pp_params);
> +	if (IS_ERR(pool))
> +		return PTR_ERR(pool);
> +
> +	self->pg_pool = pool;
> +
> +	/* On failure aq_ring_alloc() calls aq_ring_free(), which also
> +	 * destroys the page pool.
> +	 */
> 	return aq_ring_alloc(self, aq_nic);
> }
> 
> @@ -346,7 +284,11 @@ bool aq_ring_tx_clean(struct aq_ring_s *self)
> 			++self->stats.tx.packets;
> 			self->stats.tx.bytes += xdp_get_frame_len(buff->xdpf);
> 			u64_stats_update_end(&self->stats.tx.syncp);
> -			xdp_return_frame_rx_napi(buff->xdpf);
> +			/* Frames queued via ndo_xdp_xmit() may come from a
> +			 * page pool owned by another NAPI context: no direct
> +			 * recycling.
> +			 */
> +			xdp_return_frame(buff->xdpf);
> 		}
> 
> out:
> @@ -437,22 +379,15 @@ int aq_xdp_xmit(struct net_device *dev, int num_frames,
> }
> 
> static struct sk_buff *aq_xdp_build_skb(struct xdp_buff *xdp,
> -					struct net_device *dev,
> -					struct aq_ring_buff_s *buff)
> +					struct net_device *dev)
> {
> 	struct xdp_frame *xdpf;
> -	struct sk_buff *skb;
> 
> 	xdpf = xdp_convert_buff_to_frame(xdp);
> 	if (unlikely(!xdpf))
> 		return NULL;
> 
> -	skb = xdp_build_skb_from_frame(xdpf, dev);
> -	if (!skb)
> -		return NULL;
> -
> -	aq_get_rxpages_xdp(buff, xdp);
> -	return skb;
> +	return xdp_build_skb_from_frame(xdpf, dev);
> }
> 
> static struct sk_buff *aq_xdp_run_prog(struct aq_nic_s *aq_nic,
> @@ -473,8 +408,16 @@ static struct sk_buff *aq_xdp_run_prog(struct aq_nic_s *aq_nic,
> 	u64_stats_update_end(&rx_ring->stats.rx.syncp);
> 
> 	prog = READ_ONCE(rx_ring->xdp_prog);
> -	if (!prog)
> -		return aq_xdp_build_skb(xdp, aq_nic->ndev, buff);
> +	if (!prog) {
> +		skb = aq_xdp_build_skb(xdp, aq_nic->ndev);
> +		/* The ring has already handed its page pool reference to the
> +		 * xdp_buff, so if the skb could not be built the buffer must
> +		 * be returned to the pool here or its fragments would leak.
> +		 */
> +		if (!skb)
> +			xdp_return_buff(xdp);
> +		return skb;
> +	}
> 
> 	prefetchw(xdp->data_hard_start); /* xdp_frame write */
> 
> @@ -485,7 +428,7 @@ static struct sk_buff *aq_xdp_run_prog(struct aq_nic_s *aq_nic,
> 	act = bpf_prog_run_xdp(prog, xdp);
> 	switch (act) {
> 	case XDP_PASS:
> -		skb = aq_xdp_build_skb(xdp, aq_nic->ndev, buff);
> +		skb = aq_xdp_build_skb(xdp, aq_nic->ndev);
> 		if (!skb)
> 			goto out_aborted;
> 		u64_stats_update_begin(&rx_ring->stats.rx.syncp);
> @@ -503,7 +446,6 @@ static struct sk_buff *aq_xdp_run_prog(struct aq_nic_s *aq_nic,
> 		u64_stats_update_begin(&rx_ring->stats.rx.syncp);
> 		++rx_ring->stats.rx.xdp_tx;
> 		u64_stats_update_end(&rx_ring->stats.rx.syncp);
> -		aq_get_rxpages_xdp(buff, xdp);
> 		break;
> 	case XDP_REDIRECT:
> 		if (xdp_do_redirect(aq_nic->ndev, xdp, prog) < 0)
> @@ -512,7 +454,6 @@ static struct sk_buff *aq_xdp_run_prog(struct aq_nic_s *aq_nic,
> 		u64_stats_update_begin(&rx_ring->stats.rx.syncp);
> 		++rx_ring->stats.rx.xdp_redirect;
> 		u64_stats_update_end(&rx_ring->stats.rx.syncp);
> -		aq_get_rxpages_xdp(buff, xdp);
> 		break;
> 	default:
> 		fallthrough;
> @@ -523,11 +464,13 @@ static struct sk_buff *aq_xdp_run_prog(struct aq_nic_s *aq_nic,
> 		u64_stats_update_end(&rx_ring->stats.rx.syncp);
> 		trace_xdp_exception(aq_nic->ndev, prog, act);
> 		bpf_warn_invalid_xdp_action(aq_nic->ndev, prog, act);
> +		xdp_return_buff(xdp);
>  		break;
> 	case XDP_DROP:
> 		u64_stats_update_begin(&rx_ring->stats.rx.syncp);
> 		++rx_ring->stats.rx.xdp_drop;
> 		u64_stats_update_end(&rx_ring->stats.rx.syncp);
> +		xdp_return_buff(xdp);
> 		break;
> 	}
> 
> @@ -546,8 +489,11 @@ static bool aq_add_rx_fragment(struct device *dev,
> 	do {
> 		skb_frag_t *frag;
> 
> -		if (unlikely(sinfo->nr_frags >= MAX_SKB_FRAGS))
> +		if (unlikely(sinfo->nr_frags >= MAX_SKB_FRAGS)) {
> +			/* Attached frags must reach xdp_return_buff() */
> +			xdp_buff_set_frags_flag(xdp);
> 			return true;
> +		}
> 
> 		frag = &sinfo->frags[sinfo->nr_frags++];
> 		buff_ = &ring->buff_ring[buff_->next];
> @@ -571,6 +517,11 @@ static bool aq_add_rx_fragment(struct device *dev,
> 		if (page_is_pfmemalloc(buff_->rxdata.page))
> 			xdp_buff_set_frag_pfmemalloc(xdp);
> 
> +		/* The frag's page pool reference is owned by the xdp_buff
> +		 * from now on.
> +		 */
> +		buff_->rxdata.page = NULL;
> +
> 	} while (!buff_->is_eop);
>  
> 	xdp_buff_set_frags_flag(xdp);
> @@ -674,6 +625,7 @@ static int __aq_ring_rx_clean(struct aq_ring_s *self, struct napi_struct *napi,
> 			err = -ENOMEM;
> 			goto err_exit;
> 		}
> +		skb_mark_for_recycle(skb);
> 		if (is_ptp_ring)
> 			buff->len -=
> 				aq_ptp_extract_ts(self->aq_nic, skb_hwtstamps(skb),
> @@ -694,7 +646,7 @@ static int __aq_ring_rx_clean(struct aq_ring_s *self, struct napi_struct *napi,
> 					buff->rxdata.pg_off + hdr_len,
> 					buff->len - hdr_len,
> 					self->frame_max);
> -			page_ref_inc(buff->rxdata.page);
> +			buff->rxdata.page = NULL;
> 		}
> 
> 		if (!buff->is_eop) {
> @@ -713,7 +665,7 @@ static int __aq_ring_rx_clean(struct aq_ring_s *self, struct napi_struct *napi,
> 						buff_->rxdata.pg_off,
> 						buff_->len,
> 						self->frame_max);
> -				page_ref_inc(buff_->rxdata.page);
> +				buff_->rxdata.page = NULL;
> 				buff_->is_cleaned = 1;
> 
> 				buff->is_ip_cso &= buff_->is_ip_cso;
> @@ -851,6 +803,11 @@ static int __aq_ring_xdp_clean(struct aq_ring_s *rx_ring,
> 		xdp_init_buff(&xdp, frame_sz, &rx_ring->xdp_rxq);
> 		xdp_prepare_buff(&xdp, hard_start, rx_ring->page_offset,
> 				 buff->len, false);
> +		/* The xdp_buff owns the buffer's page pool reference from
> +		 * here on; it comes back through the MEM_TYPE_PAGE_POOL
> +		 * memory model on every XDP verdict.
> +		 */
> +		buff->rxdata.page = NULL;
> 		if (!buff->is_eop) {
> 			if (aq_add_rx_fragment(dev, rx_ring, buff, &xdp)) {
> 				u64_stats_update_begin(&rx_ring->stats.rx.syncp);
> @@ -858,6 +815,7 @@ static int __aq_ring_xdp_clean(struct aq_ring_s *rx_ring,
> 				rx_ring->stats.rx.bytes += xdp_get_buff_len(&xdp);
> 				++rx_ring->stats.rx.xdp_aborted;
> 				u64_stats_update_end(&rx_ring->stats.rx.syncp);
> +				xdp_return_buff(&xdp);
> 				continue;
> 			}
> 		}
> @@ -969,7 +927,9 @@ void aq_ring_rx_deinit(struct aq_ring_s *self)
> 		if (!buff->rxdata.page)
> 			continue;
> 
> -		aq_free_rxpage(&buff->rxdata, aq_nic_get_dev(self->aq_nic));
> +		page_pool_put_full_page(self->pg_pool, buff->rxdata.page,
> +					false);
> +		buff->rxdata.page = NULL;
>  	}
> 
> 	self->sw_head = self->sw_tail;
> @@ -983,6 +943,11 @@ void aq_ring_free(struct aq_ring_s *self)
> 	kfree(self->buff_ring);
> 	self->buff_ring = NULL;
> 
> +	if (self->pg_pool) {
> +		page_pool_destroy(self->pg_pool);
> +		self->pg_pool = NULL;
> +	}
> +
> 	if (self->dx_ring) {
> 		dma_free_coherent(aq_nic_get_dev(self->aq_nic),
> 				  self->size * self->dx_size, self->dx_ring,
> @@ -1021,9 +986,6 @@ unsigned int aq_ring_fill_stats_data(struct aq_ring_s *self, u64 *data)
> 			data[++count] = self->stats.rx.alloc_fails;
> 			data[++count] = self->stats.rx.skb_alloc_fails;
> 			data[++count] = self->stats.rx.polls;
> -			data[++count] = self->stats.rx.pg_flips;
> -			data[++count] = self->stats.rx.pg_reuses;
> -			data[++count] = self->stats.rx.pg_losts;
> 			data[++count] = self->stats.rx.xdp_aborted;
> 			data[++count] = self->stats.rx.xdp_drop;
> 			data[++count] = self->stats.rx.xdp_pass;
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.h b/drivers/net/ethernet/aquantia/atlantic/aq_ring.h
> index 6431cc62962f..58bcadb3e3cc 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.h
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.h
> @@ -17,12 +17,12 @@
> #define AQ_XDP_TAILROOM		SKB_DATA_ALIGN(sizeof(struct skb_shared_info))
> 
> struct page;
> +struct page_pool;
> struct aq_nic_cfg_s;
> 
> struct aq_rxpage {
> 	struct page *page;
> 	dma_addr_t daddr;
> -	unsigned int order;
> 	unsigned int pg_off;
> };
> 
> @@ -105,9 +105,6 @@ struct aq_ring_stats_rx_s {
> 	u64 alloc_fails;
> 	u64 skb_alloc_fails;
> 	u64 polls;
> -	u64 pg_losts;
> -	u64 pg_flips;
> -	u64 pg_reuses;
> 	u64 xdp_aborted;
> 	u64 xdp_drop;
> 	u64 xdp_pass;
> @@ -151,6 +148,7 @@ struct aq_ring_s {
> 	u16 tail_size;
> 	union aq_ring_stats_s stats;
> 	dma_addr_t dx_ring_pa;
> +	struct page_pool *pg_pool;
> 	struct bpf_prog *xdp_prog;
> 	enum atl_ring_type ring_type;
> 	struct xdp_rxq_info xdp_rxq;
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_vec.c b/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
> index 05814fea0f5f..8e15405d4941 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
> @@ -146,25 +146,32 @@ int aq_vec_ring_alloc(struct aq_vec_s *self, struct aq_nic_s *aq_nic,
> 		aq_nic_set_tx_ring(aq_nic, idx_ring, ring);
> 
> 		ring = &self->ring[i][AQ_VEC_RX_ID];
> +		/* Registering the MEM_TYPE_PAGE_POOL memory model below needs
> +		 * the page pool created by aq_ring_rx_alloc(), so the ring is
> +		 * allocated first. If a registration fails, the ring has to be
> +		 * freed explicitly: rx_rings is not incremented yet, so the
> +		 * unwind through aq_vec_ring_free() would not cover it.
> +		 */
> +		err = aq_ring_rx_alloc(ring, aq_nic, idx_ring, aq_nic_cfg);
> +		if (err)
> +			goto err_exit;
> +
> 		if (xdp_rxq_info_reg(&ring->xdp_rxq,
> 				     aq_nic->ndev, idx,
> 				     self->napi.napi_id) < 0) {
> +			aq_ring_free(ring);
> 			err = -ENOMEM;
> 			goto err_exit;
> 		}
> 		if (xdp_rxq_info_reg_mem_model(&ring->xdp_rxq,
> -					       MEM_TYPE_PAGE_SHARED, NULL) < 0) {
> +					       MEM_TYPE_PAGE_POOL,
> +					       ring->pg_pool) < 0) {
> 			xdp_rxq_info_unreg(&ring->xdp_rxq);
> +			aq_ring_free(ring);
> 			err = -ENOMEM;
> 			goto err_exit;
> 		}
> 
> -		err = aq_ring_rx_alloc(ring, aq_nic, idx_ring, aq_nic_cfg);
> -		if (err) {
> -			xdp_rxq_info_unreg(&ring->xdp_rxq);
> -			goto err_exit;
> -		}
> -
> 		++self->rx_rings;
> 	}
> 
> -- 
> 2.47.3

Reviewed the full series (patches 1-3) - looks good.
Not creating any issues with the newly added PTP feature for AQC113(Tested with PTP).

Reviewed-by: Sukhdeep Singh <sukhdeeps@marvell.com>

Re: [PATCH net-next v2 3/3] net: atlantic: convert RX path to page_pool

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Encountering an RX packet with more than MAX_SKB_FRAGS fragments causes an infinite NAPI polling loop.
- [High] In the XDP path, if a packet exceeds MAX_SKB_FRAGS, the remainder of its fragments are erroneously parsed as independent new packets.
- [High] NULL pointer dereference when an XDP program returns XDP_TX for a packet received on the PTP ring.
- [High] Incorrect reporting of dropped XDP frames in `ndo_xdp_xmit` causes dropped frames to be leaked and successfully transmitted frames to be double-freed.
--

--- Patch [3]: [PATCH net-next v2 3/3] net: atlantic: convert RX path to page_pool ---
commit 18978fa8fbab5efde73aa39b92ef15e773290438
Author: Yangyu Chen <cyy@cyyself.name>

net: atlantic: convert RX path to page_pool

The driver currently allocates RX buffers with dev_alloc_pages(), maps
them with dma_map_page(), and uses a hand-rolled page-flip scheme to
subdivide high-order pages. Behind an IOMMU, the map/unmap churn is a
major RX cost. This patch converts RX buffers to page_pool fragments.

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> index e1193c6719d9d..9dd8817105942 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c

[ ... ]

> @@ -437,22 +379,15 @@ int aq_xdp_xmit(struct net_device *dev, int num_frames,
>  }

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for
aq_xdp_xmit() to mislead the caller into freeing the wrong frames?

When a transmission fails, the loop increments drop but does not break
or reorganize the frames array:

        for (i = 0; i < num_frames; i++) {
                struct xdp_frame *xdpf = frames[i];

                if (aq_nic_xmit_xdpf(aq_nic, ring, xdpf) == NETDEV_TX_BUSY)
                        drop++;
        }

        return num_frames - drop;

If the failure occurs in the middle of the array, the kernel's XDP stack will
assume the last drop frames failed and free them. Could this result in a
use-after-free of successfully transmitted frames while they are still being
accessed by hardware, and leak the actual dropped frames?

>  static struct sk_buff *aq_xdp_build_skb(struct xdp_buff *xdp,

[ ... ]

> @@ -503,7 +446,6 @@ static struct sk_buff *aq_xdp_run_prog(struct aq_nic_s *aq_nic,
>  		u64_stats_update_begin(&rx_ring->stats.rx.syncp);
>  		++rx_ring->stats.rx.xdp_tx;
>  		u64_stats_update_end(&rx_ring->stats.rx.syncp);
> -		aq_get_rxpages_xdp(buff, xdp);
>  		break;

[Severity: High]
This is a pre-existing issue, but could this dereference a NULL pointer
if an XDP program returns XDP_TX for a packet on the PTP ring?

The PTP RX ring shares the RX clean path, meaning __aq_ring_xdp_clean() and
aq_xdp_run_prog() are executed for it. If an XDP program returns XDP_TX:

        tx_ring = aq_nic->aq_ring_tx[rx_ring->idx];
        result = aq_nic_xmit_xdpf(aq_nic, tx_ring, xdpf);

Since the PTP TX ring is never placed into the aq_ring_tx array during
allocation, tx_ring would be NULL here.

>  	case XDP_REDIRECT:

[ ... ]

> @@ -546,8 +489,11 @@ static bool aq_add_rx_fragment(struct device *dev,
>  	do {
>  		skb_frag_t *frag;
>  
> -		if (unlikely(sinfo->nr_frags >= MAX_SKB_FRAGS))
> +		if (unlikely(sinfo->nr_frags >= MAX_SKB_FRAGS)) {
> +			/* Attached frags must reach xdp_return_buff() */
> +			xdp_buff_set_frags_flag(xdp);
>  			return true;
> +		}

[Severity: High]
This isn't a bug introduced by this patch, but does the early return in
aq_add_rx_fragment() leave remaining descriptors uncleaned?

If a packet exceeds MAX_SKB_FRAGS, it seems aq_add_rx_fragment() aborts
and returns true without setting buff_->is_cleaned = 1 on the rest of the
fragments.

Then, in __aq_ring_xdp_clean(), the outer loop continues, increments sw_head,
and encounters those uncleaned fragments:

                if (!buff->is_eop) {
                        if (aq_add_rx_fragment(dev, rx_ring, buff, &xdp)) {
                                ...
                                xdp_return_buff(&xdp);
                                continue;
                        }
                }

Since their is_cleaned flag is false, could they be treated as the start of a
brand new packet and erroneously passed to the XDP program?

>  		frag = &sinfo->frags[sinfo->nr_frags++];

[ ... ]

> @@ -674,6 +625,7 @@ static int __aq_ring_rx_clean(struct aq_ring_s *self, struct napi_struct *napi,
>  			err = -ENOMEM;
>  			goto err_exit;
>  		}
> +		skb_mark_for_recycle(skb);

[Severity: High]
This is a pre-existing issue, but I have a question about the error handling
in __aq_ring_rx_clean().

If hardware aggregates a packet resulting in more than MAX_SKB_FRAGS fragments
(for example, via LRO), the code takes this error path before getting here:

        if (unlikely(!is_rsc_completed) ||
            frag_cnt > MAX_SKB_FRAGS) {
                err = 0;
                goto err_exit;
        }

Does this exit the function without advancing sw_head or consuming the
malformed multi-fragment packet? On the next NAPI poll, it seems the exact
same state would be encountered, permanently stalling the queue in an infinite
loop.

>  		if (is_ptp_ring)
>  			buff->len -=

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_1F173E0FC1606D2AC704DC9C98AF10984607@qq.com?part=3