On Mon, Mar 21, 2022 at 4:04 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Tue, Mar 15, 2022 at 5:44 PM Kui-Feng Lee <kuifeng@xxxxxx> wrote:
> >
> > BPF trampolines will create a bpf_trace_run_ctx on their stacks, and
> > set/reset the current bpf_run_ctx whenever calling/returning from a
> > bpf_prog.
> >
> > Signed-off-by: Kui-Feng Lee <kuifeng@xxxxxx>
> > ---
> > arch/x86/net/bpf_jit_comp.c | 32 ++++++++++++++++++++++++++++++++
> > include/linux/bpf.h | 12 ++++++++----
> > kernel/bpf/syscall.c | 4 ++--
> > kernel/bpf/trampoline.c | 21 +++++++++++++++++----
> > 4 files changed, 59 insertions(+), 10 deletions(-)
> >
>
> [...]
>
> > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> > index 54c695d49ec9..0b050aa2f159 100644
> > --- a/kernel/bpf/trampoline.c
> > +++ b/kernel/bpf/trampoline.c
> > @@ -580,9 +580,12 @@ static void notrace inc_misses_counter(struct bpf_prog *prog)
> > * [2..MAX_U64] - execute bpf prog and record execution time.
> > * This is start time.
> > */
> > -u64 notrace __bpf_prog_enter(struct bpf_prog *prog)
> > +u64 notrace __bpf_prog_enter(struct bpf_prog *prog, struct bpf_trace_run_ctx *run_ctx)
> > __acquires(RCU)
> > {
> > + if (run_ctx)
> > + run_ctx->saved_run_ctx = bpf_set_run_ctx(&run_ctx->run_ctx);
> > +
>
> In all current cases we bpf_set_run_ctx() after migrate_disable and
> rcu_read_lock, let's keep this consistent (even if I don't remember if
> that order matters or not).
>
> > rcu_read_lock();
> > migrate_disable();
> > if (unlikely(__this_cpu_inc_return(*(prog->active)) != 1)) {
> > @@ -614,17 +617,23 @@ static void notrace update_prog_stats(struct bpf_prog *prog,
> > }
> > }
> >
> > -void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start)
> > +void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start, struct bpf_trace_run_ctx *run_ctx)
> > __releases(RCU)
> > {
> > + if (run_ctx)
> > + bpf_reset_run_ctx(run_ctx->saved_run_ctx);
> > +
> > update_prog_stats(prog, start);
> > __this_cpu_dec(*(prog->active));
> > migrate_enable();
> > rcu_read_unlock();
> > }
> >
> > -u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog)
> > +u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog, struct bpf_trace_run_ctx *run_ctx)
> > {
> > + if (run_ctx)
> > + run_ctx->saved_run_ctx = bpf_set_run_ctx(&run_ctx->run_ctx);
> > +
> > rcu_read_lock_trace();
> > migrate_disable();
> > might_fault();
> > @@ -635,8 +644,12 @@ u64 notrace __bpf_prog_enter_sleepable(struct bpf_prog *prog)
> > return bpf_prog_start_time();
> > }
> >
> > -void notrace __bpf_prog_exit_sleepable(struct bpf_prog *prog, u64 start)
> > +void notrace __bpf_prog_exit_sleepable(struct bpf_prog *prog, u64 start,
> > + struct bpf_trace_run_ctx *run_ctx)
>
> now that we have entire run_ctx, can we move `start` into run_ctx and
> simplify __bpf_prog_enter/exit calls a bit? Or extra indirection will
> hurt performance and won't be compensated by simpler enter/exit
> calling convention?
The "start" is an optional and temporary argument.
I suspect it will look odd inside run_ctx.
imo the current way is simpler.
