On Fri, May 28, 2021 at 10:30 AM John Fastabend
<john.fastabend@xxxxxxxxx> wrote:
>
> Jesper Dangaard Brouer wrote:
>
> [...]
>
> I'll try to respond to both Toke and Jesper here and make it coherent so
> we don't split this thread yet again.
>
> Wish me luck.
>
> @Toke "react" -> "not break" hopefully gives you my opinion on this.
>
> @Toke "five fields gives 32 different metadata formats" OK let me take
> five fields,
>
> struct meta {
> __u32 f1;
> __u32 f2;
> __u32 f3;
> __u32 f4;
> __u32 f5;
> }
>
> I'm still confused why the meta data would change just because the feature
> is enabled or disabled. I've written drivers before and I don't want to
> move around where I write f1 based on some combination of features f2,f3,f4,f5
> state of enabled/disabled. If features are mutual exclusive I can build a
> sensible union. If its possible for all fields to enabled then I just lay
> them out like above.
>
>
> @Toke "completely reprogramming the NIC" -> sounds like some basic agreement.
>
>
> > > > >> > > union and independent set of BTFs are two different things, I'll let
> > > > >> > > you guys figure out which one you need, but I replied how it could
> > > > >> > > look like in CO-RE world
> > > > >> >
> > > > >> > I think a union is sufficient and more aligned with how the
> > > > >> > hardware would actually work.
> > > > >>
> > > > >> Sure. And I think those are two orthogonal concerns. You can start
> > > > >> with a single struct mynic_metadata with union inside it, and later
> > > > >> add the ability to swap mynic_metadata with another
> > > > >> mynic_metadata___v2 that will have a similar union but with a
> > > > >> different layout.
> > > > >
> > > > > Right and then you just have normal upgrade/downgrade problems with
> > > > > any struct.
> > > > >
> > > > > Seems like a workable path to me. But, need to circle back to the
> > > > > what we want to do with it part that Jesper replied to.
> > > >
> > > > So while this seems to be a viable path for getting libbpf to do all the
> > > > relocations (and thanks for hashing that out, I did not have a good grip
> > > > of the details), doing it all in userspace means that there is no way
> > > > for the XDP program to react to changes once it has been loaded. So this
> > > > leaves us with a selection of non-very-attractive options, IMO. I.e.,
> > > > we would have to:
>
>
> > >
> > > I don't really understand what this means 'having XDP program to
> > > react to changes once it has been loaded.' What would a program look
> > > like thats dynamic? You can always version your metadata and
> > > write programs like this,
> > >
> > > if (meta->version == VERSION1) {do_foo}
> > > else {do_bar}
> > >
> > > And then have a headeer,
> > >
> > > struct meta {
> > > int version;
> > > union ... // union of versions
> > > }
> > >
> > > I fail to see how a program could 'react' dynamically. An agent could
> > > load new programs dynamically into tail call maps of fentry with
> > > the need handlers, which would work as well and avoid unions.
> > >
> > > >
> > > > - have to block any modifications to the hardware config that would
> > > > change the metadata format; this will probably result in irate users
> > >
> > > I'll need a concrete example if I swap out my parser block, I should
> > > also swap out my BPF for my shiny new protocol. I don't see how a
> > > user might write programs for things they've not configured hardware
> > > for yet. Leaving aside knobs like VLAN on/off, VXLAN on/off, and
> > > such which brings the next point.
> > >
> > > >
> > > > - require XDP programs to deal with all possible metadata permutations
> > > > supported by that driver (by exporting them all via a BTF union or
> > > > similar); this means a potential for combinatorial explosion of config
> > > > options and as NICs become programmable themselves I'm not even sure
> > > > if it's possible for the driver to know ahead of time
> > >
> > > I don't see the problem sorry. For current things that exist I can't
> > > think up too many fields vlan, timestamp, checksum(?), pkt_type,
> > > hash maybe.
> > >
> > > For programmable pipelines (P4) then I don't see a problem with
> > > reloading your program or swapping out a program. I don't see the
> > > value of adding a new protocol for example dynamically. Surely
> > > the hardware is going to get hit with a big reset anyways.
> > >
> > > >
> > > > - throw up our hands and just let the user deal with it (i.e., to
> > > > nothing and so require XDP programs to be reloaded if the NIC config
> > > > changes); this is not very friendly and is likely to lead to subtle
> > > > bugs if an XDP program parses the metadata assuming it is in a
> > > > different format than it is
> > >
> > > I'm not opposed to user error causing logic bugs. If I give
> > > users power to reprogram their NICs they should be capabable
> > > of managing a few BPF programs. And if not then its a space
> > > where a distro/vendor should help them with tooling.
> > >
> > > >
> > > > Given that hardware config changes are not just done by ethtool, but
> > > > also by things like running `tcpdump -j`, I really think we have to
> > > > assume that they can be quite dynamic; which IMO means we have to solve
> > > > this as part of the initial design. And I have a hard time seeing how
> > > > this is possible without involving the kernel somehow.
> > >
> > > I guess here your talking about building an skb? Wouldn't it
> > > use whatever logic it uses today to include the timestamp.
> > > This is a bit of an aside from metadata in the BPF program.
> > >
> > > Building timestamps into
> > > skbs doesn't require BPF program to have the data. Or maybe
> > > the point is an XDP variant of tcpdump would like timestamps.
> > > But then it should be in the metadata IMO.
> >
> > It sounds like we are all agreeing that the HW RX timestamp should be
> > stored in the XDP-metadata area right?
> >
> > As I understand, John don't like multiple structs, but want a single
> > struct, so lets create below simple struct that the driver code fills
> > out before calling our XDP-prog:
> >
> > struct meta {
> > u32 timestamp_type;
> > u64 rx_timestamp;
> > u32 rxhash32;
> > u32 version;
> > };
>
> From driver side I don't think you want to dynamically move around
> fields too much. Meaning when feature X is enabled I write it in
> some offset and then when X+Y is enabled I write into another offset.
> This adds complexity on driver side and likely makes said driver
> slower due to complexity.
>
> Perhaps exception to above is on pkt_type where its natural to
> have hardware engine write different fields, but that fits
> naturally into a union around pkt_types.
>
> >
> > This NIC is configured for PTP, but hardware can only do rx_timestamp
> > for PTP packets (like ixgbe). (Assume both my XDP-prog and PTP
> > userspace prog want to see this HW TS).
> >
> > What should I do as a driver developer to tell XDP-prog that the HW
> > rx_timestamp is not valid for this packet ?
>
> Driver developer should do nothing. When enable write it into rx_timestamp.
> When disabled don't. Keep the drivers as simple as possible and don't
> make the problem hard.
>
> >
> > 1. Always clear 'rx_timestamp' + 'timestamp_type' for non-PTP packets?
> > 2. or, set version to something else ?
> >
> > I don't like option 1, because it will slowdown the normal none-PTP
> > packets, that doesn't need this timestamp.
> >
>
> 1, no and 2, no. When timestamps are wanted just set a global variable
> in the program. From XDP program,
>
> if (rx_timestamp_enabled) {
> meta->timestamp; // do something
> } else {
> meta->timestamp = bpf_get_timestamp(); // software fallback if you want
> }
>
> then when userspace enables the timestamp through whatever means it
> has to do this it also sets 'rx_timestamp_enabled = true' which can
> be done today no problem.
>
> Now its up to hardware and user to build something coherent. You
> don't need me to agree with you that its useful to add timestamps you
> have all the tools you need to do it. Presumably the user buys the
> hardware so they can buy whats most useful for them.
>
> >
> >
> > Now I want to redirect this packet into a veth. The veth device could
> > be running an XDP-prog, that also want to look at this timestamp info.
> > How can the veth XDP-prog know howto interpret metadata area. What if I
> > stored the bpf_id in the version fields in the struct?.
>
> Well presumably someone is managing the system so with above XDP prog
> on real nic could just populate the metadata with software if needed
> and veth would not care if it came from hardware or software. Or
> use same fallback trick with global variable.
>
> >
> > (Details: I also need this timestamp info transferred to xdp_frame,
> > because when redirecting into a veth (container) then I want this
> > timestamp set on the SKB to survive. I wonder how can I know what the
> > BTF-layout, guess it would be useful to have btf_id at this point)
>
> Why do you need to know the layout? Just copy the metadata. The core
> infrastructure can not know the layout or we are back to being
> gate-keepers of hardware features.
>
> >
> > > >
> > > > Unless I'm missing something? WDYT?
> > >
> > > Distilling above down. I think we disagree on how useful
> > > dynamic programs are because of two reasons. First I don't
> > > see a large list of common attributes that would make the
> > > union approach as painful as you fear. And two, I believe
> > > users who are touching core hardware firmware need to also
> > > be smart enough (or have smart tools) to swap out their
> > > BPF programs in the correct order so as to not create
> > > subtle races. I didn't do it here but if we agree walking
> > > through that program swap flow with firmware update would
> > > be useful.
> >
> > Hmm, I sense we are perhaps talking past each-other(?). I am not
> > talking about firmware upgrades. I'm arguing that enable/disable of HW
> > RX-timestamps will change the XDP-metadata usage dynamically runtime.
> > This is simply a normal userspace program that cause this changes e.g.
> > running 'tcpdump -j'.
>
> I'm not sure why it would change the layout. The hardware is going
> to start writing completely different metadata? If so just pivot
> on a global value like above with two structs.
>
> if (timestamp_enabled) {
> struct timestamp_meta *meta = data->meta_data;
> // do stuff
> } else {
> struct normal_meta *meta = data->meta_data;
> }
>
> The powerful part of above is you have all the pieces you need today
> sans exporting a couple internal libbpf calls, but that should
> be doable. Although Andrii would probably object to such a ugly
> hack so a proper API would be nice. But, again not strictly needed
Of course I would :) But I had the ability to specify custom vmlinux
BTF in libbpf (through bpf_object__load_xattr) from the very beginning
(at least for testing purposes), though it bit-rotted a bit with all
the further BTF-enabled features. But I'm all for cleaning that up and
formalizing the ability to specify alternative vmlinux BTF and/or
additional external BTF.
> to get above working.
>
> Addressing Tokes example which I think is the same, Instead of building
> a metadata struct like this,
>
> struct meta {
> u32 rxhash;
> u8 vlan;
> };
>
> Use the second example as your metadata always
>
> struct meta {
> u32 rxhash;
> u32 timestamp;
> u8 vlan;
> };
>
> Then pivot on what to do with that timestamp using a global variable or
> map or any of the other ways we do features dynamically in kprobes and
> other prog types.
>
> Thanks,
> John
