Re: [PATCH net-next v6 09/15] net: ethernet: cortina: Recycle claimed free queue pages

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -964,9 +986,69 @@ static int geth_freeq_add_page(struct gemini_ethernet *geth, unsigned int wptr,
>  		xa_erase(&geth->freeq_mappings,
>  			 geth_freeq_mapping_index(geth, mapping));
>  	}
> -	gpage->page = NULL;
> -	gpage->mapping = 0;
> -	__clear_bit(slot, geth->freeq_page_bitmap);
> +	return ret;
> +}
> +
> +static int geth_freeq_add_page(struct gemini_ethernet *geth,
> +			       unsigned int wptr,
> +			       struct page *page, dma_addr_t page_mapping)
> +{
> +	struct gmac_queue_page *gpage;
> +	int ret;
> +	int slot;
> +
> +	lockdep_assert_held(&geth->freeq_lock);
> +
> +	slot = geth_freeq_alloc_slot(geth);
> +	if (slot < 0)
> +		return slot;
> +
> +	gpage = &geth->freeq_pages[slot];
> +	gpage->page = page;
> +	gpage->mapping = page_mapping;
> +	ret = geth_freeq_post_page(geth, wptr, gpage, false);
> +	if (ret) {
> +		gpage->page = NULL;
> +		gpage->mapping = 0;
> +		__clear_bit(slot, geth->freeq_page_bitmap);
> +	}
> +
> +	return ret;
> +}
> +
> +static int geth_freeq_recycle_page(struct gemini_ethernet *geth,
> +				   unsigned int wptr)
> +{
> +	struct gmac_queue_page *gpage;
> +	struct page *page;
> +	int ret;
> +	int slot;
> +
> +	lockdep_assert_held(&geth->freeq_lock);
> +
> +	slot = geth_freeq_recycle_slot(geth);
> +	if (slot < 0)
> +		return slot;
> +
> +	gpage = &geth->freeq_pages[slot];
> +	if (page_ref_count(gpage->page) > 1) {
> +		page = gpage->page;
> +		dma_unmap_single_attrs(geth->dev, gpage->mapping, PAGE_SIZE,
> +				       DMA_FROM_DEVICE,
> +				       DMA_ATTR_SKIP_CPU_SYNC);
> +		gpage->page = NULL;
> +		gpage->mapping = 0;
> +		__clear_bit(slot, geth->freeq_page_bitmap);
> +		put_page(page);
> +		return -EAGAIN;
> +	}
> +
> +	ret = geth_freeq_post_page(geth, wptr, gpage, true);
> +	if (ret) {
> +		__set_bit(slot, geth->freeq_recycle_bitmap);
> +		geth->freeq_recycle_pending++;
> +	}
> +
>  	return ret;
>  }
>  

[Severity: Medium]
geth_freeq_recycle_page() only checks page_ref_count() before handing
the page back to geth_freeq_post_page(). The pages come from
alloc_page(GFP_ATOMIC) in geth_freeq_map_page(). Under memory pressure
that allocation can return a page from the PFMEMALLOC reserves.

When gmac_rx() attaches a fragment with skb_fill_page_desc(), the skb
inherits skb->pfmemalloc. The core then drops it for any socket that
does not have SOCK_MEMALLOC set. That drop happens inside the NAPI
poll, so by the time geth_fill_freeq() runs the refcount is back to
one and the page is reposted to the free queue.

Nothing ever returns that page to the allocator. The reserve page stays
pinned, and every frame that lands in it is dropped, long after the
pressure has gone away.

Before this patch the base reference was dropped on the last claim in
geth_freeq_claim(), so a reserve page was freed once the stack let go
of it. The problem is new with recycling.

Other drivers that recycle RX pages gate reuse on
dev_page_is_reusable(), which also rejects pages from a remote NUMA
node. Could the discard branch here also be taken when
dev_page_is_reusable(gpage->page) is false? Pfmemalloc pages would then
go through the dma_unmap_single_attrs() and put_page() path instead of
being reposted.