Re: [PATCH bpf v2 2/2] selftests/bpf: add send_signal_sched_switch test

[Yonghong Song <yhs@fb.com>]

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
>