Zvi Effron <zeffron@xxxxxxxxxxxxx> writes:
> On Wed, Sep 22, 2021 at 1:03 PM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
>>
>> Jakub Kicinski <kuba@xxxxxxxxxx> writes:
>>
>> > On Tue, 21 Sep 2021 18:06:35 +0200 Toke Høiland-Jørgensen wrote:
>> >> 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
