netdev - Re: [RFC bpf-next 0/4] Add XDP rx hw hints support performing XDP

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: <Zv_5KdpkaYY-6z1f@lore-desk>
Date: Fri, 4 Oct 2024 16:18:17 +0200
From: Lorenzo Bianconi <lorenzo@...nel.org>
To: Jesper Dangaard Brouer <hawk@...nel.org>
Cc: Arthur Fabre <afabre@...udflare.com>, Daniel Xu <dxu@...uu.xyz>,
	Stanislav Fomichev <stfomichev@...il.com>,
	Toke Høiland-Jørgensen <toke@...hat.com>,
	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 Oct 04, Jesper Dangaard Brouer wrote:
> 
> 
> 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.

+1. I am fine with the KV approach for nic metadata as well if it is fast enough.
If you want I can modify my series to use kfunc sto store data after xdp_frame
and then you can plug the KV encoding. What do you think? Up to you.

Regards,
Lorenzo

> 
> --Jesper

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)