Zvi Effron [off-list ref] writes:

On Wed, Sep 22, 2021 at 1:03 PM Toke Høiland-Jørgensen [off-list ref] wrote:

quoted

Jakub Kicinski [off-list ref] writes:

quoted

On Tue, 21 Sep 2021 18:06:35 +0200 Toke Høiland-Jørgensen wrote:

quoted

1. Do nothing. This would make it up to users / sysadmins to avoid
anything breaking by manually making sure to not enable multi-buffer
support while loading any XDP programs that will malfunction if
presented with an mb frame. This will probably break in interesting
ways, but it's nice and simple from an implementation PoV. With this
we don't need the declaration discussed above either.

2. Add a check at runtime and drop the frames if they are mb-enabled and
the program doesn't understand it. This is relatively simple to
implement, but it also makes for difficult-to-understand issues (why
are my packets suddenly being dropped?), and it will incur runtime
overhead.

3. Reject loading of programs that are not MB-aware when running in an
MB-enabled mode. This would make things break in more obvious ways,
and still allow a userspace loader to declare a program "MB-aware" to
force it to run if necessary. The problem then becomes at what level
to block this?

Doing this at the driver level is not enough: while a particular
driver knows if it's running in multi-buff mode, we can't know for
sure if a particular XDP program is multi-buff aware at attach time:
it could be tail-calling other programs, or redirecting packets to
another interface where it will be processed by a non-MB aware
program.

So another option is to make it a global toggle: e.g., create a new
sysctl to enable multi-buffer. If this is set, reject loading any XDP
program that doesn't support multi-buffer mode, and if it's unset,
disable multi-buffer mode in all drivers. This will make it explicit
when the multi-buffer mode is used, and prevent any accidental subtle
malfunction of existing XDP programs. The drawback is that it's a
mode switch, so more configuration complexity.

4. Add new program type, XDP_MB. Do not allow mixing of XDP vs XDP_MB
thru tail calls.

IMHO that's very simple and covers majority of use cases.

Using the program type (or maybe the expected_attach_type) was how I was
imagining we'd encode the "I am MB aware" flag, yes. I hadn't actually
considered that this could be used to also restrict tail call/freplace
attachment, but that's a good point. So this leaves just the redirect
issue, then, see my other reply.

I really like this apporoach as well, but before we commit to it, how likely
are we to encounter this type of situation (where we need to indicate whether
an XDP program supports a new capability) again in the future. And if we do,
are we willing to require that all programs supporting that new feature are
also mb-aware? Essentially, the suboptimal case I'm envisioning is needing to
have XDP_MB, XD_MB_NEW_THING, XDP_NEW_THING, and XDP all as program types. That
leads to exponential explosion in program types.

Hmm, that's a good point. Maybe it would be better to communicate it via
a flag; we do have a prog_flags attribute on BPF_PROG_LOAD we could use.

Also, every time we add a program type to encode a feature (instead of a truly
new type), we're essentially forcing a recompilation (and redeployment) of all
existing programs that take advantage of the libbpf section name program
typing. (I'm sure there are tools that can rename a section in an ELF file
without recompilation, but recompilation seems the simplest way to correct the
ELF files for most people.)

Isn't this true regardless of how we encode it? I mean when we add a new
feature that needs an explicit support declaration, programs need to
declare that they support it somehow. No matter how it's actually
encoded, this will either entail recompiling the program, or having the
loader override the value at load-time.

For instance, say we do use a flag instead of a new prog type, how would
a BPF program set that flag? Defining a new section type in libbpf would
be an obvious answer (i.e., SEC("xdp") sets prog_type=xdp, and
SEC("xdp_mb") sets prog_type=xdp, flags=XDP_MB).

If we think this is a one-off, it's probably fine, but if we think
it'll happen again, is it worth it to find a solution that will work
for future cases now, instead of having XDP, XDP_MB, and then having
to find a solution for future cases?

Well, really the right solution is a general "XDP feature flags"
capability. There was some work done on this, but unfortunately it
stalled out. Not sure it's feasible to resurrect that as a prerequisite
for landing xdp_mb, though, so I guess we'll need a one-off thing here :/

-Toke