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

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

[Mykola Lysenko]

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@fb.com>
---
 .../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 ||
+		skel->data->find_addr_ret == -1 ||
+		skel->data->find_zero_ret != 0 ||
+		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;
 
 		/* 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 */
@@ -93,7 +96,7 @@ static void test_send_signal_common(struct perf_event_attr *attr,
 			goto destroy_skel;
 		}
 	} else {
-		pmu_fd = syscall(__NR_perf_event_open, attr, pid, -1,
+		pmu_fd = syscall(__NR_perf_event_open, attr, pid, -1 /* cpu */,
 				 -1 /* group id */, 0 /* flags */);
 		if (!ASSERT_GE(pmu_fd, 0, "perf_event_open")) {
 			err = -1;
@@ -110,9 +113,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->pid = pid;
-	skel->bss->sig = SIGUSR1;
 	skel->bss->signal_thread = signal_thread;
+	skel->bss->sig = SIGUSR1;
+	skel->bss->pid = pid;
 
 	/* notify child that bpf program can send_signal now */
 	ASSERT_EQ(write(pipe_p2c[1], buf, 1), 1, "pipe_write");
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 b4233d3efac2..92354cd72044 100644
--- a/tools/testing/selftests/bpf/progs/test_send_signal_kern.c
+++ b/tools/testing/selftests/bpf/progs/test_send_signal_kern.c
@@ -10,7 +10,7 @@ static __always_inline int bpf_send_signal_test(void *ctx)
 {
 	int ret;
 
-	if (status != 0 || sig == 0 || pid == 0)
+	if (status != 0 || pid == 0)
 		return 0;
 
 	if ((bpf_get_current_pid_tgid() >> 32) == pid) {
-- 
2.30.2

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

[Yonghong Song]

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@fb.com>

LGTM with a few nits below.

Acked-by: Yonghong Song <yhs@fb.com>

> ---
>   .../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.

> +		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.

>   
>   		/* 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 */
[...]

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

[Mykola Lysenko]

> On Mar 1, 2022, at 9:23 PM, Yonghong Song <yhs@fb.com> 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@fb.com>
> 
> LGTM with a few nits below.
> 
> Acked-by: Yonghong Song <yhs@fb.com>

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?

> 
>> +		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 */
> [...]

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

[Mykola Lysenko]

> On Mar 2, 2022, at 1:03 PM, Mykola Lysenko <mykolal@fb.com> wrote:
> 
> 
> 
>> On Mar 1, 2022, at 9:23 PM, Yonghong Song <yhs@fb.com> 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@fb.com>
>> 
>> LGTM with a few nits below.
>> 
>> Acked-by: Yonghong Song <yhs@fb.com>
> 
> 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 */
>> [...]