* Re: [RFC PATCH bpf-next] bpf, xdp: per-map bpf_redirect_map functions for XDP [not found] <20210129153215.190888-1-bjorn.topel@gmail.com> @ 2021-01-29 16:45 ` Toke Høiland-Jørgensen 2021-02-01 6:27 ` Björn Töpel [not found] ` <CAJ+HfNiFtRd-KKMB1t3Mi3MZ=C+u5TTM5YFnzJFfR4Ruzc7c9Q@mail.gmail.com> 1 sibling, 1 reply; 5+ messages in thread From: Toke Høiland-Jørgensen @ 2021-01-29 16:45 UTC (permalink / raw) To: Björn Töpel, ast, daniel, netdev, bpf Cc: Björn Töpel, magnus.karlsson, maciej.fijalkowski, kuba, jonathan.lemon, maximmi, davem, hawk, john.fastabend Björn Töpel <bjorn.topel@gmail.com> writes: > From: Björn Töpel <bjorn.topel@intel.com> > > Currently the bpf_redirect_map() implementation dispatches to the > correct map-lookup function via a switch-statement. To avoid the > dispatching, this change adds one bpf_redirect_map() implementation per > map. Correct function is automatically selected by the BPF verifier. > > Signed-off-by: Björn Töpel <bjorn.topel@intel.com> > --- > Hi XDP-folks! > > This is another take on my bpf_redirect_xsk() patch [1]. I figured I > send it as an RFC for some early input. My plan is to include it as > part of the xdp_do_redirect() optimization of [1]. Assuming the maintainers are OK with the special-casing in the verifier, this looks like a neat way to avoid the runtime overhead to me. The macro hackery is not the prettiest; I wonder if the same effect could be achieved by using inline functions? If not, at least a comment explaining the reasoning (and that the verifier will substitute the right function) might be nice? Mostly in relation to this bit: > static const struct bpf_func_proto bpf_xdp_redirect_map_proto = { > - .func = bpf_xdp_redirect_map, > + .func = bpf_xdp_redirect_devmap, Ah, if only we were writing the kernel in a language with proper macro support... One can dream! :) >> For AF_XDP rxdrop this yields +600Mpps. I'll do CPU/DEVMAP >> measurements for the patch proper. >> > > Kpps, not Mpps. :-P Aww, too bad ;) Still, nice! -Toke ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH bpf-next] bpf, xdp: per-map bpf_redirect_map functions for XDP 2021-01-29 16:45 ` [RFC PATCH bpf-next] bpf, xdp: per-map bpf_redirect_map functions for XDP Toke Høiland-Jørgensen @ 2021-02-01 6:27 ` Björn Töpel 2021-02-01 9:31 ` Jesper Dangaard Brouer 0 siblings, 1 reply; 5+ messages in thread From: Björn Töpel @ 2021-02-01 6:27 UTC (permalink / raw) To: Toke Høiland-Jørgensen, Björn Töpel, ast, daniel, netdev, bpf Cc: magnus.karlsson, maciej.fijalkowski, kuba, jonathan.lemon, maximmi, davem, hawk, john.fastabend On 2021-01-29 17:45, Toke Høiland-Jørgensen wrote: > Björn Töpel <bjorn.topel@gmail.com> writes: > >> From: Björn Töpel <bjorn.topel@intel.com> >> >> Currently the bpf_redirect_map() implementation dispatches to the >> correct map-lookup function via a switch-statement. To avoid the >> dispatching, this change adds one bpf_redirect_map() implementation per >> map. Correct function is automatically selected by the BPF verifier. >> >> Signed-off-by: Björn Töpel <bjorn.topel@intel.com> >> --- >> Hi XDP-folks! >> >> This is another take on my bpf_redirect_xsk() patch [1]. I figured I >> send it as an RFC for some early input. My plan is to include it as >> part of the xdp_do_redirect() optimization of [1]. > > Assuming the maintainers are OK with the special-casing in the verifier, > this looks like a neat way to avoid the runtime overhead to me. The > macro hackery is not the prettiest; I wonder if the same effect could be > achieved by using inline functions? If not, at least a comment > explaining the reasoning (and that the verifier will substitute the > right function) might be nice? Mostly in relation to this bit: > Yeah, I agree with the macro part. I'll replace it with a __always_inline function, instead. >> static const struct bpf_func_proto bpf_xdp_redirect_map_proto = { >> - .func = bpf_xdp_redirect_map, >> + .func = bpf_xdp_redirect_devmap, > I'll try to clean this up as well. Thanks for taking a look! Björn > Ah, if only we were writing the kernel in a language with proper macro > support... One can dream! :) > >>> For AF_XDP rxdrop this yields +600Mpps. I'll do CPU/DEVMAP >>> measurements for the patch proper. >>> >> >> Kpps, not Mpps. :-P > > Aww, too bad ;) > Still, nice! > > -Toke > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH bpf-next] bpf, xdp: per-map bpf_redirect_map functions for XDP 2021-02-01 6:27 ` Björn Töpel @ 2021-02-01 9:31 ` Jesper Dangaard Brouer 2021-02-01 9:49 ` Björn Töpel 0 siblings, 1 reply; 5+ messages in thread From: Jesper Dangaard Brouer @ 2021-02-01 9:31 UTC (permalink / raw) To: Björn Töpel Cc: Toke Høiland-Jørgensen, Björn Töpel, ast, daniel, netdev, bpf, magnus.karlsson, maciej.fijalkowski, kuba, jonathan.lemon, maximmi, davem, hawk, john.fastabend On Mon, 1 Feb 2021 07:27:57 +0100 Björn Töpel <bjorn.topel@intel.com> wrote: > On 2021-01-29 17:45, Toke Høiland-Jørgensen wrote: > > Björn Töpel <bjorn.topel@gmail.com> writes: > > > >> From: Björn Töpel <bjorn.topel@intel.com> > >> > >> Currently the bpf_redirect_map() implementation dispatches to the > >> correct map-lookup function via a switch-statement. To avoid the > >> dispatching, this change adds one bpf_redirect_map() implementation per > >> map. Correct function is automatically selected by the BPF verifier. > >> > >> Signed-off-by: Björn Töpel <bjorn.topel@intel.com> > >> --- > >> Hi XDP-folks! > >> > >> This is another take on my bpf_redirect_xsk() patch [1]. I figured I > >> send it as an RFC for some early input. My plan is to include it as > >> part of the xdp_do_redirect() optimization of [1]. > > > > Assuming the maintainers are OK with the special-casing in the verifier, > > this looks like a neat way to avoid the runtime overhead to me. The > > macro hackery is not the prettiest; I wonder if the same effect could be > > achieved by using inline functions? If not, at least a comment > > explaining the reasoning (and that the verifier will substitute the > > right function) might be nice? Mostly in relation to this bit: > > > > Yeah, I agree with the macro part. I'll replace it with a > __always_inline function, instead. > Yes, I also prefer __always_inline over the macro. > >> static const struct bpf_func_proto bpf_xdp_redirect_map_proto = { > >> - .func = bpf_xdp_redirect_map, > >> + .func = bpf_xdp_redirect_devmap, > > > > I'll try to clean this up as well. I do like the optimization of having the verifier call the right map func directly. Could you please add a descriptive comment that describe this above "bpf_xdp_redirect_map_proto", that this is happening in fixup_bpf_calls and use get_xdp_redirect_func (what you define). It is a cool trick, but people reading the code will have a hard time following. Surprisingly people do read this code and tries to follow. I've had discussions on the Cilium Slack channel, where people misunderstood how our bpf_fib_lookup() calls gets mapped to two different functions depending on context (SKB vs XDP). And that remapping happens in the same file (net/core/filter.c). -- Best regards, Jesper Dangaard Brouer MSc.CS, Principal Kernel Engineer at Red Hat LinkedIn: http://www.linkedin.com/in/brouer ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH bpf-next] bpf, xdp: per-map bpf_redirect_map functions for XDP 2021-02-01 9:31 ` Jesper Dangaard Brouer @ 2021-02-01 9:49 ` Björn Töpel 0 siblings, 0 replies; 5+ messages in thread From: Björn Töpel @ 2021-02-01 9:49 UTC (permalink / raw) To: Jesper Dangaard Brouer Cc: Toke Høiland-Jørgensen, Björn Töpel, ast, daniel, netdev, bpf, magnus.karlsson, maciej.fijalkowski, kuba, jonathan.lemon, maximmi, davem, hawk, john.fastabend On 2021-02-01 10:31, Jesper Dangaard Brouer wrote: > On Mon, 1 Feb 2021 07:27:57 +0100 > Björn Töpel <bjorn.topel@intel.com> wrote: > >> On 2021-01-29 17:45, Toke Høiland-Jørgensen wrote: >>> Björn Töpel <bjorn.topel@gmail.com> writes: >>> >>>> From: Björn Töpel <bjorn.topel@intel.com> >>>> >>>> Currently the bpf_redirect_map() implementation dispatches to the >>>> correct map-lookup function via a switch-statement. To avoid the >>>> dispatching, this change adds one bpf_redirect_map() implementation per >>>> map. Correct function is automatically selected by the BPF verifier. >>>> >>>> Signed-off-by: Björn Töpel <bjorn.topel@intel.com> >>>> --- >>>> Hi XDP-folks! >>>> >>>> This is another take on my bpf_redirect_xsk() patch [1]. I figured I >>>> send it as an RFC for some early input. My plan is to include it as >>>> part of the xdp_do_redirect() optimization of [1]. >>> >>> Assuming the maintainers are OK with the special-casing in the verifier, >>> this looks like a neat way to avoid the runtime overhead to me. The >>> macro hackery is not the prettiest; I wonder if the same effect could be >>> achieved by using inline functions? If not, at least a comment >>> explaining the reasoning (and that the verifier will substitute the >>> right function) might be nice? Mostly in relation to this bit: >>> >> >> Yeah, I agree with the macro part. I'll replace it with a >> __always_inline function, instead. >> > > Yes, I also prefer __always_inline over the macro. > Ok! Good! > >>>> static const struct bpf_func_proto bpf_xdp_redirect_map_proto = { >>>> - .func = bpf_xdp_redirect_map, >>>> + .func = bpf_xdp_redirect_devmap, >>> >> >> I'll try to clean this up as well. > > I do like the optimization of having the verifier call the right map > func directly. Could you please add a descriptive comment that > describe this above "bpf_xdp_redirect_map_proto", that this is > happening in fixup_bpf_calls and use get_xdp_redirect_func (what you > define). It is a cool trick, but people reading the code will have a > hard time following. > Good idea, and makes sense! I'll make sure to do that! Thanks for the input! Cheers, Björn > Surprisingly people do read this code and tries to follow. I've had > discussions on the Cilium Slack channel, where people misunderstood how > our bpf_fib_lookup() calls gets mapped to two different functions > depending on context (SKB vs XDP). And that remapping happens in the > same file (net/core/filter.c). > ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <CAJ+HfNiFtRd-KKMB1t3Mi3MZ=C+u5TTM5YFnzJFfR4Ruzc7c9Q@mail.gmail.com>]
* Re: [RFC PATCH bpf-next] bpf, xdp: per-map bpf_redirect_map functions for XDP [not found] ` <CAJ+HfNiFtRd-KKMB1t3Mi3MZ=C+u5TTM5YFnzJFfR4Ruzc7c9Q@mail.gmail.com> @ 2021-01-29 18:06 ` Jesper Dangaard Brouer 0 siblings, 0 replies; 5+ messages in thread From: Jesper Dangaard Brouer @ 2021-01-29 18:06 UTC (permalink / raw) To: Björn Töpel Cc: Alexei Starovoitov, Daniel Borkmann, Netdev, bpf, Björn Töpel, Karlsson, Magnus, Fijalkowski, Maciej, Jakub Kicinski, Jonathan Lemon, maximmi, David Miller, Jesper Dangaard Brouer, John Fastabend, Toke Høiland-Jørgensen On Fri, 29 Jan 2021 16:35:47 +0100 Björn Töpel <bjorn.topel@gmail.com> wrote: > On Fri, 29 Jan 2021 at 16:32, Björn Töpel <bjorn.topel@gmail.com> wrote: > > > > From: Björn Töpel <bjorn.topel@intel.com> > > > [...] > > > > For AF_XDP rxdrop this yields +600Mpps. I'll do CPU/DEVMAP > > measurements for the patch proper. > > > > Kpps, not Mpps. :-P +600Kpps from 24Mpps to 24.6Mpps I assume. This corresponds to approx 1 ns ((1/24-1/24.6)*1000 = 1.01626 ns). This also correlate with saving one function call, which is basically what the patch does. Fresh measurement "Intel(R) Xeon(R) CPU E5-1650 v4 @ 3.60GHz" with [1]: time_bench: Type:funcion_call_cost Per elem: 3 cycles(tsc) 1.053 ns time_bench: Type:func_ptr_call_cost Per elem: 4 cycles(tsc) 1.317 ns [1] https://github.com/netoptimizer/prototype-kernel/blob/master/kernel/lib/time_bench_sample.c -- Best regards, Jesper Dangaard Brouer MSc.CS, Principal Kernel Engineer at Red Hat LinkedIn: http://www.linkedin.com/in/brouer ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-02-01 9:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20210129153215.190888-1-bjorn.topel@gmail.com>
2021-01-29 16:45 ` [RFC PATCH bpf-next] bpf, xdp: per-map bpf_redirect_map functions for XDP Toke Høiland-Jørgensen
2021-02-01 6:27 ` Björn Töpel
2021-02-01 9:31 ` Jesper Dangaard Brouer
2021-02-01 9:49 ` Björn Töpel
[not found] ` <CAJ+HfNiFtRd-KKMB1t3Mi3MZ=C+u5TTM5YFnzJFfR4Ruzc7c9Q@mail.gmail.com>
2021-01-29 18:06 ` Jesper Dangaard Brouer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).