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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2c5lwvqvqicmthhu6uyu4r6rfanxxz6iloaysz7uvw4qpoeojb@g5whyniwnf67>
Date: Wed, 9 Apr 2025 12:40:17 +0000
From: Dragos Tatulea <dtatulea@...dia.com>
To: Mina Almasry <almasrymina@...gle.com>, 
	Saeed Mahameed <saeed@...nel.org>
Cc: "David S. Miller" <davem@...emloft.net>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Eric Dumazet <edumazet@...gle.com>, Saeed Mahameed <saeedm@...dia.com>, netdev@...r.kernel.org, 
	Tariq Toukan <tariqt@...dia.com>, Gal Pressman <gal@...dia.com>, 
	Leon Romanovsky <leonro@...dia.com>, cratiu@...dia.com
Subject: Re: [net-next 07/11] net/mlx5e: Convert over to netmem

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@...nel.org> wrote:
> >
> > From: Saeed Mahameed <saeedm@...dia.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@...dia.com>
> > Reviewed-by: Dragos Tatulea <dtatulea@...dia.com>
> > Reviewed-by: Tariq Toukan <tariqt@...dia.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