| Message ID | 20210728170502.351010-11-johan.almbladh@anyfinetworks.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf/tests: Extend the eBPF test suite | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On 7/28/21 10:04 AM, Johan Almbladh wrote: > Some JITs may need to convert a conditional jump instruction to > to short PC-relative branch and a long unconditional jump, if the > PC-relative offset exceeds offset field width in the CPU instruction. > This test triggers such branch conversion on the 32-bit MIPS JIT. > > Signed-off-by: Johan Almbladh <johan.almbladh@anyfinetworks.com> > --- > lib/test_bpf.c | 38 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/lib/test_bpf.c b/lib/test_bpf.c > index 8b94902702ed..55914b6236aa 100644 > --- a/lib/test_bpf.c > +++ b/lib/test_bpf.c > @@ -461,6 +461,36 @@ static int bpf_fill_stxdw(struct bpf_test *self) > return __bpf_fill_stxdw(self, BPF_DW); > } > > +static int bpf_fill_long_jmp(struct bpf_test *self) > +{ > + unsigned int len = BPF_MAXINSNS; > + struct bpf_insn *insn; > + int i; > + > + insn = kmalloc_array(len, sizeof(*insn), GFP_KERNEL); > + if (!insn) > + return -ENOMEM; When insn will be freed? > + > + insn[0] = BPF_ALU64_IMM(BPF_MOV, R0, 1); > + insn[1] = BPF_JMP_IMM(BPF_JEQ, R0, 1, len - 2 - 1); > + > + /* > + * Fill with a complex 64-bit operation that expands to a lot of > + * instructions on 32-bit JITs. The large jump offset can then > + * overflow the conditional branch field size, triggering a branch > + * conversion mechanism in some JITs. > + */ > + for (i = 2; i < len - 1; i++) > + insn[i] = BPF_ALU64_IMM(BPF_MUL, R0, (i << 16) + i); > + > + insn[len - 1] = BPF_EXIT_INSN(); > + > + self->u.ptr.insns = insn; > + self->u.ptr.len = len; > + > + return 0; > +} > + [...]
On 7/28/21 10:04 AM, Johan Almbladh wrote: > Some JITs may need to convert a conditional jump instruction to > to short PC-relative branch and a long unconditional jump, if the > PC-relative offset exceeds offset field width in the CPU instruction. > This test triggers such branch conversion on the 32-bit MIPS JIT. > > Signed-off-by: Johan Almbladh <johan.almbladh@anyfinetworks.com> > --- > lib/test_bpf.c | 38 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/lib/test_bpf.c b/lib/test_bpf.c > index 8b94902702ed..55914b6236aa 100644 > --- a/lib/test_bpf.c > +++ b/lib/test_bpf.c > @@ -461,6 +461,36 @@ static int bpf_fill_stxdw(struct bpf_test *self) > return __bpf_fill_stxdw(self, BPF_DW); > } > > +static int bpf_fill_long_jmp(struct bpf_test *self) > +{ > + unsigned int len = BPF_MAXINSNS; BPF_MAXINSNS is 4096 as defined in uapi/linux/bpf_common.h. Will it be able to trigger a PC relative branch + long conditional jump? > + struct bpf_insn *insn; > + int i; > + > + insn = kmalloc_array(len, sizeof(*insn), GFP_KERNEL); > + if (!insn) > + return -ENOMEM; > + > + insn[0] = BPF_ALU64_IMM(BPF_MOV, R0, 1); > + insn[1] = BPF_JMP_IMM(BPF_JEQ, R0, 1, len - 2 - 1); > + > + /* > + * Fill with a complex 64-bit operation that expands to a lot of > + * instructions on 32-bit JITs. The large jump offset can then > + * overflow the conditional branch field size, triggering a branch > + * conversion mechanism in some JITs. > + */ > + for (i = 2; i < len - 1; i++) > + insn[i] = BPF_ALU64_IMM(BPF_MUL, R0, (i << 16) + i); > + > + insn[len - 1] = BPF_EXIT_INSN(); > + > + self->u.ptr.insns = insn; > + self->u.ptr.len = len; > + > + return 0; > +} > + [...]
On Thu, Jul 29, 2021 at 1:59 AM Yonghong Song <yhs@fb.com> wrote: > > +static int bpf_fill_long_jmp(struct bpf_test *self) > > +{ > > + unsigned int len = BPF_MAXINSNS; > > + struct bpf_insn *insn; > > + int i; > > + > > + insn = kmalloc_array(len, sizeof(*insn), GFP_KERNEL); > > + if (!insn) > > + return -ENOMEM; > > When insn will be freed? It is freed by the existing test runner code. If the fill_helper member is set, the function destroy_bpf_tests frees the insn pointer in that test case. This is the same as with other tests that use the fill_helper facility.
On Thu, Jul 29, 2021 at 2:55 AM Yonghong Song <yhs@fb.com> wrote: > > +static int bpf_fill_long_jmp(struct bpf_test *self) > > +{ > > + unsigned int len = BPF_MAXINSNS; > > BPF_MAXINSNS is 4096 as defined in uapi/linux/bpf_common.h. > Will it be able to trigger a PC relative branch + long > conditional jump? It does, on the MIPS32 JIT. The ALU64 MUL instruction with a large immediate was chosen since it expands to a lot of MIPS32 instructions: 2 to load the immediate, 1 to zero/sign extend it, and then 9 for the 64x64 multiply. Other JITs will be different of course. On the other hand, other architectures have other limitations that this test may not trigger anyway. I added the test because I was implementing a non-trivial iterative branch conversion logic in the MIPS32 JIT. One can argue that when such complex JIT mechanisms are added, the test suite should also be updated to cover that, especially if the mechanism handles something that almost never occur in practice. Since I was able to trigger the branch conversion with BPF_MAXINSNS instructions, and no other test was using more, I left it at that. However, should I or someone else work on the MIPS64 JIT, I think updating the test suite so that similar special cases there are triggered would be a valuable contribution.
On 7/29/21 5:45 AM, Johan Almbladh wrote: > On Thu, Jul 29, 2021 at 1:59 AM Yonghong Song <yhs@fb.com> wrote: >>> +static int bpf_fill_long_jmp(struct bpf_test *self) >>> +{ >>> + unsigned int len = BPF_MAXINSNS; >>> + struct bpf_insn *insn; >>> + int i; >>> + >>> + insn = kmalloc_array(len, sizeof(*insn), GFP_KERNEL); >>> + if (!insn) >>> + return -ENOMEM; >> >> When insn will be freed? > > It is freed by the existing test runner code. If the fill_helper > member is set, the function destroy_bpf_tests frees the insn pointer > in that test case. This is the same as with other tests that use the > fill_helper facility. Sounds good. Thanks for explanation.
On 7/29/21 6:24 AM, Johan Almbladh wrote: > On Thu, Jul 29, 2021 at 2:55 AM Yonghong Song <yhs@fb.com> wrote: >>> +static int bpf_fill_long_jmp(struct bpf_test *self) >>> +{ >>> + unsigned int len = BPF_MAXINSNS; >> >> BPF_MAXINSNS is 4096 as defined in uapi/linux/bpf_common.h. >> Will it be able to trigger a PC relative branch + long >> conditional jump? > > It does, on the MIPS32 JIT. The ALU64 MUL instruction with a large > immediate was chosen since it expands to a lot of MIPS32 instructions: > 2 to load the immediate, 1 to zero/sign extend it, and then 9 for the > 64x64 multiply. Maybe added a comment in the code to mention that with BPF_MAXINSNS PC relative branch + long conditional jump can be triggered on MIPS32 JIT. Other architecture may need a different/larger number? > > Other JITs will be different of course. On the other hand, other > architectures have other limitations that this test may not trigger > anyway. I added the test because I was implementing a non-trivial > iterative branch conversion logic in the MIPS32 JIT. One can argue > that when such complex JIT mechanisms are added, the test suite should > also be updated to cover that, especially if the mechanism handles > something that almost never occur in practice. > > Since I was able to trigger the branch conversion with BPF_MAXINSNS > instructions, and no other test was using more, I left it at that. > However, should I or someone else work on the MIPS64 JIT, I think > updating the test suite so that similar special cases there are > triggered would be a valuable contribution. >
diff --git a/lib/test_bpf.c b/lib/test_bpf.c index 8b94902702ed..55914b6236aa 100644 --- a/lib/test_bpf.c +++ b/lib/test_bpf.c @@ -461,6 +461,36 @@ static int bpf_fill_stxdw(struct bpf_test *self) return __bpf_fill_stxdw(self, BPF_DW); } +static int bpf_fill_long_jmp(struct bpf_test *self) +{ + unsigned int len = BPF_MAXINSNS; + struct bpf_insn *insn; + int i; + + insn = kmalloc_array(len, sizeof(*insn), GFP_KERNEL); + if (!insn) + return -ENOMEM; + + insn[0] = BPF_ALU64_IMM(BPF_MOV, R0, 1); + insn[1] = BPF_JMP_IMM(BPF_JEQ, R0, 1, len - 2 - 1); + + /* + * Fill with a complex 64-bit operation that expands to a lot of + * instructions on 32-bit JITs. The large jump offset can then + * overflow the conditional branch field size, triggering a branch + * conversion mechanism in some JITs. + */ + for (i = 2; i < len - 1; i++) + insn[i] = BPF_ALU64_IMM(BPF_MUL, R0, (i << 16) + i); + + insn[len - 1] = BPF_EXIT_INSN(); + + self->u.ptr.insns = insn; + self->u.ptr.len = len; + + return 0; +} + static struct bpf_test tests[] = { { "TAX", @@ -6892,6 +6922,14 @@ static struct bpf_test tests[] = { { }, { { 0, 1 } }, }, + { /* Mainly checking JIT here. */ + "BPF_MAXINSNS: Very long conditional jump", + { }, + INTERNAL | FLAG_NO_DATA, + { }, + { { 0, 1 } }, + .fill_helper = bpf_fill_long_jmp, + }, { "JMP_JA: Jump, gap, jump, ...", { },
Some JITs may need to convert a conditional jump instruction to to short PC-relative branch and a long unconditional jump, if the PC-relative offset exceeds offset field width in the CPU instruction. This test triggers such branch conversion on the 32-bit MIPS JIT. Signed-off-by: Johan Almbladh <johan.almbladh@anyfinetworks.com> --- lib/test_bpf.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)