On 10/20/20 5:12 AM, David Ahern wrote:
On 10/19/20 12:23 PM, Daniel Borkmann wrote:
Looks good to me, thanks! I'll wait till David gets a chance as well to
review.
One thing that would have made sense to me (probably worth a v2) is to
keep the
fib lookup flag you had back then, meaning sth like BPF_FIB_SKIP_NEIGH
which
would then return a BPF_FIB_LKUP_RET_NO_NEIGH before doing the neigh
lookup inside
the bpf_ipv{4,6}_fib_lookup() so that programs can just unconditionally
use the
combination of bpf_fib_lookup(skb, [...], BPF_FIB_SKIP_NEIGH) with the
bpf_redirect_neigh([...]) extension in that case and not do this
bpf_redirect()
vs bpf_redirect_neigh() dance as you have in the selftest in patch 2/2.
That puts the overhead on bpf_ipv{4,6}_fib_lookup. The existiong helpers
return BPF_FIB_LKUP_RET_NO_NEIGH which is the key to the bpf program to
call the bpf_redirect_neigh - making the program deal with the overhead
as needed on failures.
But if I know there's high chance anyway that __ipv*_neigh_lookup_noref*()
is failing for bpf_ipv{4,6}_fib_lookup() why even paying the price of the
hash table lookup in there? Simple test to skip and return early would be
much cheaper, and branch predictor should work it out just fine given that
setting is pretty much static anyway; I'm not sure I'm seeing why this would
be much of a concern..
