Re: [PATCH bpf-next 3/3] bpf,x86: inline bpf_get_smp_processor_id() on x86-64

[Jiri Olsa <olsajiri@gmail.com>]

On Thu, Mar 21, 2024 at 02:09:41PM -0700, Andrii Nakryiko wrote:
> On Thu, Mar 21, 2024 at 2:08 PM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Thu, Mar 21, 2024 at 11:05:01AM -0700, Andrii Nakryiko wrote:
> > > Add arch-specific inlining of bpf_get_smp_processor_id() using x86-64's
> > > gs segment-based addressing.
> > >
> > > Just to be on the safer side both rip-relative addressing is implemented
> > > (providing a shorter instruction, but limiting offset to signed 32 bits)
> > > and more universal absolute memory offset addressing is used as
> > > a fallback in (unlikely) scenario that given offset doesn't fit int s32.
> > > The latter is 5 bytes longer, and it seems compilers prefer rip-relative
> > > instructions when compiling kernel code.
> > >
> > > Both instructions were tested and confirmed using gdb. We also already
> > > have a BPF selftest (raw_tp_test_run) that validates correctness of
> > > bpf_get_smp_processor_id(), while running target BPF program on each
> > > online CPU.
> > >
> > > Here's a disassembly of bpf_get_smp_processor_id() helper:
> > >
> > > $ gdb -batch -ex 'file vmlinux' -ex 'set disassembly-flavor intel' -ex 'disassemble/r bpf_get_smp_processor_id'
> > > Dump of assembler code for function bpf_get_smp_processor_id:
> > >    0xffffffff810fa890 <+0>:     0f 1f 44 00 00          nop    DWORD PTR [rax+rax*1+0x0]
> > >    0xffffffff810fa895 <+5>:     65 8b 05 70 62 f3 7e    mov    eax,DWORD PTR gs:[rip+0x7ef36270]        # 0x30b0c <pcpu_hot+12>
> > >    0xffffffff810fa89c <+12>:    48 98                   cdqe
> > >    0xffffffff810fa89e <+14>:    c3                      ret
> > > End of assembler dump.
> > >
> > > And here's a GDB disassembly dump of a piece of BPF program calling
> > > bpf_get_smp_processor_id().
> > >
> > >   $ sudo cat /proc/kallsyms | rg 'pcpu_hot|bpf_prog_2b455b4f8a8d48c5_kexit'
> > >   000000000002d840 A pcpu_hot
> > >   ffffffffa000f8a8 t bpf_prog_2b455b4f8a8d48c5_kexit      [bpf]
> > >
> > > Then attaching GDB to the running kernel in QEMU and breaking inside BPF
> > > program:
> > >
> > > (gdb) b *0xffffffffa000f8e2
> > > Breakpoint 1 at 0xffffffffa000f8e2
> > >
> > > When RIP-relative instruction is used:
> > >
> > >   0xffffffffa000f8e2      mov    %gs:0x6001df63(%rip),%eax        # 0x2d84c <pcpu_hot+12>
> > >   0xffffffffa000f8e9      cltq
> > >
> > > You can see that final address is resolved to <pcpu_hot+12> as expected.
> > >
> > > When absolute addressing is used:
> > >
> > >   0xffffffffa000f8e2      movabs %gs:0x2d84c,%eax
> > >   0xffffffffa000f8ed      cltq
> > >
> > > And here 0x2d84c matches pcpu_hot address from kallsyms (0x2d840),
> > > plus 12 (0xc) bytes offset of cpu_number field.
> > >
> > > This inlining eliminates entire function call for this (rather trivial in terms
> > > of instructions executed) helper, saving a bit of performance, but foremost
> > > saving LBR records (1 for PERF_SAMPLE_BRANCH_ANY_RETURN mode, and 2 for
> > > PERF_SAMPLE_BRANCH_ANY), which is what motivated this work in the first
> > > place.
> >
> > this should also 'fix' the k[ret]probe-multi-fast benchmark issue right?
> 
> I already fixed it locally by switching to bpf_get_numa_node_id(), but
> this change would generally make my original approach not work because
> bpf_get_smp_processor_id() isn't actually called at runtime on x86-64
> :)

hm, but the reason was that program attached to bpf_get_smp_processor_id
called bpf_get_smp_processor_id helper:
  https://lore.kernel.org/bpf/CAEf4BzayNECKkmc4=XfLW5fzsPozMnjqOEmGO+r2UmEQXt1XyA@mail.gmail.com/

inlining of bpf_get_smp_processor_id helper call would prevent that, no?

jirka


> 
> >
> > https://lore.kernel.org/bpf/20240315051813.1320559-2-andrii@kernel.org/
> >
> > jirka
> >
> > >
> > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > > ---
> > >  arch/x86/net/bpf_jit_comp.c | 26 +++++++++++++++++++++++++-
> > >  1 file changed, 25 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > > index 4900b1ee019f..5b7fdc24b5b8 100644
> > > --- a/arch/x86/net/bpf_jit_comp.c
> > > +++ b/arch/x86/net/bpf_jit_comp.c
> > > @@ -457,6 +457,9 @@ static void emit_prologue(u8 **pprog, u32 stack_depth, bool ebpf_from_cbpf,
> > >       *pprog = prog;
> > >  }
> > >
> > > +/* reference to bpf_get_smp_processor_id() helper implementation to detect it for inlining */
> > > +extern u64 bpf_get_smp_processor_id(u64, u64, u64, u64, u64);
> > > +
> > >  static int emit_patch(u8 **pprog, void *func, void *ip, u8 opcode)
> > >  {
> > >       u8 *prog = *pprog;
> > > @@ -467,7 +470,28 @@ static int emit_patch(u8 **pprog, void *func, void *ip, u8 opcode)
> > >               pr_err("Target call %p is out of range\n", func);
> > >               return -ERANGE;
> > >       }
> > > -     EMIT1_off32(opcode, offset);
> > > +
> > > +     /* inline bpf_get_smp_processor_id() to avoid calls */
> > > +     if (opcode == 0xE8 && func == &bpf_get_smp_processor_id) {
> > > +             /* 7 to account for the mov instruction itself,
> > > +              * as rip value *after* mov instruction is used
> > > +              */
> > > +             offset = (void *)&pcpu_hot.cpu_number - ip - 7;
> > > +             if (is_simm32(offset)) {
> > > +                     /* mov eax,DWORD PTR gs:[rip+<offset>] ; <pcpu_hot+12> */
> > > +                     EMIT3_off32(0x65, 0x8b, 0x05, (u32)offset);
> > > +             } else {
> > > +                     /* mov eax,DWORD PTR gs:<offset> ; <pcpu_hot+12> */
> > > +                     offset = (s64)(void *)&pcpu_hot.cpu_number;
> > > +                     EMIT2(0x65, 0xa1);
> > > +                     EMIT((u32)offset, 4);
> > > +                     EMIT((u64)offset >> 32, 4);
> > > +             }
> > > +             EMIT2(0x48, 0x98); /* cdqe, zero-extend eax to rax */
> > > +     } else {
> > > +             EMIT1_off32(opcode, offset);
> > > +     }
> > > +
> > >       *pprog = prog;
> > >       return 0;
> > >  }
> > > --
> > > 2.43.0
> > >
> > >