[PATCH bpf v2 0/5] fix test_sockmap

[PATCH bpf v2 0/5] fix test_sockmap

[Prashant Bhole]

This series fixes error handling, timeout and data verification in
test_sockmap. Previously it was not able to detect failure/timeout in
RX/TX thread because error was not notified to the main thread.

Also slightly improved test output by printing parameter values (cork,
apply, start, end) so that parameters for all tests are displayed.

Prashant Bhole (5):
  selftests/bpf: test_sockmap, check test failure
  selftests/bpf: test_sockmap, join cgroup in selftest mode
  selftests/bpf: test_sockmap, fix test timeout
  selftests/bpf: test_sockmap, fix data verification
  selftests/bpf: test_sockmap, print additional test options

 tools/testing/selftests/bpf/test_sockmap.c | 76 +++++++++++++++++-----
 1 file changed, 58 insertions(+), 18 deletions(-)

-- 
2.17.0

[PATCH bpf v2 1/5] selftests/bpf: test_sockmap, check test failure

[Prashant Bhole]

Test failures are not identified because exit code of RX/TX threads
is not checked. Also threads are not returning correct exit code.

- Return exit code from threads depending on test execution status
- In main thread, check the exit code of RX/TX threads

Fixes: 16962b2404ac ("bpf: sockmap, add selftests")
Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
---
 tools/testing/selftests/bpf/test_sockmap.c | 25 ++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index eb17fae458e6..34feb74c95c4 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -429,8 +429,8 @@ static int sendmsg_test(struct sockmap_options *opt)
 	struct msg_stats s = {0};
 	int iov_count = opt->iov_count;
 	int iov_buf = opt->iov_length;
+	int rx_status, tx_status;
 	int cnt = opt->rate;
-	int status;
 
 	errno = 0;
 
@@ -442,7 +442,7 @@ static int sendmsg_test(struct sockmap_options *opt)
 	rxpid = fork();
 	if (rxpid == 0) {
 		if (opt->drop_expected)
-			exit(1);
+			exit(0);
 
 		if (opt->sendpage)
 			iov_count = 1;
@@ -463,7 +463,7 @@ static int sendmsg_test(struct sockmap_options *opt)
 				"rx_sendmsg: TX: %zuB %fB/s %fGB/s RX: %zuB %fB/s %fGB/s\n",
 				s.bytes_sent, sent_Bps, sent_Bps/giga,
 				s.bytes_recvd, recvd_Bps, recvd_Bps/giga);
-		exit(1);
+		exit(err ? 1 : 0);
 	} else if (rxpid == -1) {
 		perror("msg_loop_rx: ");
 		return errno;
@@ -491,14 +491,27 @@ static int sendmsg_test(struct sockmap_options *opt)
 				"tx_sendmsg: TX: %zuB %fB/s %f GB/s RX: %zuB %fB/s %fGB/s\n",
 				s.bytes_sent, sent_Bps, sent_Bps/giga,
 				s.bytes_recvd, recvd_Bps, recvd_Bps/giga);
-		exit(1);
+		exit(err ? 1 : 0);
 	} else if (txpid == -1) {
 		perror("msg_loop_tx: ");
 		return errno;
 	}
 
-	assert(waitpid(rxpid, &status, 0) == rxpid);
-	assert(waitpid(txpid, &status, 0) == txpid);
+	assert(waitpid(rxpid, &rx_status, 0) == rxpid);
+	assert(waitpid(txpid, &tx_status, 0) == txpid);
+	if (WIFEXITED(rx_status)) {
+		err = WEXITSTATUS(rx_status);
+		if (err) {
+			fprintf(stderr, "rx thread exited with err %d. ", err);
+			goto out;
+		}
+	}
+	if (WIFEXITED(tx_status)) {
+		err = WEXITSTATUS(tx_status);
+		if (err)
+			fprintf(stderr, "tx thread exited with err %d. ", err);
+	}
+out:
 	return err;
 }
 
-- 
2.17.0

Re: [PATCH bpf v2 1/5] selftests/bpf: test_sockmap, check test failure

[John Fastabend]

On 05/27/2018 09:37 PM, Prashant Bhole wrote:
> Test failures are not identified because exit code of RX/TX threads
> is not checked. Also threads are not returning correct exit code.
> 
> - Return exit code from threads depending on test execution status
> - In main thread, check the exit code of RX/TX threads
> 
> Fixes: 16962b2404ac ("bpf: sockmap, add selftests")
> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> ---

Acked-by: John Fastabend <john.fastabend@gmail.com>

>  tools/testing/selftests/bpf/test_sockmap.c | 25 ++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
> index eb17fae458e6..34feb74c95c4 100644
> --- a/tools/testing/selftests/bpf/test_sockmap.c
> +++ b/tools/testing/selftests/bpf/test_sockmap.c
> @@ -429,8 +429,8 @@ static int sendmsg_test(struct sockmap_options *opt)
>  	struct msg_stats s = {0};
>  	int iov_count = opt->iov_count;
>  	int iov_buf = opt->iov_length;
> +	int rx_status, tx_status;
>  	int cnt = opt->rate;
> -	int status;
>  
>  	errno = 0;
>  
> @@ -442,7 +442,7 @@ static int sendmsg_test(struct sockmap_options *opt)
>  	rxpid = fork();
>  	if (rxpid == 0) {
>  		if (opt->drop_expected)
> -			exit(1);
> +			exit(0);
>  
>  		if (opt->sendpage)
>  			iov_count = 1;
> @@ -463,7 +463,7 @@ static int sendmsg_test(struct sockmap_options *opt)
>  				"rx_sendmsg: TX: %zuB %fB/s %fGB/s RX: %zuB %fB/s %fGB/s\n",
>  				s.bytes_sent, sent_Bps, sent_Bps/giga,
>  				s.bytes_recvd, recvd_Bps, recvd_Bps/giga);
> -		exit(1);
> +		exit(err ? 1 : 0);
>  	} else if (rxpid == -1) {
>  		perror("msg_loop_rx: ");
>  		return errno;
> @@ -491,14 +491,27 @@ static int sendmsg_test(struct sockmap_options *opt)
>  				"tx_sendmsg: TX: %zuB %fB/s %f GB/s RX: %zuB %fB/s %fGB/s\n",
>  				s.bytes_sent, sent_Bps, sent_Bps/giga,
>  				s.bytes_recvd, recvd_Bps, recvd_Bps/giga);
> -		exit(1);
> +		exit(err ? 1 : 0);
>  	} else if (txpid == -1) {
>  		perror("msg_loop_tx: ");
>  		return errno;
>  	}
>  
> -	assert(waitpid(rxpid, &status, 0) == rxpid);
> -	assert(waitpid(txpid, &status, 0) == txpid);
> +	assert(waitpid(rxpid, &rx_status, 0) == rxpid);
> +	assert(waitpid(txpid, &tx_status, 0) == txpid);
> +	if (WIFEXITED(rx_status)) {
> +		err = WEXITSTATUS(rx_status);
> +		if (err) {
> +			fprintf(stderr, "rx thread exited with err %d. ", err);
> +			goto out;
> +		}
> +	}
> +	if (WIFEXITED(tx_status)) {
> +		err = WEXITSTATUS(tx_status);
> +		if (err)
> +			fprintf(stderr, "tx thread exited with err %d. ", err);
> +	}
> +out:
>  	return err;
>  }
>  
> 

[PATCH bpf v2 2/5] selftests/bpf: test_sockmap, join cgroup in selftest mode

[Prashant Bhole]

In case of selftest mode, temporary cgroup environment is created but
cgroup is not joined. It causes test failures. Fixed by joining the
cgroup

Fixes: 16962b2404ac ("bpf: sockmap, add selftests")
Acked-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
---
 tools/testing/selftests/bpf/test_sockmap.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index 34feb74c95c4..8a81ea0e9fb6 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -1342,6 +1342,11 @@ static int __test_suite(char *bpf_file)
 		return cg_fd;
 	}
 
+	if (join_cgroup(CG_PATH)) {
+		fprintf(stderr, "ERROR: failed to join cgroup\n");
+		return -EINVAL;
+	}
+
 	/* Tests basic commands and APIs with range of iov values */
 	txmsg_start = txmsg_end = 0;
 	err = test_txmsg(cg_fd);
-- 
2.17.0

[PATCH bpf v2 3/5] selftests/bpf: test_sockmap, fix test timeout

[Prashant Bhole]

In order to reduce runtime of tests, recently timout for select() call
was reduced from 1sec to 10usec. This was causing many tests failures.
It was caught with failure handling commits in this series.

Restoring the timeout from 10usec to 1sec

Fixes: a18fda1a62c3 ("bpf: reduce runtime of test_sockmap tests")
Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
---
 tools/testing/selftests/bpf/test_sockmap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index 8a81ea0e9fb6..f79397513362 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -345,8 +345,8 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
 		if (err < 0)
 			perror("recv start time: ");
 		while (s->bytes_recvd < total_bytes) {
-			timeout.tv_sec = 0;
-			timeout.tv_usec = 10;
+			timeout.tv_sec = 1;
+			timeout.tv_usec = 0;
 
 			/* FD sets */
 			FD_ZERO(&w);
-- 
2.17.0

[PATCH bpf v2 4/5] selftests/bpf: test_sockmap, fix data verification

[Prashant Bhole]

When data verification is enabled, some tests fail because verification is done
incorrectly. Following changes fix it.

- Identify the size of data block to be verified
- Reset verification counter when data block size is reached
- Fixed the value printed in case of verfication failure

Fixes: 16962b2404ac ("bpf: sockmap, add selftests")
Acked-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
---
 tools/testing/selftests/bpf/test_sockmap.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index f79397513362..e8faf4596dac 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -337,8 +337,15 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
 		int fd_flags = O_NONBLOCK;
 		struct timeval timeout;
 		float total_bytes;
+		int bytes_cnt = 0;
+		int chunk_sz;
 		fd_set w;
 
+		if (opt->sendpage)
+			chunk_sz = iov_length * cnt;
+		else
+			chunk_sz = iov_length * iov_count;
+
 		fcntl(fd, fd_flags);
 		total_bytes = (float)iov_count * (float)iov_length * (float)cnt;
 		err = clock_gettime(CLOCK_MONOTONIC, &s->start);
@@ -388,9 +395,14 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
 							errno = -EIO;
 							fprintf(stderr,
 								"detected data corruption @iov[%i]:%i %02x != %02x, %02x ?= %02x\n",
-								i, j, d[j], k - 1, d[j+1], k + 1);
+								i, j, d[j], k - 1, d[j+1], k);
 							goto out_errno;
 						}
+						bytes_cnt++;
+						if (bytes_cnt == chunk_sz) {
+							k = 0;
+							bytes_cnt = 0;
+						}
 						recv--;
 					}
 				}
-- 
2.17.0

[PATCH bpf v2 5/5] selftests/bpf: test_sockmap, print additional test options

[Prashant Bhole]

Print values of test options like apply, cork, start, end so that
individual failed tests can be identified for manual run

Acked-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
---
 tools/testing/selftests/bpf/test_sockmap.c | 28 +++++++++++++++-------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index e8faf4596dac..7970d48c4acb 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -869,6 +869,8 @@ static char *test_to_str(int test)
 #define OPTSTRING 60
 static void test_options(char *options)
 {
+	char tstr[OPTSTRING];
+
 	memset(options, 0, OPTSTRING);
 
 	if (txmsg_pass)
@@ -881,14 +883,22 @@ static void test_options(char *options)
 		strncat(options, "redir_noisy,", OPTSTRING);
 	if (txmsg_drop)
 		strncat(options, "drop,", OPTSTRING);
-	if (txmsg_apply)
-		strncat(options, "apply,", OPTSTRING);
-	if (txmsg_cork)
-		strncat(options, "cork,", OPTSTRING);
-	if (txmsg_start)
-		strncat(options, "start,", OPTSTRING);
-	if (txmsg_end)
-		strncat(options, "end,", OPTSTRING);
+	if (txmsg_apply) {
+		snprintf(tstr, OPTSTRING, "apply %d,", txmsg_apply);
+		strncat(options, tstr, OPTSTRING);
+	}
+	if (txmsg_cork) {
+		snprintf(tstr, OPTSTRING, "cork %d,", txmsg_cork);
+		strncat(options, tstr, OPTSTRING);
+	}
+	if (txmsg_start) {
+		snprintf(tstr, OPTSTRING, "start %d,", txmsg_start);
+		strncat(options, tstr, OPTSTRING);
+	}
+	if (txmsg_end) {
+		snprintf(tstr, OPTSTRING, "end %d,", txmsg_end);
+		strncat(options, tstr, OPTSTRING);
+	}
 	if (txmsg_ingress)
 		strncat(options, "ingress,", OPTSTRING);
 	if (txmsg_skb)
@@ -897,7 +907,7 @@ static void test_options(char *options)
 
 static int __test_exec(int cgrp, int test, struct sockmap_options *opt)
 {
-	char *options = calloc(60, sizeof(char));
+	char *options = calloc(OPTSTRING, sizeof(char));
 	int err;
 
 	if (test == SENDPAGE)
-- 
2.17.0

Re: [PATCH bpf v2 0/5] fix test_sockmap

[John Fastabend]

On 05/27/2018 09:37 PM, Prashant Bhole wrote:
> This series fixes error handling, timeout and data verification in
> test_sockmap. Previously it was not able to detect failure/timeout in
> RX/TX thread because error was not notified to the main thread.
> 
> Also slightly improved test output by printing parameter values (cork,
> apply, start, end) so that parameters for all tests are displayed.
> 
> Prashant Bhole (5):
>   selftests/bpf: test_sockmap, check test failure
>   selftests/bpf: test_sockmap, join cgroup in selftest mode
>   selftests/bpf: test_sockmap, fix test timeout
>   selftests/bpf: test_sockmap, fix data verification
>   selftests/bpf: test_sockmap, print additional test options
> 
>  tools/testing/selftests/bpf/test_sockmap.c | 76 +++++++++++++++++-----
>  1 file changed, 58 insertions(+), 18 deletions(-)
> 

After first patch "check test failure" how do we handle the case
where test is known to cause timeouts because we are specifically testing
these cases. This is the 'cork' parameter we discussed in the last
series. It looks like with this series the test may still throw an
error?

Thanks,
John

Re: [PATCH bpf v2 0/5] fix test_sockmap

[Prashant Bhole]

On 5/30/2018 12:48 AM, John Fastabend wrote:
> On 05/27/2018 09:37 PM, Prashant Bhole wrote:
>> This series fixes error handling, timeout and data verification in
>> test_sockmap. Previously it was not able to detect failure/timeout in
>> RX/TX thread because error was not notified to the main thread.
>>
>> Also slightly improved test output by printing parameter values (cork,
>> apply, start, end) so that parameters for all tests are displayed.
>>
>> Prashant Bhole (5):
>>    selftests/bpf: test_sockmap, check test failure
>>    selftests/bpf: test_sockmap, join cgroup in selftest mode
>>    selftests/bpf: test_sockmap, fix test timeout
>>    selftests/bpf: test_sockmap, fix data verification
>>    selftests/bpf: test_sockmap, print additional test options
>>
>>   tools/testing/selftests/bpf/test_sockmap.c | 76 +++++++++++++++++-----
>>   1 file changed, 58 insertions(+), 18 deletions(-)
>>
> 
> After first patch "check test failure" how do we handle the case
> where test is known to cause timeouts because we are specifically testing
> these cases. This is the 'cork' parameter we discussed in the last
> series. It looks like with this series the test may still throw an
> error?

Sorry. In your comment in last series, did you mean to skip error 
checking only for all cork tests (for now)?

-Prashant

Re: [PATCH bpf v2 0/5] fix test_sockmap

[John Fastabend]

On 05/29/2018 05:44 PM, Prashant Bhole wrote:
> 
> 
> On 5/30/2018 12:48 AM, John Fastabend wrote:
>> On 05/27/2018 09:37 PM, Prashant Bhole wrote:
>>> This series fixes error handling, timeout and data verification in
>>> test_sockmap. Previously it was not able to detect failure/timeout in
>>> RX/TX thread because error was not notified to the main thread.
>>>
>>> Also slightly improved test output by printing parameter values (cork,
>>> apply, start, end) so that parameters for all tests are displayed.
>>>
>>> Prashant Bhole (5):
>>>    selftests/bpf: test_sockmap, check test failure
>>>    selftests/bpf: test_sockmap, join cgroup in selftest mode
>>>    selftests/bpf: test_sockmap, fix test timeout
>>>    selftests/bpf: test_sockmap, fix data verification
>>>    selftests/bpf: test_sockmap, print additional test options
>>>
>>>   tools/testing/selftests/bpf/test_sockmap.c | 76 +++++++++++++++++-----
>>>   1 file changed, 58 insertions(+), 18 deletions(-)
>>>
>>
>> After first patch "check test failure" how do we handle the case
>> where test is known to cause timeouts because we are specifically testing
>> these cases. This is the 'cork' parameter we discussed in the last
>> series. It looks like with this series the test may still throw an
>> error?
> 
> Sorry. In your comment in last series, did you mean to skip error
> checking only for all cork tests (for now)?
> 
> -Prashant
> 

Hi, After this is applied are any errors returned from test_sockmap?
When I read the first patch it looked like timeouts from the cork
tests may result in errors "FAILED" tests. If this is the case then
yes we need skip error checking on all tests or just the corked
tests.

.John

Re: [PATCH bpf v2 0/5] fix test_sockmap

[Prashant Bhole]

On 5/30/2018 2:12 PM, John Fastabend wrote:
> On 05/29/2018 05:44 PM, Prashant Bhole wrote:
>>
>>
>> On 5/30/2018 12:48 AM, John Fastabend wrote:
>>> On 05/27/2018 09:37 PM, Prashant Bhole wrote:
>>>> This series fixes error handling, timeout and data verification in
>>>> test_sockmap. Previously it was not able to detect failure/timeout in
>>>> RX/TX thread because error was not notified to the main thread.
>>>>
>>>> Also slightly improved test output by printing parameter values (cork,
>>>> apply, start, end) so that parameters for all tests are displayed.
>>>>
>>>> Prashant Bhole (5):
>>>>     selftests/bpf: test_sockmap, check test failure
>>>>     selftests/bpf: test_sockmap, join cgroup in selftest mode
>>>>     selftests/bpf: test_sockmap, fix test timeout
>>>>     selftests/bpf: test_sockmap, fix data verification
>>>>     selftests/bpf: test_sockmap, print additional test options
>>>>
>>>>    tools/testing/selftests/bpf/test_sockmap.c | 76 +++++++++++++++++-----
>>>>    1 file changed, 58 insertions(+), 18 deletions(-)
>>>>
>>>
>>> After first patch "check test failure" how do we handle the case
>>> where test is known to cause timeouts because we are specifically testing
>>> these cases. This is the 'cork' parameter we discussed in the last
>>> series. It looks like with this series the test may still throw an
>>> error?
>>
>> Sorry. In your comment in last series, did you mean to skip error
>> checking only for all cork tests (for now)?
>>
>> -Prashant
>>
> 
> Hi, After this is applied are any errors returned from test_sockmap?
> When I read the first patch it looked like timeouts from the cork
> tests may result in errors "FAILED" tests. If this is the case then
> yes we need skip error checking on all tests or just the corked
> tests.

Yes errors returned after applying this series. I will skip error 
checking on just corked tests.

-Prashant