Re: [PATCH net-next V2 07/13] net/mlx5e: SHAMPO, Use page_pool DMA sync helper for SKB frags

[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] The commit message describes this patch as preparation for data
  direct only.

--

net/mlx5e: SHAMPO, Use page_pool DMA sync helper for SKB frags

This patch changes mlx5e_add_skb_frag() to call
page_pool_dma_sync_netmem_for_cpu() instead of calling
dma_sync_single_for_cpu() on rq->pdev directly. The stated reason is the
upcoming data direct support, where SHAMPO payload netmem is mapped
against the data direct device rather than rq->pdev.

> Switch to the page_pool DMA API which takes care of not calling
> dma_sync_* for unreadable netmem. Do it only for the place where it
> makes sense: SHAMPO payloads, the only RX buffers that can be
> unreadable.
>
> Until now it was ok to call the dma_sync_* API directly. But with the
> introduction of data direct it will no longer be correct: the payload
> netmem of a data direct RQ is mapped against the data direct device,
> not rq->pdev.

[Severity: Low]
Is "Until now it was ok to call the dma_sync_* API directly" accurate?

Since commit db3010bb5a01 ("net/mlx5e: Add support for UNREADABLE netmem
page pools"), mlx5e_alloc_rq() allows unreadable netmem for SHAMPO RQs:

drivers/net/ethernet/mellanox/mlx5/core/en_main.c:mlx5e_alloc_rq() {
    ...
		/* Shampo header data split allow for unreadable netmem */
		if (test_bit(MLX5E_RQ_STATE_SHAMPO, &rq->state))
			pp_params.flags |= PP_FLAG_ALLOW_UNREADABLE_NETMEM;
    ...
}

This means these RQs can be backed by the dma-buf devmem memory provider,
which does this:

net/core/devmem.c:mp_dmabuf_devmem_init() {
    ...
	/* dma-buf dma addresses do not need and should not be used with
	 * dma_sync_for_cpu/device. Force disable dma_sync.
	 */
	pool->dma_sync = false;
	pool->dma_sync_for_cpu = false;
    ...
}

That rule was added by commit 7dba339faae9 ("page_pool: disable sync for
cpu for dmabuf memory provider"), before mlx5 supported unreadable
netmem. Its commit message asks drivers to switch to this helper when
they add netmem support.

Before this patch, mlx5e_add_skb_frag() always called
dma_sync_single_for_cpu(rq->pdev, addr + frag_offset, ...) on these
dma-buf addresses.

At this commit, pp_params.dev is rq->pdev and pp_params.dma_dir is
rq->buff.map_dir. For regular page pools the new helper therefore does
the same thing as the old code. The only functional change is that the
sync is now skipped for dma-buf devmem pools, and that fixes the
existing behaviour.

Should the commit message say that this also fixes the devmem case? It
could also carry a Fixes: db3010bb5a01 tag, so backporters don't treat it
as prep work for data direct only.

A smaller point: page_pool_dma_sync_netmem_for_cpu() does not check for
unreadable netmem in general. It checks a per-pool flag:

	if (!pool->dma_sync_for_cpu)
		return;

Only the dma-buf provider clears that flag. io_uring zcrx pools are also
unreadable, but they leave it set and are still synced through
pool->p.dev.

Could the wording "takes care of not calling dma_sync_* for unreadable
netmem" be made more precise?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110183826.2927274-1-tariqt%40nvidia.com