* [PATCH bpf v2 0/2] bpf: Fix deadlock with rq_lock in bpf_send_signal()
@ 2020-03-04 17:53 Yonghong Song
2020-03-04 17:53 ` [PATCH bpf v2 1/2] " Yonghong Song
2020-03-04 17:53 ` [PATCH bpf v2 2/2] selftests/bpf: add send_signal_sched_switch test Yonghong Song
0 siblings, 2 replies; 7+ messages in thread
From: Yonghong Song @ 2020-03-04 17:53 UTC (permalink / raw)
To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team
Commit 8b401f9ed244 ("bpf: implement bpf_send_signal() helper")
introduced bpf_send_signal() helper and Commit 8482941f0906
("bpf: Add bpf_send_signal_thread() helper") added bpf_send_signal_thread()
helper. Both helpers try to send a signel to current process or thread.
When bpf_prog is called with scheduler rq_lock held, a deadlock
could happen since bpf_send_signal() and bpf_send_signal_thread()
will call group_send_sig_info() which may ultimately want to acquire
rq_lock() again. This happens in 5.2 and 4.16 based kernels in our
production environment with perf_sw_event.
In a different scenario, the following is also possible in the last kernel:
cpu 1:
do_task_stat <- holding sighand->siglock
...
task_sched_runtime <- trying to grab rq_lock
cpu 2:
__schedule <- holding rq_lock
...
do_send_sig_info <- trying to grab sighand->siglock
Commit eac9153f2b58 ("bpf/stackmap: Fix deadlock with
rq_lock in bpf_get_stack()") has a similar issue with above
rq_lock() deadlock. This patch set addressed the issue
in a similar way. Patch #1 provided kernel solution and
Patch #2 added a selftest.
Changelogs:
v1 -> v2:
. previous fix using task_work in nmi() is incorrect.
there is no nmi() involvement here. Using task_work
in all cases might be a solution. But decided to
use a similar fix as in Commit eac9153f2b58.
Yonghong Song (2):
bpf: Fix deadlock with rq_lock in bpf_send_signal()
selftests/bpf: add send_signal_sched_switch test
kernel/trace/bpf_trace.c | 5 +-
.../bpf/prog_tests/send_signal_sched_switch.c | 89 +++++++++++++++++++
.../bpf/progs/test_send_signal_kern.c | 6 ++
3 files changed, 99 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/send_signal_sched_switch.c
--
2.17.1
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH bpf v2 1/2] bpf: Fix deadlock with rq_lock in bpf_send_signal() 2020-03-04 17:53 [PATCH bpf v2 0/2] bpf: Fix deadlock with rq_lock in bpf_send_signal() Yonghong Song @ 2020-03-04 17:53 ` Yonghong Song 2020-03-04 17:53 ` [PATCH bpf v2 2/2] selftests/bpf: add send_signal_sched_switch test Yonghong Song 1 sibling, 0 replies; 7+ messages in thread From: Yonghong Song @ 2020-03-04 17:53 UTC (permalink / raw) To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, Song Liu When experimenting with bpf_send_signal() helper in our production environment (5.2 based), we experienced a deadlock in NMI mode: #5 [ffffc9002219f770] queued_spin_lock_slowpath at ffffffff8110be24 #6 [ffffc9002219f770] _raw_spin_lock_irqsave at ffffffff81a43012 #7 [ffffc9002219f780] try_to_wake_up at ffffffff810e7ecd #8 [ffffc9002219f7e0] signal_wake_up_state at ffffffff810c7b55 #9 [ffffc9002219f7f0] __send_signal at ffffffff810c8602 #10 [ffffc9002219f830] do_send_sig_info at ffffffff810ca31a #11 [ffffc9002219f868] bpf_send_signal at ffffffff8119d227 #12 [ffffc9002219f988] bpf_overflow_handler at ffffffff811d4140 #13 [ffffc9002219f9e0] __perf_event_overflow at ffffffff811d68cf #14 [ffffc9002219fa10] perf_swevent_overflow at ffffffff811d6a09 #15 [ffffc9002219fa38] ___perf_sw_event at ffffffff811e0f47 #16 [ffffc9002219fc30] __schedule at ffffffff81a3e04d #17 [ffffc9002219fc90] schedule at ffffffff81a3e219 #18 [ffffc9002219fca0] futex_wait_queue_me at ffffffff8113d1b9 #19 [ffffc9002219fcd8] futex_wait at ffffffff8113e529 #20 [ffffc9002219fdf0] do_futex at ffffffff8113ffbc #21 [ffffc9002219fec0] __x64_sys_futex at ffffffff81140d1c #22 [ffffc9002219ff38] do_syscall_64 at ffffffff81002602 #23 [ffffc9002219ff50] entry_SYSCALL_64_after_hwframe at ffffffff81c00068 The above call stack is actually very similar to an issue reported by Commit eac9153f2b58 ("bpf/stackmap: Fix deadlock with rq_lock in bpf_get_stack()") by Song Liu. The only difference is bpf_send_signal() helper instead of bpf_get_stack() helper. The above deadlock is triggered with a perf_sw_event. Similar to Commit eac9153f2b58, the above reproducer used tracepoint point sched/sched_switch so the issue can be easily catched. /* stress_test.c */ #include <stdio.h> #include <stdlib.h> #include <sys/mman.h> #include <pthread.h> #include <sys/types.h> #include <sys/stat.h> #include <fcntl.h> #define THREAD_COUNT 1000 char *filename; void *worker(void *p) { void *ptr; int fd; char *pptr; fd = open(filename, O_RDONLY); if (fd < 0) return NULL; while (1) { struct timespec ts = {0, 1000 + rand() % 2000}; ptr = mmap(NULL, 4096 * 64, PROT_READ, MAP_PRIVATE, fd, 0); usleep(1); if (ptr == MAP_FAILED) { printf("failed to mmap\n"); break; } munmap(ptr, 4096 * 64); usleep(1); pptr = malloc(1); usleep(1); pptr[0] = 1; usleep(1); free(pptr); usleep(1); nanosleep(&ts, NULL); } close(fd); return NULL; } int main(int argc, char *argv[]) { void *ptr; int i; pthread_t threads[THREAD_COUNT]; if (argc < 2) return 0; filename = argv[1]; for (i = 0; i < THREAD_COUNT; i++) { if (pthread_create(threads + i, NULL, worker, NULL)) { fprintf(stderr, "Error creating thread\n"); return 0; } } for (i = 0; i < THREAD_COUNT; i++) pthread_join(threads[i], NULL); return 0; } and the following command: 1. run `stress_test /bin/ls` in one windown 2. hack bcc trace.py with the following change: --- a/tools/trace.py +++ b/tools/trace.py @@ -513,6 +513,7 @@ BPF_PERF_OUTPUT(%s); __data.tgid = __tgid; __data.pid = __pid; bpf_get_current_comm(&__data.comm, sizeof(__data.comm)); + bpf_send_signal(10); %s %s %s.perf_submit(%s, &__data, sizeof(__data)); 3. in a different window run ./trace.py -p $(pidof stress_test) t:sched:sched_switch The deadlock can be reproduced. Similar to Song's fix, the fix is to delay sending signal if irqs is disabled to avoid deadlocks involving with rq_lock. With this change, my above stress-test in our production system won't cause deadlock any more. I also implemented a scale-down version of reproducer in the selftest (a subsequent commit). With latest bpf-next, it complains for the following deadlock. [ 32.832450] -> #1 (&p->pi_lock){-.-.}: [ 32.833100] _raw_spin_lock_irqsave+0x44/0x80 [ 32.833696] task_rq_lock+0x2c/0xa0 [ 32.834182] task_sched_runtime+0x59/0xd0 [ 32.834721] thread_group_cputime+0x250/0x270 [ 32.835304] thread_group_cputime_adjusted+0x2e/0x70 [ 32.835959] do_task_stat+0x8a7/0xb80 [ 32.836461] proc_single_show+0x51/0xb0 ... [ 32.839512] -> #0 (&(&sighand->siglock)->rlock){....}: [ 32.840275] __lock_acquire+0x1358/0x1a20 [ 32.840826] lock_acquire+0xc7/0x1d0 [ 32.841309] _raw_spin_lock_irqsave+0x44/0x80 [ 32.841916] __lock_task_sighand+0x79/0x160 [ 32.842465] do_send_sig_info+0x35/0x90 [ 32.842977] bpf_send_signal+0xa/0x10 [ 32.843464] bpf_prog_bc13ed9e4d3163e3_send_signal_tp_sched+0x465/0x1000 [ 32.844301] trace_call_bpf+0x115/0x270 [ 32.844809] perf_trace_run_bpf_submit+0x4a/0xc0 [ 32.845411] perf_trace_sched_switch+0x10f/0x180 [ 32.846014] __schedule+0x45d/0x880 [ 32.846483] schedule+0x5f/0xd0 ... [ 32.853148] Chain exists of: [ 32.853148] &(&sighand->siglock)->rlock --> &p->pi_lock --> &rq->lock [ 32.853148] [ 32.854451] Possible unsafe locking scenario: [ 32.854451] [ 32.855173] CPU0 CPU1 [ 32.855745] ---- ---- [ 32.856278] lock(&rq->lock); [ 32.856671] lock(&p->pi_lock); [ 32.857332] lock(&rq->lock); [ 32.857999] lock(&(&sighand->siglock)->rlock); Deadlock happens on CPU0 when it tries to acquire &sighand->siglock but it has been held by CPU1 and CPU1 tries to grab &rq->lock and cannot get it. This is not exactly the callstack in our production environment, but sympotom is similar and both locks are using spin_lock_irqsave() to acquire the lock, and both involves rq_lock. The fix to delay sending signal when irq is disabled also fixed this issue. Cc: Song Liu <songliubraving@fb.com> Signed-off-by: Yonghong Song <yhs@fb.com> --- kernel/trace/bpf_trace.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index 07764c761073..43ccfdbacb03 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -730,7 +730,10 @@ static int bpf_send_signal_common(u32 sig, enum pid_type type) if (unlikely(!nmi_uaccess_okay())) return -EPERM; - if (in_nmi()) { + /* Delay sending signal if irq is disabled. Otherwise, + * we risk deadlock with rq_lock. + */ + if (irqs_disabled()) { /* Do an early check on signal validity. Otherwise, * the error is lost in deferred irq_work. */ -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH bpf v2 2/2] selftests/bpf: add send_signal_sched_switch test 2020-03-04 17:53 [PATCH bpf v2 0/2] bpf: Fix deadlock with rq_lock in bpf_send_signal() Yonghong Song 2020-03-04 17:53 ` [PATCH bpf v2 1/2] " Yonghong Song @ 2020-03-04 17:53 ` Yonghong Song 2020-03-04 18:08 ` Andrii Nakryiko 1 sibling, 1 reply; 7+ messages in thread From: Yonghong Song @ 2020-03-04 17:53 UTC (permalink / raw) To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, Song Liu Added one test, send_signal_sched_switch, to test bpf_send_signal() helper triggered by sched/sched_switch tracepoint. This test can be used to verify kernel deadlocks fixed by the previous commit. The test itself is heavily borrowed from Commit eac9153f2b58 ("bpf/stackmap: Fix deadlock with rq_lock in bpf_get_stack()"). Cc: Song Liu <songliubraving@fb.com> Signed-off-by: Yonghong Song <yhs@fb.com> --- .../bpf/prog_tests/send_signal_sched_switch.c | 89 +++++++++++++++++++ .../bpf/progs/test_send_signal_kern.c | 6 ++ 2 files changed, 95 insertions(+) create mode 100644 tools/testing/selftests/bpf/prog_tests/send_signal_sched_switch.c diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal_sched_switch.c b/tools/testing/selftests/bpf/prog_tests/send_signal_sched_switch.c new file mode 100644 index 000000000000..f5c9dbdeb173 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/send_signal_sched_switch.c @@ -0,0 +1,89 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <test_progs.h> +#include <stdio.h> +#include <stdlib.h> +#include <sys/mman.h> +#include <pthread.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include "test_send_signal_kern.skel.h" + +static void sigusr1_handler(int signum) +{ +} + +#define THREAD_COUNT 100 + +static char *filename; + +static void *worker(void *p) +{ + int err, fd, i = 0; + u32 duration = 0; + char *pptr; + void *ptr; + + fd = open(filename, O_RDONLY); + if (CHECK(fd < 0, "open", "open failed %s\n", strerror(errno))) + return NULL; + + while (i < 100) { + struct timespec ts = {0, 1000 + rand() % 2000}; + + ptr = mmap(NULL, 4096 * 64, PROT_READ, MAP_PRIVATE, fd, 0); + err = errno; + usleep(1); + if (CHECK(ptr == MAP_FAILED, "mmap", "mmap failed: %s\n", + strerror(err))) + break; + + munmap(ptr, 4096 * 64); + usleep(1); + pptr = malloc(1); + usleep(1); + pptr[0] = 1; + usleep(1); + free(pptr); + usleep(1); + nanosleep(&ts, NULL); + i++; + } + close(fd); + return NULL; +} + +void test_send_signal_sched_switch(void) +{ + struct test_send_signal_kern *skel; + pthread_t threads[THREAD_COUNT]; + u32 duration = 0; + int i, err; + + filename = "/bin/ls"; + signal(SIGUSR1, sigusr1_handler); + + skel = test_send_signal_kern__open_and_load(); + if (CHECK(!skel, "skel_open_and_load", "skeleton open_and_load failed\n")) + return; + + skel->bss->pid = getpid(); + skel->bss->sig = SIGUSR1; + + err = test_send_signal_kern__attach(skel); + if (CHECK(err, "skel_attach", "skeleton attach failed\n")) + goto destroy_skel; + + for (i = 0; i < THREAD_COUNT; i++) { + err = pthread_create(threads + i, NULL, worker, NULL); + if (CHECK(err, "pthread_create", "Error creating thread, %s\n", + strerror(errno))) + goto destroy_skel; + } + + for (i = 0; i < THREAD_COUNT; i++) + pthread_join(threads[i], NULL); + +destroy_skel: + test_send_signal_kern__destroy(skel); +} diff --git a/tools/testing/selftests/bpf/progs/test_send_signal_kern.c b/tools/testing/selftests/bpf/progs/test_send_signal_kern.c index 1acc91e87bfc..b4233d3efac2 100644 --- a/tools/testing/selftests/bpf/progs/test_send_signal_kern.c +++ b/tools/testing/selftests/bpf/progs/test_send_signal_kern.c @@ -31,6 +31,12 @@ int send_signal_tp(void *ctx) return bpf_send_signal_test(ctx); } +SEC("tracepoint/sched/sched_switch") +int send_signal_tp_sched(void *ctx) +{ + return bpf_send_signal_test(ctx); +} + SEC("perf_event") int send_signal_perf(void *ctx) { -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH bpf v2 2/2] selftests/bpf: add send_signal_sched_switch test 2020-03-04 17:53 ` [PATCH bpf v2 2/2] selftests/bpf: add send_signal_sched_switch test Yonghong Song @ 2020-03-04 18:08 ` Andrii Nakryiko 2020-03-04 18:12 ` Yonghong Song 0 siblings, 1 reply; 7+ messages in thread From: Andrii Nakryiko @ 2020-03-04 18:08 UTC (permalink / raw) To: Yonghong Song Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team, Song Liu On Wed, Mar 4, 2020 at 9:53 AM Yonghong Song <yhs@fb.com> wrote: > > Added one test, send_signal_sched_switch, to test bpf_send_signal() > helper triggered by sched/sched_switch tracepoint. This test can be used > to verify kernel deadlocks fixed by the previous commit. The test itself > is heavily borrowed from Commit eac9153f2b58 ("bpf/stackmap: Fix deadlock > with rq_lock in bpf_get_stack()"). > > Cc: Song Liu <songliubraving@fb.com> > Signed-off-by: Yonghong Song <yhs@fb.com> > --- > .../bpf/prog_tests/send_signal_sched_switch.c | 89 +++++++++++++++++++ > .../bpf/progs/test_send_signal_kern.c | 6 ++ > 2 files changed, 95 insertions(+) > create mode 100644 tools/testing/selftests/bpf/prog_tests/send_signal_sched_switch.c > > diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal_sched_switch.c b/tools/testing/selftests/bpf/prog_tests/send_signal_sched_switch.c > new file mode 100644 > index 000000000000..f5c9dbdeb173 > --- /dev/null > +++ b/tools/testing/selftests/bpf/prog_tests/send_signal_sched_switch.c > @@ -0,0 +1,89 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#include <test_progs.h> > +#include <stdio.h> > +#include <stdlib.h> > +#include <sys/mman.h> > +#include <pthread.h> > +#include <sys/types.h> > +#include <sys/stat.h> > +#include <fcntl.h> > +#include "test_send_signal_kern.skel.h" > + > +static void sigusr1_handler(int signum) > +{ > +} > + > +#define THREAD_COUNT 100 > + > +static char *filename; > + > +static void *worker(void *p) > +{ > + int err, fd, i = 0; > + u32 duration = 0; > + char *pptr; > + void *ptr; > + > + fd = open(filename, O_RDONLY); > + if (CHECK(fd < 0, "open", "open failed %s\n", strerror(errno))) > + return NULL; > + > + while (i < 100) { > + struct timespec ts = {0, 1000 + rand() % 2000}; > + > + ptr = mmap(NULL, 4096 * 64, PROT_READ, MAP_PRIVATE, fd, 0); > + err = errno; > + usleep(1); > + if (CHECK(ptr == MAP_FAILED, "mmap", "mmap failed: %s\n", > + strerror(err))) > + break; > + > + munmap(ptr, 4096 * 64); > + usleep(1); > + pptr = malloc(1); > + usleep(1); > + pptr[0] = 1; > + usleep(1); > + free(pptr); > + usleep(1); > + nanosleep(&ts, NULL); > + i++; Any specific reason to do mmap()/munmap() in a loop? Would, say, getpid syscall work just fine as well to trigger a bunch of context switches? Or event just usleep(1) alone? > + } > + close(fd); > + return NULL; > +} > + > +void test_send_signal_sched_switch(void) > +{ > + struct test_send_signal_kern *skel; > + pthread_t threads[THREAD_COUNT]; > + u32 duration = 0; > + int i, err; > + > + filename = "/bin/ls"; > + signal(SIGUSR1, sigusr1_handler); > + > + skel = test_send_signal_kern__open_and_load(); > + if (CHECK(!skel, "skel_open_and_load", "skeleton open_and_load failed\n")) > + return; > + > + skel->bss->pid = getpid(); > + skel->bss->sig = SIGUSR1; > + > + err = test_send_signal_kern__attach(skel); > + if (CHECK(err, "skel_attach", "skeleton attach failed\n")) > + goto destroy_skel; > + > + for (i = 0; i < THREAD_COUNT; i++) { > + err = pthread_create(threads + i, NULL, worker, NULL); > + if (CHECK(err, "pthread_create", "Error creating thread, %s\n", > + strerror(errno))) > + goto destroy_skel; > + } > + > + for (i = 0; i < THREAD_COUNT; i++) > + pthread_join(threads[i], NULL); > + > +destroy_skel: > + test_send_signal_kern__destroy(skel); > +} > diff --git a/tools/testing/selftests/bpf/progs/test_send_signal_kern.c b/tools/testing/selftests/bpf/progs/test_send_signal_kern.c > index 1acc91e87bfc..b4233d3efac2 100644 > --- a/tools/testing/selftests/bpf/progs/test_send_signal_kern.c > +++ b/tools/testing/selftests/bpf/progs/test_send_signal_kern.c > @@ -31,6 +31,12 @@ int send_signal_tp(void *ctx) > return bpf_send_signal_test(ctx); > } > > +SEC("tracepoint/sched/sched_switch") > +int send_signal_tp_sched(void *ctx) > +{ > + return bpf_send_signal_test(ctx); > +} > + > SEC("perf_event") > int send_signal_perf(void *ctx) > { > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf v2 2/2] selftests/bpf: add send_signal_sched_switch test 2020-03-04 18:08 ` Andrii Nakryiko @ 2020-03-04 18:12 ` Yonghong Song 2020-03-04 18:30 ` Song Liu 0 siblings, 1 reply; 7+ messages in thread From: Yonghong Song @ 2020-03-04 18:12 UTC (permalink / raw) To: Andrii Nakryiko Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team, Song Liu On 3/4/20 10:08 AM, Andrii Nakryiko wrote: > On Wed, Mar 4, 2020 at 9:53 AM Yonghong Song <yhs@fb.com> wrote: >> >> Added one test, send_signal_sched_switch, to test bpf_send_signal() >> helper triggered by sched/sched_switch tracepoint. This test can be used >> to verify kernel deadlocks fixed by the previous commit. The test itself >> is heavily borrowed from Commit eac9153f2b58 ("bpf/stackmap: Fix deadlock >> with rq_lock in bpf_get_stack()"). >> >> Cc: Song Liu <songliubraving@fb.com> >> Signed-off-by: Yonghong Song <yhs@fb.com> >> --- >> .../bpf/prog_tests/send_signal_sched_switch.c | 89 +++++++++++++++++++ >> .../bpf/progs/test_send_signal_kern.c | 6 ++ >> 2 files changed, 95 insertions(+) >> create mode 100644 tools/testing/selftests/bpf/prog_tests/send_signal_sched_switch.c >> >> diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal_sched_switch.c b/tools/testing/selftests/bpf/prog_tests/send_signal_sched_switch.c >> new file mode 100644 >> index 000000000000..f5c9dbdeb173 >> --- /dev/null >> +++ b/tools/testing/selftests/bpf/prog_tests/send_signal_sched_switch.c >> @@ -0,0 +1,89 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +#include <test_progs.h> >> +#include <stdio.h> >> +#include <stdlib.h> >> +#include <sys/mman.h> >> +#include <pthread.h> >> +#include <sys/types.h> >> +#include <sys/stat.h> >> +#include <fcntl.h> >> +#include "test_send_signal_kern.skel.h" >> + >> +static void sigusr1_handler(int signum) >> +{ >> +} >> + >> +#define THREAD_COUNT 100 >> + >> +static char *filename; >> + >> +static void *worker(void *p) >> +{ >> + int err, fd, i = 0; >> + u32 duration = 0; >> + char *pptr; >> + void *ptr; >> + >> + fd = open(filename, O_RDONLY); >> + if (CHECK(fd < 0, "open", "open failed %s\n", strerror(errno))) >> + return NULL; >> + >> + while (i < 100) { >> + struct timespec ts = {0, 1000 + rand() % 2000}; >> + >> + ptr = mmap(NULL, 4096 * 64, PROT_READ, MAP_PRIVATE, fd, 0); >> + err = errno; >> + usleep(1); >> + if (CHECK(ptr == MAP_FAILED, "mmap", "mmap failed: %s\n", >> + strerror(err))) >> + break; >> + >> + munmap(ptr, 4096 * 64); >> + usleep(1); >> + pptr = malloc(1); >> + usleep(1); >> + pptr[0] = 1; >> + usleep(1); >> + free(pptr); >> + usleep(1); >> + nanosleep(&ts, NULL); >> + i++; > > > Any specific reason to do mmap()/munmap() in a loop? Would, say, > getpid syscall work just fine as well to trigger a bunch of context > switches? Or event just usleep(1) alone? No particular reason. Copied from Song's original reproducer and it is working. Maybe Song can comment why it is written this way. > >> + } >> + close(fd); >> + return NULL; >> +} >> + >> +void test_send_signal_sched_switch(void) >> +{ >> + struct test_send_signal_kern *skel; >> + pthread_t threads[THREAD_COUNT]; >> + u32 duration = 0; >> + int i, err; >> + >> + filename = "/bin/ls"; >> + signal(SIGUSR1, sigusr1_handler); >> + >> + skel = test_send_signal_kern__open_and_load(); >> + if (CHECK(!skel, "skel_open_and_load", "skeleton open_and_load failed\n")) >> + return; >> + >> + skel->bss->pid = getpid(); >> + skel->bss->sig = SIGUSR1; >> + >> + err = test_send_signal_kern__attach(skel); >> + if (CHECK(err, "skel_attach", "skeleton attach failed\n")) >> + goto destroy_skel; >> + >> + for (i = 0; i < THREAD_COUNT; i++) { >> + err = pthread_create(threads + i, NULL, worker, NULL); >> + if (CHECK(err, "pthread_create", "Error creating thread, %s\n", >> + strerror(errno))) >> + goto destroy_skel; >> + } >> + >> + for (i = 0; i < THREAD_COUNT; i++) >> + pthread_join(threads[i], NULL); >> + >> +destroy_skel: >> + test_send_signal_kern__destroy(skel); >> +} >> diff --git a/tools/testing/selftests/bpf/progs/test_send_signal_kern.c b/tools/testing/selftests/bpf/progs/test_send_signal_kern.c >> index 1acc91e87bfc..b4233d3efac2 100644 >> --- a/tools/testing/selftests/bpf/progs/test_send_signal_kern.c >> +++ b/tools/testing/selftests/bpf/progs/test_send_signal_kern.c >> @@ -31,6 +31,12 @@ int send_signal_tp(void *ctx) >> return bpf_send_signal_test(ctx); >> } >> >> +SEC("tracepoint/sched/sched_switch") >> +int send_signal_tp_sched(void *ctx) >> +{ >> + return bpf_send_signal_test(ctx); >> +} >> + >> SEC("perf_event") >> int send_signal_perf(void *ctx) >> { >> -- >> 2.17.1 >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf v2 2/2] selftests/bpf: add send_signal_sched_switch test 2020-03-04 18:12 ` Yonghong Song @ 2020-03-04 18:30 ` Song Liu 2020-03-04 18:34 ` Yonghong Song 0 siblings, 1 reply; 7+ messages in thread From: Song Liu @ 2020-03-04 18:30 UTC (permalink / raw) To: Yonghong Song Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team > On Mar 4, 2020, at 10:12 AM, Yonghong Song <yhs@fb.com> wrote: > > > > On 3/4/20 10:08 AM, Andrii Nakryiko wrote: >> On Wed, Mar 4, 2020 at 9:53 AM Yonghong Song <yhs@fb.com> wrote: >>> >>> Added one test, send_signal_sched_switch, to test bpf_send_signal() >>> helper triggered by sched/sched_switch tracepoint. This test can be used >>> to verify kernel deadlocks fixed by the previous commit. The test itself >>> is heavily borrowed from Commit eac9153f2b58 ("bpf/stackmap: Fix deadlock >>> with rq_lock in bpf_get_stack()"). >>> >>> Cc: Song Liu <songliubraving@fb.com> >>> Signed-off-by: Yonghong Song <yhs@fb.com> >>> --- >>> .../bpf/prog_tests/send_signal_sched_switch.c | 89 +++++++++++++++++++ >>> .../bpf/progs/test_send_signal_kern.c | 6 ++ >>> 2 files changed, 95 insertions(+) >>> create mode 100644 tools/testing/selftests/bpf/prog_tests/send_signal_sched_switch.c >>> >>> diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal_sched_switch.c b/tools/testing/selftests/bpf/prog_tests/send_signal_sched_switch.c >>> new file mode 100644 >>> index 000000000000..f5c9dbdeb173 >>> --- /dev/null >>> +++ b/tools/testing/selftests/bpf/prog_tests/send_signal_sched_switch.c >>> @@ -0,0 +1,89 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +#include <test_progs.h> >>> +#include <stdio.h> >>> +#include <stdlib.h> >>> +#include <sys/mman.h> >>> +#include <pthread.h> >>> +#include <sys/types.h> >>> +#include <sys/stat.h> >>> +#include <fcntl.h> >>> +#include "test_send_signal_kern.skel.h" >>> + >>> +static void sigusr1_handler(int signum) >>> +{ >>> +} >>> + >>> +#define THREAD_COUNT 100 >>> + >>> +static char *filename; >>> + >>> +static void *worker(void *p) >>> +{ >>> + int err, fd, i = 0; >>> + u32 duration = 0; >>> + char *pptr; >>> + void *ptr; >>> + >>> + fd = open(filename, O_RDONLY); >>> + if (CHECK(fd < 0, "open", "open failed %s\n", strerror(errno))) >>> + return NULL; >>> + >>> + while (i < 100) { >>> + struct timespec ts = {0, 1000 + rand() % 2000}; >>> + >>> + ptr = mmap(NULL, 4096 * 64, PROT_READ, MAP_PRIVATE, fd, 0); >>> + err = errno; >>> + usleep(1); >>> + if (CHECK(ptr == MAP_FAILED, "mmap", "mmap failed: %s\n", >>> + strerror(err))) >>> + break; >>> + >>> + munmap(ptr, 4096 * 64); >>> + usleep(1); >>> + pptr = malloc(1); >>> + usleep(1); >>> + pptr[0] = 1; >>> + usleep(1); >>> + free(pptr); >>> + usleep(1); >>> + nanosleep(&ts, NULL); >>> + i++; >> Any specific reason to do mmap()/munmap() in a loop? Would, say, >> getpid syscall work just fine as well to trigger a bunch of context >> switches? Or event just usleep(1) alone? > > No particular reason. Copied from Song's original reproducer and > it is working. Maybe Song can comment why it is written this way. In that test, mmap/munmap are used to lock mmap_sem. I guess we don't really need them in this test. Thanks, song ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf v2 2/2] selftests/bpf: add send_signal_sched_switch test 2020-03-04 18:30 ` Song Liu @ 2020-03-04 18:34 ` Yonghong Song 0 siblings, 0 replies; 7+ messages in thread From: Yonghong Song @ 2020-03-04 18:34 UTC (permalink / raw) To: Song Liu Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team On 3/4/20 10:30 AM, Song Liu wrote: > > >> On Mar 4, 2020, at 10:12 AM, Yonghong Song <yhs@fb.com> wrote: >> >> >> >> On 3/4/20 10:08 AM, Andrii Nakryiko wrote: >>> On Wed, Mar 4, 2020 at 9:53 AM Yonghong Song <yhs@fb.com> wrote: >>>> >>>> Added one test, send_signal_sched_switch, to test bpf_send_signal() >>>> helper triggered by sched/sched_switch tracepoint. This test can be used >>>> to verify kernel deadlocks fixed by the previous commit. The test itself >>>> is heavily borrowed from Commit eac9153f2b58 ("bpf/stackmap: Fix deadlock >>>> with rq_lock in bpf_get_stack()"). >>>> >>>> Cc: Song Liu <songliubraving@fb.com> >>>> Signed-off-by: Yonghong Song <yhs@fb.com> >>>> --- >>>> .../bpf/prog_tests/send_signal_sched_switch.c | 89 +++++++++++++++++++ >>>> .../bpf/progs/test_send_signal_kern.c | 6 ++ >>>> 2 files changed, 95 insertions(+) >>>> create mode 100644 tools/testing/selftests/bpf/prog_tests/send_signal_sched_switch.c >>>> >>>> diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal_sched_switch.c b/tools/testing/selftests/bpf/prog_tests/send_signal_sched_switch.c >>>> new file mode 100644 >>>> index 000000000000..f5c9dbdeb173 >>>> --- /dev/null >>>> +++ b/tools/testing/selftests/bpf/prog_tests/send_signal_sched_switch.c >>>> @@ -0,0 +1,89 @@ >>>> +// SPDX-License-Identifier: GPL-2.0 >>>> +#include <test_progs.h> >>>> +#include <stdio.h> >>>> +#include <stdlib.h> >>>> +#include <sys/mman.h> >>>> +#include <pthread.h> >>>> +#include <sys/types.h> >>>> +#include <sys/stat.h> >>>> +#include <fcntl.h> >>>> +#include "test_send_signal_kern.skel.h" >>>> + >>>> +static void sigusr1_handler(int signum) >>>> +{ >>>> +} >>>> + >>>> +#define THREAD_COUNT 100 >>>> + >>>> +static char *filename; >>>> + >>>> +static void *worker(void *p) >>>> +{ >>>> + int err, fd, i = 0; >>>> + u32 duration = 0; >>>> + char *pptr; >>>> + void *ptr; >>>> + >>>> + fd = open(filename, O_RDONLY); >>>> + if (CHECK(fd < 0, "open", "open failed %s\n", strerror(errno))) >>>> + return NULL; >>>> + >>>> + while (i < 100) { >>>> + struct timespec ts = {0, 1000 + rand() % 2000}; >>>> + >>>> + ptr = mmap(NULL, 4096 * 64, PROT_READ, MAP_PRIVATE, fd, 0); >>>> + err = errno; >>>> + usleep(1); >>>> + if (CHECK(ptr == MAP_FAILED, "mmap", "mmap failed: %s\n", >>>> + strerror(err))) >>>> + break; >>>> + >>>> + munmap(ptr, 4096 * 64); >>>> + usleep(1); >>>> + pptr = malloc(1); >>>> + usleep(1); >>>> + pptr[0] = 1; >>>> + usleep(1); >>>> + free(pptr); >>>> + usleep(1); >>>> + nanosleep(&ts, NULL); >>>> + i++; >>> Any specific reason to do mmap()/munmap() in a loop? Would, say, >>> getpid syscall work just fine as well to trigger a bunch of context >>> switches? Or event just usleep(1) alone? >> >> No particular reason. Copied from Song's original reproducer and >> it is working. Maybe Song can comment why it is written this way. > > In that test, mmap/munmap are used to lock mmap_sem. I guess we > don't really need them in this test. Thanks. I will simplify the test then. The goal is to get more context switches. > > Thanks, > song > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-03-04 18:35 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-04 17:53 [PATCH bpf v2 0/2] bpf: Fix deadlock with rq_lock in bpf_send_signal() Yonghong Song 2020-03-04 17:53 ` [PATCH bpf v2 1/2] " Yonghong Song 2020-03-04 17:53 ` [PATCH bpf v2 2/2] selftests/bpf: add send_signal_sched_switch test Yonghong Song 2020-03-04 18:08 ` Andrii Nakryiko 2020-03-04 18:12 ` Yonghong Song 2020-03-04 18:30 ` Song Liu 2020-03-04 18:34 ` Yonghong Song
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).