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

On Sun, Aug 2, 2026 at 8:46 AM Yangyu Chen <[email protected]> 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: [email protected] # v5.2+
> Reviewed-by: Sukhdeep Singh <[email protected]>
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Yangyu Chen <[email protected]>


FWIW,

Acked-by: Mina Almasry <[email protected]>

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