Re: [PATCH v8 bpf-next 0/7] bpf: tailcalls in BPF subprograms

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Thu, Sep 17, 2020 at 08:26:34PM -0700, Alexei Starovoitov wrote:
> On Wed, Sep 16, 2020 at 2:16 PM Maciej Fijalkowski
> <maciej.fijalkowski@intel.com> wrote:
> >
> > Changelog:
> >
> > v7->v8:
> > - teach bpf_patch_insn_data to adjust insn_idx of tailcall insn
> > - address case of clearing tail call counter when bpf2bpf with tailcalls
> >   are mixed
> > - drop unnecessary checks against cbpf in JIT
> > - introduce more tailcall_bpf2bpf[X] selftests that are making sure the
> >   combination of tailcalls with bpf2bpf calls works correctly
> > - add test cases to test_verifier to make sure logic from
> >   check_max_stack_depth that limits caller's stack depth down to 256 is
> >   correct
> > - move the main patch to appear after the one that limits the caller's
> >   stack depth so that 'has_tail_call' can be used by 'tail_call_reachable'
> >   logic
> 
> Thanks a lot for your hard work on this set. 5 month effort!

Thanks for the whole collaboration! It was quite a ride :)

> I think it's a huge milestone that will enable cilium, cloudflare, katran to
> use bpf functions. Removing always_inline will improve performance.
> Switching to global functions with function-by-function verification
> will drastically improve program load times.
> libbpf has full support for subprogram composition and call relocations.
> Until now these verifier and libbpf features were impossible to use in XDP
> programs, since most of them use tail_calls.
> It's great to see all these building blocks finally coming together.
> 
> I've applied the set with few changes.
> In patch 4 I've removed ifdefs and redundant ().
> In patch 5 removed redundant !tail_call_reachable check.
> In patch 6 replaced CONFIG_JIT_ALWAYS_ON dependency with
> jit_requested && IS_ENABLED(CONFIG_X86_64).
> It's more user friendly.
> I also added patch 7 that checks that ld_abs and tail_call are only
> allowed in subprograms that return 'int'.

Thank you for this last touch! I went through patches and I agree with the
changes.

> I felt that the fix is simple enough, so I just pushed it, since
> without it the set is not safe. Please review it here:
> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=09b28d76eac48e922dc293da1aa2b2b85c32aeee

LGTM.

> I'll address any issues in the followups.
> Because of the above changes I tweaked patch 8 to increase test coverage
> with ld_abs and combination of global/static subprogs.
> Also did s/__attribute__((noinline))/__noinline/.
> 
> John and Daniel,
> I wasn't able to test it on cilium programs.
> When you have a chance please give it a thorough run.
> tail_call poke logic is delicate.
> 
> Lorenz,
> if you can test it on cloudflare progs would be awesome.
> 
> Thanks a lot everyone!