On Thu, 7 May 2020 at 16:48, Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote: > [] > Well, my immediate thought would be that the added complexity would not > be worth it, because: > > - A new action would mean either you'd need to patch all drivers or > (more likely) we'd end up with yet another difference between drivers' > XDP support. > Right, but it would be trivial to add for drivers that already support XDP_REDIRECT, so I'm not worried about that particular problem. That aside, let's move on. I agree that adding action should be avoided! > - BPF developers would suddenly have to choose - do this new faster > thing, or be compatible? And manage the choice based on drivers they > expect to run on, etc. This was already confusing with > bpf_redirect()/bpf_redirect_map(), and this would introduce a third > option! > True. For the sake of the argument; Adding flags vs adding a new helper, i.e. bpf_redirect_map(flag_with_new_semantic) vs a new helper. Today XDP developers that use bpf_redirect_map() need to consider whether the kernel support the "pass action via flags" or not, so this would be a *fourth* option. :-P I'm with you here. The best option would be a transparent one. > So in light of this, I'd say the performance benefit would have to be > quite substantial for this to be worth it. Which we won't know until you > try it, I guess :) > Hear, hear! > Thinking of alternatives - couldn't you shoe-horn this into the existing > helper and return code? Say, introduce an IMMEDIATE_RETURN flag to the > existing helpers, which would change the behaviour to the tail call > semantics. When used, xdp_do_redirect() would then return immediately > (or you could even turn xdp_do_redirect() into an inlined wrapper that > checks the flag before issuing a CALL to the existing function). Any > reason why that wouldn't work? > Sure, but this wouldn't remove the per-cpu/bpf_redirect_info lookup. Then again, maybe it's better to start there. To clarify, just a flag isn't sufficient. It would need to be a guarantee that the program exists after the call, i.e. tail call support. From clang/BPF instruction (Alexei's/John's replies), or something like bpf_tail_call(). Unless I'm missing something... Or do you mean that the flag IMMEDIATE_RETURN would perform the action in the helper? The context would be stale after that call, and the verifier should reject a program that pokes the context, but the flag is a runtime thing. It sounds like a pretty complex verifier task to determine if IMMEDIATE_RETURN is set, and then reject ctx accesses there. Thanks for the input, and good ideas! Björn > -Toke >
