From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Daniel Borkmann <daniel@iogearbox.net>
Cc: Andrii Nakryiko <andrii@kernel.org>, bpf <bpf@vger.kernel.org>,
Alexei Starovoitov <ast@kernel.org>,
Kernel Team <kernel-team@fb.com>,
Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH v3 bpf-next 01/14] bpf: refactor BPF_PROG_RUN into a function
Date: Mon, 9 Aug 2021 17:28:03 -0700 [thread overview]
Message-ID: <CAEf4BzYGqb6HdAuBciauv-NZLBoVF4X8WdFXNKmKQ9hcZfSC3w@mail.gmail.com> (raw)
In-Reply-To: <21246244-fa7e-700f-e767-3f9edf9e4c19@iogearbox.net>
On Mon, Aug 9, 2021 at 3:43 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 7/30/21 7:34 AM, Andrii Nakryiko wrote:
> > Turn BPF_PROG_RUN into a proper always inlined function. No functional and
> > performance changes are intended, but it makes it much easier to understand
> > what's going on with how BPF programs are actually get executed. It's more
> > obvious what types and callbacks are expected. Also extra () around input
> > parameters can be dropped, as well as `__` variable prefixes intended to avoid
> > naming collisions, which makes the code simpler to read and write.
> >
> > This refactoring also highlighted one possible issue. BPF_PROG_RUN is both
> > a macro and an enum value (BPF_PROG_RUN == BPF_PROG_TEST_RUN). Turning
> > BPF_PROG_RUN into a function causes naming conflict compilation error. So
> > rename BPF_PROG_RUN into lower-case bpf_prog_run(), similar to
> > bpf_prog_run_xdp(), bpf_prog_run_pin_on_cpu(), etc. To avoid unnecessary code
> > churn across many networking calls to BPF_PROG_RUN, #define BPF_PROG_RUN as an
> > alias to bpf_prog_run.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>
> Change itself looks good, small nit below:
>
> > ---
> > include/linux/filter.h | 58 +++++++++++++++++++++++++++---------------
> > 1 file changed, 37 insertions(+), 21 deletions(-)
> >
> > diff --git a/include/linux/filter.h b/include/linux/filter.h
> > index ba36989f711a..18518e321ce4 100644
> > --- a/include/linux/filter.h
> > +++ b/include/linux/filter.h
> > @@ -585,25 +585,41 @@ struct sk_filter {
> >
> > DECLARE_STATIC_KEY_FALSE(bpf_stats_enabled_key);
> >
> > -#define __BPF_PROG_RUN(prog, ctx, dfunc) ({ \
> > - u32 __ret; \
> > - cant_migrate(); \
> > - if (static_branch_unlikely(&bpf_stats_enabled_key)) { \
> > - struct bpf_prog_stats *__stats; \
> > - u64 __start = sched_clock(); \
> > - __ret = dfunc(ctx, (prog)->insnsi, (prog)->bpf_func); \
> > - __stats = this_cpu_ptr(prog->stats); \
> > - u64_stats_update_begin(&__stats->syncp); \
> > - __stats->cnt++; \
> > - __stats->nsecs += sched_clock() - __start; \
> > - u64_stats_update_end(&__stats->syncp); \
> > - } else { \
> > - __ret = dfunc(ctx, (prog)->insnsi, (prog)->bpf_func); \
> > - } \
> > - __ret; })
> > -
> > -#define BPF_PROG_RUN(prog, ctx) \
> > - __BPF_PROG_RUN(prog, ctx, bpf_dispatcher_nop_func)
> > +typedef unsigned int (*bpf_dispatcher_fn)(const void *ctx,
> > + const struct bpf_insn *insnsi,
> > + unsigned int (*bpf_func)(const void *,
> > + const struct bpf_insn *));
> > +
> > +static __always_inline u32 __bpf_prog_run(const struct bpf_prog *prog,
> > + const void *ctx,
> > + bpf_dispatcher_fn dfunc)
> > +{
> > + u32 ret;
> > +
> > + cant_migrate();
> > + if (static_branch_unlikely(&bpf_stats_enabled_key)) {
> > + struct bpf_prog_stats *stats;
> > + u64 start = sched_clock();
> > +
> > + ret = dfunc(ctx, prog->insnsi, prog->bpf_func);
> > + stats = this_cpu_ptr(prog->stats);
> > + u64_stats_update_begin(&stats->syncp);
> > + stats->cnt++;
> > + stats->nsecs += sched_clock() - start;
> > + u64_stats_update_end(&stats->syncp);
> > + } else {
> > + ret = dfunc(ctx, prog->insnsi, prog->bpf_func);
> > + }
> > + return ret;
> > +}
> > +
> > +static __always_inline u32 bpf_prog_run(const struct bpf_prog *prog, const void *ctx)
> > +{
> > + return __bpf_prog_run(prog, ctx, bpf_dispatcher_nop_func);
> > +}
> > +
> > +/* avoids name conflict with BPF_PROG_RUN enum definedi uapi/linux/bpf.h */
>
> (definedi)
>
oops, will fix
> > +#define BPF_PROG_RUN bpf_prog_run
>
> Given the unfortunate conflict in BPF_PROG_RUN, can't we just toss the BPF_PROG_RUN to
> bpf_prog_run altogether and bite the bullet once to remove it from the tree? (Same as the
> other macro names in next patch.) There are a number of instances, but still to the extend
> that it should be doable.
Yeah, absolutely. I wasn't sure if you'd hate the renaming noise. I'll
get rid of BPF_PROG_RUN macro in the next revision.
>
> > /*
> > * Use in preemptible and therefore migratable context to make sure that
> > @@ -622,7 +638,7 @@ static inline u32 bpf_prog_run_pin_on_cpu(const struct bpf_prog *prog,
> > u32 ret;
> >
> > migrate_disable();
> > - ret = __BPF_PROG_RUN(prog, ctx, bpf_dispatcher_nop_func);
> > + ret = bpf_prog_run(prog, ctx);
> > migrate_enable();
> > return ret;
> > }
> > @@ -768,7 +784,7 @@ static __always_inline u32 bpf_prog_run_xdp(const struct bpf_prog *prog,
> > * under local_bh_disable(), which provides the needed RCU protection
> > * for accessing map entries.
> > */
> > - return __BPF_PROG_RUN(prog, xdp, BPF_DISPATCHER_FUNC(xdp));
> > + return __bpf_prog_run(prog, xdp, BPF_DISPATCHER_FUNC(xdp));
> > }
> >
> > void bpf_prog_change_xdp(struct bpf_prog *prev_prog, struct bpf_prog *prog);
> >
>
next prev parent reply other threads:[~2021-08-10 0:28 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-30 5:33 [PATCH v3 bpf-next 00/14] BPF perf link and user-provided bpf_cookie Andrii Nakryiko
2021-07-30 5:34 ` [PATCH v3 bpf-next 01/14] bpf: refactor BPF_PROG_RUN into a function Andrii Nakryiko
2021-07-30 17:19 ` Yonghong Song
2021-08-09 22:43 ` Daniel Borkmann
2021-08-10 0:28 ` Andrii Nakryiko [this message]
2021-07-30 5:34 ` [PATCH v3 bpf-next 02/14] bpf: refactor BPF_PROG_RUN_ARRAY family of macros into functions Andrii Nakryiko
2021-08-09 23:00 ` Daniel Borkmann
2021-08-10 0:36 ` Andrii Nakryiko
2021-07-30 5:34 ` [PATCH v3 bpf-next 03/14] bpf: refactor perf_event_set_bpf_prog() to use struct bpf_prog input Andrii Nakryiko
2021-07-30 5:34 ` [PATCH v3 bpf-next 04/14] bpf: implement minimal BPF perf link Andrii Nakryiko
2021-07-30 17:24 ` Yonghong Song
2021-07-30 5:34 ` [PATCH v3 bpf-next 05/14] bpf: allow to specify user-provided bpf_cookie for BPF perf links Andrii Nakryiko
2021-07-30 22:02 ` Yonghong Song
2021-08-09 23:30 ` Daniel Borkmann
2021-08-10 0:52 ` Andrii Nakryiko
2021-07-30 5:34 ` [PATCH v3 bpf-next 06/14] bpf: add bpf_get_attach_cookie() BPF helper to access bpf_cookie value Andrii Nakryiko
2021-07-30 22:13 ` Yonghong Song
2021-07-30 5:34 ` [PATCH v3 bpf-next 07/14] libbpf: re-build libbpf.so when libbpf.map changes Andrii Nakryiko
2021-07-30 22:31 ` Yonghong Song
2021-07-30 5:34 ` [PATCH v3 bpf-next 08/14] libbpf: remove unused bpf_link's destroy operation, but add dealloc Andrii Nakryiko
2021-07-30 5:34 ` [PATCH v3 bpf-next 09/14] libbpf: use BPF perf link when supported by kernel Andrii Nakryiko
2021-07-30 5:34 ` [PATCH v3 bpf-next 10/14] libbpf: add bpf_cookie support to bpf_link_create() API Andrii Nakryiko
2021-07-30 5:34 ` [PATCH v3 bpf-next 11/14] libbpf: add bpf_cookie to perf_event, kprobe, uprobe, and tp attach APIs Andrii Nakryiko
2021-07-30 5:34 ` [PATCH v3 bpf-next 12/14] selftests/bpf: test low-level perf BPF link API Andrii Nakryiko
2021-07-30 5:34 ` [PATCH v3 bpf-next 13/14] selftests/bpf: extract uprobe-related helpers into trace_helpers.{c,h} Andrii Nakryiko
2021-07-30 5:34 ` [PATCH v3 bpf-next 14/14] selftests/bpf: add bpf_cookie selftests for high-level APIs Andrii Nakryiko
2021-08-01 5:11 ` [PATCH bpf-next v4] libbpf: introduce legacy kprobe events support Rafael David Tinoco
2021-08-06 22:29 ` Andrii Nakryiko
2021-09-12 6:48 ` [PATCH bpf-next v5] " Rafael David Tinoco
2021-09-12 6:52 ` Rafael David Tinoco
2021-09-14 4:44 ` Andrii Nakryiko
2021-09-14 5:02 ` Rafael David Tinoco
2021-09-14 5:21 ` Andrii Nakryiko
2021-09-14 14:27 ` sunyucong
2021-09-14 15:42 ` Rafael David Tinoco
2021-09-14 20:34 ` [PATCH bpf-next] libbpf: fix build error introduced by legacy kprobe feature Rafael David Tinoco
2021-09-14 21:04 ` Alexei Starovoitov
2021-09-14 21:35 ` [PATCH bpf-next v2] " Rafael David Tinoco
2021-09-14 21:39 ` Rafael David Tinoco
2021-09-14 21:48 ` Andrii Nakryiko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAEf4BzYGqb6HdAuBciauv-NZLBoVF4X8WdFXNKmKQ9hcZfSC3w@mail.gmail.com \
--to=andrii.nakryiko@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=kernel-team@fb.com \
--cc=peterz@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).