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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] geth_fill_freeq() (drivers/net/ethernet/cortina/gemini.c) sets
  its local `recycle` flag to false the first time…

--

net: ethernet: cortina: Recycle claimed free queue pages

With this patch, a free queue page keeps its base reference and streaming
mapping after its last fragment is claimed. The slot is marked in a new
recycle bitmap. During refill, geth_fill_freeq() reuses the page if its
refcount has dropped back to one, and otherwise unmaps and releases it
before allocating a replacement.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 3bb96c51e0b71..943c31c026743 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -997,9 +1078,26 @@ static unsigned int geth_fill_freeq(struct gemini_ethernet *geth)
>  		pn = (rw.bits.wptr >> fpp_order) & m_pn;
>  		epn = (rw.bits.rptr >> fpp_order) - 1;
>  		epn &= m_pn;
> +		ret = -ENOSPC;
> +		if (pn != epn && recycle) {
> +			ret = geth_freeq_recycle_page(geth, pn);
> +			if (!ret) {
> +				count += 1 << fpp_order;
> +				pn++;
> +				pn &= m_pn;
> +				writew(pn << fpp_order,
> +				       geth->base + GLOBAL_SWFQ_RWPTR_REG + 2);
> +			}
> +			if (ret == -ENOSPC)
> +				recycle = false;

[Severity: Low]

Once geth_freeq_recycle_page() returns -ENOSPC because freeq_recycle_pending
was 0, recycle stays false for the rest of this geth_fill_freeq() call.
Can new recyclable slots show up after that point?

freeq_lock is dropped at the end of each iteration and around
geth_freeq_map_page(). In that window gmac_rx()->geth_freeq_claim() can
retire pages. One example is the NAPI softirq running at hardirq exit while
gemini_port_irq_thread() is inside geth_fill_freeq():

geth_freeq_claim()
    if (!--gpage->fragments) {
        slot = gpage - geth->freeq_pages;
        __set_bit(slot, geth->freeq_recycle_bitmap);
        geth->freeq_recycle_pending++;
    }

These retired slots keep their freeq_page_bitmap bit set. Even so, the
remaining iterations still allocate and map a fresh page. Each one takes a
new metadata slot through geth_freeq_add_page()->geth_freeq_alloc_slot().

Can slot usage then exceed what geth_freeq_page_slots() sizes for (ring
pages plus rxq entries)? Retired pages now count on top of posted pages and
pages held by unclaimed RX descriptors.

If the table fills up, geth_freeq_alloc_slot() returns -ENOSPC.
geth_fill_freeq() then throws away the page it just mapped and stops:

    if (ret) {
        dma_unmap_single(geth->dev, page_mapping, PAGE_SIZE,
                         DMA_FROM_DEVICE);
        put_page(page);
        break;
    }

This leaves the free queue under-filled while recyclable slots are still
pending.

The next geth_fill_freeq() call starts with recycle = true and drains them,
so this recovers on its own. It does still cause extra fresh page
allocations and mappings during the call, which the commit message is
trying to avoid.

Could the latch be dropped, or freeq_recycle_pending re-checked under
freeq_lock, before falling back to a fresh allocation?

> +		}
>  		spin_unlock_irqrestore(&geth->freeq_lock, flags);
>  		if (pn == epn)
>  			break;
> +		if (!ret)
> +			continue;
> +		if (ret != -ENOSPC && ret != -EAGAIN)
> +			break;
>  
>  		ret = geth_freeq_map_page(geth, &page, &page_mapping);
>  		if (ret)

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011-gemini-ethernet-fixes-3-v4-0-4b1631b08f5d%40kernel.org