| Message ID | 20240209040608.98927-17-alexei.starovoitov@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Introduce BPF arena. | expand |
On Fri, 9 Feb 2024 at 05:07, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > From: Alexei Starovoitov <ast@kernel.org> > > Introduce helper macro bpf_arena_cast() that emits: > rX = rX > instruction with off = BPF_ARENA_CAST_KERN or off = BPF_ARENA_CAST_USER > and encodes address_space into imm32. > > It's useful with older LLVM that doesn't emit this insn automatically. > > Signed-off-by: Alexei Starovoitov <ast@kernel.org> > --- Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> But could this simply be added to libbpf along with bpf_cast_user and bpf_cast_kern? I believe since LLVM and the verifier support the new cast instructions, they are unlikely to disappear any time soon. It would probably also make it easier to use elsewhere (e.g. sched-ext) without having to copy them. I plan on doing the same eventually with assert macros too.
On Sat, Feb 10, 2024 at 12:54 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Fri, 9 Feb 2024 at 05:07, Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > From: Alexei Starovoitov <ast@kernel.org> > > > > Introduce helper macro bpf_arena_cast() that emits: > > rX = rX > > instruction with off = BPF_ARENA_CAST_KERN or off = BPF_ARENA_CAST_USER > > and encodes address_space into imm32. > > > > It's useful with older LLVM that doesn't emit this insn automatically. > > > > Signed-off-by: Alexei Starovoitov <ast@kernel.org> > > --- > > Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > But could this simply be added to libbpf along with bpf_cast_user and > bpf_cast_kern? I believe since LLVM and the verifier support the new > cast instructions, they are unlikely to disappear any time soon. It > would probably also make it easier to use elsewhere (e.g. sched-ext) > without having to copy them. This arena bpf_arena_cast() macro probably will be removed once llvm 19 is released and we upgrade bpf CI to it. It's here for selftests only. It's quite tricky and fragile to use in practice. Notice it does: "r"(__var) which is not quite correct, since llvm won't recognize it as output that changes __var and will use a copy of __var in a different register later. But if the macro changes to "=r" or "+r" then llvm allocates a register and that screws up codegen even more. The __var;}) also doesn't always work. So this macro is not suited for all to use. > I plan on doing the same eventually with assert macros too. I think it's too early to move them.
On Tue, 2024-02-13 at 14:35 -0800, Alexei Starovoitov wrote: [...] > This arena bpf_arena_cast() macro probably will be removed > once llvm 19 is released and we upgrade bpf CI to it. > It's here for selftests only. > It's quite tricky and fragile to use in practice. > Notice it does: > "r"(__var) > which is not quite correct, > since llvm won't recognize it as output that changes __var and > will use a copy of __var in a different register later. > But if the macro changes to "=r" or "+r" then llvm allocates > a register and that screws up codegen even more. > > The __var;}) also doesn't always work. > So this macro is not suited for all to use. Could you please elaborate a bit on why is this macro fragile? I toyed a bit with a version patched as below and it seems to work fine. Don't see how ": [reg]"+r"(var) : ..." could be broken by the compiler (when "+r" is in the "output constraint" position): from clang pov the variable 'var' would be in register and updated after the asm volatile part. --- diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h index e73b7d48439f..488001236506 100644 --- a/tools/testing/selftests/bpf/bpf_experimental.h +++ b/tools/testing/selftests/bpf/bpf_experimental.h @@ -334,8 +334,6 @@ l_true: \ /* emit instruction: rX=rX .off = mode .imm32 = address_space */ #ifndef bpf_arena_cast #define bpf_arena_cast(var, mode, addr_space) \ - ({ \ - typeof(var) __var = var; \ asm volatile(".byte 0xBF; \ .ifc %[reg], r0; \ .byte 0x00; \ @@ -368,8 +366,7 @@ l_true: \ .byte 0x99; \ .endif; \ .short %[off]; .long %[as]" \ - :: [reg]"r"(__var), [off]"i"(mode), [as]"i"(addr_space)); __var; \ - }) + : [reg]"+r"(var) : [off]"i"(mode), [as]"i"(addr_space)) #endif
On Wed, Feb 14, 2024 at 8:47 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Tue, 2024-02-13 at 14:35 -0800, Alexei Starovoitov wrote: > [...] > > > This arena bpf_arena_cast() macro probably will be removed > > once llvm 19 is released and we upgrade bpf CI to it. > > It's here for selftests only. > > It's quite tricky and fragile to use in practice. > > Notice it does: > > "r"(__var) > > which is not quite correct, > > since llvm won't recognize it as output that changes __var and > > will use a copy of __var in a different register later. > > But if the macro changes to "=r" or "+r" then llvm allocates > > a register and that screws up codegen even more. > > > > The __var;}) also doesn't always work. > > So this macro is not suited for all to use. > > Could you please elaborate a bit on why is this macro fragile? > I toyed a bit with a version patched as below and it seems to work fine. > Don't see how ": [reg]"+r"(var) : ..." could be broken by the compiler > (when "+r" is in the "output constraint" position): > from clang pov the variable 'var' would be in register and updated > after the asm volatile part. > > --- > > diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h > index e73b7d48439f..488001236506 100644 > --- a/tools/testing/selftests/bpf/bpf_experimental.h > +++ b/tools/testing/selftests/bpf/bpf_experimental.h > @@ -334,8 +334,6 @@ l_true: \ > /* emit instruction: rX=rX .off = mode .imm32 = address_space */ > #ifndef bpf_arena_cast > #define bpf_arena_cast(var, mode, addr_space) \ > - ({ \ > - typeof(var) __var = var; \ > asm volatile(".byte 0xBF; \ > .ifc %[reg], r0; \ > .byte 0x00; \ > @@ -368,8 +366,7 @@ l_true: \ > .byte 0x99; \ > .endif; \ > .short %[off]; .long %[as]" \ > - :: [reg]"r"(__var), [off]"i"(mode), [as]"i"(addr_space)); __var; \ > - }) > + : [reg]"+r"(var) : [off]"i"(mode), [as]"i"(addr_space)) Earlier I tried "+r" while keeping __var. Directly using var seems to work indeed. I'll apply this change.
diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h index 0d749006d107..e73b7d48439f 100644 --- a/tools/testing/selftests/bpf/bpf_experimental.h +++ b/tools/testing/selftests/bpf/bpf_experimental.h @@ -331,6 +331,47 @@ l_true: \ asm volatile("%[reg]=%[reg]"::[reg]"r"((short)var)) #endif +/* emit instruction: rX=rX .off = mode .imm32 = address_space */ +#ifndef bpf_arena_cast +#define bpf_arena_cast(var, mode, addr_space) \ + ({ \ + typeof(var) __var = var; \ + asm volatile(".byte 0xBF; \ + .ifc %[reg], r0; \ + .byte 0x00; \ + .endif; \ + .ifc %[reg], r1; \ + .byte 0x11; \ + .endif; \ + .ifc %[reg], r2; \ + .byte 0x22; \ + .endif; \ + .ifc %[reg], r3; \ + .byte 0x33; \ + .endif; \ + .ifc %[reg], r4; \ + .byte 0x44; \ + .endif; \ + .ifc %[reg], r5; \ + .byte 0x55; \ + .endif; \ + .ifc %[reg], r6; \ + .byte 0x66; \ + .endif; \ + .ifc %[reg], r7; \ + .byte 0x77; \ + .endif; \ + .ifc %[reg], r8; \ + .byte 0x88; \ + .endif; \ + .ifc %[reg], r9; \ + .byte 0x99; \ + .endif; \ + .short %[off]; .long %[as]" \ + :: [reg]"r"(__var), [off]"i"(mode), [as]"i"(addr_space)); __var; \ + }) +#endif + /* Description * Assert that a conditional expression is true. * Returns