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

[Jesper Dangaard Brouer <hawk@kernel.org>]

On 08/05/2025 05.18, Jon Kohler wrote:
> 
>> On May 7, 2025, at 4:58 PM, Jesper Dangaard Brouer<hawk@kernel.org>  wrote:
>>
>> On 07/05/2025 21.57, Jon Kohler wrote:
>>>> On May 7, 2025, at 3:04 PM, Jesper Dangaard Brouer<hawk@kernel.org>  wrote:
>>>>
>>>> On 07/05/2025 19.47, Jon Kohler wrote:
>>>>>> On May 7, 2025, at 1:21 PM, Willem de Bruijn<willemdebruijn.kernel@gmail.com>  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@kernel.org>  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