[PATCH bpf-next 0/5] fix test_sockmap

[PATCH bpf-next 0/5] fix test_sockmap

[Prashant Bhole]

This series fixes bugs in test_sockmap code. They weren't caught
previously because failure in RX/TX thread was not notified to the
main thread.

Also fixed data verification logic and slightly improved test output
such that parameters values (cork, apply, start, end) of failed test
can be easily seen.

Note: Even after fixing above problems there are issues with tests
which set cork parameter. Tests fail (RX thread timeout) when cork
value is non-zero and overall data sent by TX thread isn't multiples
of cork value.

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

[PATCH bpf-next 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.14.3

[PATCH bpf-next 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")
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.14.3

[PATCH bpf-next 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.14.3

[PATCH bpf-next 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 verification failure

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

[PATCH bpf-next 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

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

Re: [PATCH bpf-next 0/5] fix test_sockmap

[John Fastabend]

On 05/18/2018 12:17 AM, Prashant Bhole wrote:
> This series fixes bugs in test_sockmap code. They weren't caught
> previously because failure in RX/TX thread was not notified to the
> main thread.
> 
> Also fixed data verification logic and slightly improved test output
> such that parameters values (cork, apply, start, end) of failed test
> can be easily seen.
> 

Great, this was on my list so thanks for taking care of it.

> Note: Even after fixing above problems there are issues with tests
> which set cork parameter. Tests fail (RX thread timeout) when cork
> value is non-zero and overall data sent by TX thread isn't multiples
> of cork value.


This is expected. When 'cork' is set the sender should only xmit
the data when 'cork' bytes are available. If the user doesn't
provide the N bytes the data is cork'ed waiting for the bytes and
if the socket is closed the state is cleaned up. What these tests
are testing is the cleanup path when a user doesn't provide the
N bytes. In practice this is used to validate headers and prevent
users from sending partial headers. We want to keep these tests because
they verify a tear-down path in the code.

After your changes do these get reported as failures? If so we
need to account for the above in the calculations.

> 
> 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(-)
> 

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

[John Fastabend]

On 05/18/2018 12:17 AM, Prashant Bhole wrote:
> 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")
> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> --

Thanks, LGTM. Should this be the first patch in the series though?
I wonder if after patch 1 if you would get failures without this
patch.

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

Re: [PATCH bpf-next 3/5] selftests/bpf: test_sockmap, fix test timeout

[John Fastabend]

On 05/18/2018 12:17 AM, Prashant Bhole wrote:
> 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>
> ---

Whats the runtime for the entire test suite after this? I agree
I was probably to aggressive in setting this but on the other
hand I was trying to avoid letting the test run for minutes.

.John

Re: [PATCH bpf-next 4/5] selftests/bpf: test_sockmap, fix data verification

[John Fastabend]

On 05/18/2018 12:17 AM, Prashant Bhole wrote:
> 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 verification failure
> 
> Fixes: 16962b2404ac ("bpf: sockmap, add selftests")
> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> ---

Thanks!

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

Re: [PATCH bpf-next 5/5] selftests/bpf: test_sockmap, print additional test options

[John Fastabend]

On 05/18/2018 12:17 AM, Prashant Bhole wrote:
> Print values of test options like apply, cork, start, end so that
> individual failed tests can be identified for manual run
> 
> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> ---

Nice improvement.

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

Re: [PATCH bpf-next 0/5] fix test_sockmap

[Shuah Khan]

On 05/18/2018 01:17 AM, Prashant Bhole wrote:
> This series fixes bugs in test_sockmap code. They weren't caught
> previously because failure in RX/TX thread was not notified to the
> main thread.
> 
> Also fixed data verification logic and slightly improved test output
> such that parameters values (cork, apply, start, end) of failed test
> can be easily seen.
> 
> Note: Even after fixing above problems there are issues with tests
> which set cork parameter. Tests fail (RX thread timeout) when cork
> value is non-zero and overall data sent by TX thread isn't multiples
> of cork value.
> 
> 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(-)
> 

Please remember to cc linux-kselftest mailing list as well. I would like to see
all the test patches cc'ed to it. Linaro and other test users watch the kselftest
mailing list. I also have patchwork project now to manage the patch volume.

I am okay with patches going through net/bpf trees - there are always test
dependencies on net/bpf trees.

thanks,
-- Shuah

[PATCH bpf-next 0/5] fix test_sockmap

[shuah]

On 05/18/2018 01:17 AM, Prashant Bhole wrote:
> This series fixes bugs in test_sockmap code. They weren't caught
> previously because failure in RX/TX thread was not notified to the
> main thread.
> 
> Also fixed data verification logic and slightly improved test output
> such that parameters values (cork, apply, start, end) of failed test
> can be easily seen.
> 
> Note: Even after fixing above problems there are issues with tests
> which set cork parameter. Tests fail (RX thread timeout) when cork
> value is non-zero and overall data sent by TX thread isn't multiples
> of cork value.
> 
> 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(-)
> 

Please remember to cc linux-kselftest mailing list as well. I would like to see
all the test patches cc'ed to it. Linaro and other test users watch the kselftest
mailing list. I also have patchwork project now to manage the patch volume.

I am okay with patches going through net/bpf trees - there are always test
dependencies on net/bpf trees.

thanks,
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH bpf-next 0/5] fix test_sockmap

[Shuah Khan]

On 05/18/2018 01:17 AM, Prashant Bhole wrote:
> This series fixes bugs in test_sockmap code. They weren't caught
> previously because failure in RX/TX thread was not notified to the
> main thread.
> 
> Also fixed data verification logic and slightly improved test output
> such that parameters values (cork, apply, start, end) of failed test
> can be easily seen.
> 
> Note: Even after fixing above problems there are issues with tests
> which set cork parameter. Tests fail (RX thread timeout) when cork
> value is non-zero and overall data sent by TX thread isn't multiples
> of cork value.
> 
> 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(-)
> 

Please remember to cc linux-kselftest mailing list as well. I would like to see
all the test patches cc'ed to it. Linaro and other test users watch the kselftest
mailing list. I also have patchwork project now to manage the patch volume.

I am okay with patches going through net/bpf trees - there are always test
dependencies on net/bpf trees.

thanks,
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH bpf-next 0/5] fix test_sockmap

[Daniel Borkmann]

On 05/18/2018 06:54 PM, Shuah Khan wrote:
> On 05/18/2018 01:17 AM, Prashant Bhole wrote:
>> This series fixes bugs in test_sockmap code. They weren't caught
>> previously because failure in RX/TX thread was not notified to the
>> main thread.
>>
>> Also fixed data verification logic and slightly improved test output
>> such that parameters values (cork, apply, start, end) of failed test
>> can be easily seen.
>>
>> Note: Even after fixing above problems there are issues with tests
>> which set cork parameter. Tests fail (RX thread timeout) when cork
>> value is non-zero and overall data sent by TX thread isn't multiples
>> of cork value.
>>
>> 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(-)
> 
> Please remember to cc linux-kselftest mailing list as well. I would like to see
> all the test patches cc'ed to it. Linaro and other test users watch the kselftest
> mailing list. I also have patchwork project now to manage the patch volume.
> 
> I am okay with patches going through net/bpf trees - there are always test
> dependencies on net/bpf trees.

Yep, routing all the BPF selftest patches via bpf/bpf-next tree is the only
viable model that works for us in BPF case, in fact also looks like BPF
selftests are the busiest subdir in #commits so avoiding merge conflicts is
crucial. Whenever appropriate, most fixes or new features are very often
accompanied in a patch set with extensive selftests for BPF, so it has a
deep dependency on the two trees.

Best,
Daniel

[PATCH bpf-next 0/5] fix test_sockmap

[daniel]

On 05/18/2018 06:54 PM, Shuah Khan wrote:
> On 05/18/2018 01:17 AM, Prashant Bhole wrote:
>> This series fixes bugs in test_sockmap code. They weren't caught
>> previously because failure in RX/TX thread was not notified to the
>> main thread.
>>
>> Also fixed data verification logic and slightly improved test output
>> such that parameters values (cork, apply, start, end) of failed test
>> can be easily seen.
>>
>> Note: Even after fixing above problems there are issues with tests
>> which set cork parameter. Tests fail (RX thread timeout) when cork
>> value is non-zero and overall data sent by TX thread isn't multiples
>> of cork value.
>>
>> 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(-)
> 
> Please remember to cc linux-kselftest mailing list as well. I would like to see
> all the test patches cc'ed to it. Linaro and other test users watch the kselftest
> mailing list. I also have patchwork project now to manage the patch volume.
> 
> I am okay with patches going through net/bpf trees - there are always test
> dependencies on net/bpf trees.

Yep, routing all the BPF selftest patches via bpf/bpf-next tree is the only
viable model that works for us in BPF case, in fact also looks like BPF
selftests are the busiest subdir in #commits so avoiding merge conflicts is
crucial. Whenever appropriate, most fixes or new features are very often
accompanied in a patch set with extensive selftests for BPF, so it has a
deep dependency on the two trees.

Best,
Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH bpf-next 0/5] fix test_sockmap

[Daniel Borkmann]

On 05/18/2018 06:54 PM, Shuah Khan wrote:
> On 05/18/2018 01:17 AM, Prashant Bhole wrote:
>> This series fixes bugs in test_sockmap code. They weren't caught
>> previously because failure in RX/TX thread was not notified to the
>> main thread.
>>
>> Also fixed data verification logic and slightly improved test output
>> such that parameters values (cork, apply, start, end) of failed test
>> can be easily seen.
>>
>> Note: Even after fixing above problems there are issues with tests
>> which set cork parameter. Tests fail (RX thread timeout) when cork
>> value is non-zero and overall data sent by TX thread isn't multiples
>> of cork value.
>>
>> 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(-)
> 
> Please remember to cc linux-kselftest mailing list as well. I would like to see
> all the test patches cc'ed to it. Linaro and other test users watch the kselftest
> mailing list. I also have patchwork project now to manage the patch volume.
> 
> I am okay with patches going through net/bpf trees - there are always test
> dependencies on net/bpf trees.

Yep, routing all the BPF selftest patches via bpf/bpf-next tree is the only
viable model that works for us in BPF case, in fact also looks like BPF
selftests are the busiest subdir in #commits so avoiding merge conflicts is
crucial. Whenever appropriate, most fixes or new features are very often
accompanied in a patch set with extensive selftests for BPF, so it has a
deep dependency on the two trees.

Best,
Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH bpf-next 0/5] fix test_sockmap

[Prashant Bhole]

On 5/19/2018 1:42 AM, John Fastabend wrote:
> On 05/18/2018 12:17 AM, Prashant Bhole wrote:
>> This series fixes bugs in test_sockmap code. They weren't caught
>> previously because failure in RX/TX thread was not notified to the
>> main thread.
>>
>> Also fixed data verification logic and slightly improved test output
>> such that parameters values (cork, apply, start, end) of failed test
>> can be easily seen.
>>
> 
> Great, this was on my list so thanks for taking care of it.
> 
>> Note: Even after fixing above problems there are issues with tests
>> which set cork parameter. Tests fail (RX thread timeout) when cork
>> value is non-zero and overall data sent by TX thread isn't multiples
>> of cork value.
> 
> 
> This is expected. When 'cork' is set the sender should only xmit
> the data when 'cork' bytes are available. If the user doesn't
> provide the N bytes the data is cork'ed waiting for the bytes and
> if the socket is closed the state is cleaned up. What these tests
> are testing is the cleanup path when a user doesn't provide the
> N bytes. In practice this is used to validate headers and prevent
> users from sending partial headers. We want to keep these tests because
> they verify a tear-down path in the code.

Ok.

> 
> After your changes do these get reported as failures? If so we
> need to account for the above in the calculations.

Yes, cork related test are reported as failures because of RX thread 
timeout.

So with your above description, I think we need to differentiate cork 
tests with partial data and full data. In partial data test we can have 
something like "timeout_expected" flag. Any other way to fix it?

-Prashant
> 
>>
>> 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(-)
>>
> 
> 
> 

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

[Prashant Bhole]

On 5/19/2018 1:45 AM, John Fastabend wrote:
> On 05/18/2018 12:17 AM, Prashant Bhole wrote:
>> 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")
>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
>> --
> 
> Thanks, LGTM. Should this be the first patch in the series though?
> I wonder if after patch 1 if you would get failures without this
> patch.

Patch 1 fixes selftest mode as well as manual mode. This patch 2 is 
specifically for selftest mode, hence the sequence.

- Prashant

Re: [PATCH bpf-next 3/5] selftests/bpf: test_sockmap, fix test timeout

[Prashant Bhole]

On 5/19/2018 1:47 AM, John Fastabend wrote:
> On 05/18/2018 12:17 AM, Prashant Bhole wrote:
>> 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>
>> ---
> 
> Whats the runtime for the entire test suite after this? I agree
> I was probably to aggressive in setting this but on the other
> hand I was trying to avoid letting the test run for minutes.
> 

Currently any failure stops further tests.
So I made local change to force cork=0. With this change, it takes 
around 60 seconds for all 648 tests. It will change after fixing cork tests.

-Prashant

Re: [PATCH bpf-next 0/5] fix test_sockmap

[Prashant Bhole]

On 5/19/2018 1:54 AM, Shuah Khan wrote:
> On 05/18/2018 01:17 AM, Prashant Bhole wrote:
>> This series fixes bugs in test_sockmap code. They weren't caught
>> previously because failure in RX/TX thread was not notified to the
>> main thread.
>>
>> Also fixed data verification logic and slightly improved test output
>> such that parameters values (cork, apply, start, end) of failed test
>> can be easily seen.
>>
>> Note: Even after fixing above problems there are issues with tests
>> which set cork parameter. Tests fail (RX thread timeout) when cork
>> value is non-zero and overall data sent by TX thread isn't multiples
>> of cork value.
>>
>> 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(-)
>>
> 
> Please remember to cc linux-kselftest mailing list as well. I would like to see
> all the test patches cc'ed to it. Linaro and other test users watch the kselftest
> mailing list. I also have patchwork project now to manage the patch volume. >
> I am okay with patches going through net/bpf trees - there are always test
> dependencies on net/bpf trees.
> 
Ok. I will remember this. Thanks.

-Prashant

[PATCH bpf-next 0/5] fix test_sockmap

[bhole_prashant_q7]

On 5/19/2018 1:54 AM, Shuah Khan wrote:
> On 05/18/2018 01:17 AM, Prashant Bhole wrote:
>> This series fixes bugs in test_sockmap code. They weren't caught
>> previously because failure in RX/TX thread was not notified to the
>> main thread.
>>
>> Also fixed data verification logic and slightly improved test output
>> such that parameters values (cork, apply, start, end) of failed test
>> can be easily seen.
>>
>> Note: Even after fixing above problems there are issues with tests
>> which set cork parameter. Tests fail (RX thread timeout) when cork
>> value is non-zero and overall data sent by TX thread isn't multiples
>> of cork value.
>>
>> 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(-)
>>
> 
> Please remember to cc linux-kselftest mailing list as well. I would like to see
> all the test patches cc'ed to it. Linaro and other test users watch the kselftest
> mailing list. I also have patchwork project now to manage the patch volume. >
> I am okay with patches going through net/bpf trees - there are always test
> dependencies on net/bpf trees.
> 
Ok. I will remember this. Thanks.

-Prashant


--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH bpf-next 0/5] fix test_sockmap

[Prashant Bhole]

On 5/19/2018 1:54 AM, Shuah Khan wrote:
> On 05/18/2018 01:17 AM, Prashant Bhole wrote:
>> This series fixes bugs in test_sockmap code. They weren't caught
>> previously because failure in RX/TX thread was not notified to the
>> main thread.
>>
>> Also fixed data verification logic and slightly improved test output
>> such that parameters values (cork, apply, start, end) of failed test
>> can be easily seen.
>>
>> Note: Even after fixing above problems there are issues with tests
>> which set cork parameter. Tests fail (RX thread timeout) when cork
>> value is non-zero and overall data sent by TX thread isn't multiples
>> of cork value.
>>
>> 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(-)
>>
> 
> Please remember to cc linux-kselftest mailing list as well. I would like to see
> all the test patches cc'ed to it. Linaro and other test users watch the kselftest
> mailing list. I also have patchwork project now to manage the patch volume. >
> I am okay with patches going through net/bpf trees - there are always test
> dependencies on net/bpf trees.
> 
Ok. I will remember this. Thanks.

-Prashant


--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH bpf-next 0/5] fix test_sockmap

[John Fastabend]

On 05/20/2018 10:13 PM, Prashant Bhole wrote:
> 
> 
> On 5/19/2018 1:42 AM, John Fastabend wrote:
>> On 05/18/2018 12:17 AM, Prashant Bhole wrote:
>>> This series fixes bugs in test_sockmap code. They weren't caught
>>> previously because failure in RX/TX thread was not notified to the
>>> main thread.
>>>
>>> Also fixed data verification logic and slightly improved test output
>>> such that parameters values (cork, apply, start, end) of failed test
>>> can be easily seen.
>>>
>>
>> Great, this was on my list so thanks for taking care of it.
>>
>>> Note: Even after fixing above problems there are issues with tests
>>> which set cork parameter. Tests fail (RX thread timeout) when cork
>>> value is non-zero and overall data sent by TX thread isn't multiples
>>> of cork value.
>>
>>
>> This is expected. When 'cork' is set the sender should only xmit
>> the data when 'cork' bytes are available. If the user doesn't
>> provide the N bytes the data is cork'ed waiting for the bytes and
>> if the socket is closed the state is cleaned up. What these tests
>> are testing is the cleanup path when a user doesn't provide the
>> N bytes. In practice this is used to validate headers and prevent
>> users from sending partial headers. We want to keep these tests because
>> they verify a tear-down path in the code.
> 
> Ok.
> 
>>
>> After your changes do these get reported as failures? If so we
>> need to account for the above in the calculations.
> 
> Yes, cork related test are reported as failures because of RX thread
> timeout.
> 
> So with your above description, I think we need to differentiate cork
> tests with partial data and full data. In partial data test we can have
> something like "timeout_expected" flag. Any other way to fix it?
> 

Adding a flag seems reasonable to me. Lets do this for now. Also I
plan to add more negative tests so we can either use the same
flag or a new one for those cases as well.

Thanks,
John

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

[John Fastabend]

On 05/20/2018 10:15 PM, Prashant Bhole wrote:
> 
> 
> On 5/19/2018 1:45 AM, John Fastabend wrote:
>> On 05/18/2018 12:17 AM, Prashant Bhole wrote:
>>> 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")
>>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
>>> -- 
>>
>> Thanks, LGTM. Should this be the first patch in the series though?
>> I wonder if after patch 1 if you would get failures without this
>> patch.
> 
> Patch 1 fixes selftest mode as well as manual mode. This patch 2 is
> specifically for selftest mode, hence the sequence.
> 
> - Prashant
> 
> 

OK.

Re: [PATCH bpf-next 3/5] selftests/bpf: test_sockmap, fix test timeout

[John Fastabend]

On 05/20/2018 10:15 PM, Prashant Bhole wrote:
> 
> 
> On 5/19/2018 1:47 AM, John Fastabend wrote:
>> On 05/18/2018 12:17 AM, Prashant Bhole wrote:
>>> 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>
>>> ---
>>
>> Whats the runtime for the entire test suite after this? I agree
>> I was probably to aggressive in setting this but on the other
>> hand I was trying to avoid letting the test run for minutes.
>>
> 
> Currently any failure stops further tests.
> So I made local change to force cork=0. With this change, it takes
> around 60 seconds for all 648 tests. It will change after fixing cork
> tests.
> 
> -Prashant
> 


OK, lets see how long it takes with the cork tests fixed. Maybe if
the cork flag is set we should lower the timeout to 100us.

.John

Re: [PATCH bpf-next 0/5] fix test_sockmap

[Prashant Bhole]

On 5/22/2018 2:08 AM, John Fastabend wrote:
> On 05/20/2018 10:13 PM, Prashant Bhole wrote:
>>
>>
>> On 5/19/2018 1:42 AM, John Fastabend wrote:
>>> On 05/18/2018 12:17 AM, Prashant Bhole wrote:
>>>> This series fixes bugs in test_sockmap code. They weren't caught
>>>> previously because failure in RX/TX thread was not notified to the
>>>> main thread.
>>>>
>>>> Also fixed data verification logic and slightly improved test output
>>>> such that parameters values (cork, apply, start, end) of failed test
>>>> can be easily seen.
>>>>
>>>
>>> Great, this was on my list so thanks for taking care of it.
>>>
>>>> Note: Even after fixing above problems there are issues with tests
>>>> which set cork parameter. Tests fail (RX thread timeout) when cork
>>>> value is non-zero and overall data sent by TX thread isn't multiples
>>>> of cork value.
>>>
>>>
>>> This is expected. When 'cork' is set the sender should only xmit
>>> the data when 'cork' bytes are available. If the user doesn't
>>> provide the N bytes the data is cork'ed waiting for the bytes and
>>> if the socket is closed the state is cleaned up. What these tests
>>> are testing is the cleanup path when a user doesn't provide the
>>> N bytes. In practice this is used to validate headers and prevent
>>> users from sending partial headers. We want to keep these tests because
>>> they verify a tear-down path in the code.
>>
>> Ok.
>>
>>>
>>> After your changes do these get reported as failures? If so we
>>> need to account for the above in the calculations.
>>
>> Yes, cork related test are reported as failures because of RX thread
>> timeout.
>>
>> So with your above description, I think we need to differentiate cork
>> tests with partial data and full data. In partial data test we can have
>> something like "timeout_expected" flag. Any other way to fix it?
>>
> 
> Adding a flag seems reasonable to me. Lets do this for now. Also I
> plan to add more negative tests so we can either use the same
> flag or a new one for those cases as well.
> 

John,
I worked on this for some time and noticed that the RX-timeout of tests 
with cork parameter is dependent on various parameters. So we can not 
set a flag like the way 'drop_expected' flag is set before executing the 
test.

So I decided to write a function which judges all parameters before each 
test and decides whether a test with cork parameter will timeout or not. 
Then the conditions in the function became complicated. For example some 
tests fail if opt->rate < 17 (with some other conditions). Here is 17 is 
related to FRAGS_PER_SKB. Consider following two examples.

./test_sockmap --cgroup /mnt/cgroup2 -r 16 -i 1 -l 30 -t sendpage 
--txmsg --txmsg_cork 1024   # RX timeout occurs

./test_sockmap --cgroup /mnt/cgroup2 -r 17 -i 1 -l 30 -t sendpage 
--txmsg --txmsg_cork 1024   # Success!

Do we need to keep such tests? if yes, then I will continue with adding 
such conditions in the function.


-Prashant

Re: [PATCH bpf-next 0/5] fix test_sockmap

[Prashant Bhole]

On 5/23/2018 6:44 PM, Prashant Bhole wrote:
> 
> 
> On 5/22/2018 2:08 AM, John Fastabend wrote:
>> On 05/20/2018 10:13 PM, Prashant Bhole wrote:
>>>
>>>
>>> On 5/19/2018 1:42 AM, John Fastabend wrote:
>>>> On 05/18/2018 12:17 AM, Prashant Bhole wrote:
>>>>> This series fixes bugs in test_sockmap code. They weren't caught
>>>>> previously because failure in RX/TX thread was not notified to the
>>>>> main thread.
>>>>>
>>>>> Also fixed data verification logic and slightly improved test output
>>>>> such that parameters values (cork, apply, start, end) of failed test
>>>>> can be easily seen.
>>>>>
>>>>
>>>> Great, this was on my list so thanks for taking care of it.
>>>>
>>>>> Note: Even after fixing above problems there are issues with tests
>>>>> which set cork parameter. Tests fail (RX thread timeout) when cork
>>>>> value is non-zero and overall data sent by TX thread isn't multiples
>>>>> of cork value.
>>>>
>>>>
>>>> This is expected. When 'cork' is set the sender should only xmit
>>>> the data when 'cork' bytes are available. If the user doesn't
>>>> provide the N bytes the data is cork'ed waiting for the bytes and
>>>> if the socket is closed the state is cleaned up. What these tests
>>>> are testing is the cleanup path when a user doesn't provide the
>>>> N bytes. In practice this is used to validate headers and prevent
>>>> users from sending partial headers. We want to keep these tests because
>>>> they verify a tear-down path in the code.
>>>
>>> Ok.
>>>
>>>>
>>>> After your changes do these get reported as failures? If so we
>>>> need to account for the above in the calculations.
>>>
>>> Yes, cork related test are reported as failures because of RX thread
>>> timeout.
>>>
>>> So with your above description, I think we need to differentiate cork
>>> tests with partial data and full data. In partial data test we can have
>>> something like "timeout_expected" flag. Any other way to fix it?
>>>
>>
>> Adding a flag seems reasonable to me. Lets do this for now. Also I
>> plan to add more negative tests so we can either use the same
>> flag or a new one for those cases as well.
>>
> 
> John,
> I worked on this for some time and noticed that the RX-timeout of tests 
> with cork parameter is dependent on various parameters. So we can not 
> set a flag like the way 'drop_expected' flag is set before executing the 
> test.
> 
> So I decided to write a function which judges all parameters before each 
> test and decides whether a test with cork parameter will timeout or not. 
> Then the conditions in the function became complicated. For example some 
> tests fail if opt->rate < 17 (with some other conditions). Here is 17 is 
> related to FRAGS_PER_SKB. Consider following two examples.

I'm sorry. Correction: s/FRAGS_PER_SKB/MAX_SKB_FRAGS/

> 
> ./test_sockmap --cgroup /mnt/cgroup2 -r 16 -i 1 -l 30 -t sendpage 
> --txmsg --txmsg_cork 1024   # RX timeout occurs
> 
> ./test_sockmap --cgroup /mnt/cgroup2 -r 17 -i 1 -l 30 -t sendpage 
> --txmsg --txmsg_cork 1024   # Success!
> 
> Do we need to keep such tests? if yes, then I will continue with adding 
> such conditions in the function.
> 
> 
> -Prashant
> 
> 
> 

Re: [PATCH bpf-next 0/5] fix test_sockmap

[John Fastabend]

On 05/23/2018 09:47 PM, Prashant Bhole wrote:
> 
> 
> On 5/23/2018 6:44 PM, Prashant Bhole wrote:
>>
>>
>> On 5/22/2018 2:08 AM, John Fastabend wrote:
>>> On 05/20/2018 10:13 PM, Prashant Bhole wrote:
>>>>
>>>>
>>>> On 5/19/2018 1:42 AM, John Fastabend wrote:
>>>>> On 05/18/2018 12:17 AM, Prashant Bhole wrote:
>>>>>> This series fixes bugs in test_sockmap code. They weren't caught
>>>>>> previously because failure in RX/TX thread was not notified to the
>>>>>> main thread.
>>>>>>
>>>>>> Also fixed data verification logic and slightly improved test output
>>>>>> such that parameters values (cork, apply, start, end) of failed test
>>>>>> can be easily seen.
>>>>>>
>>>>>
>>>>> Great, this was on my list so thanks for taking care of it.
>>>>>
>>>>>> Note: Even after fixing above problems there are issues with tests
>>>>>> which set cork parameter. Tests fail (RX thread timeout) when cork
>>>>>> value is non-zero and overall data sent by TX thread isn't multiples
>>>>>> of cork value.
>>>>>
>>>>>
>>>>> This is expected. When 'cork' is set the sender should only xmit
>>>>> the data when 'cork' bytes are available. If the user doesn't
>>>>> provide the N bytes the data is cork'ed waiting for the bytes and
>>>>> if the socket is closed the state is cleaned up. What these tests
>>>>> are testing is the cleanup path when a user doesn't provide the
>>>>> N bytes. In practice this is used to validate headers and prevent
>>>>> users from sending partial headers. We want to keep these tests because
>>>>> they verify a tear-down path in the code.
>>>>
>>>> Ok.
>>>>
>>>>>
>>>>> After your changes do these get reported as failures? If so we
>>>>> need to account for the above in the calculations.
>>>>
>>>> Yes, cork related test are reported as failures because of RX thread
>>>> timeout.
>>>>
>>>> So with your above description, I think we need to differentiate cork
>>>> tests with partial data and full data. In partial data test we can have
>>>> something like "timeout_expected" flag. Any other way to fix it?
>>>>
>>>
>>> Adding a flag seems reasonable to me. Lets do this for now. Also I
>>> plan to add more negative tests so we can either use the same
>>> flag or a new one for those cases as well.
>>>
>>
>> John,
>> I worked on this for some time and noticed that the RX-timeout of
>> tests with cork parameter is dependent on various parameters. So we
>> can not set a flag like the way 'drop_expected' flag is set before
>> executing the test.
>> 
>> So I decided to write a function which judges all parameters before
>> each test and decides whether a test with cork parameter will
>> timeout or not. Then the conditions in the function became
>> complicated. For example some tests fail if opt->rate < 17 (with
>> some other conditions). Here is 17 is related to FRAGS_PER_SKB.
>> Consider following two examples.
> I'm sorry. Correction: s/FRAGS_PER_SKB/MAX_SKB_FRAGS/
> 
>>
>> ./test_sockmap --cgroup /mnt/cgroup2 -r 16 -i 1 -l 30 -t sendpage
>> --txmsg --txmsg_cork 1024   # RX timeout occurs
>> 
>> ./test_sockmap --cgroup /mnt/cgroup2 -r 17 -i 1 -l 30 -t sendpage
>> --txmsg --txmsg_cork 1024   # Success!
>> 

Ah yes this hits the buffer limit and flushes the queue. The kernel
side doesn't know how to merge those specific sendpage requests so
it gives each request its own buffer and when the limit is reached
we flush it.

>> Do we need to keep such tests? if yes, then I will continue with
>> adding such conditions in the function.
>> 

Yes, these tests are needed because they are testing the edge cases.
These are probably the most important tests because my normal usage
will catch any issues in the "good" cases its these types of things
that can go unnoticed (at least for a short while) if we don't have
specific tests for them.

Thanks for doing this.
John

>> -Prashant
>>
>>
>>
> 

Re: [PATCH bpf-next 0/5] fix test_sockmap

[Prashant Bhole]

On 5/24/2018 1:58 PM, John Fastabend wrote:
> On 05/23/2018 09:47 PM, Prashant Bhole wrote:
>>
>>
>> On 5/23/2018 6:44 PM, Prashant Bhole wrote:
>>>
>>>
>>> On 5/22/2018 2:08 AM, John Fastabend wrote:
>>>> On 05/20/2018 10:13 PM, Prashant Bhole wrote:
>>>>>
>>>>>
>>>>> On 5/19/2018 1:42 AM, John Fastabend wrote:
>>>>>> On 05/18/2018 12:17 AM, Prashant Bhole wrote:
>>>>>>> This series fixes bugs in test_sockmap code. They weren't caught
>>>>>>> previously because failure in RX/TX thread was not notified to the
>>>>>>> main thread.
>>>>>>>
>>>>>>> Also fixed data verification logic and slightly improved test output
>>>>>>> such that parameters values (cork, apply, start, end) of failed test
>>>>>>> can be easily seen.
>>>>>>>
>>>>>>
>>>>>> Great, this was on my list so thanks for taking care of it.
>>>>>>
>>>>>>> Note: Even after fixing above problems there are issues with tests
>>>>>>> which set cork parameter. Tests fail (RX thread timeout) when cork
>>>>>>> value is non-zero and overall data sent by TX thread isn't multiples
>>>>>>> of cork value.
>>>>>>
>>>>>>
>>>>>> This is expected. When 'cork' is set the sender should only xmit
>>>>>> the data when 'cork' bytes are available. If the user doesn't
>>>>>> provide the N bytes the data is cork'ed waiting for the bytes and
>>>>>> if the socket is closed the state is cleaned up. What these tests
>>>>>> are testing is the cleanup path when a user doesn't provide the
>>>>>> N bytes. In practice this is used to validate headers and prevent
>>>>>> users from sending partial headers. We want to keep these tests because
>>>>>> they verify a tear-down path in the code.
>>>>>
>>>>> Ok.
>>>>>
>>>>>>
>>>>>> After your changes do these get reported as failures? If so we
>>>>>> need to account for the above in the calculations.
>>>>>
>>>>> Yes, cork related test are reported as failures because of RX thread
>>>>> timeout.
>>>>>
>>>>> So with your above description, I think we need to differentiate cork
>>>>> tests with partial data and full data. In partial data test we can have
>>>>> something like "timeout_expected" flag. Any other way to fix it?
>>>>>
>>>>
>>>> Adding a flag seems reasonable to me. Lets do this for now. Also I
>>>> plan to add more negative tests so we can either use the same
>>>> flag or a new one for those cases as well.
>>>>
>>>
>>> John,
>>> I worked on this for some time and noticed that the RX-timeout of
>>> tests with cork parameter is dependent on various parameters. So we
>>> can not set a flag like the way 'drop_expected' flag is set before
>>> executing the test.
>>>
>>> So I decided to write a function which judges all parameters before
>>> each test and decides whether a test with cork parameter will
>>> timeout or not. Then the conditions in the function became
>>> complicated. For example some tests fail if opt->rate < 17 (with
>>> some other conditions). Here is 17 is related to FRAGS_PER_SKB.
>>> Consider following two examples.
>> I'm sorry. Correction: s/FRAGS_PER_SKB/MAX_SKB_FRAGS/
>>
>>>
>>> ./test_sockmap --cgroup /mnt/cgroup2 -r 16 -i 1 -l 30 -t sendpage
>>> --txmsg --txmsg_cork 1024   # RX timeout occurs
>>>
>>> ./test_sockmap --cgroup /mnt/cgroup2 -r 17 -i 1 -l 30 -t sendpage
>>> --txmsg --txmsg_cork 1024   # Success!
>>>
> 
> Ah yes this hits the buffer limit and flushes the queue. The kernel
> side doesn't know how to merge those specific sendpage requests so
> it gives each request its own buffer and when the limit is reached
> we flush it.
> 
>>> Do we need to keep such tests? if yes, then I will continue with
>>> adding such conditions in the function.
>>>
> 
> Yes, these tests are needed because they are testing the edge cases.
> These are probably the most important tests because my normal usage
> will catch any issues in the "good" cases its these types of things
> that can go unnoticed (at least for a short while) if we don't have
> specific tests for them.

I tried but it is difficult to come up with a right set of conditions 
which lead to test failure.

-Prashant
> 
> Thanks for doing this.
> John

Re: [PATCH bpf-next 0/5] fix test_sockmap

[John Fastabend]

On 05/25/2018 01:28 AM, Prashant Bhole wrote:
> 
> 
> On 5/24/2018 1:58 PM, John Fastabend wrote:
>> On 05/23/2018 09:47 PM, Prashant Bhole wrote:
>>>
>>>
>>> On 5/23/2018 6:44 PM, Prashant Bhole wrote:
>>>>
>>>>
>>>> On 5/22/2018 2:08 AM, John Fastabend wrote:
>>>>> On 05/20/2018 10:13 PM, Prashant Bhole wrote:
>>>>>>
>>>>>>
>>>>>> On 5/19/2018 1:42 AM, John Fastabend wrote:
>>>>>>> On 05/18/2018 12:17 AM, Prashant Bhole wrote:
>>>>>>>> This series fixes bugs in test_sockmap code. They weren't caught
>>>>>>>> previously because failure in RX/TX thread was not notified to the
>>>>>>>> main thread.
>>>>>>>>
>>>>>>>> Also fixed data verification logic and slightly improved test
>>>>>>>> output
>>>>>>>> such that parameters values (cork, apply, start, end) of failed
>>>>>>>> test
>>>>>>>> can be easily seen.
>>>>>>>>
>>>>>>>
>>>>>>> Great, this was on my list so thanks for taking care of it.
>>>>>>>
>>>>>>>> Note: Even after fixing above problems there are issues with tests
>>>>>>>> which set cork parameter. Tests fail (RX thread timeout) when cork
>>>>>>>> value is non-zero and overall data sent by TX thread isn't
>>>>>>>> multiples
>>>>>>>> of cork value.
>>>>>>>
>>>>>>>
>>>>>>> This is expected. When 'cork' is set the sender should only xmit
>>>>>>> the data when 'cork' bytes are available. If the user doesn't
>>>>>>> provide the N bytes the data is cork'ed waiting for the bytes and
>>>>>>> if the socket is closed the state is cleaned up. What these tests
>>>>>>> are testing is the cleanup path when a user doesn't provide the
>>>>>>> N bytes. In practice this is used to validate headers and prevent
>>>>>>> users from sending partial headers. We want to keep these tests
>>>>>>> because
>>>>>>> they verify a tear-down path in the code.
>>>>>>
>>>>>> Ok.
>>>>>>
>>>>>>>
>>>>>>> After your changes do these get reported as failures? If so we
>>>>>>> need to account for the above in the calculations.
>>>>>>
>>>>>> Yes, cork related test are reported as failures because of RX thread
>>>>>> timeout.
>>>>>>
>>>>>> So with your above description, I think we need to differentiate cork
>>>>>> tests with partial data and full data. In partial data test we can
>>>>>> have
>>>>>> something like "timeout_expected" flag. Any other way to fix it?
>>>>>>
>>>>>
>>>>> Adding a flag seems reasonable to me. Lets do this for now. Also I
>>>>> plan to add more negative tests so we can either use the same
>>>>> flag or a new one for those cases as well.
>>>>>
>>>>
>>>> John,
>>>> I worked on this for some time and noticed that the RX-timeout of
>>>> tests with cork parameter is dependent on various parameters. So we
>>>> can not set a flag like the way 'drop_expected' flag is set before
>>>> executing the test.
>>>>
>>>> So I decided to write a function which judges all parameters before
>>>> each test and decides whether a test with cork parameter will
>>>> timeout or not. Then the conditions in the function became
>>>> complicated. For example some tests fail if opt->rate < 17 (with
>>>> some other conditions). Here is 17 is related to FRAGS_PER_SKB.
>>>> Consider following two examples.
>>> I'm sorry. Correction: s/FRAGS_PER_SKB/MAX_SKB_FRAGS/
>>>
>>>>
>>>> ./test_sockmap --cgroup /mnt/cgroup2 -r 16 -i 1 -l 30 -t sendpage
>>>> --txmsg --txmsg_cork 1024   # RX timeout occurs
>>>>
>>>> ./test_sockmap --cgroup /mnt/cgroup2 -r 17 -i 1 -l 30 -t sendpage
>>>> --txmsg --txmsg_cork 1024   # Success!
>>>>
>>
>> Ah yes this hits the buffer limit and flushes the queue. The kernel
>> side doesn't know how to merge those specific sendpage requests so
>> it gives each request its own buffer and when the limit is reached
>> we flush it.
>>
>>>> Do we need to keep such tests? if yes, then I will continue with
>>>> adding such conditions in the function.
>>>>
>>
>> Yes, these tests are needed because they are testing the edge cases.
>> These are probably the most important tests because my normal usage
>> will catch any issues in the "good" cases its these types of things
>> that can go unnoticed (at least for a short while) if we don't have
>> specific tests for them.
> 
> I tried but it is difficult to come up with a right set of conditions
> which lead to test failure.
> 

Agreed, it can be yes. How about adding your logic for all tests except
"cork" cases. If there is a flag to set if the timeout is expected we
can always manually set it in the test invocation. Might not be as
nice as automatically learning the expected results but possibly easier
than building some complicated logic to figure it out.

Would you mind submitting your series again without the "cork" tests
being tracked? And if you want add a bit to tell if the "cork" tests are
going to timeout or not setting it per test manually. But I think
your series can just omit the cork test for now and still be useful.

> -Prashant
>>
>> Thanks for doing this.
>> John
> 
> 

Re: [PATCH bpf-next 0/5] fix test_sockmap

[Prashant Bhole]

On 5/25/2018 11:01 PM, John Fastabend wrote:
> On 05/25/2018 01:28 AM, Prashant Bhole wrote:
>>
>>
>> On 5/24/2018 1:58 PM, John Fastabend wrote:
>>> On 05/23/2018 09:47 PM, Prashant Bhole wrote:
>>>>
>>>>
>>>> On 5/23/2018 6:44 PM, Prashant Bhole wrote:
>>>>>
>>>>>
>>>>> On 5/22/2018 2:08 AM, John Fastabend wrote:
>>>>>> On 05/20/2018 10:13 PM, Prashant Bhole wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 5/19/2018 1:42 AM, John Fastabend wrote:
>>>>>>>> On 05/18/2018 12:17 AM, Prashant Bhole wrote:
>>>>>>>>> This series fixes bugs in test_sockmap code. They weren't caught
>>>>>>>>> previously because failure in RX/TX thread was not notified to the
>>>>>>>>> main thread.
>>>>>>>>>
>>>>>>>>> Also fixed data verification logic and slightly improved test
>>>>>>>>> output
>>>>>>>>> such that parameters values (cork, apply, start, end) of failed
>>>>>>>>> test
>>>>>>>>> can be easily seen.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Great, this was on my list so thanks for taking care of it.
>>>>>>>>
>>>>>>>>> Note: Even after fixing above problems there are issues with tests
>>>>>>>>> which set cork parameter. Tests fail (RX thread timeout) when cork
>>>>>>>>> value is non-zero and overall data sent by TX thread isn't
>>>>>>>>> multiples
>>>>>>>>> of cork value.
>>>>>>>>
>>>>>>>>
>>>>>>>> This is expected. When 'cork' is set the sender should only xmit
>>>>>>>> the data when 'cork' bytes are available. If the user doesn't
>>>>>>>> provide the N bytes the data is cork'ed waiting for the bytes and
>>>>>>>> if the socket is closed the state is cleaned up. What these tests
>>>>>>>> are testing is the cleanup path when a user doesn't provide the
>>>>>>>> N bytes. In practice this is used to validate headers and prevent
>>>>>>>> users from sending partial headers. We want to keep these tests
>>>>>>>> because
>>>>>>>> they verify a tear-down path in the code.
>>>>>>>
>>>>>>> Ok.
>>>>>>>
>>>>>>>>
>>>>>>>> After your changes do these get reported as failures? If so we
>>>>>>>> need to account for the above in the calculations.
>>>>>>>
>>>>>>> Yes, cork related test are reported as failures because of RX thread
>>>>>>> timeout.
>>>>>>>
>>>>>>> So with your above description, I think we need to differentiate cork
>>>>>>> tests with partial data and full data. In partial data test we can
>>>>>>> have
>>>>>>> something like "timeout_expected" flag. Any other way to fix it?
>>>>>>>
>>>>>>
>>>>>> Adding a flag seems reasonable to me. Lets do this for now. Also I
>>>>>> plan to add more negative tests so we can either use the same
>>>>>> flag or a new one for those cases as well.
>>>>>>
>>>>>
>>>>> John,
>>>>> I worked on this for some time and noticed that the RX-timeout of
>>>>> tests with cork parameter is dependent on various parameters. So we
>>>>> can not set a flag like the way 'drop_expected' flag is set before
>>>>> executing the test.
>>>>>
>>>>> So I decided to write a function which judges all parameters before
>>>>> each test and decides whether a test with cork parameter will
>>>>> timeout or not. Then the conditions in the function became
>>>>> complicated. For example some tests fail if opt->rate < 17 (with
>>>>> some other conditions). Here is 17 is related to FRAGS_PER_SKB.
>>>>> Consider following two examples.
>>>> I'm sorry. Correction: s/FRAGS_PER_SKB/MAX_SKB_FRAGS/
>>>>
>>>>>
>>>>> ./test_sockmap --cgroup /mnt/cgroup2 -r 16 -i 1 -l 30 -t sendpage
>>>>> --txmsg --txmsg_cork 1024   # RX timeout occurs
>>>>>
>>>>> ./test_sockmap --cgroup /mnt/cgroup2 -r 17 -i 1 -l 30 -t sendpage
>>>>> --txmsg --txmsg_cork 1024   # Success!
>>>>>
>>>
>>> Ah yes this hits the buffer limit and flushes the queue. The kernel
>>> side doesn't know how to merge those specific sendpage requests so
>>> it gives each request its own buffer and when the limit is reached
>>> we flush it.
>>>
>>>>> Do we need to keep such tests? if yes, then I will continue with
>>>>> adding such conditions in the function.
>>>>>
>>>
>>> Yes, these tests are needed because they are testing the edge cases.
>>> These are probably the most important tests because my normal usage
>>> will catch any issues in the "good" cases its these types of things
>>> that can go unnoticed (at least for a short while) if we don't have
>>> specific tests for them.
>>
>> I tried but it is difficult to come up with a right set of conditions
>> which lead to test failure.
>>
> 
> Agreed, it can be yes. How about adding your logic for all tests except
> "cork" cases. If there is a flag to set if the timeout is expected we
> can always manually set it in the test invocation. Might not be as
> nice as automatically learning the expected results but possibly easier
> than building some complicated logic to figure it out.
> 
> Would you mind submitting your series again without the "cork" tests
> being tracked? And if you want add a bit to tell if the "cork" tests are
> going to timeout or not setting it per test manually. But I think
> your series can just omit the cork test for now and still be useful.

Ok. I will submit the series again. Without any change in actual 
patches, but cover letter reorganized. Thanks.

-Prashant