On Mon, Mar 21, 2022 at 4:25 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> 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.
So is saved_run_ctx (they have identical lifetimes), but I'm fine
either way. Was thinking it would result in simpler trampoline
generation code (both for humans and CPU), that's all.
