Message ID | 20200304175311.2389987-1-yhs@fb.com |
---|---|
State | Changes Requested |
Delegated to: | BPF Maintainers |
Headers | show |
Series | bpf: Fix deadlock with rq_lock in bpf_send_signal() | expand |
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 >
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 >>
> 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
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 >
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) {
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