| Message ID | 20210914091842.4186267-14-johan.almbladh@anyfinetworks.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | fe89f6cabaedd9f89d017ff32245720da6567b4b |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf/tests: Extend JIT test suite coverage | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 11 of 11 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 100 this patch: 100 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 82 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 100 this patch: 100 |
| netdev/header_inline | success | Link |
| bpf/vmtest-bpf-PR | success | PR summary |
| bpf/vmtest-bpf | success | VM_Test |
On 09/14/2021 05:18 PM, Johan Almbladh wrote: > This patch fixes an error in the tail call limit test that caused the > test to fail on for x86-64 JIT. Previously, the register R0 was used to > report the total number of tail calls made. However, after a tail call > fall-through, the value of the R0 register is undefined. Now, all tail > call error path tests instead use context state to store the count. > > Fixes: 874be05f525e ("bpf, tests: Add tail call test suite") > Reported-by: Paul Chaignon <paul@cilium.io> > Reported-by: Tiezhu Yang <yangtiezhu@loongson.cn> > Signed-off-by: Johan Almbladh <johan.almbladh@anyfinetworks.com> > --- > lib/test_bpf.c | 37 +++++++++++++++++++++++++++---------- > 1 file changed, 27 insertions(+), 10 deletions(-) > > diff --git a/lib/test_bpf.c b/lib/test_bpf.c > index 7475abfd2186..ddb9a8089d2e 100644 > --- a/lib/test_bpf.c > +++ b/lib/test_bpf.c > @@ -12179,10 +12179,15 @@ static __init int test_bpf(void) > struct tail_call_test { > const char *descr; > struct bpf_insn insns[MAX_INSNS]; > + int flags; > int result; > int stack_depth; > }; > > +/* Flags that can be passed to tail call test cases */ > +#define FLAG_NEED_STATE BIT(0) > +#define FLAG_RESULT_IN_STATE BIT(1) > + > /* > * Magic marker used in test snippets for tail calls below. > * BPF_LD/MOV to R2 and R2 with this immediate value is replaced > @@ -12252,32 +12257,38 @@ static struct tail_call_test tail_call_tests[] = { > { > "Tail call error path, max count reached", > .insns = { > - BPF_ALU64_IMM(BPF_ADD, R1, 1), > - BPF_ALU64_REG(BPF_MOV, R0, R1), > + BPF_LDX_MEM(BPF_W, R2, R1, 0), > + BPF_ALU64_IMM(BPF_ADD, R2, 1), > + BPF_STX_MEM(BPF_W, R1, R2, 0), > TAIL_CALL(0), > BPF_EXIT_INSN(), > }, > - .result = MAX_TAIL_CALL_CNT + 1, > + .flags = FLAG_NEED_STATE | FLAG_RESULT_IN_STATE, > + .result = (MAX_TAIL_CALL_CNT + 1 + 1) * MAX_TESTRUNS, Hi Johan, I have tested this patch, It should be "MAX_TAIL_CALL_CNT + 1" instead of "MAX_TAIL_CALL_CNT + 1 + 1"? [...] Thanks, Tiezhu
On 09/14/2021 08:41 PM, Tiezhu Yang wrote: > On 09/14/2021 05:18 PM, Johan Almbladh wrote: >> This patch fixes an error in the tail call limit test that caused the >> test to fail on for x86-64 JIT. Previously, the register R0 was used to >> report the total number of tail calls made. However, after a tail call >> fall-through, the value of the R0 register is undefined. Now, all tail >> call error path tests instead use context state to store the count. >> >> Fixes: 874be05f525e ("bpf, tests: Add tail call test suite") >> Reported-by: Paul Chaignon <paul@cilium.io> >> Reported-by: Tiezhu Yang <yangtiezhu@loongson.cn> >> Signed-off-by: Johan Almbladh <johan.almbladh@anyfinetworks.com> >> --- >> lib/test_bpf.c | 37 +++++++++++++++++++++++++++---------- >> 1 file changed, 27 insertions(+), 10 deletions(-) >> >> diff --git a/lib/test_bpf.c b/lib/test_bpf.c >> index 7475abfd2186..ddb9a8089d2e 100644 >> --- a/lib/test_bpf.c >> +++ b/lib/test_bpf.c >> @@ -12179,10 +12179,15 @@ static __init int test_bpf(void) >> struct tail_call_test { >> const char *descr; >> struct bpf_insn insns[MAX_INSNS]; >> + int flags; >> int result; >> int stack_depth; >> }; >> +/* Flags that can be passed to tail call test cases */ >> +#define FLAG_NEED_STATE BIT(0) >> +#define FLAG_RESULT_IN_STATE BIT(1) >> + >> /* >> * Magic marker used in test snippets for tail calls below. >> * BPF_LD/MOV to R2 and R2 with this immediate value is replaced >> @@ -12252,32 +12257,38 @@ static struct tail_call_test >> tail_call_tests[] = { >> { >> "Tail call error path, max count reached", >> .insns = { >> - BPF_ALU64_IMM(BPF_ADD, R1, 1), >> - BPF_ALU64_REG(BPF_MOV, R0, R1), >> + BPF_LDX_MEM(BPF_W, R2, R1, 0), >> + BPF_ALU64_IMM(BPF_ADD, R2, 1), >> + BPF_STX_MEM(BPF_W, R1, R2, 0), >> TAIL_CALL(0), >> BPF_EXIT_INSN(), >> }, >> - .result = MAX_TAIL_CALL_CNT + 1, >> + .flags = FLAG_NEED_STATE | FLAG_RESULT_IN_STATE, >> + .result = (MAX_TAIL_CALL_CNT + 1 + 1) * MAX_TESTRUNS, > > Hi Johan, > > I have tested this patch, > It should be "MAX_TAIL_CALL_CNT + 1" instead of "MAX_TAIL_CALL_CNT + 1 > + 1"? Oh, sorry, it is right when MAX_TAIL_CALL_CNT is 32, I have tested it based on MAX_TAIL_CALL_CNT is 33, so I need to modify here if MAX_TAIL_CALL_CNT is 33 in my v3 patch. Tested-by: Tiezhu Yang <yangtiezhu@loongson.cn> > > [...] > > Thanks, > Tiezhu
On Tue, Sep 14, 2021 at 2:55 PM Tiezhu Yang <yangtiezhu@loongson.cn> wrote: > > On 09/14/2021 08:41 PM, Tiezhu Yang wrote: > > On 09/14/2021 05:18 PM, Johan Almbladh wrote: > >> This patch fixes an error in the tail call limit test that caused the > >> test to fail on for x86-64 JIT. Previously, the register R0 was used to > >> report the total number of tail calls made. However, after a tail call > >> fall-through, the value of the R0 register is undefined. Now, all tail > >> call error path tests instead use context state to store the count. > >> > >> Fixes: 874be05f525e ("bpf, tests: Add tail call test suite") > >> Reported-by: Paul Chaignon <paul@cilium.io> > >> Reported-by: Tiezhu Yang <yangtiezhu@loongson.cn> > >> Signed-off-by: Johan Almbladh <johan.almbladh@anyfinetworks.com> > >> --- > >> lib/test_bpf.c | 37 +++++++++++++++++++++++++++---------- > >> 1 file changed, 27 insertions(+), 10 deletions(-) > >> > >> diff --git a/lib/test_bpf.c b/lib/test_bpf.c > >> index 7475abfd2186..ddb9a8089d2e 100644 > >> --- a/lib/test_bpf.c > >> +++ b/lib/test_bpf.c > >> @@ -12179,10 +12179,15 @@ static __init int test_bpf(void) > >> struct tail_call_test { > >> const char *descr; > >> struct bpf_insn insns[MAX_INSNS]; > >> + int flags; > >> int result; > >> int stack_depth; > >> }; > >> +/* Flags that can be passed to tail call test cases */ > >> +#define FLAG_NEED_STATE BIT(0) > >> +#define FLAG_RESULT_IN_STATE BIT(1) > >> + > >> /* > >> * Magic marker used in test snippets for tail calls below. > >> * BPF_LD/MOV to R2 and R2 with this immediate value is replaced > >> @@ -12252,32 +12257,38 @@ static struct tail_call_test > >> tail_call_tests[] = { > >> { > >> "Tail call error path, max count reached", > >> .insns = { > >> - BPF_ALU64_IMM(BPF_ADD, R1, 1), > >> - BPF_ALU64_REG(BPF_MOV, R0, R1), > >> + BPF_LDX_MEM(BPF_W, R2, R1, 0), > >> + BPF_ALU64_IMM(BPF_ADD, R2, 1), > >> + BPF_STX_MEM(BPF_W, R1, R2, 0), > >> TAIL_CALL(0), > >> BPF_EXIT_INSN(), > >> }, > >> - .result = MAX_TAIL_CALL_CNT + 1, > >> + .flags = FLAG_NEED_STATE | FLAG_RESULT_IN_STATE, > >> + .result = (MAX_TAIL_CALL_CNT + 1 + 1) * MAX_TESTRUNS, > > > > Hi Johan, > > > > I have tested this patch, > > It should be "MAX_TAIL_CALL_CNT + 1" instead of "MAX_TAIL_CALL_CNT + 1 > > + 1"? > > Oh, sorry, it is right when MAX_TAIL_CALL_CNT is 32, > I have tested it based on MAX_TAIL_CALL_CNT is 33, > so I need to modify here if MAX_TAIL_CALL_CNT is 33 in my v3 patch. No worries! I wrote it that way to indicate that there are two +1s. The first is from the behaviour that actual count (33) = configured count (32) + 1. The second is for the initial BPF program call, which increments the counter but is not in itself a tail call. > > Tested-by: Tiezhu Yang <yangtiezhu@loongson.cn> Thanks! Johan > > > > > [...] > > > > Thanks, > > Tiezhu >
diff --git a/lib/test_bpf.c b/lib/test_bpf.c index 7475abfd2186..ddb9a8089d2e 100644 --- a/lib/test_bpf.c +++ b/lib/test_bpf.c @@ -12179,10 +12179,15 @@ static __init int test_bpf(void) struct tail_call_test { const char *descr; struct bpf_insn insns[MAX_INSNS]; + int flags; int result; int stack_depth; }; +/* Flags that can be passed to tail call test cases */ +#define FLAG_NEED_STATE BIT(0) +#define FLAG_RESULT_IN_STATE BIT(1) + /* * Magic marker used in test snippets for tail calls below. * BPF_LD/MOV to R2 and R2 with this immediate value is replaced @@ -12252,32 +12257,38 @@ static struct tail_call_test tail_call_tests[] = { { "Tail call error path, max count reached", .insns = { - BPF_ALU64_IMM(BPF_ADD, R1, 1), - BPF_ALU64_REG(BPF_MOV, R0, R1), + BPF_LDX_MEM(BPF_W, R2, R1, 0), + BPF_ALU64_IMM(BPF_ADD, R2, 1), + BPF_STX_MEM(BPF_W, R1, R2, 0), TAIL_CALL(0), BPF_EXIT_INSN(), }, - .result = MAX_TAIL_CALL_CNT + 1, + .flags = FLAG_NEED_STATE | FLAG_RESULT_IN_STATE, + .result = (MAX_TAIL_CALL_CNT + 1 + 1) * MAX_TESTRUNS, }, { "Tail call error path, NULL target", .insns = { - BPF_ALU64_IMM(BPF_MOV, R0, -1), + BPF_LDX_MEM(BPF_W, R2, R1, 0), + BPF_ALU64_IMM(BPF_ADD, R2, 1), + BPF_STX_MEM(BPF_W, R1, R2, 0), TAIL_CALL(TAIL_CALL_NULL), - BPF_ALU64_IMM(BPF_MOV, R0, 1), BPF_EXIT_INSN(), }, - .result = 1, + .flags = FLAG_NEED_STATE | FLAG_RESULT_IN_STATE, + .result = MAX_TESTRUNS, }, { "Tail call error path, index out of range", .insns = { - BPF_ALU64_IMM(BPF_MOV, R0, -1), + BPF_LDX_MEM(BPF_W, R2, R1, 0), + BPF_ALU64_IMM(BPF_ADD, R2, 1), + BPF_STX_MEM(BPF_W, R1, R2, 0), TAIL_CALL(TAIL_CALL_INVALID), - BPF_ALU64_IMM(BPF_MOV, R0, 1), BPF_EXIT_INSN(), }, - .result = 1, + .flags = FLAG_NEED_STATE | FLAG_RESULT_IN_STATE, + .result = MAX_TESTRUNS, }, }; @@ -12383,6 +12394,8 @@ static __init int test_tail_calls(struct bpf_array *progs) for (i = 0; i < ARRAY_SIZE(tail_call_tests); i++) { struct tail_call_test *test = &tail_call_tests[i]; struct bpf_prog *fp = progs->ptrs[i]; + int *data = NULL; + int state = 0; u64 duration; int ret; @@ -12399,7 +12412,11 @@ static __init int test_tail_calls(struct bpf_array *progs) if (fp->jited) jit_cnt++; - ret = __run_one(fp, NULL, MAX_TESTRUNS, &duration); + if (test->flags & FLAG_NEED_STATE) + data = &state; + ret = __run_one(fp, data, MAX_TESTRUNS, &duration); + if (test->flags & FLAG_RESULT_IN_STATE) + ret = state; if (ret == test->result) { pr_cont("%lld PASS", duration); pass_cnt++;
This patch fixes an error in the tail call limit test that caused the test to fail on for x86-64 JIT. Previously, the register R0 was used to report the total number of tail calls made. However, after a tail call fall-through, the value of the R0 register is undefined. Now, all tail call error path tests instead use context state to store the count. Fixes: 874be05f525e ("bpf, tests: Add tail call test suite") Reported-by: Paul Chaignon <paul@cilium.io> Reported-by: Tiezhu Yang <yangtiezhu@loongson.cn> Signed-off-by: Johan Almbladh <johan.almbladh@anyfinetworks.com> --- lib/test_bpf.c | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-)