| Message ID | 20191123071226.6501-3-bjorn.topel@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | Introduce the BPF dispatcher and xdp_call.h | expand |
On Sat, Nov 23, 2019 at 08:12:21AM +0100, Björn Töpel wrote: > From: Björn Töpel <bjorn.topel@intel.com> > > The xdp_call.h header wraps a more user-friendly API around the BPF > dispatcher. A user adds a trampoline/XDP caller using the > DEFINE_XDP_CALL macro, and updates the BPF dispatcher via > xdp_call_update(). The actual dispatch is done via xdp_call(). > > Note that xdp_call() is only supported for builtin drivers. Module > builds will fallback to bpf_prog_run_xdp(). > > The next patch will show-case how the i40e driver uses xdp_call. > > Signed-off-by: Björn Töpel <bjorn.topel@intel.com> > --- > include/linux/xdp_call.h | 66 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 66 insertions(+) > create mode 100644 include/linux/xdp_call.h > > diff --git a/include/linux/xdp_call.h b/include/linux/xdp_call.h > new file mode 100644 > index 000000000000..69b2d325a787 > --- /dev/null > +++ b/include/linux/xdp_call.h > @@ -0,0 +1,66 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* Copyright(c) 2019 Intel Corporation. */ > +#ifndef _LINUX_XDP_CALL_H > +#define _LINUX_XDP_CALL_H > + > +#include <linux/filter.h> > + > +#if defined(CONFIG_BPF_JIT) && defined(CONFIG_RETPOLINE) && !defined(MODULE) > + > +void bpf_dispatcher_change_prog(void *func, struct bpf_prog *from, > + struct bpf_prog *to); > + > +#define XDP_CALL_TRAMP(name) ____xdp_call_##name##_tramp > + > +#define DEFINE_XDP_CALL(name) \ > + unsigned int XDP_CALL_TRAMP(name)( \ > + const void *xdp_ctx, \ > + const struct bpf_insn *insnsi, \ > + unsigned int (*bpf_func)(const void *, \ > + const struct bpf_insn *)) \ > + { \ > + return bpf_func(xdp_ctx, insnsi); \ > + } > + > +#define DECLARE_XDP_CALL(name) \ > + unsigned int XDP_CALL_TRAMP(name)( \ > + const void *xdp_ctx, \ > + const struct bpf_insn *insnsi, \ > + unsigned int (*bpf_func)(const void *, \ > + const struct bpf_insn *)) > + > +#define xdp_call_run(name, prog, ctx) ({ \ > + u32 ret; \ > + cant_sleep(); \ > + if (static_branch_unlikely(&bpf_stats_enabled_key)) { \ > + struct bpf_prog_stats *stats; \ > + u64 start = sched_clock(); \ > + ret = XDP_CALL_TRAMP(name)(ctx, \ > + (prog)->insnsi, \ > + (prog)->bpf_func); \ > + stats = this_cpu_ptr((prog)->aux->stats); \ > + u64_stats_update_begin(&stats->syncp); \ > + stats->cnt++; \ > + stats->nsecs += sched_clock() - start; \ > + u64_stats_update_end(&stats->syncp); \ > + } else { \ > + ret = XDP_CALL_TRAMP(name)(ctx, \ > + (prog)->insnsi, \ > + (prog)->bpf_func); \ > + } \ > + ret; }) I cannot help but wonder whether it's possible to avoid copy-paste from BPF_PROG_RUN(). At least could you place this new macro right next to BPF_PROG_RUN? If it's in a different file eventually they may diverge.
On Sun, 24 Nov 2019 at 02:59, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Sat, Nov 23, 2019 at 08:12:21AM +0100, Björn Töpel wrote: > > From: Björn Töpel <bjorn.topel@intel.com> > > > > The xdp_call.h header wraps a more user-friendly API around the BPF > > dispatcher. A user adds a trampoline/XDP caller using the > > DEFINE_XDP_CALL macro, and updates the BPF dispatcher via > > xdp_call_update(). The actual dispatch is done via xdp_call(). > > > > Note that xdp_call() is only supported for builtin drivers. Module > > builds will fallback to bpf_prog_run_xdp(). > > > > The next patch will show-case how the i40e driver uses xdp_call. > > > > Signed-off-by: Björn Töpel <bjorn.topel@intel.com> > > --- > > include/linux/xdp_call.h | 66 ++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 66 insertions(+) > > create mode 100644 include/linux/xdp_call.h > > > > diff --git a/include/linux/xdp_call.h b/include/linux/xdp_call.h > > new file mode 100644 > > index 000000000000..69b2d325a787 > > --- /dev/null > > +++ b/include/linux/xdp_call.h > > @@ -0,0 +1,66 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > +/* Copyright(c) 2019 Intel Corporation. */ > > +#ifndef _LINUX_XDP_CALL_H > > +#define _LINUX_XDP_CALL_H > > + > > +#include <linux/filter.h> > > + > > +#if defined(CONFIG_BPF_JIT) && defined(CONFIG_RETPOLINE) && !defined(MODULE) > > + > > +void bpf_dispatcher_change_prog(void *func, struct bpf_prog *from, > > + struct bpf_prog *to); > > + > > +#define XDP_CALL_TRAMP(name) ____xdp_call_##name##_tramp > > + > > +#define DEFINE_XDP_CALL(name) \ > > + unsigned int XDP_CALL_TRAMP(name)( \ > > + const void *xdp_ctx, \ > > + const struct bpf_insn *insnsi, \ > > + unsigned int (*bpf_func)(const void *, \ > > + const struct bpf_insn *)) \ > > + { \ > > + return bpf_func(xdp_ctx, insnsi); \ > > + } > > + > > +#define DECLARE_XDP_CALL(name) \ > > + unsigned int XDP_CALL_TRAMP(name)( \ > > + const void *xdp_ctx, \ > > + const struct bpf_insn *insnsi, \ > > + unsigned int (*bpf_func)(const void *, \ > > + const struct bpf_insn *)) > > + > > +#define xdp_call_run(name, prog, ctx) ({ \ > > + u32 ret; \ > > + cant_sleep(); \ > > + if (static_branch_unlikely(&bpf_stats_enabled_key)) { \ > > + struct bpf_prog_stats *stats; \ > > + u64 start = sched_clock(); \ > > + ret = XDP_CALL_TRAMP(name)(ctx, \ > > + (prog)->insnsi, \ > > + (prog)->bpf_func); \ > > + stats = this_cpu_ptr((prog)->aux->stats); \ > > + u64_stats_update_begin(&stats->syncp); \ > > + stats->cnt++; \ > > + stats->nsecs += sched_clock() - start; \ > > + u64_stats_update_end(&stats->syncp); \ > > + } else { \ > > + ret = XDP_CALL_TRAMP(name)(ctx, \ > > + (prog)->insnsi, \ > > + (prog)->bpf_func); \ > > + } \ > > + ret; }) > > I cannot help but wonder whether it's possible to avoid copy-paste from > BPF_PROG_RUN(). > At least could you place this new macro right next to BPF_PROG_RUN? > If it's in a different file eventually they may diverge. > Yeah, I'll take a stab at that! Thanks, Björn
Björn Töpel <bjorn.topel@gmail.com> writes: > From: Björn Töpel <bjorn.topel@intel.com> > > The xdp_call.h header wraps a more user-friendly API around the BPF > dispatcher. A user adds a trampoline/XDP caller using the > DEFINE_XDP_CALL macro, and updates the BPF dispatcher via > xdp_call_update(). The actual dispatch is done via xdp_call(). > > Note that xdp_call() is only supported for builtin drivers. Module > builds will fallback to bpf_prog_run_xdp(). I don't like this restriction. Distro kernels are not likely to start shipping all the network drivers builtin, so they won't benefit from the performance benefits from this dispatcher. What is the reason these dispatcher blocks have to reside in the driver? Couldn't we just allocate one system-wide, and then simply change bpf_prog_run_xdp() to make use of it transparently (from the driver PoV)? That would also remove the need to modify every driver... -Toke
On Mon, 25 Nov 2019 at 12:18, Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > Björn Töpel <bjorn.topel@gmail.com> writes: > > > From: Björn Töpel <bjorn.topel@intel.com> > > > > The xdp_call.h header wraps a more user-friendly API around the BPF > > dispatcher. A user adds a trampoline/XDP caller using the > > DEFINE_XDP_CALL macro, and updates the BPF dispatcher via > > xdp_call_update(). The actual dispatch is done via xdp_call(). > > > > Note that xdp_call() is only supported for builtin drivers. Module > > builds will fallback to bpf_prog_run_xdp(). > > I don't like this restriction. Distro kernels are not likely to start > shipping all the network drivers builtin, so they won't benefit from the > performance benefits from this dispatcher. > > What is the reason these dispatcher blocks have to reside in the driver? > Couldn't we just allocate one system-wide, and then simply change > bpf_prog_run_xdp() to make use of it transparently (from the driver > PoV)? That would also remove the need to modify every driver... > Good idea! I'll try that out. Thanks for the suggestion! Björn > -Toke >
Björn Töpel <bjorn.topel@gmail.com> writes: > On Mon, 25 Nov 2019 at 12:18, Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> Björn Töpel <bjorn.topel@gmail.com> writes: >> >> > From: Björn Töpel <bjorn.topel@intel.com> >> > >> > The xdp_call.h header wraps a more user-friendly API around the BPF >> > dispatcher. A user adds a trampoline/XDP caller using the >> > DEFINE_XDP_CALL macro, and updates the BPF dispatcher via >> > xdp_call_update(). The actual dispatch is done via xdp_call(). >> > >> > Note that xdp_call() is only supported for builtin drivers. Module >> > builds will fallback to bpf_prog_run_xdp(). >> >> I don't like this restriction. Distro kernels are not likely to start >> shipping all the network drivers builtin, so they won't benefit from the >> performance benefits from this dispatcher. >> >> What is the reason these dispatcher blocks have to reside in the driver? >> Couldn't we just allocate one system-wide, and then simply change >> bpf_prog_run_xdp() to make use of it transparently (from the driver >> PoV)? That would also remove the need to modify every driver... >> > > Good idea! I'll try that out. Thanks for the suggestion! Awesome! I guess the table may need to be a bit bigger if it's system-wide? But since you've already gone to all that trouble with the binary search, I guess that shouldn't have too much of a performance impact? Maybe the size could even be a config option so users/distros can make their own size tradeoff? -Toke
On Mon, 25 Nov 2019 at 16:56, Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > Björn Töpel <bjorn.topel@gmail.com> writes: > > > On Mon, 25 Nov 2019 at 12:18, Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >> > >> Björn Töpel <bjorn.topel@gmail.com> writes: > >> > >> > From: Björn Töpel <bjorn.topel@intel.com> > >> > > >> > The xdp_call.h header wraps a more user-friendly API around the BPF > >> > dispatcher. A user adds a trampoline/XDP caller using the > >> > DEFINE_XDP_CALL macro, and updates the BPF dispatcher via > >> > xdp_call_update(). The actual dispatch is done via xdp_call(). > >> > > >> > Note that xdp_call() is only supported for builtin drivers. Module > >> > builds will fallback to bpf_prog_run_xdp(). > >> > >> I don't like this restriction. Distro kernels are not likely to start > >> shipping all the network drivers builtin, so they won't benefit from the > >> performance benefits from this dispatcher. > >> > >> What is the reason these dispatcher blocks have to reside in the driver? > >> Couldn't we just allocate one system-wide, and then simply change > >> bpf_prog_run_xdp() to make use of it transparently (from the driver > >> PoV)? That would also remove the need to modify every driver... > >> > > > > Good idea! I'll try that out. Thanks for the suggestion! > > Awesome! I guess the table may need to be a bit bigger if it's > system-wide? But since you've already gone to all that trouble with the > binary search, I guess that shouldn't have too much of a performance > impact? Maybe the size could even be a config option so users/distros > can make their own size tradeoff? > My bigger concern is not the dispatcher size, but that any XDP update will be a system wide text-poke. OTOH, this is still the case even if there are multiple dispatchers. No more "quickly swap XDP program in one packet latency". Still, definitely worth a try. And configurable dispatcher size might be a good idea as well! Thanks! Cheers, Björn > -Toke >
Björn Töpel <bjorn.topel@gmail.com> writes: > On Mon, 25 Nov 2019 at 16:56, Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> Björn Töpel <bjorn.topel@gmail.com> writes: >> >> > On Mon, 25 Nov 2019 at 12:18, Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> >> >> Björn Töpel <bjorn.topel@gmail.com> writes: >> >> >> >> > From: Björn Töpel <bjorn.topel@intel.com> >> >> > >> >> > The xdp_call.h header wraps a more user-friendly API around the BPF >> >> > dispatcher. A user adds a trampoline/XDP caller using the >> >> > DEFINE_XDP_CALL macro, and updates the BPF dispatcher via >> >> > xdp_call_update(). The actual dispatch is done via xdp_call(). >> >> > >> >> > Note that xdp_call() is only supported for builtin drivers. Module >> >> > builds will fallback to bpf_prog_run_xdp(). >> >> >> >> I don't like this restriction. Distro kernels are not likely to start >> >> shipping all the network drivers builtin, so they won't benefit from the >> >> performance benefits from this dispatcher. >> >> >> >> What is the reason these dispatcher blocks have to reside in the driver? >> >> Couldn't we just allocate one system-wide, and then simply change >> >> bpf_prog_run_xdp() to make use of it transparently (from the driver >> >> PoV)? That would also remove the need to modify every driver... >> >> >> > >> > Good idea! I'll try that out. Thanks for the suggestion! >> >> Awesome! I guess the table may need to be a bit bigger if it's >> system-wide? But since you've already gone to all that trouble with the >> binary search, I guess that shouldn't have too much of a performance >> impact? Maybe the size could even be a config option so users/distros >> can make their own size tradeoff? >> > > My bigger concern is not the dispatcher size, but that any XDP update > will be a system wide text-poke. OTOH, this is still the case even if > there are multiple dispatchers. No more "quickly swap XDP program in > one packet latency". Ah, right. I don't actually know the details of how all this kernel text rewriting happens. I just assumed it was magic faerie dust that just made everything faster; but now you're telling me there are tradeoffs?! ;) When you say "no more quickly swap XDP programs" you mean that the attach operation itself will take longer, right? I.e., it's not that it will disrupt packet flow to the old program while it's happening? Also, how much longer? -Toke
diff --git a/include/linux/xdp_call.h b/include/linux/xdp_call.h new file mode 100644 index 000000000000..69b2d325a787 --- /dev/null +++ b/include/linux/xdp_call.h @@ -0,0 +1,66 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* Copyright(c) 2019 Intel Corporation. */ +#ifndef _LINUX_XDP_CALL_H +#define _LINUX_XDP_CALL_H + +#include <linux/filter.h> + +#if defined(CONFIG_BPF_JIT) && defined(CONFIG_RETPOLINE) && !defined(MODULE) + +void bpf_dispatcher_change_prog(void *func, struct bpf_prog *from, + struct bpf_prog *to); + +#define XDP_CALL_TRAMP(name) ____xdp_call_##name##_tramp + +#define DEFINE_XDP_CALL(name) \ + unsigned int XDP_CALL_TRAMP(name)( \ + const void *xdp_ctx, \ + const struct bpf_insn *insnsi, \ + unsigned int (*bpf_func)(const void *, \ + const struct bpf_insn *)) \ + { \ + return bpf_func(xdp_ctx, insnsi); \ + } + +#define DECLARE_XDP_CALL(name) \ + unsigned int XDP_CALL_TRAMP(name)( \ + const void *xdp_ctx, \ + const struct bpf_insn *insnsi, \ + unsigned int (*bpf_func)(const void *, \ + const struct bpf_insn *)) + +#define xdp_call_run(name, prog, ctx) ({ \ + u32 ret; \ + cant_sleep(); \ + if (static_branch_unlikely(&bpf_stats_enabled_key)) { \ + struct bpf_prog_stats *stats; \ + u64 start = sched_clock(); \ + ret = XDP_CALL_TRAMP(name)(ctx, \ + (prog)->insnsi, \ + (prog)->bpf_func); \ + stats = this_cpu_ptr((prog)->aux->stats); \ + u64_stats_update_begin(&stats->syncp); \ + stats->cnt++; \ + stats->nsecs += sched_clock() - start; \ + u64_stats_update_end(&stats->syncp); \ + } else { \ + ret = XDP_CALL_TRAMP(name)(ctx, \ + (prog)->insnsi, \ + (prog)->bpf_func); \ + } \ + ret; }) + +#define xdp_call_update(name, from_xdp_prog, to_xdp_prog) \ + bpf_dispatcher_change_prog(&XDP_CALL_TRAMP(name), \ + from_xdp_prog, \ + to_xdp_prog) + +#else + +#define DEFINE_XDP_CALL(name) +#define DECLARE_XDP_CALL(name) +#define xdp_call_run(name, xdp_prog, xdp) bpf_prog_run_xdp(xdp_prog, xdp) +#define xdp_call_update(name, from_xdp_prog, to_xdp_prog) + +#endif +#endif /* _LINUX_XDP_CALL_H */