| Message ID | 160079992560.8301.11225602391403157558.stgit@toke.dk |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: Support multi-attach for freplace programs | expand |
On Tue, Sep 22, 2020 at 11:39 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > From: Toke Høiland-Jørgensen <toke@redhat.com> > > The benchmark code and the test_overhead prog_test included fmod_ret > programs that attached to various functions in the kernel. However, these > functions were never listed as allowed for return modification, so this > only worked because of the verifier skipping tests when a trampoline > already existed for the attach point. Now that the verifier checks have > been fixed, remove fmod_ret from the affected tests so they all work again. > > Fixes: 4eaf0b5c5e04 ("selftest/bpf: Fmod_ret prog and implement test_overhead as part of bench") > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > --- Acked-by: Andrii Nakryiko <andriin@fb.com> > tools/testing/selftests/bpf/bench.c | 5 ----- > tools/testing/selftests/bpf/benchs/bench_rename.c | 17 ----------------- > tools/testing/selftests/bpf/benchs/bench_trigger.c | 17 ----------------- > .../selftests/bpf/prog_tests/test_overhead.c | 14 +------------- > tools/testing/selftests/bpf/progs/test_overhead.c | 6 ------ > tools/testing/selftests/bpf/progs/trigger_bench.c | 7 ------- > 6 files changed, 1 insertion(+), 65 deletions(-) > [...]
On Tue, Sep 22, 2020 at 08:38:45PM +0200, Toke Høiland-Jørgensen wrote: > -const struct bench bench_trig_fmodret = { > - .name = "trig-fmodret", > - .validate = trigger_validate, > - .setup = trigger_fmodret_setup, > - .producer_thread = trigger_producer, > - .consumer_thread = trigger_consumer, > - .measure = trigger_measure, > - .report_progress = hits_drops_report_progress, > - .report_final = hits_drops_report_final, > -}; > diff --git a/tools/testing/selftests/bpf/progs/trigger_bench.c b/tools/testing/selftests/bpf/progs/trigger_bench.c > index 9a4d09590b3d..1af23ac0c37c 100644 > --- a/tools/testing/selftests/bpf/progs/trigger_bench.c > +++ b/tools/testing/selftests/bpf/progs/trigger_bench.c > @@ -45,10 +45,3 @@ int bench_trigger_fentry_sleep(void *ctx) > __sync_add_and_fetch(&hits, 1); > return 0; > } > - > -SEC("fmod_ret/__x64_sys_getpgid") > -int bench_trigger_fmodret(void *ctx) > -{ > - __sync_add_and_fetch(&hits, 1); > - return -22; > -} why are you removing this? There is no problem here. All syscalls are error-injectable. I'm surprised Andrii acked this :(
On Wed, Sep 23, 2020 at 6:08 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Sep 22, 2020 at 08:38:45PM +0200, Toke Høiland-Jørgensen wrote: > > -const struct bench bench_trig_fmodret = { > > - .name = "trig-fmodret", > > - .validate = trigger_validate, > > - .setup = trigger_fmodret_setup, > > - .producer_thread = trigger_producer, > > - .consumer_thread = trigger_consumer, > > - .measure = trigger_measure, > > - .report_progress = hits_drops_report_progress, > > - .report_final = hits_drops_report_final, > > -}; > > diff --git a/tools/testing/selftests/bpf/progs/trigger_bench.c b/tools/testing/selftests/bpf/progs/trigger_bench.c > > index 9a4d09590b3d..1af23ac0c37c 100644 > > --- a/tools/testing/selftests/bpf/progs/trigger_bench.c > > +++ b/tools/testing/selftests/bpf/progs/trigger_bench.c > > @@ -45,10 +45,3 @@ int bench_trigger_fentry_sleep(void *ctx) > > __sync_add_and_fetch(&hits, 1); > > return 0; > > } > > - > > -SEC("fmod_ret/__x64_sys_getpgid") > > -int bench_trigger_fmodret(void *ctx) > > -{ > > - __sync_add_and_fetch(&hits, 1); > > - return -22; > > -} > > why are you removing this? There is no problem here. > All syscalls are error-injectable. > I'm surprised Andrii acked this :( Andrii didn't know that all syscalls are error-injectable, thanks for catching :) after fmod_ret/__set_task_comm I just assumed that I've been abusing fmod_ret all this time...
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Wed, Sep 23, 2020 at 6:08 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: >> >> On Tue, Sep 22, 2020 at 08:38:45PM +0200, Toke Høiland-Jørgensen wrote: >> > -const struct bench bench_trig_fmodret = { >> > - .name = "trig-fmodret", >> > - .validate = trigger_validate, >> > - .setup = trigger_fmodret_setup, >> > - .producer_thread = trigger_producer, >> > - .consumer_thread = trigger_consumer, >> > - .measure = trigger_measure, >> > - .report_progress = hits_drops_report_progress, >> > - .report_final = hits_drops_report_final, >> > -}; >> > diff --git a/tools/testing/selftests/bpf/progs/trigger_bench.c b/tools/testing/selftests/bpf/progs/trigger_bench.c >> > index 9a4d09590b3d..1af23ac0c37c 100644 >> > --- a/tools/testing/selftests/bpf/progs/trigger_bench.c >> > +++ b/tools/testing/selftests/bpf/progs/trigger_bench.c >> > @@ -45,10 +45,3 @@ int bench_trigger_fentry_sleep(void *ctx) >> > __sync_add_and_fetch(&hits, 1); >> > return 0; >> > } >> > - >> > -SEC("fmod_ret/__x64_sys_getpgid") >> > -int bench_trigger_fmodret(void *ctx) >> > -{ >> > - __sync_add_and_fetch(&hits, 1); >> > - return -22; >> > -} >> >> why are you removing this? There is no problem here. >> All syscalls are error-injectable. >> I'm surprised Andrii acked this :( > > Andrii didn't know that all syscalls are error-injectable, thanks for > catching :) after fmod_ret/__set_task_comm I just assumed that I've > been abusing fmod_ret all this time... I didn't know that either. Shall I just drop your ACK from the next version so you can take another look? -Toke
diff --git a/tools/testing/selftests/bpf/bench.c b/tools/testing/selftests/bpf/bench.c index 1a427685a8a8..d1a4a55335f8 100644 --- a/tools/testing/selftests/bpf/bench.c +++ b/tools/testing/selftests/bpf/bench.c @@ -311,14 +311,12 @@ extern const struct bench bench_rename_kretprobe; extern const struct bench bench_rename_rawtp; extern const struct bench bench_rename_fentry; extern const struct bench bench_rename_fexit; -extern const struct bench bench_rename_fmodret; extern const struct bench bench_trig_base; extern const struct bench bench_trig_tp; extern const struct bench bench_trig_rawtp; extern const struct bench bench_trig_kprobe; extern const struct bench bench_trig_fentry; extern const struct bench bench_trig_fentry_sleep; -extern const struct bench bench_trig_fmodret; extern const struct bench bench_rb_libbpf; extern const struct bench bench_rb_custom; extern const struct bench bench_pb_libbpf; @@ -333,14 +331,12 @@ static const struct bench *benchs[] = { &bench_rename_rawtp, &bench_rename_fentry, &bench_rename_fexit, - &bench_rename_fmodret, &bench_trig_base, &bench_trig_tp, &bench_trig_rawtp, &bench_trig_kprobe, &bench_trig_fentry, &bench_trig_fentry_sleep, - &bench_trig_fmodret, &bench_rb_libbpf, &bench_rb_custom, &bench_pb_libbpf, @@ -464,4 +460,3 @@ int main(int argc, char **argv) return 0; } - diff --git a/tools/testing/selftests/bpf/benchs/bench_rename.c b/tools/testing/selftests/bpf/benchs/bench_rename.c index e74cff40f4fe..a967674098ad 100644 --- a/tools/testing/selftests/bpf/benchs/bench_rename.c +++ b/tools/testing/selftests/bpf/benchs/bench_rename.c @@ -106,12 +106,6 @@ static void setup_fexit() attach_bpf(ctx.skel->progs.prog5); } -static void setup_fmodret() -{ - setup_ctx(); - attach_bpf(ctx.skel->progs.prog6); -} - static void *consumer(void *input) { return NULL; @@ -182,14 +176,3 @@ const struct bench bench_rename_fexit = { .report_progress = hits_drops_report_progress, .report_final = hits_drops_report_final, }; - -const struct bench bench_rename_fmodret = { - .name = "rename-fmodret", - .validate = validate, - .setup = setup_fmodret, - .producer_thread = producer, - .consumer_thread = consumer, - .measure = measure, - .report_progress = hits_drops_report_progress, - .report_final = hits_drops_report_final, -}; diff --git a/tools/testing/selftests/bpf/benchs/bench_trigger.c b/tools/testing/selftests/bpf/benchs/bench_trigger.c index 2a0b6c9885a4..93ab7b280b25 100644 --- a/tools/testing/selftests/bpf/benchs/bench_trigger.c +++ b/tools/testing/selftests/bpf/benchs/bench_trigger.c @@ -96,12 +96,6 @@ static void trigger_fentry_sleep_setup() attach_bpf(ctx.skel->progs.bench_trigger_fentry_sleep); } -static void trigger_fmodret_setup() -{ - setup_ctx(); - attach_bpf(ctx.skel->progs.bench_trigger_fmodret); -} - static void *trigger_consumer(void *input) { return NULL; @@ -171,14 +165,3 @@ const struct bench bench_trig_fentry_sleep = { .report_progress = hits_drops_report_progress, .report_final = hits_drops_report_final, }; - -const struct bench bench_trig_fmodret = { - .name = "trig-fmodret", - .validate = trigger_validate, - .setup = trigger_fmodret_setup, - .producer_thread = trigger_producer, - .consumer_thread = trigger_consumer, - .measure = trigger_measure, - .report_progress = hits_drops_report_progress, - .report_final = hits_drops_report_final, -}; diff --git a/tools/testing/selftests/bpf/prog_tests/test_overhead.c b/tools/testing/selftests/bpf/prog_tests/test_overhead.c index 2702df2b2343..9966685866fd 100644 --- a/tools/testing/selftests/bpf/prog_tests/test_overhead.c +++ b/tools/testing/selftests/bpf/prog_tests/test_overhead.c @@ -61,10 +61,9 @@ void test_test_overhead(void) const char *raw_tp_name = "raw_tp/task_rename"; const char *fentry_name = "fentry/__set_task_comm"; const char *fexit_name = "fexit/__set_task_comm"; - const char *fmodret_name = "fmod_ret/__set_task_comm"; const char *kprobe_func = "__set_task_comm"; struct bpf_program *kprobe_prog, *kretprobe_prog, *raw_tp_prog; - struct bpf_program *fentry_prog, *fexit_prog, *fmodret_prog; + struct bpf_program *fentry_prog, *fexit_prog; struct bpf_object *obj; struct bpf_link *link; int err, duration = 0; @@ -97,11 +96,6 @@ void test_test_overhead(void) if (CHECK(!fexit_prog, "find_probe", "prog '%s' not found\n", fexit_name)) goto cleanup; - fmodret_prog = bpf_object__find_program_by_title(obj, fmodret_name); - if (CHECK(!fmodret_prog, "find_probe", - "prog '%s' not found\n", fmodret_name)) - goto cleanup; - err = bpf_object__load(obj); if (CHECK(err, "obj_load", "err %d\n", err)) goto cleanup; @@ -148,12 +142,6 @@ void test_test_overhead(void) test_run("fexit"); bpf_link__destroy(link); - /* attach fmod_ret */ - link = bpf_program__attach_trace(fmodret_prog); - if (CHECK(IS_ERR(link), "attach fmod_ret", "err %ld\n", PTR_ERR(link))) - goto cleanup; - test_run("fmod_ret"); - bpf_link__destroy(link); cleanup: prctl(PR_SET_NAME, comm, 0L, 0L, 0L); bpf_object__close(obj); diff --git a/tools/testing/selftests/bpf/progs/test_overhead.c b/tools/testing/selftests/bpf/progs/test_overhead.c index 42403d088abc..abb7344b531f 100644 --- a/tools/testing/selftests/bpf/progs/test_overhead.c +++ b/tools/testing/selftests/bpf/progs/test_overhead.c @@ -39,10 +39,4 @@ int BPF_PROG(prog5, struct task_struct *tsk, const char *buf, bool exec) return 0; } -SEC("fmod_ret/__set_task_comm") -int BPF_PROG(prog6, struct task_struct *tsk, const char *buf, bool exec) -{ - return !tsk; -} - char _license[] SEC("license") = "GPL"; diff --git a/tools/testing/selftests/bpf/progs/trigger_bench.c b/tools/testing/selftests/bpf/progs/trigger_bench.c index 9a4d09590b3d..1af23ac0c37c 100644 --- a/tools/testing/selftests/bpf/progs/trigger_bench.c +++ b/tools/testing/selftests/bpf/progs/trigger_bench.c @@ -45,10 +45,3 @@ int bench_trigger_fentry_sleep(void *ctx) __sync_add_and_fetch(&hits, 1); return 0; } - -SEC("fmod_ret/__x64_sys_getpgid") -int bench_trigger_fmodret(void *ctx) -{ - __sync_add_and_fetch(&hits, 1); - return -22; -}