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

[Yangyu Chen]

> On 23 Jul 2026, at 02:29, Mina Almasry <almasrymina@xxxxxxxxxx> wrote:
> 
> 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?

Maybe I can change it in the next revision.I would this fixes to be more like a revert of commit eb36bedf28be
("net: aquantia: remove function aq_ring_tx_deinit"), so I would
like to keep that.

Maybe I can change it in the next revision.

Thanks,
Yangyu Chen

> 
>> +
>> +       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