[PATCH net 0/2] net: atlantic: fix two ring teardown leaks

[PATCH net 0/2] net: atlantic: fix two ring teardown leaks

[Yangyu Chen]

These are the two fixes from the page_pool conversion series [1],
resent against net as requested in the review of that series. The
page_pool conversion itself stays in net-next and is not part of this
posting; it depends on these fixes, but they stand on their own.

Both patches are unchanged from [1] apart from the collected
Reviewed-by tags, and each carries a Fixes tag and a Cc: stable with
the affected range (patch 1: v4.11+, patch 2: v5.2+). They apply and
were build- and runtime-tested independently of each other and of the
conversion.

Patch 1: aq_vec_deinit() drains the TX rings with a single
aq_ring_tx_clean() call, which is capped at AQ_CFG_TX_CLEAN_BUDGET
descriptors and stops at hw_head, frozen once the hardware and NAPI
have been stopped. Everything beyond that keeps its skb or xdp_frame
when the interface goes down and is lost when the buffer ring is
freed.

Patch 2: aq_ring_rx_deinit() only walks [sw_head, sw_tail). Since the
page reuse strategy was added, a cleaned RX buffer keeps its page for
reuse and refill is batched, so consumed but not yet reposted slots
accumulate in the [sw_tail, sw_head) gap and their pages and DMA
mappings are never released.

Reproduction logs for both leaks (as page_pool stalled shutdowns,
which is how they become visible) are in the notes of the respective
patches.

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

Yangyu Chen (2):
  net: atlantic: free stranded TX buffers on ring deinit
  net: atlantic: free RX pages of consumed but not refilled buffers

 .../net/ethernet/aquantia/atlantic/aq_ring.c  | 51 +++++++++++++++++--
 .../net/ethernet/aquantia/atlantic/aq_ring.h  |  1 +
 .../net/ethernet/aquantia/atlantic/aq_vec.c   |  2 +-
 3 files changed, 49 insertions(+), 5 deletions(-)


base-commit: af39eb111ce6b5eba9c08513b62c4868eb7e7fd5
-- 
2.47.3

[PATCH net 1/2] 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 conversion of the RX path to page_pool posted
for net-next 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+
Reviewed-by: Sukhdeep Singh <sukhdeeps@marvell.com>
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 (posted
    separately for net-next) 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.

 .../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]);
 	}
 

base-commit: af39eb111ce6b5eba9c08513b62c4868eb7e7fd5
-- 
2.47.3

[PATCH net 2/2] 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+
Reviewed-by: Sukhdeep Singh <sukhdeeps@marvell.com>
Assisted-by: Claude:claude-fable-5
Signed-off-by: Yangyu Chen <cyy@cyyself.name>
---

Notes:
    Without this fix, the page_pool conversion posted for net-next 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
    page_pool conversion, against the current page reuse scheme.

 .../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: [PATCH net 1/2] net: atlantic: free stranded TX buffers on ring deinit

[sashiko-bot]

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

Pre-existing issues:
- [Critical] `aq_xdp_xmit()` does not break on transmission failures or compact the frames array, leading to double-frees and memory leaks.
- [High] `aq_ptp_ring_deinit()` uses `aq_ring_tx_clean()` instead of `aq_ring_tx_deinit()`, leaving the PTP TX ring vulnerable to the same stranded buffer memory leak.
--

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

This commit brings back aq_ring_tx_deinit() to replace aq_ring_tx_clean()
during TX ring deinitialization. This ensures that completed descriptors
beyond the clean budget and frames stuck between hw_head and sw_tail are
properly freed, resolving a memory leak when an interface goes down.

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> index 8ff07de2bd524..81685a4dc5a6d 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);
> +		}
> +	}
> +}

[Severity: Critical]
This is a pre-existing issue, but while we are looking at XDP frame handling
in this file, does aq_xdp_xmit() handle transmission failures correctly?

In aq_xdp_xmit(), if aq_nic_xmit_xdpf() fails, it just increments a drop
counter and continues:

drivers/net/ethernet/aquantia/atlantic/aq_ring.c:aq_xdp_xmit() {
    ...
	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;
}

Callers of ndo_xdp_xmit (like bq_xmit_all() in kernel/bpf/devmap.c) assume a
return value of N means the first N contiguous frames in the array were
successfully transmitted, and they free the remaining frames starting at
index N.

If aq_xdp_xmit() processes 3 frames, fails on the first, and succeeds on the
next two, it returns 2.

Will the caller then free frames[2] which was successfully queued to the NIC,
causing a double-free, while leaking the failed frames[0]?

> 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]);

[Severity: High]
This isn't a bug introduced by this patch, but does the PTP TX ring need the
same fix?

Looking at aq_ptp_ring_deinit(), the PTP TX ring is still deinitialized
using aq_ring_tx_clean():

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);
}

If the interface is brought down while PTP traffic is in flight, will this
lead to the exact same memory leak of SKBs and DMA mappings for the PTP ring
when it is subsequently freed?

>  		aq_ring_rx_deinit(&ring[AQ_VEC_RX_ID]);
>  	}

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

Re: [PATCH net 2/2] 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:
- [High] Malformed or oversized RX descriptor chains cause a permanent NAPI polling lockup or hard infinite loop.
--

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

net: atlantic: free RX pages of consumed but not refilled buffers

aq_ring_rx_deinit() only walks the region posted to hardware. Slots that
were consumed but not yet reposted sit in a gap with a live page, and the
deinit walk never visits them, causing a page and DMA mapping leak. This
patch walks the whole ring instead and releases whatever is still there.

Cc: stable@vger.kernel.org # v5.2+

> 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

[ ... ]

> @@ -950,15 +950,29 @@ int aq_ring_rx_fill(struct aq_ring_s *self)
>  
>  void aq_ring_rx_deinit(struct aq_ring_s *self)
>  {

[ ... ]

[Severity: High]
This is a pre-existing issue, but does the error handling in
__aq_ring_rx_clean() and __aq_ring_xdp_clean() cause a permanent NAPI polling
lockup or an infinite loop when processing malformed RX descriptors?

In __aq_ring_rx_clean(), if a descriptor chain is out of bounds or exceeds the 
maximum fragment count, the function exits early without advancing sw_head:

drivers/net/ethernet/aquantia/atlantic/aq_ring.c:__aq_ring_rx_clean() {
    ...
                if (buff_->next >= self->size) {
                    err = -EIO;
                    goto err_exit;
                }
    ...
                if (unlikely(!is_rsc_completed) ||
                    frag_cnt > MAX_SKB_FRAGS) {
                    err = 0;
                    goto err_exit;
                }
    ...
}

Because sw_head is not advanced, wouldn't the next NAPI poll process the
exact same malformed descriptor again, resulting in an infinite NAPI polling
loop?

Additionally, in __aq_ring_xdp_clean(), there is a do-while loop that lacks a
fragment count limit:

drivers/net/ethernet/aquantia/atlantic/aq_ring.c:__aq_ring_xdp_clean() {
    ...
            do {
                if (buff_->next >= rx_ring->size) {
                    err = -EIO;
                    goto err_exit;
                }
                next_ = buff_->next;
                buff_ = &rx_ring->buff_ring[next_];
                is_rsc_completed =
                    aq_ring_dx_in_range(rx_ring->sw_head,
                                next_,
                                rx_ring->hw_head);

                if (unlikely(!is_rsc_completed))
                    break;

                buff->is_error |= buff_->is_error;
                buff->is_cso_err |= buff_->is_cso_err;
            } while (!buff_->is_eop);
    ...
}

If the descriptor chain is cyclic and within the valid range, could this
cause a hard infinite loop inside the softirq handler?

Also, if the index is out of bounds in __aq_ring_xdp_clean(), it jumps to
err_exit without advancing sw_head. Could this cause the same NAPI poll storm
as in __aq_ring_rx_clean()?

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

Re: [PATCH net 1/2] net: atlantic: free stranded TX buffers on ring deinit

[Mina Almasry]

On Sun, Aug 2, 2026 at 8:46 AM Yangyu Chen <cyy@cyyself.name> 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 conversion of the RX path to page_pool posted
> for net-next 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+
> Reviewed-by: Sukhdeep Singh <sukhdeeps@marvell.com>
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Yangyu Chen <cyy@cyyself.name>

FWIW,

Acked-by: Mina Almasry <almasrymina@google.com>

I've noticed these claude-generated commit messages are unusually
large compared to the human written ones. In the future try reviewing
the commit msg manually and making sure the amount of detail makes
sense.

You can put extra details under the `---`

-- 
Thanks,
Mina

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

[Mina Almasry]

On Sun, Aug 2, 2026 at 8:46 AM Yangyu Chen <cyy@cyyself.name> 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: stable@vger.kernel.org # v5.2+
> Reviewed-by: Sukhdeep Singh <sukhdeeps@marvell.com>
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Yangyu Chen <cyy@cyyself.name>


FWIW,

Acked-by: Mina Almasry <almasrymina@google.com>

> ---
>
> Notes:
>     Without this fix, the page_pool conversion posted for net-next 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
>     page_pool conversion, against the current page reuse scheme.
>
>  .../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.
> +        */

I don't like these AI-generated comments FWIW. It's describing the old
code, which is now gone. No future reader is going to wonder why we're
not only walking sw_head -> sw_tail, I think. Do consider deleting it
if you submit another iteration.

-- 
Thanks,
Mina