Andrii Nakryiko <andrii.nakry...@gmail.com> writes: > On Sat, Jun 22, 2019 at 7:19 PM Toke Høiland-Jørgensen <t...@redhat.com> > wrote: >> >> When using the bpf_redirect_map() helper to redirect packets from XDP, the >> eBPF >> program cannot currently know whether the redirect will succeed, which makes >> it >> impossible to gracefully handle errors. To properly fix this will probably >> require deeper changes to the way TX resources are allocated, but one thing >> that >> is fairly straight forward to fix is to allow lookups into devmaps, so >> programs >> can at least know when a redirect is *guaranteed* to fail because there is no >> entry in the map. Currently, programs work around this by keeping a shadow >> map >> of another type which indicates whether a map index is valid. >> >> This series contains two changes that are complementary ways to fix this >> issue: >> >> - Moving the map lookup into the bpf_redirect_map() helper (and caching the >> result), so the helper can return an error if no value is found in the map. >> This includes a refactoring of the devmap and cpumap code to not care about >> the index on enqueue. >> >> - Allowing regular lookups into devmaps from eBPF programs, using the >> read-only >> flag to make sure they don't change the values. >> >> The performance impact of the series is negligible, in the sense that I >> cannot >> measure it because the variance between test runs is higher than the >> difference >> pre/post series. >> >> Changelog: >> >> v5: >> - Rebase on latest bpf-next. >> - Update documentation for bpf_redirect_map() with the new meaning of >> flags. >> >> v4: >> - Fix a few nits from Andrii >> - Lose the #defines in bpf.h and just compare the flags argument directly >> to >> XDP_TX in bpf_xdp_redirect_map(). >> >> v3: >> - Adopt Jonathan's idea of using the lower two bits of the flag value as >> the >> return code. >> - Always do the lookup, and cache the result for use in xdp_do_redirect(); >> to >> achieve this, refactor the devmap and cpumap code to get rid the bitmap >> for >> selecting which devices to flush. >> >> v2: >> - For patch 1, make it clear that the change works for any map type. >> - For patch 2, just use the new BPF_F_RDONLY_PROG flag to make the return >> value read-only. >> >> --- >> >> Toke Høiland-Jørgensen (3): >> devmap/cpumap: Use flush list instead of bitmap >> bpf_xdp_redirect_map: Perform map lookup in eBPF helper >> devmap: Allow map lookups from eBPF >> >> >> include/linux/filter.h | 1 >> include/uapi/linux/bpf.h | 7 ++- >> kernel/bpf/cpumap.c | 106 ++++++++++++++++++++----------------------- >> kernel/bpf/devmap.c | 113 >> ++++++++++++++++++++++------------------------ >> kernel/bpf/verifier.c | 7 +-- >> net/core/filter.c | 29 +++++------- >> 6 files changed, 123 insertions(+), 140 deletions(-) >> > > > Looks like you forgot to add my Acked-by's for your patches?
Ah yes, did not carry those forward for the individual patches, my apologies. Could you perhaps be persuaded to send a new one (I believe a response to the cover letter acking the whole series would suffice)? I'll make sure to add the carrying forward of acks into my workflow in the future :) -Toke
