Re: [PATCH net-next V6 2/3] net/mlx5e: Avoid copying payload to the skb's linear part

[Amery Hung]

On Fri, May 8, 2026 at 2:15 AM Dragos Tatulea <dtatulea@xxxxxxxxxx> wrote:
>
>
>
> On 07.05.26 22:50, Amery Hung wrote:
> > On Thu, May 7, 2026 at 4:50 PM Dragos Tatulea <dtatulea@xxxxxxxxxx> wrote:
> >>
> >>
> >> Hi Amery,
> >>
> >> On 07.05.26 15:53, Amery Hung wrote:
> >>> [...]
> >>> Am I understanding correctly that the better performance comes with
> >>> the assumption that the XDP does not change headers?
> >>>
> >>> headlen is determined before the XDP program runs. If it push/pop
> >>> headers, there could be headers in frags or data in the linear region
> >>> after __pskb_pull_tail().
> >>>
> >> That's right.
> >>
> >>>>                         if (__test_and_clear_bit(MLX5E_RQ_FLAG_XDP_XMIT, rq->flags)) {
> >>>>                                 struct mlx5e_frag_page *pfp;
> >>>> @@ -2060,8 +2066,7 @@ mlx5e_skb_from_cqe_mpwrq_nonlinear(struct mlx5e_rq *rq, struct mlx5e_mpw_info *w
> >>>>                                 pagep->frags++;
> >>>>                         while (++pagep < frag_page);
> >>>>
> >>>> -                       headlen = min_t(u16, MLX5E_RX_MAX_HEAD - len,
> >>>> -                                       skb->data_len);
> >>>> +                       headlen = min_t(u16, headlen - len, skb->data_len);
> >>>
> >>> headlen - len can underflow but will be capped by skb->data_len, so
> >>> this should be okay, right?
> >> It is safe. But it might trigger an extra allocation in the pull when
> >> len > headlen. We could also skip the pull in that case. Or do a
> >> min(headlen - len, min(skb->data_len, MLX5E_RX_MAX_HEAD)). WDYT?
> >
> > Make sense, but this line took me a bit to understand. Maybe consider
> > checking len < headlen first?
> >
> > if (len < headlen) {
> >         headlen = min_t(u32, headlen - len, skb->data_len);
> >         __pskb_pull_tail(skb, headlen);
> > }
> >
> Yes, that's what I had in mind when skipping the pull. I would also
> tag this as likely.
>
> > Another clarifying question. So this patch will improve the
> > performance when the XDP programs don't change header length. For
> > those that encap/decap, they should precisely pull only headers into
> > the linear area for optimal performance. Is it correct?
> >
> Right for encap, but for decap not quite:
>
> Let's say that the XDP program pulls 64B header into the linear part
> and snips 4B of the encap out. This would result in a pull of an
> additional 4B (headlen (64B) - len (60B) = 4B) which are now
> data bytes => sub-optimal layout.
>
> I don't see how we can improve this corner case though.

I see. Thanks for the clarification.

I think the "if (len < headlen)" makes too many assumptions about what
the XDP program did.

How about this policy instead: If the XDP program did not create/pull
data into the linear area, pull the parsed headers; otherwise, assume
the XDP program owns the geometry. min() is still needed since the
program can shrink the packet.

if (!len) {
        headlen = min(headlen, skb->data_len);
        __pskb_pull_tail(skb, headen);
}

This preserves the optimization for the default no-modification case,
and most importantly allow XDP program to get the optimal performance
if it gets the final geometry right.

>
> Thanks,
> Dragos