Re: [net-next 07/11] net/mlx5e: Convert over to netmem

Dragos Tatulea
2025-04-09T12:40:29Z

On Wed, Feb 05, 2025 at 12:14:08PM -0800, Mina Almasry wrote:
> On Thu, Jan 16, 2025 at 1:56 PM Saeed Mahameed <saeed@kernel.org> wrote:
> >
> > From: Saeed Mahameed <saeedm@nvidia.com>
> >
> > mlx5e_page_frag holds the physical page itself, to naturally support
> > zc page pools, remove physical page reference from mlx5 and replace it
> > with netmem_ref, to avoid internal handling in mlx5 for net_iov backed
> > pages.
> >
> > No performance degradation observed.
> >
> > Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
> > Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
> > Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
> > ---
> > drivers/net/ethernet/mellanox/mlx5/core/en.h | 2 +-
> > .../net/ethernet/mellanox/mlx5/core/en_rx.c | 80 ++++++++++---------
> > 2 files changed, 43 insertions(+), 39 deletions(-)
> >
> ...
> > @@ -514,9 +514,9 @@ mlx5e_add_skb_shared_info_frag(struct mlx5e_rq *rq, struct skb_shared_info *sinf
> > }
> >
> > frag = &sinfo->frags[sinfo->nr_frags++];
> > - skb_frag_fill_page_desc(frag, frag_page->page, frag_offset, len);
> > + skb_frag_fill_netmem_desc(frag, netmem, frag_offset, len);
> >
> > - if (page_is_pfmemalloc(frag_page->page))
> > + if (!netmem_is_net_iov(netmem) && page_is_pfmemalloc(netmem_to_page(netmem)))
> > xdp_buff_set_frag_pfmemalloc(xdp);
>
> Consider using:
>
> netmem_is_pfmemalloc(netmem_ref netmem)
>
> In general we try to avoid netmem_to_page() casts in the driver. These
> assumptions may break in the future.
>
We will fix in v2 which we are preparing.

> > sinfo->xdp_frags_size += len;
> > }
> > @@ -527,27 +527,29 @@ mlx5e_add_skb_frag(struct mlx5e_rq *rq, struct sk_buff *skb,
> > u32 frag_offset, u32 len,
> > unsigned int truesize)
> > {
> > - dma_addr_t addr = page_pool_get_dma_addr(frag_page->page);
> > + dma_addr_t addr = page_pool_get_dma_addr_netmem(frag_page->netmem);
> > + struct page *page = netmem_to_page(frag_page->netmem);
> > u8 next_frag = skb_shinfo(skb)->nr_frags;
> >
> > dma_sync_single_for_cpu(rq->pdev, addr + frag_offset, len,
> > rq->buff.map_dir);
> >
> > - if (skb_can_coalesce(skb, next_frag, frag_page->page, frag_offset)) {
> > + if (skb_can_coalesce(skb, next_frag, page, frag_offset)) {
>
> Similarly here, consider adding skb_can_coalesce_netmem() that handles
> this correctly in core code (which future drivers can reuse) rather
> than doing 1-off handling in the driver.
>
Good point. It is definitely worth adding as coalescing is desirable.

> Also, from a quick look at skb_can_coalesce(), I think it can work
> fine with netmems? Because it just needs to be converted to use
> skb_frag_netmem istead of skb_frag_page() inside of the function, but
> otherwise the function looks applicable to netmem for me.
>
Having an extra skb_can_coalesce_netmems() which can be called
by the driver on the rx path makes sense. I don't think we can drop the
skb_zcopy() check from skb_can_coalesce() as this is also used on the tx
path.

Thanks,
Dragos