Re: [PATCH net-next v3] xdp: Add helpers for head length, headroom, and metadata length

[Jesper Dangaard Brouer]

On 08/05/2025 05.18, Jon Kohler wrote:
> > On May 7, 2025, at 4:58 PM, Jesper Dangaard Brouer<hawk@xxxxxxxxxx>  wrote:
> >
> > On 07/05/2025 21.57, Jon Kohler wrote:
> > > > On May 7, 2025, at 3:04 PM, Jesper Dangaard Brouer<hawk@xxxxxxxxxx>  wrote:
> > > >
> > > > On 07/05/2025 19.47, Jon Kohler wrote:
> > > > > > On May 7, 2025, at 1:21 PM, Willem de Bruijn<willemdebruijn.kernel@xxxxxxxxx>  wrote:
> > > > > >
> > > > > > Jesper Dangaard Brouer wrote:
> > > > > > > On 07/05/2025 19.02, Zvi Effron wrote:
> > > > > > > > On Wed, May 7, 2025 at 9:37 AM Jesper Dangaard Brouer<hawk@xxxxxxxxxx>  wrote:
> > > > > > > > > On 07/05/2025 15.29, Willem de Bruijn wrote:
> > > > > > > > > > Stanislav Fomichev wrote:
> > > > > > > > > > > On 05/06, Jon Kohler wrote:
> > > > > > > > > > > > Introduce new XDP helpers:
> > > > > > > > > > > > - xdp_headlen: Similar to skb_headlen
> > > > > > > > > I really dislike xdp_headlen(). This "headlen" originates from an SKB
> > > > > > > > > implementation detail, that I don't think we should carry over into XDP
> > > > > > > > > land.
> > > > > > > > > We need to come up with something that isn't easily mis-read as the
> > > > > > > > > header-length.
> > > > > > > > ... snip ...
> > > > > > > >
> > > > > > > > > > > + * xdp_headlen - Calculate the length of the data in an XDP buffer
> > > > > > > > How about xdp_datalen()?
> > > > > > > Yes, I like xdp_datalen() 🙂
> > > > > > This is confusing in that it is the inverse of skb->data_len:
> > > > > > which is exactly the part of the data not in the skb head.
> > > > > >
> > > > > > There is value in consistent naming. I've never confused headlen
> > > > > > with header len.
> > > > > >
> > > > > > But if diverging, at least let's choose something not
> > > > > > associated with skbs with a different meaning.
> > > > > Brainstorming a few options:
> > > > > - xdp_head_datalen() ?
> > > > > - xdp_base_datalen() ?
> > > > > - xdp_base_headlen() ?
> > > > > - xdp_buff_datalen() ?
> > > > > - xdp_buff_headlen() ?
> > > > > - xdp_datalen() ? (ZivE, JesperB)
> > > > > - xdp_headlen() ? (WillemB, JonK, StanislavF, JacobK, DanielB)
> > > > What about keeping it really simple: xdp_buff_len() ?
> > > This is suspiciously close to xdp_get_buff_len(), so there could be some
> > > confusion there, since that takes paged/frags into account transparently.
> > Good point.
> >
> > > > Or even simpler: xdp_len() as the function documentation already
> > > > describe this doesn't include frags.
> > > There is a neat hint from Lorenzo’s change in bpf.h for bpf_xdp_get_buff_len()
> > > that talks about both linear and paged length. Also, xdp_buff_flags’s
> > > XDP_FLAGS_HAS_FRAGS says non-linear xdp buff.
> > > Taking those hints, what about:
> > > xdp_linear_len() == xdp->data_end - xdp->data
> > > xdp_paged_len() == sinfo->xdp_frags_size
> > > xdp_get_buff_len() == xdp_linear_len() + xdp_paged_len()
> > I like xdp_linear_len() as it is descriptive/clear.
>
> Ok thanks, I’ll send out a v4 to codify that.

I'll ack a V4 with that change.

I do notice Jakub isn't a fan of the patch in general, but it seems
quite popular given the other high profile kernel developers that acked
in V3.  I think it increase code readability for people that are less
familiar with XDP code and meaning of the pointers (e.g. data_hard_start
vs. data_end vs. data vs. data_meta). (We don't even have some ascii art
showing these pointers).

--Jesper