Date:   Fri, 15 Jul 2022 13:11:03 +0200
From:   Magnus Karlsson <magnus.karlsson@...il.com>
To:     Jesper Dangaard Brouer <jbrouer@...hat.com>
Cc:     Toke Høiland-Jørgensen <toke@...hat.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        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 <bpf@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        open list <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

On Tue, Jul 12, 2022 at 4:15 PM Jesper Dangaard Brouer
<jbrouer@...hat.com> wrote:
>
>
>
> On 12/07/2022 12.33, Magnus Karlsson wrote:
> > On Thu, Jul 7, 2022 at 1:25 AM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
> >>
> >> Alexander Lobakin <alexandr.lobakin@...el.com> writes:
> >>
> >>> From: Toke H??iland-J??rgensen <toke@...hat.com>
> >>> Date: Tue, 05 Jul 2022 20:51:14 +0200
> >>>
> >>>> Alexander Lobakin <alexandr.lobakin@...el.com> writes:
> >>>>
> >>>> [... snipping a bit of context here ...]
> >>>>
> >>>>>>>> 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?
> >>>>
> >>>> Ah, I think we may be getting closer to identifying the disconnect
> >>>> between our way of thinking about this!
> >>>>
> >>>> In my mind, there's no separate "configuration of the metadata" step.
> >>>> You simply tell the hardware what features you want (say, "enable
> >>>> timestamps and VLAN offload"), and the driver will then provide the
> >>>> information related to these features in the metadata area
> >>>> unconditionally. All XDP hints is about, then, is a way for the driver
> >>>> to inform the rest of the system how that information is actually laid
> >>>> out in the metadata area.
> >>>>
> >>>> Having a separate configuration knob to tell the driver "please lay out
> >>>> these particular bits of metadata this way" seems like a totally
> >>>> unnecessary (and quite complicated) feature to have when we can just let
> >>>> the driver decide and use CO-RE to consume it?
> >>>
> >>> Magnus (he's currently on vacation) told me it would be useful for
> >>> AF_XDP to enable/disable particular metadata, at least from perf
> >>> perspective. Let's say, just fetching of one "checksum ok" bit in
> >>> the driver is faster than walking through all the descriptor words
> >>> and driver logics (i.e. there's several hundred locs in ice which
> >>> just parse descriptor data and build an skb or metadata from it).
> >>> But if we would just enable/disable corresponding features through
> >>> Ethtool, that would hurt XDP_PASS. Maybe it's a bad example, but
> >>> what if I want to have only RSS hash in the metadata (and don't
> >>> want to spend cycles on parsing the rest), but at the same time
> >>> still want skb path to have checksum status to not die at CPU
> >>> checksum calculation?
> >>
> >> Hmm, so this feels a little like a driver-specific optimisation? I.e.,
> >> my guess is that not all drivers have a measurable overhead for pulling
> >> out the metadata. Also, once the XDP metadata bits are in place, we can
> >> move in the direction of building SKBs from the same source, so I'm not
> >> sure it's a good idea to assume that the XDP metadata is separate from
> >> what the stack consumes...
> >>
> >> In any case, if such an optimisation does turn out to be useful, we can
> >> add it later (backed by rigorous benchmarks, of course), so I think we
> >> can still start with the simple case and iterate from there?
> >
> > Just to check if my intuition was correct or not I ran some benchmarks
> > around this. I ported Jesper's patch set to the zero-copy driver of
> > i40e, which was really simple thanks to Jesper's refactoring. One line
> > of code added to the data path of the zc driver and making
> > i40e_process_xdp_hints() a global function so it can be reached from
> > the zc driver.
>
> Happy to hear it was simple to extend this to AF_XDP in the driver.
> Code design wise I'm trying to keep it simple for drivers to add this.
> I have a co-worker that have already extended ixgbe.
>
> > I also moved the prefetch Jesper added to after the
> > check if xdp_hints are available since it really degrades performance
> > in the xdp_hints off case.
>
> Good to know.
>
> > First number is the throughput change with hints on, and the second
> > number is with hints off. All are compared to the performance without
> > Jesper's patch set applied. The application is xdpsock -r (which used
> > to be part of the samples/bpf directory).
>
> For reviewer to relate to these numbers we need to understand/explain
> the extreme numbers we are dealing with.  In my system with i40e and
> xdpsock --rx-drop I can AF_XDP drop packets with a rate of 33.633.761 pps.
>
> This corresponds to a processing time per packet: 29.7 ns (nanosec)
>   - Calc: (1/33633761)*10^9
>
> > Copy mode with all hints: -21% / -2%

On my system, the overhead is 66 cycles/packet or 31 ns/packet (2.1
GHz CPU with TurboBoost disabled). Copy-mode only drops packets at a
rate of 8.5 Mpps or 118 ns/packet on my system. The rate you quote
must be for zero-copy as I see something similar there if I enable
TurboBoost on my system.

> The -21% for enabling all hints does sound like an excessive overhead,
> but time-wise this is a reduction/overhead of 6.2 ns.
>
> The real question: Is this 6.2 ns overhead that gives us e.g.
> RX-checksumming lower than the gain we can obtain from avoiding doin.
> RX-checksumming in software?
>   - A: My previous experiments conclude[1] that for 1500 bytes frames we
>     can save 54 ns (or increase performance with 8% for normal netstack).

If you use Rx-checksumming alone, it is a good idea for packets that
are bigger than something around 500 bytes, if you use copy mode. This
is a very rough estimation since I cannot mix your numbers with mine.
But there is a substantial window where it pays off for sure. For ZC,
this window is even larger, see below.

>
> I was going for zero overhead when disabling xdp-hints, which is almost
> true as the -2% is time-wise a reduction/overhead of 0.59 ns.
>
>   [1]
> https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp_frame01_checksum.org#measurements-compare-results--conclusion
>
>
> > Zero-copy mode with all hints: -29% / -9%
>
> I'm unsure why the percentages increase here, perhaps because zero-copy
> is faster and thus the overhead becomes a larger percentage?

For zero-copy, the overhead is 31 cycles/packet or 15 ns/packet on my
system. I would have expected the cycles/packet overhead for copy-mode
and zero-copy mode to be about the same since they use the same hints
code, but it is roughly half for zero-copy. Have not examined why. The
packet processing time without your patches on my system is 36
ns/packet or 27.65 Mpps for zero-copy.

>
> > Copy mode rx timestamp only (the rest removed with an #if 0): -11%
> > Zero-copy mode rx timestamp only: -20%
> >
> > So, if you only want rx timestamp, but can only enable every hint or
> > nothing, then you get a 10% performance degradation with copy mode and
> > 9% with zero-copy mode compared to if you were able just to enable rx
> > timestamp alone. With these rough numbers (a real implementation would
> > not have an #if 0) I would say it matters, but that does not mean we
> > should not start simple and just have a big switch to start with. But
> > as we add hints (to the same btfid), this will just get worse.
>
> IMHO we *do* already have individual enable/disable hints features via
> ethtool.
> Have you tried to use the individual ethtool switches. e.g.:
>
>   ethtool -K i40e2 rx-checksumming off
>
> The i40e code uses bitfields for extracting the descriptor, which cause
> code that isn't optimal or fully optimized by the compiler.  On my setup
> I gained 4.2% (or 1.24 ns) by doing this.

Forgot about that one. Will replace the bitfields and rerun the
experiments to get the overhead down.

>
> > Here are some other numbers I got, in case someone is interested. They
> > are XDP numbers from xdp_rxq_info in samples/bpf.
> >
> > hints on / hints off
> > XDP_DROP: -18% / -1.5%
>
> My xdp_rxq_info (no-touch XDP_DROP) nanosec numbers are:
>
>            hints on / hints off
>   XDP_DROP: 35.97ns / 29.80ns  (diff 6.17 ns)
>
> Maybe interesting if I touch data (via option --read), then the overhead
> is reduced to 4.84 ns.

Good point. We should always touch the data. Will include that in the
next set of experiments.

> --Jesper
>
> > XDP_TX: -10% / -2.5%
> >
> >>>>>>> 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...
> >>>>
> >>>> Right, but even if we don't *implement* it straight away we still need
> >>>> to take it into consideration in the design. And expecting libxdp to
> >>>> arbitrate between different XDP programs' metadata formats sounds like a
> >>>> royal PITA :)
> >>>>
> >>>>>> 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.
> >>>>
> >>>> See above: you turn on the features because you want the stack to
> >>>> consume them.
> >>>>
> >>>>> 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 :)
> >>>>
> >>>> Well, as a program author who wants to consume hints, you'd use
> >>>> lib{bpf,xdp} APIs to do so (probably in the form of suitable CO-RE
> >>>> macros)...
> >>>>
> >>>>>> 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?
> >>>>
> >>>> You provide a library to read the fields. Jesper actually already
> >>>> implemented this, did you look at his code?
> >>>>
> >>>> https://github.com/xdp-project/bpf-examples/tree/master/AF_XDP-interaction
> >>>>
> >>>> It basically builds a lookup table at load-time using BTF information
> >>>> from the kernel, keyed on BTF ID and field name, resolving them into
> >>>> offsets. It's not quite the zero-overhead of CO-RE, but it's fairly
> >>>> close and can be improved upon (CO-RE for userspace being one way of
> >>>> doing that).
> >>>
> >>> Aaaah, sorry, I completely missed that. I thought of something
> >>> similar as well, but then thought "variable field offsets, that
> >>> would annihilate optimization and performance", and our Xsk team
> >>> is super concerned about performance hits when using Hints.
> >>>
> >>>>
> >>>>>     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)?
> >>>>
> >>>> You're quite right, we could probably implement all the access to
> >>>> existing (fixed) metadata without using any BTF at all - just define a
> >>>> common struct and some flags to designate which fields are set. In my
> >>>> mind, there are a couple of reasons for going the BTF route instead:
> >>>>
> >>>> - We can leverage CO-RE to get close to optimal efficiency in field
> >>>>    access.
> >>>>
> >>>> and, more importantly:
> >>>>
> >>>> - It's infinitely extensible. With the infrastructure in place to make
> >>>>    it really easy to consume metadata described by BTF, we lower the bar
> >>>>    for future innovation in hardware offloads. Both for just adding new
> >>>>    fixed-function stuff to hardware, but especially for fully
> >>>>    programmable hardware.
> >>>
> >>> Agree :) That libxdp lookup translator fixed lots of stuff in my
> >>> mind.
> >>
> >> Great! Looks like we're slowly converging towards a shared
> >> understanding, then! :)
> >>
> >>>>> 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?
> >>>>
> >>>> In the short term, I think the "generic structure" approach is fine for
> >>>> leveraging this in the stack. Both your and Jesper's series include
> >>>> this, and I think that's totally fine. Longer term, if it turns out to
> >>>> be useful to have something more dynamic for the stack consumption as
> >>>> well, we could extend it to be CO-RE based as well (most likely by
> >>>> having the stack load a "translator" BPF program or something along
> >>>> those lines).
> >>>
> >>> Oh, that translator prog sounds nice BTW!
> >>
> >> Yeah, it's only a rough idea Jesper and I discussed at some point, but I
> >> think it could have potential (see also point above re: making XDP hints
> >> *the* source of metadata for the whole stack; wouldn't it be nice if
> >> drivers didn't have to deal with the intricacies of assembling SKBs?).
> >>
> >> -Toke
> >>
> >
>

Powered by blists - more mailing lists