Re: [PATCH net-next v6 06/15] net: ethernet: cortina: Index free queue fragments with XArray

[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.
---
net: ethernet: cortina: Index free queue fragments with XArray

This commit replaces the linear scan over freeq_pages in
geth_freeq_lookup() with an XArray lookup. The XArray is indexed by
fragment DMA address in fragment units. geth_freeq_alloc_map_page()
inserts the entries under freeq_lock and erases them when a page is
replaced.

> RX descriptors return the DMA address of the free queue fragment used by
> the hardware. The driver currently recovers its page by scanning every
> free queue page and comparing address ranges.
>
> This isn't very efficient...

[Severity: Low]
This isn't a bug, but should the commit message say that later fixes in
this series depend on this patch?

The patch is described only as a lookup speed-up and has no Fixes: tag.
Later patches in the series do carry Fixes: tags, and they build on the
per-fragment index added here.

"Correct free queue DMA mappings" (Fixes: 4d5ae32f5e1e) drops the
replacement unmap. It says the lifetime of the replaced mapping "is
handled ... later in the series".

"Preserve in-flight free queue pages" (Fixes: 4d5ae32f5e1e) says "Keep
each exact fragment DMA address in the XArray until software claims its
descriptor". Its geth_freeq_claim() calls xa_erase() on one fragment at a
time.

If those fixes go to net or stable, they won't apply or build without
freeq_mappings, geth_freeq_mapping_index() and the per-fragment
xa_insert()/xa_erase() calls from this patch. Could the dependency be
noted here, so this patch isn't mistaken for net-next material?

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 006362a033952..c1c42e1f93f42 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -726,43 +728,47 @@ static int gmac_setup_rxq(struct net_device *netdev)

[ ... ]

> -unlock:
> +	spin_lock_irqsave(&geth->freeq_lock, flags);
> +	gpage = xa_load(&geth->freeq_mappings, index);
> +	if (!gpage || !gpage->page)
> +		goto err_unlock;
> +
> +	page = gpage->page;
> +	page_mapping = gpage->mapping;
> +	valid = mapping >= page_mapping &&
> +		mapping - page_mapping <= PAGE_SIZE - frag_len &&
> +		!((mapping - page_mapping) & (frag_len - 1));
> +	if (!valid)
> +		goto err_unlock;
> +
> +	*page_offs = mapping - page_mapping;
>  	spin_unlock_irqrestore(&geth->freeq_lock, flags);
>  	return page;

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch, but
is a CPU sync missing before the fragment is handed back to gmac_rx()?

geth_freeq_lookup() returns a fragment that the device has written,
without calling dma_sync_single_range_for_cpu().

In the other direction, geth_fill_freeq() gives a page with refcount 1
back to the hardware without calling dma_sync_single_for_device():

geth_fill_freeq() {
    ...
		/* Add one reference per fragment in the page */
		page_ref_add(page, 1 << fpp_order);
    ...
}

There are no dma_sync_* calls anywhere in gemini.c at this commit. With
non-coherent DMA, could the CPU read stale cache lines for RX data?

Later patches in this series appear to fix this.
geth_freeq_claim() calls dma_sync_single_range_for_cpu() for each
claimed fragment. geth_freeq_post_page() calls
dma_sync_single_for_device() when recycling a page.
geth_freeq_release_pages() syncs the fragments that hardware still owns.

The commit message of "Preserve in-flight free queue pages" also notes
that the CPU sync is currently a no-op on FA526.

[ ... ]