Re: [PATCH bpf-next] Improve BPF test stability (related to perf events and scheduling)

[Mykola Lysenko <mykolal@fb.com>]

Thanks for your comments Song!


> On Feb 18, 2022, at 5:47 PM, Song Liu <song@kernel.org> wrote:
> 
> On Fri, Feb 18, 2022 at 4:30 PM Mykola Lysenko <mykolal@fb.com> wrote:
>> 
>> In send_signal, replace sleep with dummy cpu intensive computation
>> to increase probability of child process being scheduled. Add few
>> more asserts.
> 
> How often do we see the test fail because the child process is not
> scheduled?

I just ran this test, non-modified, 100 times in a loop and got 94 failures. However, it is on my setup when using qemu for testing.

> 
> [...]
> 
>> static void sigusr1_handler(int signum)
>> {
>> -       sigusr1_received++;
>> +       sigusr1_received = 1;
>> }
>> 
>> static void test_send_signal_common(struct perf_event_attr *attr,
>> @@ -42,7 +43,9 @@ static void test_send_signal_common(struct perf_event_attr *attr,
>>                int old_prio;
>> 
>>                /* install signal handler and notify parent */
>> +               errno = 0;
>>                signal(SIGUSR1, sigusr1_handler);
>> +               ASSERT_OK(errno, "signal");
>> 
>>                close(pipe_c2p[0]); /* close read */
>>                close(pipe_p2c[1]); /* close write */
>> @@ -63,9 +66,12 @@ static void test_send_signal_common(struct perf_event_attr *attr,
>>                ASSERT_EQ(read(pipe_p2c[0], buf, 1), 1, "pipe_read");
>> 
>>                /* wait a little for signal handler */
>> -               sleep(1);
>> +               for (int i = 0; i < 1000000000; i++)
>> +                       volatile_variable++;
>> 
>>                buf[0] = sigusr1_received ? '2' : '0';
> 
> ^^^^ this "? :" seems useless as we assert sigusr1_received == 1. Let's fix it.

In this branch (true for "pid == 0” condition), we execute in the child process. Just asserting here would not fail the test unfortunately. Child process will die (after exit(0)) and test will succeed. However, you then may ask why do we need assert at all. It is useful if we want to debug what happens in the child process. Right now, child process does not print anything and I will fix it in the second version of this patch by substituting exit(0) with return; 

> 
>> +               ASSERT_EQ(sigusr1_received, 1, "sigusr1_received");
>> +
>>                ASSERT_EQ(write(pipe_c2p[1], buf, 1), 1, "pipe_write");
>> 
>>                /* wait for parent notification and exit */
>> @@ -110,9 +116,9 @@ static void test_send_signal_common(struct perf_event_attr *attr,
>>        ASSERT_EQ(read(pipe_c2p[0], buf, 1), 1, "pipe_read");
>> 
>>        /* trigger the bpf send_signal */
>> +       skel->bss->signal_thread = signal_thread;
>>        skel->bss->pid = pid;
>>        skel->bss->sig = SIGUSR1;
>> -       skel->bss->signal_thread = signal_thread;
> 
> Does the order matter here?

Unfortunately, yes. The bpf logic will start executing after sig or pid became non-zero (see /d/u/m/l/t/t/s/b/p/test_send_signal_kern.c, line 13). I am trying to remove ambiguity here with this small change. Although, pid and sig still may conflict. Let me think if it will be better to fix the bpf code itself.