> On Mar 2, 2022, at 1:03 PM, Mykola Lysenko <mykolal@xxxxxx> wrote:
>
>
>
>> On Mar 1, 2022, at 9:23 PM, Yonghong Song <yhs@xxxxxx> wrote:
>>
>>
>>
>> On 2/28/22 7:39 PM, Mykola Lysenko wrote:
>>> In send_signal, replace sleep with dummy cpu intensive computation
>>> to increase probability of child process being scheduled. Add few
>>> more asserts.
>>> In find_vma, reduce sample_freq as higher values may be rejected in
>>> some qemu setups, remove usleep and increase length of cpu intensive
>>> computation.
>>> In bpf_cookie, perf_link and perf_branches, reduce sample_freq as
>>> higher values may be rejected in some qemu setups
>>> Signed-off-by: Mykola Lysenko <mykolal@xxxxxx>
>>
>> LGTM with a few nits below.
>>
>> Acked-by: Yonghong Song <yhs@xxxxxx>
>
> Thanks for the review!
>
>>
>>> ---
>>> .../selftests/bpf/prog_tests/bpf_cookie.c | 2 +-
>>> .../testing/selftests/bpf/prog_tests/find_vma.c | 13 ++++++++++---
>>> .../selftests/bpf/prog_tests/perf_branches.c | 4 ++--
>>> .../selftests/bpf/prog_tests/perf_link.c | 2 +-
>>> .../selftests/bpf/prog_tests/send_signal.c | 17 ++++++++++-------
>>> .../selftests/bpf/progs/test_send_signal_kern.c | 2 +-
>>> 6 files changed, 25 insertions(+), 15 deletions(-)
>>> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
>>> index cd10df6cd0fc..0612e79a9281 100644
>>> --- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
>>> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
>>> @@ -199,7 +199,7 @@ static void pe_subtest(struct test_bpf_cookie *skel)
>>> attr.type = PERF_TYPE_SOFTWARE;
>>> attr.config = PERF_COUNT_SW_CPU_CLOCK;
>>> attr.freq = 1;
>>> - attr.sample_freq = 4000;
>>> + attr.sample_freq = 1000;
>>> pfd = syscall(__NR_perf_event_open, &attr, -1, 0, -1, PERF_FLAG_FD_CLOEXEC);
>>> if (!ASSERT_GE(pfd, 0, "perf_fd"))
>>> goto cleanup;
>>> diff --git a/tools/testing/selftests/bpf/prog_tests/find_vma.c b/tools/testing/selftests/bpf/prog_tests/find_vma.c
>>> index b74b3c0c555a..a0b68381cd79 100644
>>> --- a/tools/testing/selftests/bpf/prog_tests/find_vma.c
>>> +++ b/tools/testing/selftests/bpf/prog_tests/find_vma.c
>>> @@ -30,12 +30,20 @@ static int open_pe(void)
>>> attr.type = PERF_TYPE_HARDWARE;
>>> attr.config = PERF_COUNT_HW_CPU_CYCLES;
>>> attr.freq = 1;
>>> - attr.sample_freq = 4000;
>>> + attr.sample_freq = 1000;
>>> pfd = syscall(__NR_perf_event_open, &attr, 0, -1, -1, PERF_FLAG_FD_CLOEXEC);
>>> return pfd >= 0 ? pfd : -errno;
>>> }
>>> +static bool find_vma_pe_condition(struct find_vma *skel)
>>> +{
>>> + return skel->bss->found_vm_exec != 1 ||
>>
>> In test_and_reset_skel(), we have following codes for reset/default values:
>> skel->bss->found_vm_exec = 0;
>> skel->data->find_addr_ret = -1;
>> skel->data->find_zero_ret = -1;
>> skel->bss->d_iname[0] = 0;
>>
>> I think we should stick to them, so it would be good
>> to change
>> skel->bss->found_vm_exec != 1
>> to
>> skel->bss->found_vm_exec == 0
>>
>>> + skel->data->find_addr_ret == -1 ||
>>> + skel->data->find_zero_ret != 0 ||
>>
>> Change
>> skel->data->find_zero_ret != 0
>> to
>> skel->data->find_zero_ret == -1
>>
>> The bpf program may set skel->data->find_zero_ret to
>> -ENOENT (-2) or -EBUSY (-16) in which case we should
>> stop the iteration.
>
> Debugged this test a bit more, and it seems we should continue iterating when -16 is returned, as it converges to 0 eventually and test passes.
>
> Will you be ok to add check that find_zero_ret == -1 or == -16?
Correction, I read your comment incorrectly. Will do the change as you asked.
>
>>
>>> + skel->bss->d_iname[0] == 0;
>>> +}
>>> +
>>> static void test_find_vma_pe(struct find_vma *skel)
>>> {
>>> struct bpf_link *link = NULL;
>>> @@ -57,7 +65,7 @@ static void test_find_vma_pe(struct find_vma *skel)
>>> if (!ASSERT_OK_PTR(link, "attach_perf_event"))
>>> goto cleanup;
>>> - for (i = 0; i < 1000000; ++i)
>>> + for (i = 0; i < 1000000000 && find_vma_pe_condition(skel); ++i)
>>> ++j;
>>> test_and_reset_skel(skel, -EBUSY /* in nmi, irq_work is busy */);
>>> @@ -108,7 +116,6 @@ void serial_test_find_vma(void)
>>> skel->bss->addr = (__u64)(uintptr_t)test_find_vma_pe;
>>> test_find_vma_pe(skel);
>>> - usleep(100000); /* allow the irq_work to finish */
>>> test_find_vma_kprobe(skel);
>>> find_vma__destroy(skel);
>>> diff --git a/tools/testing/selftests/bpf/prog_tests/perf_branches.c b/tools/testing/selftests/bpf/prog_tests/perf_branches.c
>>> index 12c4f45cee1a..bc24f83339d6 100644
>>> --- a/tools/testing/selftests/bpf/prog_tests/perf_branches.c
>>> +++ b/tools/testing/selftests/bpf/prog_tests/perf_branches.c
>>> @@ -110,7 +110,7 @@ static void test_perf_branches_hw(void)
>>> attr.type = PERF_TYPE_HARDWARE;
>>> attr.config = PERF_COUNT_HW_CPU_CYCLES;
>>> attr.freq = 1;
>>> - attr.sample_freq = 4000;
>>> + attr.sample_freq = 1000;
>>> attr.sample_type = PERF_SAMPLE_BRANCH_STACK;
>>> attr.branch_sample_type = PERF_SAMPLE_BRANCH_USER | PERF_SAMPLE_BRANCH_ANY;
>>> pfd = syscall(__NR_perf_event_open, &attr, -1, 0, -1, PERF_FLAG_FD_CLOEXEC);
>>> @@ -151,7 +151,7 @@ static void test_perf_branches_no_hw(void)
>>> attr.type = PERF_TYPE_SOFTWARE;
>>> attr.config = PERF_COUNT_SW_CPU_CLOCK;
>>> attr.freq = 1;
>>> - attr.sample_freq = 4000;
>>> + attr.sample_freq = 1000;
>>> pfd = syscall(__NR_perf_event_open, &attr, -1, 0, -1, PERF_FLAG_FD_CLOEXEC);
>>> if (CHECK(pfd < 0, "perf_event_open", "err %d\n", pfd))
>>> return;
>>> diff --git a/tools/testing/selftests/bpf/prog_tests/perf_link.c b/tools/testing/selftests/bpf/prog_tests/perf_link.c
>>> index ede07344f264..224eba6fef2e 100644
>>> --- a/tools/testing/selftests/bpf/prog_tests/perf_link.c
>>> +++ b/tools/testing/selftests/bpf/prog_tests/perf_link.c
>>> @@ -39,7 +39,7 @@ void serial_test_perf_link(void)
>>> attr.type = PERF_TYPE_SOFTWARE;
>>> attr.config = PERF_COUNT_SW_CPU_CLOCK;
>>> attr.freq = 1;
>>> - attr.sample_freq = 4000;
>>> + attr.sample_freq = 1000;
>>> pfd = syscall(__NR_perf_event_open, &attr, -1, 0, -1, PERF_FLAG_FD_CLOEXEC);
>>> if (!ASSERT_GE(pfd, 0, "perf_fd"))
>>> goto cleanup;
>>> diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
>>> index 776916b61c40..b1b574c7016a 100644
>>> --- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
>>> +++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
>>> @@ -4,11 +4,11 @@
>>> #include <sys/resource.h>
>>> #include "test_send_signal_kern.skel.h"
>>> -int sigusr1_received = 0;
>>> +static int sigusr1_received;
>>> static void sigusr1_handler(int signum)
>>> {
>>> - sigusr1_received++;
>>> + sigusr1_received = 1;
>>> }
>>> static void test_send_signal_common(struct perf_event_attr *attr,
>>> @@ -40,9 +40,10 @@ static void test_send_signal_common(struct perf_event_attr *attr,
>>> if (pid == 0) {
>>> int old_prio;
>>> + volatile int volatile_variable = 0;
>>
>> I think it is okay to use variable 'j' to be consistent with other
>> similar codes in selftests.
>
> Sounds good
>
>>
>>> /* install signal handler and notify parent */
>>> - signal(SIGUSR1, sigusr1_handler);
>>> + ASSERT_NEQ(signal(SIGUSR1, sigusr1_handler), SIG_ERR, "signal");
>>> close(pipe_c2p[0]); /* close read */
>>> close(pipe_p2c[1]); /* close write */
>>> @@ -63,9 +64,11 @@ 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 < 100000000 && !sigusr1_received; i++)
>>> + volatile_variable /= i + 1;
>>> buf[0] = sigusr1_received ? '2' : '0';
>>> + ASSERT_EQ(sigusr1_received, 1, "sigusr1_received");
>>> ASSERT_EQ(write(pipe_c2p[1], buf, 1), 1, "pipe_write");
>>> /* wait for parent notification and exit */
>> [...]
