Message-ID: <75fb1dd3-fe14-426c-bc59-9a582c4b0e8d@kernel.org>
Date: Fri, 4 Oct 2024 16:14:35 +0200
From: Jesper Dangaard Brouer <hawk@...nel.org>
To: Arthur Fabre <afabre@...udflare.com>, Daniel Xu <dxu@...uu.xyz>,
 Stanislav Fomichev <stfomichev@...il.com>
Cc: Toke Høiland-Jørgensen <toke@...hat.com>,
 Lorenzo Bianconi <lorenzo@...nel.org>,
 Lorenzo Bianconi <lorenzo.bianconi@...hat.com>,
 Jakub Sitnicki <jakub@...udflare.com>,
 Alexander Lobakin <aleksander.lobakin@...el.com>, bpf@...r.kernel.org,
 netdev@...r.kernel.org, ast@...nel.org, daniel@...earbox.net,
 davem@...emloft.net, kuba@...nel.org, john.fastabend@...il.com,
 edumazet@...gle.com, pabeni@...hat.com, sdf@...ichev.me, tariqt@...dia.com,
 saeedm@...dia.com, anthony.l.nguyen@...el.com, przemyslaw.kitszel@...el.com,
 intel-wired-lan@...ts.osuosl.org, mst@...hat.com, jasowang@...hat.com,
 mcoquelin.stm32@...il.com, alexandre.torgue@...s.st.com,
 kernel-team <kernel-team@...udflare.com>, Yan Zhai <yan@...udflare.com>
Subject: Re: [RFC bpf-next 0/4] Add XDP rx hw hints support performing
 XDP_REDIRECT



On 04/10/2024 15.55, Arthur Fabre wrote:
> On Fri Oct 4, 2024 at 12:38 PM CEST, Jesper Dangaard Brouer wrote:
>> [...]
>>>>> There are two different use-cases for the metadata:
>>>>>
>>>>> * "Hardware" metadata (like the hash, rx_timestamp...). There are only a
>>>>>     few well known fields, and only XDP can access them to set them as
>>>>>     metadata, so storing them in a struct somewhere could make sense.
>>>>>
>>>>> * Arbitrary metadata used by services. Eg a TC filter could set a field
>>>>>     describing which service a packet is for, and that could be reused for
>>>>>     iptables, routing, socket dispatch...
>>>>>     Similarly we could set a "packet_id" field that uniquely identifies a
>>>>>     packet so we can trace it throughout the network stack (through
>>>>>     clones, encap, decap, userspace services...).
>>>>>     The skb->mark, but with more room, and better support for sharing it.
>>>>>
>>>>> We can only know the layout ahead of time for the first one. And they're
>>>>> similar enough in their requirements (need to be stored somewhere in the
>>>>> SKB, have a way of retrieving each one individually, that it seems to
>>>>> make sense to use a common API).
>>>>
>>>> Why not have the following layout then?
>>>>
>>>> +---------------+-------------------+----------------------------------------+------+
>>>> | more headroom | user-defined meta | hw-meta (potentially fixed skb format) | data |
>>>> +---------------+-------------------+----------------------------------------+------+
>>>>                   ^                                                            ^
>>>>               data_meta                                                      data
>>>>
>>>> You obviously still have a problem of communicating the layout if you
>>>> have some redirects in between, but you, in theory still have this
>>>> problem with user-defined metadata anyway (unless I'm missing
>>>> something).
>>>>
>>
>> Hmm, I think you are missing something... As far as I'm concerned we are
>> discussing placing the KV data after the xdp_frame, and not in the XDP
>> data_meta area (as your drawing suggests).  The xdp_frame is stored at
>> the very top of the headroom.  Lorenzo's patchset is extending struct
>> xdp_frame and now we are discussing to we can make a more flexible API
>> for extending this. I understand that Toke confirmed this here [3].  Let
>> me know if I missed something :-)
>>
>>    [3] https://lore.kernel.org/all/874j62u1lb.fsf@toke.dk/
>>
>> As part of designing this flexible API, we/Toke are trying hard not to
>> tie this to a specific data area.  This is a good API design, keeping it
>> flexible enough that we can move things around should the need arise.
> 
> +1. And if we have an API for doing this for user-defined metadata, it
> seems like we might as well use it for hardware metadata too.
> 
> With something roughly like:
> 
>      *val get(id)
> 
>      set(id, *val)
> 
> with pre-defined ids for hardware metadata, consumers don't need to know
> the layout, or where / how the data is stored.
> 
> Under the hood we can implement it however we want, and change it in the
> future.
> 
> I was initially thinking we could store hardware metadata the same way
> as user defined metadata, but Toke and Lorenzo seem to prefer storing it
> in a fixed struct.

If the API hide the actual location then we can always move things
around, later.  If your popcnt approach is fast enough, then IMO we
don't need a fixed struct for hardware metadata.

--Jesper

Powered by blists - more mailing lists