Message-Id: <87a69o94wz.fsf@toke.dk>
Date:   Tue,  5 Jul 2022 17:15:09 +0200
From:   Alexander Lobakin <alexandr.lobakin@...el.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>
Cc:     Alexander Lobakin <alexandr.lobakin@...el.com>,
        John Fastabend <john.fastabend@...il.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Larysa Zaremba <larysa.zaremba@...el.com>,
        Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        Björn Töpel <bjorn@...nel.org>,
        Magnus Karlsson <magnus.karlsson@...el.com>,
        Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        Jonathan Lemon <jonathan.lemon@...il.com>,
        Lorenzo Bianconi <lorenzo@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Jesse Brandeburg <jesse.brandeburg@...el.com>,
        Yajun Deng <yajun.deng@...ux.dev>,
        Willem de Bruijn <willemb@...gle.com>, bpf@...r.kernel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        xdp-hints@...-project.net
Subject: Re: [xdp-hints] Re: [PATCH RFC bpf-next 00/52] bpf, xdp: introduce and use Generic Hints/metadata

From: Toke Høiland-Jørgensen <toke@...hat.com>
Date: Mon, 04 Jul 2022 19:14:04 +0200

> Alexander Lobakin <alexandr.lobakin@...el.com> writes:
> 
> > From: Toke H??iland-J??rgensen <toke@...hat.com>
> > Date: Wed, 29 Jun 2022 15:43:05 +0200
> >
> >> John Fastabend <john.fastabend@...il.com> writes:
> >> 
> >> > Alexander Lobakin wrote:
> >> >> This RFC is to give the whole picture. It will most likely be split
> >> >> onto several series, maybe even merge cycles. See the "table of
> >> >> contents" below.
> >> >
> >> > Even for RFC its a bit much. Probably improve the summary
> >> > message here as well I'm still not clear on the overall
> >> > architecture so not sure I want to dig into patches.
> >> 
> >> +1 on this, and piggybacking on your comment to chime in on the general
> >> architecture.
> >> 
> >> >> Now, a NIC driver, or even a SmartNIC itself, can put those params
> >> >> there in a well-defined format. The format is fixed, but can be of
> >> >> several different types represented by structures, which definitions
> >> >> are available to the kernel, BPF programs and the userland.
> >> >
> >> > I don't think in general the format needs to be fixed.
> >> 
> >> No, that's the whole point of BTF: it's not supposed to be UAPI, we'll
> >> use CO-RE to enable dynamic formats...
> >> 
> >> [...]
> >> 
> >> >> It is fixed due to it being almost a UAPI, and the exact format can
> >> >> be determined by reading the last 10 bytes of metadata. They contain
> >> >> a 2-byte magic ID to not confuse it with a non-compatible meta and
> >> >> a 8-byte combined BTF ID + type ID: the ID of the BTF where this
> >> >> structure is defined and the ID of that definition inside that BTF.
> >> >> Users can obtain BTF IDs by structure types using helpers available
> >> >> in the kernel, BPF (written by the CO-RE/verifier) and the userland
> >> >> (libbpf -> kernel call) and then rely on those ID when reading data
> >> >> to make sure whether they support it and what to do with it.
> >> >> Why separate magic and ID? The idea is to make different formats
> >> >> always contain the basic/"generic" structure embedded at the end.
> >> >> This way we can still benefit in purely generic consumers (like
> >> >> cpumap) while providing some "extra" data to those who support it.
> >> >
> >> > I don't follow this. If you have a struct in your driver name it
> >> > something obvious, ice_xdp_metadata. If I understand things
> >> > correctly just dump the BTF for the driver, extract the
> >> > struct and done you can use CO-RE reads. For the 'fixed' case
> >> > this looks easy. And I don't think you even need a patch for this.
> >> 
> >> ...however as we've discussed previously, we do need a bit of
> >> infrastructure around this. In particular, we need to embed the embed
> >> the BTF ID into the metadata itself so BPF can do runtime disambiguation
> >> between different formats (and add the right CO-RE primitives to make
> >> this easy). This is for two reasons:
> >> 
> >> - The metadata might be different per-packet (e.g., PTP packets with
> >>   timestamps interleaved with bulk data without them)
> >> 
> >> - With redirects we may end up processing packets from different devices
> >>   in a single XDP program (in devmap or cpumap, or on a veth) so we need
> >>   to be able to disambiguate at runtime.
> >> 
> >> So I think the part of the design that puts the BTF ID into the end of
> >> the metadata struct is sound; however, the actual format doesn't have to
> >> be fixed, we can use CO-RE to pick out the bits that a given BPF program
> >> needs; we just need a convention for how drivers report which format(s)
> >> they support. Which we should also agree on (and add core infrastructure
> >> around) so each driver doesn't go around inventing their own
> >> conventions.
> >> 
> >> >> The enablement of this feature is controlled on attaching/replacing
> >> >> XDP program on an interface with two new parameters: that combined
> >> >> BTF+type ID and metadata threshold.
> >> >> The threshold specifies the minimum frame size which a driver (or
> >> >> NIC) should start composing metadata from. It is introduced instead
> >> >> of just false/true flag due to that often it's not worth it to spend
> >> >> cycles to fetch all that data for such small frames: let's say, it
> >> >> can be even faster to just calculate checksums for them on CPU
> >> >> rather than touch non-coherent DMA zone. Simple XDP_DROP case loses
> >> >> 15 Mpps on 64 byte frames with enabled metadata, threshold can help
> >> >> mitigate that.
> >> >
> >> > I would put this in the bonus category. Can you do the simple thing
> >> > above without these extra bits and then add them later. Just
> >> > pick some overly conservative threshold to start with.
> >> 
> >> Yeah, I'd agree this kind of configuration is something that can be
> >> added later, and also it's sort of orthogonal to the consumption of the
> >> metadata itself.
> >> 
> >> Also, tying this configuration into the loading of an XDP program is a
> >> terrible interface: these are hardware configuration options, let's just
> >> put them into ethtool or 'ip link' like any other piece of device
> >> configuration.
> >
> > I don't believe it fits there, especially Ethtool. Ethtool is for
> > hardware configuration, XDP/AF_XDP is 95% software stuff (apart from
> > offload bits which is purely NFP's for now).
> 
> But XDP-hints is about consuming hardware features. When you're
> configuring which metadata items you want, you're saying "please provide
> me with these (hardware) features". So ethtool is an excellent place to
> do that :)

With Ethtool you configure the hardware, e.g. it won't strip VLAN
tags if you disable rx-cvlan-stripping. With configuring metadata
you only tell what you want to see there, don't you?

> 
> > I follow that way:
> >
> > 1) you pick a program you want to attach;
> > 2) usually they are written for special needs and usecases;
> > 3) so most likely that program will be tied with metadata/driver/etc
> >    in some way;
> > 4) so you want to enable Hints of a particular format primarily for
> >    this program and usecase, same with threshold and everything
> >    else.
> >
> > Pls explain how you see it, I might be wrong for sure.
> 
> As above: XDP hints is about giving XDP programs (and AF_XDP consumers)
> access to metadata that is not currently available. Tying the lifetime
> of that hardware configuration (i.e., which information to provide) to
> the lifetime of an XDP program is not a good interface: for one thing,
> how will it handle multiple programs? What about when XDP is not used at

Multiple progs is stuff I didn't cover, but will do later (as you
all say to me, "let's start with something simple" :)). Aaaand
multiple XDP progs (I'm not talking about attaching progs in
differeng modes) is not a kernel feature, rather a libpf feature,
so I believe it should be handled there later...

> all but you still want to configure the same features?

What's the point of configuring metadata when there are no progs
attached? To configure it once and not on every prog attach? I'm
not saying I don't like it, just want to clarify.
Maybe I need opinions from some more people, just to have an
overview of how most of folks see it and would like to configure
it. 'Cause I heard from at least one of the consumers that
libpf API is a perfect place for Hints to him :)

> 
> In addition, in every other case where we do dynamic data access (with
> CO-RE) the BPF program is a consumer that modifies itself to access the
> data provided by the kernel. I get that this is harder to achieve for
> AF_XDP, but then let's solve that instead of making a totally
> inconsistent interface for XDP.

I also see CO-RE more fitting and convenient way to use them, but
didn't manage to solve two things:

1) AF_XDP programs, so what to do with them? Prepare patches for
   LLVM to make it able to do CO-RE on AF_XDP program load? Or
   just hardcode them for particular usecases and NICs? What about
   "general-purpose" programs?
   And if hardcode, what's the point then to do Generic Hints at
   all? Then all it needs is making driver building some meta in
   front of frames via on-off button and that's it? Why BTF ID in
   the meta then if consumers will access meta hardcoded (via CO-RE
   or literally hardcoded, doesn't matter)?
2) In-kernel metadata consumers? Also do CO-RE? Otherwise, with no
   generic metadata structure they won't be able to benefit from
   Hints. But I guess we still need to provide kernel with meta?
   Or no?

> 
> I'm as excited as you about the prospect of having totally programmable

But I mostly care about current generation with no programmable
Hints...

> hardware where you can just specify any arbitrary metadata format and
> it'll provide that for you. But that is an orthogonal feature: let's
> start with creating a dynamic interface for consuming the (static)
> hardware features we already have, and then later we can have a separate
> interface for configuring more dynamic hardware features. XDP-hints is
> about adding this consumption feature in a way that's sufficiently
> dynamic that we can do the other (programmable hardware) thing on top
> later...
> 
> -Toke

Thanks,
Olek

Powered by blists - more mailing lists