netdev - Re: [PATCH net-next v5 2/2] net/mlx5: Avoid copying payload to the skb's linear part

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMB2axO3d9Wr64RRxYQd8rg5QVxt5MO=ZzRtJG8njeDYNBW-tw@mail.gmail.com>
Date: Tue, 9 Sep 2025 23:17:25 -0400
From: Amery Hung <ameryhung@...il.com>
To: Christoph Paasch <cpaasch@...nai.com>
Cc: Gal Pressman <gal@...dia.com>, Dragos Tatulea <dtatulea@...dia.com>, 
	Saeed Mahameed <saeedm@...dia.com>, Tariq Toukan <tariqt@...dia.com>, Mark Bloch <mbloch@...dia.com>, 
	Leon Romanovsky <leon@...nel.org>, Andrew Lunn <andrew+netdev@...n.ch>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Alexei Starovoitov <ast@...nel.org>, 
	Daniel Borkmann <daniel@...earbox.net>, Jesper Dangaard Brouer <hawk@...nel.org>, 
	John Fastabend <john.fastabend@...il.com>, Stanislav Fomichev <sdf@...ichev.me>, netdev@...r.kernel.org, 
	linux-rdma@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [PATCH net-next v5 2/2] net/mlx5: Avoid copying payload to the
 skb's linear part

On Tue, Sep 9, 2025 at 11:18 AM Christoph Paasch <cpaasch@...nai.com> wrote:
>
> On Mon, Sep 8, 2025 at 9:00 PM Christoph Paasch <cpaasch@...nai.com> wrote:
> >
> > On Thu, Sep 4, 2025 at 4:30 PM Amery Hung <ameryhung@...il.com> wrote:
> > >
> > > On Thu, Sep 4, 2025 at 3:57 PM Christoph Paasch via B4 Relay
> > > <devnull+cpaasch.openai.com@...nel.org> wrote:
> > > >
> > > > From: Christoph Paasch <cpaasch@...nai.com>
> > > >
> > > > mlx5e_skb_from_cqe_mpwrq_nonlinear() copies MLX5E_RX_MAX_HEAD (256)
> > > > bytes from the page-pool to the skb's linear part. Those 256 bytes
> > > > include part of the payload.
> > > >
> > > > When attempting to do GRO in skb_gro_receive, if headlen > data_offset
> > > > (and skb->head_frag is not set), we end up aggregating packets in the
> > > > frag_list.
> > > >
> > > > This is of course not good when we are CPU-limited. Also causes a worse
> > > > skb->len/truesize ratio,...
> > > >
> > > > So, let's avoid copying parts of the payload to the linear part. We use
> > > > eth_get_headlen() to parse the headers and compute the length of the
> > > > protocol headers, which will be used to copy the relevant bits ot the
> > > > skb's linear part.
> > > >
> > > > We still allocate MLX5E_RX_MAX_HEAD for the skb so that if the networking
> > > > stack needs to call pskb_may_pull() later on, we don't need to reallocate
> > > > memory.
> > > >
> > > > This gives a nice throughput increase (ARM Neoverse-V2 with CX-7 NIC and
> > > > LRO enabled):
> > > >
> > > > BEFORE:
> > > > =======
> > > > (netserver pinned to core receiving interrupts)
> > > > $ netperf -H 10.221.81.118 -T 80,9 -P 0 -l 60 -- -m 256K -M 256K
> > > >  87380  16384 262144    60.01    32547.82
> > > >
> > > > (netserver pinned to adjacent core receiving interrupts)
> > > > $ netperf -H 10.221.81.118 -T 80,10 -P 0 -l 60 -- -m 256K -M 256K
> > > >  87380  16384 262144    60.00    52531.67
> > > >
> > > > AFTER:
> > > > ======
> > > > (netserver pinned to core receiving interrupts)
> > > > $ netperf -H 10.221.81.118 -T 80,9 -P 0 -l 60 -- -m 256K -M 256K
> > > >  87380  16384 262144    60.00    52896.06
> > > >
> > > > (netserver pinned to adjacent core receiving interrupts)
> > > >  $ netperf -H 10.221.81.118 -T 80,10 -P 0 -l 60 -- -m 256K -M 256K
> > > >  87380  16384 262144    60.00    85094.90
> > > >
> > > > Additional tests across a larger range of parameters w/ and w/o LRO, w/
> > > > and w/o IPv6-encapsulation, different MTUs (1500, 4096, 9000), different
> > > > TCP read/write-sizes as well as UDP benchmarks, all have shown equal or
> > > > better performance with this patch.
> > > >
> > > > Reviewed-by: Eric Dumazet <edumazet@...gle.com>
> > > > Reviewed-by: Saeed Mahameed <saeedm@...dia.com>
> > > > Signed-off-by: Christoph Paasch <cpaasch@...nai.com>
> > > > ---
> > > >  drivers/net/ethernet/mellanox/mlx5/core/en_rx.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> > > > index 8bedbda522808cbabc8e62ae91a8c25d66725ebb..0ac31c7fb64cd60720d390de45a5b6b453ed0a3f 100644
> > > > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> > > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> > > > @@ -2047,6 +2047,8 @@ mlx5e_skb_from_cqe_mpwrq_nonlinear(struct mlx5e_rq *rq, struct mlx5e_mpw_info *w
> > > >                 dma_sync_single_for_cpu(rq->pdev, addr + head_offset, headlen,
> > > >                                         rq->buff.map_dir);
> > > >
> > > > +               headlen = eth_get_headlen(rq->netdev, head_addr, headlen);
> > > > +
> > > >                 frag_offset += headlen;
> > > >                 byte_cnt -= headlen;
> > > >                 linear_hr = skb_headroom(skb);
> > > > @@ -2123,6 +2125,9 @@ mlx5e_skb_from_cqe_mpwrq_nonlinear(struct mlx5e_rq *rq, struct mlx5e_mpw_info *w
> > > >                                 pagep->frags++;
> > > >                         while (++pagep < frag_page);
> > > >                 }
> > > > +
> > > > +               headlen = eth_get_headlen(rq->netdev, mxbuf->xdp.data, headlen);
> > > > +
> > >
> > > The size of mxbuf->xdp.data is most likely not headlen here.
> > >
> > > The driver currently generates a xdp_buff with empty linear data, pass
> > > it to the xdp program and assumes the layout If the xdp program does
> > > not change the layout of the xdp_buff through bpf_xdp_adjust_head() or
> > > bpf_xdp_adjust_tail(). The assumption is not correct and I am working
> > > on a fix. But, if we keep that assumption for now, mxbuf->xdp.data
> > > will not contain any headers or payload. The thing that you try to do
> > > probably should be:
> > >
> > >         skb_frag_t *frag = &sinfo->frags[0];
> > >
> > >         headlen = eth_get_headlen(rq->netdev, skb_frag_address(frag),
> > > skb_frag_size(frag));
>
> So, when I look at the headlen I get, it is correct (even with my old
> code using mxbuf->xdp.data).
>
> To make sure I test the right thing, which scenario would
> mxbuf->xdp.data not contain any headers or payload ? What do I need to
> do to reproduce that ?

A quick look at the code, could it be that
skb_flow_dissect_flow_keys_basic() returns false so that
eth_get_headlen() always returns sizeof(*eth)? The linear part
contains nothing meaning before __psk_pull_tail(), so it is possible
for skb_flow_dissect_flow_keys_basic() to fail.

>
> Thanks,
> Christoph
>
> >
> > Ok, I think I understand what you mean! Thanks for taking the time to explain!
> >
> > I will do some tests on my side to make sure I get it right.
> >
> > As your change goes to net and mine to netnext, I can wait until yours
> > is in the tree so that there aren't any conflicts that need to be
> > taken care of.

Will Copy you in the mlx5 non-linear xdp fixing patchset.

> >
> >
> > Christoph
> >
> > >
> > >
> > >
> > > >                 __pskb_pull_tail(skb, headlen);
> > > >         } else {
> > > >                 if (xdp_buff_has_frags(&mxbuf->xdp)) {
> > > >
> > > > --
> > > > 2.50.1
> > > >
> > > >