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. 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.) 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? --Zvi > -Toke >
