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

[Mina Almasry]

On Thu, Jul 9, 2026 at 10:41 AM Yangyu Chen <cyy@xxxxxxxxxxxx> 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")
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Yangyu Chen <cyy@xxxxxxxxxxxx>

Looks fine as far as I can tell, but don't you want this sent to net as a fix?

> ---
>
> Notes:
>     Without this fix, converting the RX path to page_pool (next patch)
>     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  | 30 +++++++++++++++++++
>  .../net/ethernet/aquantia/atlantic/aq_ring.h  |  1 +
>  .../net/ethernet/aquantia/atlantic/aq_vec.c   |  2 +-
>  3 files changed, 32 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..c59b6c0b37f1 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> @@ -360,6 +360,36 @@ bool aq_ring_tx_clean(struct aq_ring_s *self)
>         return !!budget;
>  }
>
> +void aq_ring_tx_deinit(struct aq_ring_s *self)
> +{
> +       if (!self)
> +               goto err_exit;

return; here and delete label?

> +
> +       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 (likely(buff->is_mapped)) {
> +                       if (unlikely(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);
> +                       }

I would remove the unlikelys that seem unnecassry to me. This is a
reconfig path and the unlikely is probably not improving perf.

> +               }
> +
> +               if (unlikely(buff->is_eop)) {
> +                       if (buff->skb)
> +                               dev_kfree_skb_any(buff->skb);
> +                       else if (buff->xdpf)
> +                               xdp_return_frame(buff->xdpf);
> +               }
> +       }
> +err_exit:;
> +}
> +
>  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
>
>


-- 
Thanks,
Mina