On 07/07/2022 01.22, Toke Høiland-Jørgensen wrote:
Alexander Lobakin <alexandr.lobakin@xxxxxxxxx> writes:
From: Toke H??iland-J??rgensen <toke@xxxxxxxxxx>
Date: Tue, 05 Jul 2022 20:51:14 +0200
Alexander Lobakin <alexandr.lobakin@xxxxxxxxx> 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.
I have recently talked to Magnus (in person at Kernel Recipes), where I
tried to convey my opinion, which is: At least for existing hardware
hints, we need to respect the existing Linux kernel's config interfaces,
and not invent yet-another-way to configure these.
(At least for now) the kernel module defined structs in C-code is the
source of truth, and we consume these layouts via BTF information
provided by the kernel for our XDP-hints.
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...
I agree.
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?
For every element in the generic hints data-structure, we already have a
per-element enable/disable facilities. As they are already controlled
by ethtool. Except the timestamping, which can be enabled via a sockopt.
I don't see a benefit of creating another layer (of if-statements) that
are also required to get the HW hint written to XDP-hints metadata area.
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?).
Yes, this is the longer term goal, but we should take this in steps.
(Thus, my patchset[0] focuses on the existing xdp_hints_common).
Eventually (pipe-dream?), I would like to add a new BPF-hook that runs
in the step converting xdp_frame to SKB (today handled in function
__xdp_build_skb_from_frame). This "translator" BPF program should be
tied/loaded per net_device, which makes it easier to consume the driver
specific/dynamic XDP-hints layouts and BPF-code can be smaller as it
only need to CO-RE handle xdp-hints structs known for this driver.
Default BPF-prog should be provided and maintained by driver
maintainers, but can be replaced by end-users.
--Jesper
[0]
https://lore.kernel.org/bpf/165643378969.449467.13237011812569188299.stgit@firesoul/
