[bpf,v2,2/2] selftests/bpf: add send_signal_sched_switch test

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

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
>