[PATCH bpf-next] selftests: xsk: Update poll test cases

[PATCH bpf-next] selftests: xsk: Update poll test cases

[Shibin Koikkara Reeny]

Poll test case was not testing all the functionality
of the poll feature in the testsuite. This patch
update the poll test case with 2 more testcases to
check the timeout features.

Poll test case have 4 sub test cases:

1. TEST_TYPE_RX_POLL:
Check if POLLIN function work as expect.

2. TEST_TYPE_TX_POLL:
Check if POLLOUT function work as expect.

3. TEST_TYPE_POLL_RXQ_EMPTY:
call poll function with parameter POLLIN on empty rx queue
will cause timeout.If return timeout then test case is pass.

4. TEST_TYPE_POLL_TXQ_FULL:
When txq is filled and packets are not cleaned by the
kernel then if we invoke the poll function with POLLOUT
then it should trigger timeout.If return timeout then
test case is pass.

Signed-off-by: Shibin Koikkara Reeny <shibin.koikkara.reeny@intel.com>
---
 tools/testing/selftests/bpf/xskxceiver.c | 173 +++++++++++++++++------
 tools/testing/selftests/bpf/xskxceiver.h |  10 +-
 2 files changed, 139 insertions(+), 44 deletions(-)

diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
index 74d56d971baf..8ecab3a47c9e 100644
--- a/tools/testing/selftests/bpf/xskxceiver.c
+++ b/tools/testing/selftests/bpf/xskxceiver.c
@@ -424,6 +424,8 @@ static void __test_spec_init(struct test_spec *test, struct ifobject *ifobj_tx,
 
 		ifobj->xsk = &ifobj->xsk_arr[0];
 		ifobj->use_poll = false;
+		ifobj->skip_rx = false;
+		ifobj->skip_tx = false;
 		ifobj->use_fill_ring = true;
 		ifobj->release_rx = true;
 		ifobj->pkt_stream = test->pkt_stream_default;
@@ -589,6 +591,19 @@ static struct pkt_stream *pkt_stream_clone(struct xsk_umem_info *umem,
 	return pkt_stream_generate(umem, pkt_stream->nb_pkts, pkt_stream->pkts[0].len);
 }
 
+static void pkt_stream_invalid(struct test_spec *test, u32 nb_pkts, u32 pkt_len)
+{
+	struct pkt_stream *pkt_stream;
+	u32 i;
+
+	pkt_stream = pkt_stream_generate(test->ifobj_tx->umem, nb_pkts, pkt_len);
+	for (i = 0; i < nb_pkts; i++)
+		pkt_stream->pkts[i].valid = false;
+
+	test->ifobj_tx->pkt_stream = pkt_stream;
+	test->ifobj_rx->pkt_stream = pkt_stream;
+}
+
 static void pkt_stream_replace(struct test_spec *test, u32 nb_pkts, u32 pkt_len)
 {
 	struct pkt_stream *pkt_stream;
@@ -817,9 +832,9 @@ static int complete_pkts(struct xsk_socket_info *xsk, int batch_size)
 	return TEST_PASS;
 }
 
-static int receive_pkts(struct ifobject *ifobj, struct pollfd *fds)
+static int receive_pkts(struct ifobject *ifobj, struct pollfd *fds, bool skip_tx)
 {
-	struct timeval tv_end, tv_now, tv_timeout = {RECV_TMOUT, 0};
+	struct timeval tv_end, tv_now, tv_timeout = {THREAD_TMOUT, 0};
 	u32 idx_rx = 0, idx_fq = 0, rcvd, i, pkts_sent = 0;
 	struct pkt_stream *pkt_stream = ifobj->pkt_stream;
 	struct xsk_socket_info *xsk = ifobj->xsk;
@@ -843,17 +858,28 @@ static int receive_pkts(struct ifobject *ifobj, struct pollfd *fds)
 		}
 
 		kick_rx(xsk);
+		if (ifobj->use_poll) {
+			ret = poll(fds, 1, POLL_TMOUT);
+			if (ret < 0)
+				exit_with_error(-ret);
+
+			if (!ret) {
+				if (skip_tx)
+					return TEST_PASS;
+
+				ksft_print_msg("ERROR: [%s] Poll timed out\n", __func__);
+				return TEST_FAILURE;
 
-		rcvd = xsk_ring_cons__peek(&xsk->rx, BATCH_SIZE, &idx_rx);
-		if (!rcvd) {
-			if (xsk_ring_prod__needs_wakeup(&umem->fq)) {
-				ret = poll(fds, 1, POLL_TMOUT);
-				if (ret < 0)
-					exit_with_error(-ret);
 			}
-			continue;
+
+			if (!(fds->revents & POLLIN))
+				continue;
 		}
 
+		rcvd = xsk_ring_cons__peek(&xsk->rx, BATCH_SIZE, &idx_rx);
+		if (!rcvd)
+			continue;
+
 		if (ifobj->use_fill_ring) {
 			ret = xsk_ring_prod__reserve(&umem->fq, rcvd, &idx_fq);
 			while (ret != rcvd) {
@@ -863,6 +889,7 @@ static int receive_pkts(struct ifobject *ifobj, struct pollfd *fds)
 					ret = poll(fds, 1, POLL_TMOUT);
 					if (ret < 0)
 						exit_with_error(-ret);
+					continue;
 				}
 				ret = xsk_ring_prod__reserve(&umem->fq, rcvd, &idx_fq);
 			}
@@ -900,13 +927,34 @@ static int receive_pkts(struct ifobject *ifobj, struct pollfd *fds)
 	return TEST_PASS;
 }
 
-static int __send_pkts(struct ifobject *ifobject, u32 *pkt_nb)
+static int __send_pkts(struct ifobject *ifobject, u32 *pkt_nb, bool use_poll,
+		       struct pollfd *fds, bool timeout)
 {
 	struct xsk_socket_info *xsk = ifobject->xsk;
-	u32 i, idx, valid_pkts = 0;
+	u32 i, idx, ret, valid_pkts = 0;
+
+	while (xsk_ring_prod__reserve(&xsk->tx, BATCH_SIZE, &idx) < BATCH_SIZE) {
+		if (use_poll) {
+			ret = poll(fds, 1, POLL_TMOUT);
+			if (timeout) {
+				if (ret < 0) {
+					ksft_print_msg("DEBUG: [%s] Poll error %d\n",
+						       __func__, ret);
+					return TEST_FAILURE;
+				}
+				if (ret == 0)
+					return TEST_PASS;
+				break;
+			}
+			if (ret <= 0) {
+				ksft_print_msg("DEBUG: [%s] Poll error %d\n",
+					       __func__, ret);
+				return TEST_FAILURE;
+			}
+		}
 
-	while (xsk_ring_prod__reserve(&xsk->tx, BATCH_SIZE, &idx) < BATCH_SIZE)
 		complete_pkts(xsk, BATCH_SIZE);
+	}
 
 	for (i = 0; i < BATCH_SIZE; i++) {
 		struct xdp_desc *tx_desc = xsk_ring_prod__tx_desc(&xsk->tx, idx + i);
@@ -933,11 +981,27 @@ static int __send_pkts(struct ifobject *ifobject, u32 *pkt_nb)
 
 	xsk_ring_prod__submit(&xsk->tx, i);
 	xsk->outstanding_tx += valid_pkts;
-	if (complete_pkts(xsk, i))
-		return TEST_FAILURE;
 
-	usleep(10);
-	return TEST_PASS;
+	if (use_poll) {
+		ret = poll(fds, 1, POLL_TMOUT);
+		if (ret <= 0) {
+			if (ret == 0 && timeout)
+				return TEST_PASS;
+
+			ksft_print_msg("DEBUG: [%s] Poll error %d\n", __func__, ret);
+			return TEST_FAILURE;
+		}
+	}
+
+	if (!timeout) {
+		if (complete_pkts(xsk, i))
+			return TEST_FAILURE;
+
+		usleep(10);
+		return TEST_PASS;
+	}
+
+	return TEST_CONTINUE;
 }
 
 static void wait_for_tx_completion(struct xsk_socket_info *xsk)
@@ -948,29 +1012,33 @@ static void wait_for_tx_completion(struct xsk_socket_info *xsk)
 
 static int send_pkts(struct test_spec *test, struct ifobject *ifobject)
 {
+	struct timeval tv_end, tv_now, tv_timeout = {THREAD_TMOUT, 0};
+	bool timeout = test->ifobj_rx->skip_rx;
 	struct pollfd fds = { };
-	u32 pkt_cnt = 0;
+	u32 pkt_cnt = 0, ret;
 
 	fds.fd = xsk_socket__fd(ifobject->xsk->xsk);
 	fds.events = POLLOUT;
 
-	while (pkt_cnt < ifobject->pkt_stream->nb_pkts) {
-		int err;
-
-		if (ifobject->use_poll) {
-			int ret;
-
-			ret = poll(&fds, 1, POLL_TMOUT);
-			if (ret <= 0)
-				continue;
+	ret = gettimeofday(&tv_now, NULL);
+	if (ret)
+		exit_with_error(errno);
+	timeradd(&tv_now, &tv_timeout, &tv_end);
 
-			if (!(fds.revents & POLLOUT))
-				continue;
+	while (pkt_cnt < ifobject->pkt_stream->nb_pkts) {
+		ret = gettimeofday(&tv_now, NULL);
+		if (ret)
+			exit_with_error(errno);
+		if (timercmp(&tv_now, &tv_end, >)) {
+			ksft_print_msg("ERROR: [%s] Send loop timed out\n", __func__);
+			return TEST_FAILURE;
 		}
 
-		err = __send_pkts(ifobject, &pkt_cnt);
-		if (err || test->fail)
+		ret = __send_pkts(ifobject, &pkt_cnt, ifobject->use_poll, &fds, timeout);
+		if ((ret || test->fail) && !timeout)
 			return TEST_FAILURE;
+		else if (ret == TEST_PASS && timeout)
+			return ret;
 	}
 
 	wait_for_tx_completion(ifobject->xsk);
@@ -1235,8 +1303,7 @@ static void *worker_testapp_validate_rx(void *arg)
 
 	pthread_barrier_wait(&barr);
 
-	err = receive_pkts(ifobject, &fds);
-
+	err = receive_pkts(ifobject, &fds, test->ifobj_tx->skip_tx);
 	if (!err && ifobject->validation_func)
 		err = ifobject->validation_func(ifobject);
 	if (err) {
@@ -1265,17 +1332,21 @@ static int testapp_validate_traffic(struct test_spec *test)
 	pkts_in_flight = 0;
 
 	/*Spawn RX thread */
-	pthread_create(&t0, NULL, ifobj_rx->func_ptr, test);
-
-	pthread_barrier_wait(&barr);
-	if (pthread_barrier_destroy(&barr))
-		exit_with_error(errno);
+	if (!ifobj_rx->skip_rx) {
+		pthread_create(&t0, NULL, ifobj_rx->func_ptr, test);
+		pthread_barrier_wait(&barr);
+		if (pthread_barrier_destroy(&barr))
+			exit_with_error(errno);
+	}
 
 	/*Spawn TX thread */
-	pthread_create(&t1, NULL, ifobj_tx->func_ptr, test);
+	if (!ifobj_tx->skip_tx) {
+		pthread_create(&t1, NULL, ifobj_tx->func_ptr, test);
+		pthread_join(t1, NULL);
+	}
 
-	pthread_join(t1, NULL);
-	pthread_join(t0, NULL);
+	if (!ifobj_rx->skip_rx)
+		pthread_join(t0, NULL);
 
 	return !!test->fail;
 }
@@ -1548,10 +1619,28 @@ static void run_pkt_test(struct test_spec *test, enum test_mode mode, enum test_
 
 		pkt_stream_restore_default(test);
 		break;
-	case TEST_TYPE_POLL:
+	case TEST_TYPE_RX_POLL:
+		test->ifobj_rx->use_poll = true;
+		test_spec_set_name(test, "POLL_RX");
+		testapp_validate_traffic(test);
+		break;
+	case TEST_TYPE_TX_POLL:
 		test->ifobj_tx->use_poll = true;
+		test_spec_set_name(test, "POLL_TX");
+		testapp_validate_traffic(test);
+		break;
+	case TEST_TYPE_POLL_TXQ_TMOUT:
+		test_spec_set_name(test, "POLL_TXQ_FULL");
+		test->ifobj_rx->skip_rx = true;
+		test->ifobj_tx->use_poll = true;
+		pkt_stream_invalid(test, 2 * DEFAULT_PKT_CNT, PKT_SIZE);
+		testapp_validate_traffic(test);
+		pkt_stream_restore_default(test);
+		break;
+	case TEST_TYPE_POLL_RXQ_TMOUT:
+		test_spec_set_name(test, "POLL_RXQ_EMPTY");
+		test->ifobj_tx->skip_tx = true;
 		test->ifobj_rx->use_poll = true;
-		test_spec_set_name(test, "POLL");
 		testapp_validate_traffic(test);
 		break;
 	case TEST_TYPE_ALIGNED_INV_DESC:
diff --git a/tools/testing/selftests/bpf/xskxceiver.h b/tools/testing/selftests/bpf/xskxceiver.h
index 3d17053f98e5..0db7e0acccb2 100644
--- a/tools/testing/selftests/bpf/xskxceiver.h
+++ b/tools/testing/selftests/bpf/xskxceiver.h
@@ -27,6 +27,7 @@
 
 #define TEST_PASS 0
 #define TEST_FAILURE -1
+#define TEST_CONTINUE 1
 #define MAX_INTERFACES 2
 #define MAX_INTERFACE_NAME_CHARS 7
 #define MAX_INTERFACES_NAMESPACE_CHARS 10
@@ -48,7 +49,7 @@
 #define SOCK_RECONF_CTR 10
 #define BATCH_SIZE 64
 #define POLL_TMOUT 1000
-#define RECV_TMOUT 3
+#define THREAD_TMOUT 3
 #define DEFAULT_PKT_CNT (4 * 1024)
 #define DEFAULT_UMEM_BUFFERS (DEFAULT_PKT_CNT / 4)
 #define UMEM_SIZE (DEFAULT_UMEM_BUFFERS * XSK_UMEM__DEFAULT_FRAME_SIZE)
@@ -68,7 +69,10 @@ enum test_type {
 	TEST_TYPE_RUN_TO_COMPLETION,
 	TEST_TYPE_RUN_TO_COMPLETION_2K_FRAME,
 	TEST_TYPE_RUN_TO_COMPLETION_SINGLE_PKT,
-	TEST_TYPE_POLL,
+	TEST_TYPE_RX_POLL,
+	TEST_TYPE_TX_POLL,
+	TEST_TYPE_POLL_RXQ_TMOUT,
+	TEST_TYPE_POLL_TXQ_TMOUT,
 	TEST_TYPE_UNALIGNED,
 	TEST_TYPE_ALIGNED_INV_DESC,
 	TEST_TYPE_ALIGNED_INV_DESC_2K_FRAME,
@@ -145,6 +149,8 @@ struct ifobject {
 	bool tx_on;
 	bool rx_on;
 	bool use_poll;
+	bool skip_rx;
+	bool skip_tx;
 	bool busy_poll;
 	bool use_fill_ring;
 	bool release_rx;
-- 
2.34.1

Re: [PATCH bpf-next] selftests: xsk: Update poll test cases

[Maciej Fijalkowski]

On Mon, Jul 18, 2022 at 09:57:12AM +0000, Shibin Koikkara Reeny wrote:
> Poll test case was not testing all the functionality
> of the poll feature in the testsuite. This patch
> update the poll test case with 2 more testcases to
> check the timeout features.
> 
> Poll test case have 4 sub test cases:

Hi Shibin,

Kinda not clear with count of added test cases, at first you say you add 2
more but then you mention something about 4 sub test cases.

To me these are separate test cases.

> 
> 1. TEST_TYPE_RX_POLL:
> Check if POLLIN function work as expect.
> 
> 2. TEST_TYPE_TX_POLL:
> Check if POLLOUT function work as expect.

From run_pkt_test, I don't see any difference between 1 and 2. Why split
then?

> 
> 3. TEST_TYPE_POLL_RXQ_EMPTY:

3 and 4 don't match with the code here (TEST_TYPE_POLL_{R,T}XQ_TMOUT)

> call poll function with parameter POLLIN on empty rx queue
> will cause timeout.If return timeout then test case is pass.
> 
> 4. TEST_TYPE_POLL_TXQ_FULL:
> When txq is filled and packets are not cleaned by the
> kernel then if we invoke the poll function with POLLOUT
> then it should trigger timeout.If return timeout then
> test case is pass.
> 
> Signed-off-by: Shibin Koikkara Reeny <shibin.koikkara.reeny@intel.com>
> ---
>  tools/testing/selftests/bpf/xskxceiver.c | 173 +++++++++++++++++------
>  tools/testing/selftests/bpf/xskxceiver.h |  10 +-
>  2 files changed, 139 insertions(+), 44 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> index 74d56d971baf..8ecab3a47c9e 100644
> --- a/tools/testing/selftests/bpf/xskxceiver.c
> +++ b/tools/testing/selftests/bpf/xskxceiver.c
> @@ -424,6 +424,8 @@ static void __test_spec_init(struct test_spec *test, struct ifobject *ifobj_tx,
>  
>  		ifobj->xsk = &ifobj->xsk_arr[0];
>  		ifobj->use_poll = false;
> +		ifobj->skip_rx = false;
> +		ifobj->skip_tx = false;

Any chances of trying to avoid these booleans? Not that it's a hard nack,
but the less booleans we spread around in this code the better.

>  		ifobj->use_fill_ring = true;
>  		ifobj->release_rx = true;
>  		ifobj->pkt_stream = test->pkt_stream_default;
> @@ -589,6 +591,19 @@ static struct pkt_stream *pkt_stream_clone(struct xsk_umem_info *umem,
>  	return pkt_stream_generate(umem, pkt_stream->nb_pkts, pkt_stream->pkts[0].len);
>  }
>  
> +static void pkt_stream_invalid(struct test_spec *test, u32 nb_pkts, u32 pkt_len)
> +{
> +	struct pkt_stream *pkt_stream;
> +	u32 i;
> +
> +	pkt_stream = pkt_stream_generate(test->ifobj_tx->umem, nb_pkts, pkt_len);
> +	for (i = 0; i < nb_pkts; i++)
> +		pkt_stream->pkts[i].valid = false;
> +
> +	test->ifobj_tx->pkt_stream = pkt_stream;
> +	test->ifobj_rx->pkt_stream = pkt_stream;
> +}

Please explain how this work, e.g. why you need to have to have invalid
pkt stream + avoiding launching rx thread and why one of them is not
enough.

Personally I think this is not needed. When calling pkt_stream_generate(),
validity of pkt is set based on length of packet vs frame size:

		if (pkt_len > umem->frame_size)
			pkt_stream->pkts[i].valid = false;

so couldn't you use 2k frame size and bigger length of a packet?

> +
>  static void pkt_stream_replace(struct test_spec *test, u32 nb_pkts, u32 pkt_len)
>  {
>  	struct pkt_stream *pkt_stream;
> @@ -817,9 +832,9 @@ static int complete_pkts(struct xsk_socket_info *xsk, int batch_size)
>  	return TEST_PASS;
>  }
>  
> -static int receive_pkts(struct ifobject *ifobj, struct pollfd *fds)
> +static int receive_pkts(struct ifobject *ifobj, struct pollfd *fds, bool skip_tx)
>  {
> -	struct timeval tv_end, tv_now, tv_timeout = {RECV_TMOUT, 0};
> +	struct timeval tv_end, tv_now, tv_timeout = {THREAD_TMOUT, 0};
>  	u32 idx_rx = 0, idx_fq = 0, rcvd, i, pkts_sent = 0;
>  	struct pkt_stream *pkt_stream = ifobj->pkt_stream;
>  	struct xsk_socket_info *xsk = ifobj->xsk;
> @@ -843,17 +858,28 @@ static int receive_pkts(struct ifobject *ifobj, struct pollfd *fds)
>  		}
>  
>  		kick_rx(xsk);
> +		if (ifobj->use_poll) {
> +			ret = poll(fds, 1, POLL_TMOUT);
> +			if (ret < 0)
> +				exit_with_error(-ret);
> +
> +			if (!ret) {
> +				if (skip_tx)
> +					return TEST_PASS;
> +
> +				ksft_print_msg("ERROR: [%s] Poll timed out\n", __func__);
> +				return TEST_FAILURE;
>  
> -		rcvd = xsk_ring_cons__peek(&xsk->rx, BATCH_SIZE, &idx_rx);
> -		if (!rcvd) {
> -			if (xsk_ring_prod__needs_wakeup(&umem->fq)) {

So now we don't check if fq needs to be woken up in non-poll case?
I believe this is still needed so we get to the driver and pick fq
entries. Prove me wrong of course if I'm missing something.

> -				ret = poll(fds, 1, POLL_TMOUT);
> -				if (ret < 0)
> -					exit_with_error(-ret);
>  			}
> -			continue;
> +
> +			if (!(fds->revents & POLLIN))
> +				continue;
>  		}
>  
> +		rcvd = xsk_ring_cons__peek(&xsk->rx, BATCH_SIZE, &idx_rx);
> +		if (!rcvd)
> +			continue;
> +
>  		if (ifobj->use_fill_ring) {
>  			ret = xsk_ring_prod__reserve(&umem->fq, rcvd, &idx_fq);
>  			while (ret != rcvd) {
> @@ -863,6 +889,7 @@ static int receive_pkts(struct ifobject *ifobj, struct pollfd *fds)
>  					ret = poll(fds, 1, POLL_TMOUT);
>  					if (ret < 0)
>  						exit_with_error(-ret);
> +					continue;

Why continue here?

>  				}
>  				ret = xsk_ring_prod__reserve(&umem->fq, rcvd, &idx_fq);
>  			}
> @@ -900,13 +927,34 @@ static int receive_pkts(struct ifobject *ifobj, struct pollfd *fds)
>  	return TEST_PASS;
>  }
>  
> -static int __send_pkts(struct ifobject *ifobject, u32 *pkt_nb)
> +static int __send_pkts(struct ifobject *ifobject, u32 *pkt_nb, bool use_poll,
> +		       struct pollfd *fds, bool timeout)
>  {
>  	struct xsk_socket_info *xsk = ifobject->xsk;
> -	u32 i, idx, valid_pkts = 0;
> +	u32 i, idx, ret, valid_pkts = 0;
> +
> +	while (xsk_ring_prod__reserve(&xsk->tx, BATCH_SIZE, &idx) < BATCH_SIZE) {
> +		if (use_poll) {
> +			ret = poll(fds, 1, POLL_TMOUT);
> +			if (timeout) {
> +				if (ret < 0) {
> +					ksft_print_msg("DEBUG: [%s] Poll error %d\n",
> +						       __func__, ret);
> +					return TEST_FAILURE;
> +				}
> +				if (ret == 0)
> +					return TEST_PASS;
> +				break;
> +			}
> +			if (ret <= 0) {
> +				ksft_print_msg("DEBUG: [%s] Poll error %d\n",
> +					       __func__, ret);
> +				return TEST_FAILURE;
> +			}
> +		}
>  
> -	while (xsk_ring_prod__reserve(&xsk->tx, BATCH_SIZE, &idx) < BATCH_SIZE)
>  		complete_pkts(xsk, BATCH_SIZE);
> +	}
>  
>  	for (i = 0; i < BATCH_SIZE; i++) {
>  		struct xdp_desc *tx_desc = xsk_ring_prod__tx_desc(&xsk->tx, idx + i);
> @@ -933,11 +981,27 @@ static int __send_pkts(struct ifobject *ifobject, u32 *pkt_nb)
>  
>  	xsk_ring_prod__submit(&xsk->tx, i);
>  	xsk->outstanding_tx += valid_pkts;
> -	if (complete_pkts(xsk, i))
> -		return TEST_FAILURE;
>  
> -	usleep(10);
> -	return TEST_PASS;
> +	if (use_poll) {
> +		ret = poll(fds, 1, POLL_TMOUT);
> +		if (ret <= 0) {
> +			if (ret == 0 && timeout)
> +				return TEST_PASS;
> +
> +			ksft_print_msg("DEBUG: [%s] Poll error %d\n", __func__, ret);
> +			return TEST_FAILURE;
> +		}
> +	}
> +
> +	if (!timeout) {
> +		if (complete_pkts(xsk, i))
> +			return TEST_FAILURE;
> +
> +		usleep(10);
> +		return TEST_PASS;
> +	}
> +
> +	return TEST_CONTINUE;

Why do you need this?

>  }
>  
>  static void wait_for_tx_completion(struct xsk_socket_info *xsk)
> @@ -948,29 +1012,33 @@ static void wait_for_tx_completion(struct xsk_socket_info *xsk)
>  
>  static int send_pkts(struct test_spec *test, struct ifobject *ifobject)
>  {
> +	struct timeval tv_end, tv_now, tv_timeout = {THREAD_TMOUT, 0};
> +	bool timeout = test->ifobj_rx->skip_rx;
>  	struct pollfd fds = { };
> -	u32 pkt_cnt = 0;
> +	u32 pkt_cnt = 0, ret;
>  
>  	fds.fd = xsk_socket__fd(ifobject->xsk->xsk);
>  	fds.events = POLLOUT;
>  
> -	while (pkt_cnt < ifobject->pkt_stream->nb_pkts) {
> -		int err;
> -
> -		if (ifobject->use_poll) {
> -			int ret;
> -
> -			ret = poll(&fds, 1, POLL_TMOUT);
> -			if (ret <= 0)
> -				continue;
> +	ret = gettimeofday(&tv_now, NULL);
> +	if (ret)
> +		exit_with_error(errno);
> +	timeradd(&tv_now, &tv_timeout, &tv_end);

This logic of timer on Tx side is not mentioned anywhere in the commit
message. Please try your best to describe all of the changes you're
proposing.

Also, couldn't this be a separate patch?

>  
> -			if (!(fds.revents & POLLOUT))
> -				continue;
> +	while (pkt_cnt < ifobject->pkt_stream->nb_pkts) {
> +		ret = gettimeofday(&tv_now, NULL);
> +		if (ret)
> +			exit_with_error(errno);
> +		if (timercmp(&tv_now, &tv_end, >)) {
> +			ksft_print_msg("ERROR: [%s] Send loop timed out\n", __func__);
> +			return TEST_FAILURE;
>  		}
>  
> -		err = __send_pkts(ifobject, &pkt_cnt);
> -		if (err || test->fail)
> +		ret = __send_pkts(ifobject, &pkt_cnt, ifobject->use_poll, &fds, timeout);
> +		if ((ret || test->fail) && !timeout)
>  			return TEST_FAILURE;
> +		else if (ret == TEST_PASS && timeout)
> +			return ret;
>  	}
>  
>  	wait_for_tx_completion(ifobject->xsk);
> @@ -1235,8 +1303,7 @@ static void *worker_testapp_validate_rx(void *arg)
>  
>  	pthread_barrier_wait(&barr);
>  
> -	err = receive_pkts(ifobject, &fds);
> -
> +	err = receive_pkts(ifobject, &fds, test->ifobj_tx->skip_tx);
>  	if (!err && ifobject->validation_func)
>  		err = ifobject->validation_func(ifobject);
>  	if (err) {
> @@ -1265,17 +1332,21 @@ static int testapp_validate_traffic(struct test_spec *test)
>  	pkts_in_flight = 0;
>  
>  	/*Spawn RX thread */
> -	pthread_create(&t0, NULL, ifobj_rx->func_ptr, test);
> -
> -	pthread_barrier_wait(&barr);
> -	if (pthread_barrier_destroy(&barr))
> -		exit_with_error(errno);
> +	if (!ifobj_rx->skip_rx) {
> +		pthread_create(&t0, NULL, ifobj_rx->func_ptr, test);
> +		pthread_barrier_wait(&barr);
> +		if (pthread_barrier_destroy(&barr))
> +			exit_with_error(errno);
> +	}
>  
>  	/*Spawn TX thread */
> -	pthread_create(&t1, NULL, ifobj_tx->func_ptr, test);
> +	if (!ifobj_tx->skip_tx) {
> +		pthread_create(&t1, NULL, ifobj_tx->func_ptr, test);
> +		pthread_join(t1, NULL);
> +	}
>  
> -	pthread_join(t1, NULL);
> -	pthread_join(t0, NULL);
> +	if (!ifobj_rx->skip_rx)
> +		pthread_join(t0, NULL);

Have you thought of a testapp_validate_traffic() variant with a single
thread, either Tx or Rx? In this case probably would make everything
clearer in the current pthread code. Also, wouldn't this drop the need for
skip booleans?

>  
>  	return !!test->fail;
>  }
> @@ -1548,10 +1619,28 @@ static void run_pkt_test(struct test_spec *test, enum test_mode mode, enum test_
>  
>  		pkt_stream_restore_default(test);
>  		break;
> -	case TEST_TYPE_POLL:
> +	case TEST_TYPE_RX_POLL:
> +		test->ifobj_rx->use_poll = true;
> +		test_spec_set_name(test, "POLL_RX");
> +		testapp_validate_traffic(test);
> +		break;
> +	case TEST_TYPE_TX_POLL:
>  		test->ifobj_tx->use_poll = true;
> +		test_spec_set_name(test, "POLL_TX");
> +		testapp_validate_traffic(test);
> +		break;
> +	case TEST_TYPE_POLL_TXQ_TMOUT:
> +		test_spec_set_name(test, "POLL_TXQ_FULL");
> +		test->ifobj_rx->skip_rx = true;
> +		test->ifobj_tx->use_poll = true;
> +		pkt_stream_invalid(test, 2 * DEFAULT_PKT_CNT, PKT_SIZE);
> +		testapp_validate_traffic(test);
> +		pkt_stream_restore_default(test);
> +		break;
> +	case TEST_TYPE_POLL_RXQ_TMOUT:
> +		test_spec_set_name(test, "POLL_RXQ_EMPTY");
> +		test->ifobj_tx->skip_tx = true;
>  		test->ifobj_rx->use_poll = true;
> -		test_spec_set_name(test, "POLL");
>  		testapp_validate_traffic(test);
>  		break;
>  	case TEST_TYPE_ALIGNED_INV_DESC:
> diff --git a/tools/testing/selftests/bpf/xskxceiver.h b/tools/testing/selftests/bpf/xskxceiver.h
> index 3d17053f98e5..0db7e0acccb2 100644
> --- a/tools/testing/selftests/bpf/xskxceiver.h
> +++ b/tools/testing/selftests/bpf/xskxceiver.h
> @@ -27,6 +27,7 @@
>  
>  #define TEST_PASS 0
>  #define TEST_FAILURE -1
> +#define TEST_CONTINUE 1
>  #define MAX_INTERFACES 2
>  #define MAX_INTERFACE_NAME_CHARS 7
>  #define MAX_INTERFACES_NAMESPACE_CHARS 10
> @@ -48,7 +49,7 @@
>  #define SOCK_RECONF_CTR 10
>  #define BATCH_SIZE 64
>  #define POLL_TMOUT 1000
> -#define RECV_TMOUT 3
> +#define THREAD_TMOUT 3
>  #define DEFAULT_PKT_CNT (4 * 1024)
>  #define DEFAULT_UMEM_BUFFERS (DEFAULT_PKT_CNT / 4)
>  #define UMEM_SIZE (DEFAULT_UMEM_BUFFERS * XSK_UMEM__DEFAULT_FRAME_SIZE)
> @@ -68,7 +69,10 @@ enum test_type {
>  	TEST_TYPE_RUN_TO_COMPLETION,
>  	TEST_TYPE_RUN_TO_COMPLETION_2K_FRAME,
>  	TEST_TYPE_RUN_TO_COMPLETION_SINGLE_PKT,
> -	TEST_TYPE_POLL,
> +	TEST_TYPE_RX_POLL,
> +	TEST_TYPE_TX_POLL,
> +	TEST_TYPE_POLL_RXQ_TMOUT,
> +	TEST_TYPE_POLL_TXQ_TMOUT,
>  	TEST_TYPE_UNALIGNED,
>  	TEST_TYPE_ALIGNED_INV_DESC,
>  	TEST_TYPE_ALIGNED_INV_DESC_2K_FRAME,
> @@ -145,6 +149,8 @@ struct ifobject {
>  	bool tx_on;
>  	bool rx_on;
>  	bool use_poll;
> +	bool skip_rx;
> +	bool skip_tx;
>  	bool busy_poll;
>  	bool use_fill_ring;
>  	bool release_rx;
> -- 
> 2.34.1
> 

RE: [PATCH bpf-next] selftests: xsk: Update poll test cases

[Koikkara Reeny, Shibin]

> -----Original Message-----
> From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Sent: Friday, July 22, 2022 3:16 PM
> To: Koikkara Reeny, Shibin <shibin.koikkara.reeny@intel.com>
> Cc: bpf@vger.kernel.org; ast@kernel.org; daniel@iogearbox.net;
> netdev@vger.kernel.org; Karlsson, Magnus <magnus.karlsson@intel.com>;
> bjorn@kernel.org; kuba@kernel.org; andrii@kernel.org; Loftus, Ciara
> <ciara.loftus@intel.com>
> Subject: Re: [PATCH bpf-next] selftests: xsk: Update poll test cases
> 
> On Mon, Jul 18, 2022 at 09:57:12AM +0000, Shibin Koikkara Reeny wrote:
> > Poll test case was not testing all the functionality of the poll
> > feature in the testsuite. This patch update the poll test case with 2
> > more testcases to check the timeout features.
> >
> > Poll test case have 4 sub test cases:
> 
> Hi Shibin,
> 
> Kinda not clear with count of added test cases, at first you say you add 2
> more but then you mention something about 4 sub test cases.
> 
> To me these are separate test cases.
>
Hi Maciej,

Will update it in V2 

> >
> > 1. TEST_TYPE_RX_POLL:
> > Check if POLLIN function work as expect.
> >
> > 2. TEST_TYPE_TX_POLL:
> > Check if POLLOUT function work as expect.
> 
> From run_pkt_test, I don't see any difference between 1 and 2. Why split
> then?
> 


It was done to show which case exactly broke. If RX poll event or TX poll event

> >
> > 3. TEST_TYPE_POLL_RXQ_EMPTY:
> 
> 3 and 4 don't match with the code here (TEST_TYPE_POLL_{R,T}XQ_TMOUT)
> 
> > call poll function with parameter POLLIN on empty rx queue will cause
> > timeout.If return timeout then test case is pass.
> >


True but  It was change to RXQ_EMPTY and TXQ_FULL from _TMOUT to
make it more clearer to what exactly is happening to cause timeout.

> > 4. TEST_TYPE_POLL_TXQ_FULL:
> > When txq is filled and packets are not cleaned by the kernel then if
> > we invoke the poll function with POLLOUT then it should trigger
> > timeout.If return timeout then test case is pass.
> >
> > Signed-off-by: Shibin Koikkara Reeny <shibin.koikkara.reeny@intel.com>
> > ---
> >  tools/testing/selftests/bpf/xskxceiver.c | 173
> > +++++++++++++++++------  tools/testing/selftests/bpf/xskxceiver.h |
> > 10 +-
> >  2 files changed, 139 insertions(+), 44 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/xskxceiver.c
> > b/tools/testing/selftests/bpf/xskxceiver.c
> > index 74d56d971baf..8ecab3a47c9e 100644
> > --- a/tools/testing/selftests/bpf/xskxceiver.c
> > +++ b/tools/testing/selftests/bpf/xskxceiver.c
> > @@ -424,6 +424,8 @@ static void __test_spec_init(struct test_spec
> > *test, struct ifobject *ifobj_tx,
> >
> >  		ifobj->xsk = &ifobj->xsk_arr[0];
> >  		ifobj->use_poll = false;
> > +		ifobj->skip_rx = false;
> > +		ifobj->skip_tx = false;
> 
> Any chances of trying to avoid these booleans? Not that it's a hard nack, but
> the less booleans we spread around in this code the better.


Not sure if it is possible but using any other logic will make
the code more complex and less readable.

> 
> >  		ifobj->use_fill_ring = true;
> >  		ifobj->release_rx = true;
> >  		ifobj->pkt_stream = test->pkt_stream_default; @@ -589,6
> +591,19 @@
> > static struct pkt_stream *pkt_stream_clone(struct xsk_umem_info
> *umem,
> >  	return pkt_stream_generate(umem, pkt_stream->nb_pkts,
> > pkt_stream->pkts[0].len);  }
> >
> > +static void pkt_stream_invalid(struct test_spec *test, u32 nb_pkts,
> > +u32 pkt_len) {
> > +	struct pkt_stream *pkt_stream;
> > +	u32 i;
> > +
> > +	pkt_stream = pkt_stream_generate(test->ifobj_tx->umem,
> nb_pkts, pkt_len);
> > +	for (i = 0; i < nb_pkts; i++)
> > +		pkt_stream->pkts[i].valid = false;
> > +
> > +	test->ifobj_tx->pkt_stream = pkt_stream;
> > +	test->ifobj_rx->pkt_stream = pkt_stream; }
> 
> Please explain how this work, e.g. why you need to have to have invalid pkt
> stream + avoiding launching rx thread and why one of them is not enough.
> 
> Personally I think this is not needed. When calling pkt_stream_generate(),
> validity of pkt is set based on length of packet vs frame size:
> 
> 		if (pkt_len > umem->frame_size)
> 			pkt_stream->pkts[i].valid = false;
> 
> so couldn't you use 2k frame size and bigger length of a packet?
> 
This function was introduced for TEST_TYPE_POLL_TXQ_FULL keep
the TX full and stop nofying the kernel that there is packet to cleanup.
So we are manually setting the packets to invalid. This help to keep
the __send_pkts() more generic and reduce the if conditions.
ex: xsk_ring_prod__submit() is not needed to be added inside if condition.

You are right we don't need rx stream but thought it will be good
to keep as can be used for other features in future and will be more generic.

> > +
> >  static void pkt_stream_replace(struct test_spec *test, u32 nb_pkts,
> > u32 pkt_len)  {
> >  	struct pkt_stream *pkt_stream;
> > @@ -817,9 +832,9 @@ static int complete_pkts(struct xsk_socket_info
> *xsk, int batch_size)
> >  	return TEST_PASS;
> >  }
> >
> > -static int receive_pkts(struct ifobject *ifobj, struct pollfd *fds)
> > +static int receive_pkts(struct ifobject *ifobj, struct pollfd *fds,
> > +bool skip_tx)
> >  {
> > -	struct timeval tv_end, tv_now, tv_timeout = {RECV_TMOUT, 0};
> > +	struct timeval tv_end, tv_now, tv_timeout = {THREAD_TMOUT, 0};
> >  	u32 idx_rx = 0, idx_fq = 0, rcvd, i, pkts_sent = 0;
> >  	struct pkt_stream *pkt_stream = ifobj->pkt_stream;
> >  	struct xsk_socket_info *xsk = ifobj->xsk; @@ -843,17 +858,28 @@
> > static int receive_pkts(struct ifobject *ifobj, struct pollfd *fds)
> >  		}
> >
> >  		kick_rx(xsk);
> > +		if (ifobj->use_poll) {
> > +			ret = poll(fds, 1, POLL_TMOUT);
> > +			if (ret < 0)
> > +				exit_with_error(-ret);
> > +
> > +			if (!ret) {
> > +				if (skip_tx)
> > +					return TEST_PASS;
> > +
> > +				ksft_print_msg("ERROR: [%s] Poll timed
> out\n", __func__);
> > +				return TEST_FAILURE;
> >
> > -		rcvd = xsk_ring_cons__peek(&xsk->rx, BATCH_SIZE,
> &idx_rx);
> > -		if (!rcvd) {
> > -			if (xsk_ring_prod__needs_wakeup(&umem->fq)) {
> 
> So now we don't check if fq needs to be woken up in non-poll case?
> I believe this is still needed so we get to the driver and pick fq entries. Prove
> me wrong of course if I'm missing something.

xsk_ring_prod__needs_wakeup() ==>  *r->flags & XDP_RING_NEED_WAKEUP;
This function only check if the flag is set or not and it is not updating or
triggering anything. In the original case if flag is set then trigger the 
poll event and continue.
In this patch poll event is called in any case if it enter the if (!rcvd)  is true..
We don't check if XDP_RING_NEED_WAKEUP is set or not.
	

> 
> > -				ret = poll(fds, 1, POLL_TMOUT);
> > -				if (ret < 0)
> > -					exit_with_error(-ret);
> >  			}
> > -			continue;
> > +
> > +			if (!(fds->revents & POLLIN))
> > +				continue;
> >  		}
> >
> > +		rcvd = xsk_ring_cons__peek(&xsk->rx, BATCH_SIZE,
> &idx_rx);
> > +		if (!rcvd)
> > +			continue;
> > +
> >  		if (ifobj->use_fill_ring) {
> >  			ret = xsk_ring_prod__reserve(&umem->fq, rcvd,
> &idx_fq);
> >  			while (ret != rcvd) {
> > @@ -863,6 +889,7 @@ static int receive_pkts(struct ifobject *ifobj, struct
> pollfd *fds)
> >  					ret = poll(fds, 1, POLL_TMOUT);
> >  					if (ret < 0)
> >  						exit_with_error(-ret);
> > +					continue;
> 
> Why continue here?

You are right it is not needed. Will update in V2 patch. Thanks.

> 
> >  				}
> >  				ret = xsk_ring_prod__reserve(&umem->fq,
> rcvd, &idx_fq);
> >  			}
> > @@ -900,13 +927,34 @@ static int receive_pkts(struct ifobject *ifobj, struct
> pollfd *fds)
> >  	return TEST_PASS;
> >  }
> >
> > -static int __send_pkts(struct ifobject *ifobject, u32 *pkt_nb)
> > +static int __send_pkts(struct ifobject *ifobject, u32 *pkt_nb, bool
> use_poll,
> > +		       struct pollfd *fds, bool timeout)
> >  {
> >  	struct xsk_socket_info *xsk = ifobject->xsk;
> > -	u32 i, idx, valid_pkts = 0;
> > +	u32 i, idx, ret, valid_pkts = 0;
> > +
> > +	while (xsk_ring_prod__reserve(&xsk->tx, BATCH_SIZE, &idx) <
> BATCH_SIZE) {
> > +		if (use_poll) {
> > +			ret = poll(fds, 1, POLL_TMOUT);
> > +			if (timeout) {
> > +				if (ret < 0) {
> > +					ksft_print_msg("DEBUG: [%s] Poll
> error %d\n",
> > +						       __func__, ret);
> > +					return TEST_FAILURE;
> > +				}
> > +				if (ret == 0)
> > +					return TEST_PASS;
> > +				break;
> > +			}
> > +			if (ret <= 0) {
> > +				ksft_print_msg("DEBUG: [%s] Poll error
> %d\n",
> > +					       __func__, ret);
> > +				return TEST_FAILURE;
> > +			}
> > +		}
> >
> > -	while (xsk_ring_prod__reserve(&xsk->tx, BATCH_SIZE, &idx) <
> BATCH_SIZE)
> >  		complete_pkts(xsk, BATCH_SIZE);
> > +	}
> >
> >  	for (i = 0; i < BATCH_SIZE; i++) {
> >  		struct xdp_desc *tx_desc = xsk_ring_prod__tx_desc(&xsk-
> >tx, idx +
> > i); @@ -933,11 +981,27 @@ static int __send_pkts(struct ifobject
> > *ifobject, u32 *pkt_nb)
> >
> >  	xsk_ring_prod__submit(&xsk->tx, i);
> >  	xsk->outstanding_tx += valid_pkts;
> > -	if (complete_pkts(xsk, i))
> > -		return TEST_FAILURE;
> >
> > -	usleep(10);
> > -	return TEST_PASS;
> > +	if (use_poll) {
> > +		ret = poll(fds, 1, POLL_TMOUT);
> > +		if (ret <= 0) {
> > +			if (ret == 0 && timeout)
> > +				return TEST_PASS;
> > +
> > +			ksft_print_msg("DEBUG: [%s] Poll error %d\n",
> __func__, ret);
> > +			return TEST_FAILURE;
> > +		}
> > +	}
> > +
> > +	if (!timeout) {
> > +		if (complete_pkts(xsk, i))
> > +			return TEST_FAILURE;
> > +
> > +		usleep(10);
> > +		return TEST_PASS;
> > +	}
> > +
> > +	return TEST_CONTINUE;
> 
> Why do you need this?
> 

__send_pkts is expected to return TEST_PASS or TEST_FAIL to send_pkts function and
if returned TEST_PASS then continue sending pkts and exit when all the packet are finished.
if returned TEST_FAILURE then test failed and return.

For TEST_TYPE_POLL_TXQ_TMOUT  TEST_PASS is return value when timout happened and
should not sent anymore packets and break. But this will break other test. So needed 
new return type TEST_CONTINUE to keep sending packets.

> >  }
> >
> >  static void wait_for_tx_completion(struct xsk_socket_info *xsk) @@
> > -948,29 +1012,33 @@ static void wait_for_tx_completion(struct
> > xsk_socket_info *xsk)
> >
> >  static int send_pkts(struct test_spec *test, struct ifobject
> > *ifobject)  {
> > +	struct timeval tv_end, tv_now, tv_timeout = {THREAD_TMOUT, 0};
> > +	bool timeout = test->ifobj_rx->skip_rx;
> >  	struct pollfd fds = { };
> > -	u32 pkt_cnt = 0;
> > +	u32 pkt_cnt = 0, ret;
> >
> >  	fds.fd = xsk_socket__fd(ifobject->xsk->xsk);
> >  	fds.events = POLLOUT;
> >
> > -	while (pkt_cnt < ifobject->pkt_stream->nb_pkts) {
> > -		int err;
> > -
> > -		if (ifobject->use_poll) {
> > -			int ret;
> > -
> > -			ret = poll(&fds, 1, POLL_TMOUT);
> > -			if (ret <= 0)
> > -				continue;
> > +	ret = gettimeofday(&tv_now, NULL);
> > +	if (ret)
> > +		exit_with_error(errno);
> > +	timeradd(&tv_now, &tv_timeout, &tv_end);
> 
> This logic of timer on Tx side is not mentioned anywhere in the commit
> message. Please try your best to describe all of the changes you're
> proposing.
> 

Will update in the commit message in V2 patch.

> Also, couldn't this be a separate patch?
> 
I prefer to keep it. But if you suggest otherwise I can remove.

> >
> > -			if (!(fds.revents & POLLOUT))
> > -				continue;
> > +	while (pkt_cnt < ifobject->pkt_stream->nb_pkts) {
> > +		ret = gettimeofday(&tv_now, NULL);
> > +		if (ret)
> > +			exit_with_error(errno);
> > +		if (timercmp(&tv_now, &tv_end, >)) {
> > +			ksft_print_msg("ERROR: [%s] Send loop timed
> out\n", __func__);
> > +			return TEST_FAILURE;
> >  		}
> >
> > -		err = __send_pkts(ifobject, &pkt_cnt);
> > -		if (err || test->fail)
> > +		ret = __send_pkts(ifobject, &pkt_cnt, ifobject->use_poll,
> &fds, timeout);
> > +		if ((ret || test->fail) && !timeout)
> >  			return TEST_FAILURE;
> > +		else if (ret == TEST_PASS && timeout)
> > +			return ret;
> >  	}
> >
> >  	wait_for_tx_completion(ifobject->xsk);
> > @@ -1235,8 +1303,7 @@ static void *worker_testapp_validate_rx(void
> > *arg)
> >
> >  	pthread_barrier_wait(&barr);
> >
> > -	err = receive_pkts(ifobject, &fds);
> > -
> > +	err = receive_pkts(ifobject, &fds, test->ifobj_tx->skip_tx);
> >  	if (!err && ifobject->validation_func)
> >  		err = ifobject->validation_func(ifobject);
> >  	if (err) {
> > @@ -1265,17 +1332,21 @@ static int testapp_validate_traffic(struct
> test_spec *test)
> >  	pkts_in_flight = 0;
> >
> >  	/*Spawn RX thread */
> > -	pthread_create(&t0, NULL, ifobj_rx->func_ptr, test);
> > -
> > -	pthread_barrier_wait(&barr);
> > -	if (pthread_barrier_destroy(&barr))
> > -		exit_with_error(errno);
> > +	if (!ifobj_rx->skip_rx) {
> > +		pthread_create(&t0, NULL, ifobj_rx->func_ptr, test);
> > +		pthread_barrier_wait(&barr);
> > +		if (pthread_barrier_destroy(&barr))
> > +			exit_with_error(errno);
> > +	}
> >
> >  	/*Spawn TX thread */
> > -	pthread_create(&t1, NULL, ifobj_tx->func_ptr, test);
> > +	if (!ifobj_tx->skip_tx) {
> > +		pthread_create(&t1, NULL, ifobj_tx->func_ptr, test);
> > +		pthread_join(t1, NULL);
> > +	}
> >
> > -	pthread_join(t1, NULL);
> > -	pthread_join(t0, NULL);
> > +	if (!ifobj_rx->skip_rx)
> > +		pthread_join(t0, NULL);
> 
> Have you thought of a testapp_validate_traffic() variant with a single thread,
> either Tx or Rx? In this case probably would make everything clearer in the
> current pthread code. Also, wouldn't this drop the need for skip booleans?
> 

My suggestion will be to reuse the existing functions. If you suggest otherwise
I can look into it.

> >
> >  	return !!test->fail;
> >  }
> > @@ -1548,10 +1619,28 @@ static void run_pkt_test(struct test_spec
> > *test, enum test_mode mode, enum test_
> >
> >  		pkt_stream_restore_default(test);
> >  		break;
> > -	case TEST_TYPE_POLL:
> > +	case TEST_TYPE_RX_POLL:
> > +		test->ifobj_rx->use_poll = true;
> > +		test_spec_set_name(test, "POLL_RX");
> > +		testapp_validate_traffic(test);
> > +		break;
> > +	case TEST_TYPE_TX_POLL:
> >  		test->ifobj_tx->use_poll = true;
> > +		test_spec_set_name(test, "POLL_TX");
> > +		testapp_validate_traffic(test);
> > +		break;
> > +	case TEST_TYPE_POLL_TXQ_TMOUT:
> > +		test_spec_set_name(test, "POLL_TXQ_FULL");
> > +		test->ifobj_rx->skip_rx = true;
> > +		test->ifobj_tx->use_poll = true;
> > +		pkt_stream_invalid(test, 2 * DEFAULT_PKT_CNT, PKT_SIZE);
> > +		testapp_validate_traffic(test);
> > +		pkt_stream_restore_default(test);
> > +		break;
> > +	case TEST_TYPE_POLL_RXQ_TMOUT:
> > +		test_spec_set_name(test, "POLL_RXQ_EMPTY");
> > +		test->ifobj_tx->skip_tx = true;
> >  		test->ifobj_rx->use_poll = true;
> > -		test_spec_set_name(test, "POLL");
> >  		testapp_validate_traffic(test);
> >  		break;
> >  	case TEST_TYPE_ALIGNED_INV_DESC:
> > diff --git a/tools/testing/selftests/bpf/xskxceiver.h
> > b/tools/testing/selftests/bpf/xskxceiver.h
> > index 3d17053f98e5..0db7e0acccb2 100644
> > --- a/tools/testing/selftests/bpf/xskxceiver.h
> > +++ b/tools/testing/selftests/bpf/xskxceiver.h
> > @@ -27,6 +27,7 @@
> >
> >  #define TEST_PASS 0
> >  #define TEST_FAILURE -1
> > +#define TEST_CONTINUE 1
> >  #define MAX_INTERFACES 2
> >  #define MAX_INTERFACE_NAME_CHARS 7
> >  #define MAX_INTERFACES_NAMESPACE_CHARS 10 @@ -48,7 +49,7 @@
> #define
> > SOCK_RECONF_CTR 10  #define BATCH_SIZE 64  #define POLL_TMOUT
> 1000
> > -#define RECV_TMOUT 3
> > +#define THREAD_TMOUT 3
> >  #define DEFAULT_PKT_CNT (4 * 1024)
> >  #define DEFAULT_UMEM_BUFFERS (DEFAULT_PKT_CNT / 4)  #define
> UMEM_SIZE
> > (DEFAULT_UMEM_BUFFERS * XSK_UMEM__DEFAULT_FRAME_SIZE) @@ -
> 68,7 +69,10
> > @@ enum test_type {
> >  	TEST_TYPE_RUN_TO_COMPLETION,
> >  	TEST_TYPE_RUN_TO_COMPLETION_2K_FRAME,
> >  	TEST_TYPE_RUN_TO_COMPLETION_SINGLE_PKT,
> > -	TEST_TYPE_POLL,
> > +	TEST_TYPE_RX_POLL,
> > +	TEST_TYPE_TX_POLL,
> > +	TEST_TYPE_POLL_RXQ_TMOUT,
> > +	TEST_TYPE_POLL_TXQ_TMOUT,
> >  	TEST_TYPE_UNALIGNED,
> >  	TEST_TYPE_ALIGNED_INV_DESC,
> >  	TEST_TYPE_ALIGNED_INV_DESC_2K_FRAME,
> > @@ -145,6 +149,8 @@ struct ifobject {
> >  	bool tx_on;
> >  	bool rx_on;
> >  	bool use_poll;
> > +	bool skip_rx;
> > +	bool skip_tx;
> >  	bool busy_poll;
> >  	bool use_fill_ring;
> >  	bool release_rx;
> > --
> > 2.34.1
> >

Re: [PATCH bpf-next] selftests: xsk: Update poll test cases

[Maciej Fijalkowski]

On Tue, Jul 26, 2022 at 10:43:36AM +0100, Koikkara Reeny, Shibin wrote:
> > -----Original Message-----
> > From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > Sent: Friday, July 22, 2022 3:16 PM
> > To: Koikkara Reeny, Shibin <shibin.koikkara.reeny@intel.com>
> > Cc: bpf@vger.kernel.org; ast@kernel.org; daniel@iogearbox.net;
> > netdev@vger.kernel.org; Karlsson, Magnus <magnus.karlsson@intel.com>;
> > bjorn@kernel.org; kuba@kernel.org; andrii@kernel.org; Loftus, Ciara
> > <ciara.loftus@intel.com>
> > Subject: Re: [PATCH bpf-next] selftests: xsk: Update poll test cases
> > 
> > On Mon, Jul 18, 2022 at 09:57:12AM +0000, Shibin Koikkara Reeny wrote:
> > > Poll test case was not testing all the functionality of the poll
> > > feature in the testsuite. This patch update the poll test case with 2
> > > more testcases to check the timeout features.
> > >
> > > Poll test case have 4 sub test cases:
> > 
> > Hi Shibin,
> > 
> > Kinda not clear with count of added test cases, at first you say you add 2
> > more but then you mention something about 4 sub test cases.
> > 
> > To me these are separate test cases.
> >
> Hi Maciej,
> 
> Will update it in V2 
> 
> > >
> > > 1. TEST_TYPE_RX_POLL:
> > > Check if POLLIN function work as expect.
> > >
> > > 2. TEST_TYPE_TX_POLL:
> > > Check if POLLOUT function work as expect.
> > 
> > From run_pkt_test, I don't see any difference between 1 and 2. Why split
> > then?
> > 
> 
> 
> It was done to show which case exactly broke. If RX poll event or TX poll event
> 
> > >
> > > 3. TEST_TYPE_POLL_RXQ_EMPTY:
> > 
> > 3 and 4 don't match with the code here (TEST_TYPE_POLL_{R,T}XQ_TMOUT)
> > 
> > > call poll function with parameter POLLIN on empty rx queue will cause
> > > timeout.If return timeout then test case is pass.
> > >
> 
> 
> True but  It was change to RXQ_EMPTY and TXQ_FULL from _TMOUT to
> make it more clearer to what exactly is happening to cause timeout.
> 
> > > 4. TEST_TYPE_POLL_TXQ_FULL:
> > > When txq is filled and packets are not cleaned by the kernel then if
> > > we invoke the poll function with POLLOUT then it should trigger
> > > timeout.If return timeout then test case is pass.
> > >
> > > Signed-off-by: Shibin Koikkara Reeny <shibin.koikkara.reeny@intel.com>
> > > ---
> > >  tools/testing/selftests/bpf/xskxceiver.c | 173
> > > +++++++++++++++++------  tools/testing/selftests/bpf/xskxceiver.h |
> > > 10 +-
> > >  2 files changed, 139 insertions(+), 44 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/xskxceiver.c
> > > b/tools/testing/selftests/bpf/xskxceiver.c
> > > index 74d56d971baf..8ecab3a47c9e 100644
> > > --- a/tools/testing/selftests/bpf/xskxceiver.c
> > > +++ b/tools/testing/selftests/bpf/xskxceiver.c
> > > @@ -424,6 +424,8 @@ static void __test_spec_init(struct test_spec
> > > *test, struct ifobject *ifobj_tx,
> > >
> > >  		ifobj->xsk = &ifobj->xsk_arr[0];
> > >  		ifobj->use_poll = false;
> > > +		ifobj->skip_rx = false;
> > > +		ifobj->skip_tx = false;
> > 
> > Any chances of trying to avoid these booleans? Not that it's a hard nack, but
> > the less booleans we spread around in this code the better.
> 
> 
> Not sure if it is possible but using any other logic will make
> the code more complex and less readable.

How did you come with such judgement? You didn't even try the idea that I
gave to you about having a testapp_validate_traffic() equivalent with a
single thread.

> 
> > 
> > >  		ifobj->use_fill_ring = true;
> > >  		ifobj->release_rx = true;
> > >  		ifobj->pkt_stream = test->pkt_stream_default; @@ -589,6
> > +591,19 @@
> > > static struct pkt_stream *pkt_stream_clone(struct xsk_umem_info
> > *umem,
> > >  	return pkt_stream_generate(umem, pkt_stream->nb_pkts,
> > > pkt_stream->pkts[0].len);  }
> > >
> > > +static void pkt_stream_invalid(struct test_spec *test, u32 nb_pkts,
> > > +u32 pkt_len) {
> > > +	struct pkt_stream *pkt_stream;
> > > +	u32 i;
> > > +
> > > +	pkt_stream = pkt_stream_generate(test->ifobj_tx->umem,
> > nb_pkts, pkt_len);
> > > +	for (i = 0; i < nb_pkts; i++)
> > > +		pkt_stream->pkts[i].valid = false;
> > > +
> > > +	test->ifobj_tx->pkt_stream = pkt_stream;
> > > +	test->ifobj_rx->pkt_stream = pkt_stream; }
> > 
> > Please explain how this work, e.g. why you need to have to have invalid pkt
> > stream + avoiding launching rx thread and why one of them is not enough.
> > 
> > Personally I think this is not needed. When calling pkt_stream_generate(),
> > validity of pkt is set based on length of packet vs frame size:
> > 
> > 		if (pkt_len > umem->frame_size)
> > 			pkt_stream->pkts[i].valid = false;
> > 
> > so couldn't you use 2k frame size and bigger length of a packet?
> > 
> This function was introduced for TEST_TYPE_POLL_TXQ_FULL keep
> the TX full and stop nofying the kernel that there is packet to cleanup.
> So we are manually setting the packets to invalid. This help to keep
> the __send_pkts() more generic and reduce the if conditions.
> ex: xsk_ring_prod__submit() is not needed to be added inside if condition.

I understand the intend behind it but what I was saying was that you have
everything ready to be used without a need for introducing new functions.
You could also try out what I suggested just to see if this makes things
simpler.

> 
> You are right we don't need rx stream but thought it will be good
> to keep as can be used for other features in future and will be more generic.

If there are other features that would utilize this then let's introduce
this then ;)

> 
> > > +
> > >  static void pkt_stream_replace(struct test_spec *test, u32 nb_pkts,
> > > u32 pkt_len)  {
> > >  	struct pkt_stream *pkt_stream;
> > > @@ -817,9 +832,9 @@ static int complete_pkts(struct xsk_socket_info
> > *xsk, int batch_size)
> > >  	return TEST_PASS;
> > >  }
> > >
> > > -static int receive_pkts(struct ifobject *ifobj, struct pollfd *fds)
> > > +static int receive_pkts(struct ifobject *ifobj, struct pollfd *fds,
> > > +bool skip_tx)
> > >  {
> > > -	struct timeval tv_end, tv_now, tv_timeout = {RECV_TMOUT, 0};
> > > +	struct timeval tv_end, tv_now, tv_timeout = {THREAD_TMOUT, 0};
> > >  	u32 idx_rx = 0, idx_fq = 0, rcvd, i, pkts_sent = 0;
> > >  	struct pkt_stream *pkt_stream = ifobj->pkt_stream;
> > >  	struct xsk_socket_info *xsk = ifobj->xsk; @@ -843,17 +858,28 @@
> > > static int receive_pkts(struct ifobject *ifobj, struct pollfd *fds)
> > >  		}
> > >
> > >  		kick_rx(xsk);
> > > +		if (ifobj->use_poll) {
> > > +			ret = poll(fds, 1, POLL_TMOUT);
> > > +			if (ret < 0)
> > > +				exit_with_error(-ret);
> > > +
> > > +			if (!ret) {
> > > +				if (skip_tx)
> > > +					return TEST_PASS;
> > > +
> > > +				ksft_print_msg("ERROR: [%s] Poll timed
> > out\n", __func__);
> > > +				return TEST_FAILURE;
> > >
> > > -		rcvd = xsk_ring_cons__peek(&xsk->rx, BATCH_SIZE,
> > &idx_rx);
> > > -		if (!rcvd) {
> > > -			if (xsk_ring_prod__needs_wakeup(&umem->fq)) {
> > 
> > So now we don't check if fq needs to be woken up in non-poll case?
> > I believe this is still needed so we get to the driver and pick fq entries. Prove
> > me wrong of course if I'm missing something.
> 
> xsk_ring_prod__needs_wakeup() ==>  *r->flags & XDP_RING_NEED_WAKEUP;
> This function only check if the flag is set or not and it is not updating or
> triggering anything. In the original case if flag is set then trigger the 
> poll event and continue.
> In this patch poll event is called in any case if it enter the if (!rcvd)  is true..
> We don't check if XDP_RING_NEED_WAKEUP is set or not.
> 	
> 
> > 
> > > -				ret = poll(fds, 1, POLL_TMOUT);
> > > -				if (ret < 0)
> > > -					exit_with_error(-ret);
> > >  			}
> > > -			continue;
> > > +
> > > +			if (!(fds->revents & POLLIN))
> > > +				continue;
> > >  		}
> > >
> > > +		rcvd = xsk_ring_cons__peek(&xsk->rx, BATCH_SIZE,
> > &idx_rx);
> > > +		if (!rcvd)
> > > +			continue;
> > > +
> > >  		if (ifobj->use_fill_ring) {
> > >  			ret = xsk_ring_prod__reserve(&umem->fq, rcvd,
> > &idx_fq);
> > >  			while (ret != rcvd) {
> > > @@ -863,6 +889,7 @@ static int receive_pkts(struct ifobject *ifobj, struct
> > pollfd *fds)
> > >  					ret = poll(fds, 1, POLL_TMOUT);
> > >  					if (ret < 0)
> > >  						exit_with_error(-ret);
> > > +					continue;
> > 
> > Why continue here?
> 
> You are right it is not needed. Will update in V2 patch. Thanks.
> 
> > 
> > >  				}
> > >  				ret = xsk_ring_prod__reserve(&umem->fq,
> > rcvd, &idx_fq);
> > >  			}
> > > @@ -900,13 +927,34 @@ static int receive_pkts(struct ifobject *ifobj, struct
> > pollfd *fds)
> > >  	return TEST_PASS;
> > >  }
> > >
> > > -static int __send_pkts(struct ifobject *ifobject, u32 *pkt_nb)
> > > +static int __send_pkts(struct ifobject *ifobject, u32 *pkt_nb, bool
> > use_poll,
> > > +		       struct pollfd *fds, bool timeout)
> > >  {
> > >  	struct xsk_socket_info *xsk = ifobject->xsk;
> > > -	u32 i, idx, valid_pkts = 0;
> > > +	u32 i, idx, ret, valid_pkts = 0;
> > > +
> > > +	while (xsk_ring_prod__reserve(&xsk->tx, BATCH_SIZE, &idx) <
> > BATCH_SIZE) {
> > > +		if (use_poll) {
> > > +			ret = poll(fds, 1, POLL_TMOUT);
> > > +			if (timeout) {
> > > +				if (ret < 0) {
> > > +					ksft_print_msg("DEBUG: [%s] Poll
> > error %d\n",
> > > +						       __func__, ret);
> > > +					return TEST_FAILURE;
> > > +				}
> > > +				if (ret == 0)
> > > +					return TEST_PASS;
> > > +				break;
> > > +			}
> > > +			if (ret <= 0) {
> > > +				ksft_print_msg("DEBUG: [%s] Poll error
> > %d\n",
> > > +					       __func__, ret);
> > > +				return TEST_FAILURE;
> > > +			}
> > > +		}
> > >
> > > -	while (xsk_ring_prod__reserve(&xsk->tx, BATCH_SIZE, &idx) <
> > BATCH_SIZE)
> > >  		complete_pkts(xsk, BATCH_SIZE);
> > > +	}
> > >
> > >  	for (i = 0; i < BATCH_SIZE; i++) {
> > >  		struct xdp_desc *tx_desc = xsk_ring_prod__tx_desc(&xsk-
> > >tx, idx +
> > > i); @@ -933,11 +981,27 @@ static int __send_pkts(struct ifobject
> > > *ifobject, u32 *pkt_nb)
> > >
> > >  	xsk_ring_prod__submit(&xsk->tx, i);
> > >  	xsk->outstanding_tx += valid_pkts;
> > > -	if (complete_pkts(xsk, i))
> > > -		return TEST_FAILURE;
> > >
> > > -	usleep(10);
> > > -	return TEST_PASS;
> > > +	if (use_poll) {
> > > +		ret = poll(fds, 1, POLL_TMOUT);
> > > +		if (ret <= 0) {
> > > +			if (ret == 0 && timeout)
> > > +				return TEST_PASS;
> > > +
> > > +			ksft_print_msg("DEBUG: [%s] Poll error %d\n",

avoid debug prints in upstream patches

> > __func__, ret);
> > > +			return TEST_FAILURE;
> > > +		}
> > > +	}
> > > +
> > > +	if (!timeout) {
> > > +		if (complete_pkts(xsk, i))
> > > +			return TEST_FAILURE;
> > > +
> > > +		usleep(10);
> > > +		return TEST_PASS;
> > > +	}
> > > +
> > > +	return TEST_CONTINUE;
> > 
> > Why do you need this?
> > 
> 
> __send_pkts is expected to return TEST_PASS or TEST_FAIL to send_pkts function and
> if returned TEST_PASS then continue sending pkts and exit when all the packet are finished.
> if returned TEST_FAILURE then test failed and return.
> 
> For TEST_TYPE_POLL_TXQ_TMOUT  TEST_PASS is return value when timout happened and
> should not sent anymore packets and break. But this will break other test. So needed 
> new return type TEST_CONTINUE to keep sending packets.
> 
> > >  }
> > >
> > >  static void wait_for_tx_completion(struct xsk_socket_info *xsk) @@
> > > -948,29 +1012,33 @@ static void wait_for_tx_completion(struct
> > > xsk_socket_info *xsk)
> > >
> > >  static int send_pkts(struct test_spec *test, struct ifobject
> > > *ifobject)  {
> > > +	struct timeval tv_end, tv_now, tv_timeout = {THREAD_TMOUT, 0};
> > > +	bool timeout = test->ifobj_rx->skip_rx;
> > >  	struct pollfd fds = { };
> > > -	u32 pkt_cnt = 0;
> > > +	u32 pkt_cnt = 0, ret;
> > >
> > >  	fds.fd = xsk_socket__fd(ifobject->xsk->xsk);
> > >  	fds.events = POLLOUT;
> > >
> > > -	while (pkt_cnt < ifobject->pkt_stream->nb_pkts) {
> > > -		int err;
> > > -
> > > -		if (ifobject->use_poll) {
> > > -			int ret;
> > > -
> > > -			ret = poll(&fds, 1, POLL_TMOUT);
> > > -			if (ret <= 0)
> > > -				continue;
> > > +	ret = gettimeofday(&tv_now, NULL);
> > > +	if (ret)
> > > +		exit_with_error(errno);
> > > +	timeradd(&tv_now, &tv_timeout, &tv_end);
> > 
> > This logic of timer on Tx side is not mentioned anywhere in the commit
> > message. Please try your best to describe all of the changes you're
> > proposing.
> > 
> 
> Will update in the commit message in V2 patch.
> 
> > Also, couldn't this be a separate patch?
> > 
> I prefer to keep it. But if you suggest otherwise I can remove.

I'm not talking about removing this altogether, pulling this out to
separate patch would make this one cleaner and reviewers job easier.

> 
> > >
> > > -			if (!(fds.revents & POLLOUT))
> > > -				continue;
> > > +	while (pkt_cnt < ifobject->pkt_stream->nb_pkts) {
> > > +		ret = gettimeofday(&tv_now, NULL);
> > > +		if (ret)
> > > +			exit_with_error(errno);
> > > +		if (timercmp(&tv_now, &tv_end, >)) {
> > > +			ksft_print_msg("ERROR: [%s] Send loop timed
> > out\n", __func__);
> > > +			return TEST_FAILURE;
> > >  		}
> > >
> > > -		err = __send_pkts(ifobject, &pkt_cnt);
> > > -		if (err || test->fail)
> > > +		ret = __send_pkts(ifobject, &pkt_cnt, ifobject->use_poll,
> > &fds, timeout);
> > > +		if ((ret || test->fail) && !timeout)
> > >  			return TEST_FAILURE;
> > > +		else if (ret == TEST_PASS && timeout)
> > > +			return ret;
> > >  	}
> > >
> > >  	wait_for_tx_completion(ifobject->xsk);
> > > @@ -1235,8 +1303,7 @@ static void *worker_testapp_validate_rx(void
> > > *arg)
> > >
> > >  	pthread_barrier_wait(&barr);
> > >
> > > -	err = receive_pkts(ifobject, &fds);
> > > -
> > > +	err = receive_pkts(ifobject, &fds, test->ifobj_tx->skip_tx);
> > >  	if (!err && ifobject->validation_func)
> > >  		err = ifobject->validation_func(ifobject);
> > >  	if (err) {
> > > @@ -1265,17 +1332,21 @@ static int testapp_validate_traffic(struct
> > test_spec *test)
> > >  	pkts_in_flight = 0;
> > >
> > >  	/*Spawn RX thread */
> > > -	pthread_create(&t0, NULL, ifobj_rx->func_ptr, test);
> > > -
> > > -	pthread_barrier_wait(&barr);
> > > -	if (pthread_barrier_destroy(&barr))
> > > -		exit_with_error(errno);
> > > +	if (!ifobj_rx->skip_rx) {
> > > +		pthread_create(&t0, NULL, ifobj_rx->func_ptr, test);
> > > +		pthread_barrier_wait(&barr);
> > > +		if (pthread_barrier_destroy(&barr))
> > > +			exit_with_error(errno);
> > > +	}
> > >
> > >  	/*Spawn TX thread */
> > > -	pthread_create(&t1, NULL, ifobj_tx->func_ptr, test);
> > > +	if (!ifobj_tx->skip_tx) {
> > > +		pthread_create(&t1, NULL, ifobj_tx->func_ptr, test);
> > > +		pthread_join(t1, NULL);
> > > +	}
> > >
> > > -	pthread_join(t1, NULL);
> > > -	pthread_join(t0, NULL);
> > > +	if (!ifobj_rx->skip_rx)
> > > +		pthread_join(t0, NULL);
> > 
> > Have you thought of a testapp_validate_traffic() variant with a single thread,
> > either Tx or Rx? In this case probably would make everything clearer in the
> > current pthread code. Also, wouldn't this drop the need for skip booleans?
> > 
> 
> My suggestion will be to reuse the existing functions. If you suggest otherwise
> I can look into it.

Existing function wasn't designed for single thread execution which you
need for your poll test cases. That's why I asked you to discover if
having a function designed for single threaded tests is worth the hassle.

> 
> > >
> > >  	return !!test->fail;
> > >  }
> > > @@ -1548,10 +1619,28 @@ static void run_pkt_test(struct test_spec
> > > *test, enum test_mode mode, enum test_
> > >
> > >  		pkt_stream_restore_default(test);
> > >  		break;
> > > -	case TEST_TYPE_POLL:
> > > +	case TEST_TYPE_RX_POLL:
> > > +		test->ifobj_rx->use_poll = true;
> > > +		test_spec_set_name(test, "POLL_RX");
> > > +		testapp_validate_traffic(test);
> > > +		break;
> > > +	case TEST_TYPE_TX_POLL:
> > >  		test->ifobj_tx->use_poll = true;
> > > +		test_spec_set_name(test, "POLL_TX");
> > > +		testapp_validate_traffic(test);
> > > +		break;
> > > +	case TEST_TYPE_POLL_TXQ_TMOUT:
> > > +		test_spec_set_name(test, "POLL_TXQ_FULL");
> > > +		test->ifobj_rx->skip_rx = true;
> > > +		test->ifobj_tx->use_poll = true;
> > > +		pkt_stream_invalid(test, 2 * DEFAULT_PKT_CNT, PKT_SIZE);
> > > +		testapp_validate_traffic(test);
> > > +		pkt_stream_restore_default(test);
> > > +		break;
> > > +	case TEST_TYPE_POLL_RXQ_TMOUT:
> > > +		test_spec_set_name(test, "POLL_RXQ_EMPTY");
> > > +		test->ifobj_tx->skip_tx = true;
> > >  		test->ifobj_rx->use_poll = true;
> > > -		test_spec_set_name(test, "POLL");
> > >  		testapp_validate_traffic(test);
> > >  		break;
> > >  	case TEST_TYPE_ALIGNED_INV_DESC:
> > > diff --git a/tools/testing/selftests/bpf/xskxceiver.h
> > > b/tools/testing/selftests/bpf/xskxceiver.h
> > > index 3d17053f98e5..0db7e0acccb2 100644
> > > --- a/tools/testing/selftests/bpf/xskxceiver.h
> > > +++ b/tools/testing/selftests/bpf/xskxceiver.h
> > > @@ -27,6 +27,7 @@
> > >
> > >  #define TEST_PASS 0
> > >  #define TEST_FAILURE -1
> > > +#define TEST_CONTINUE 1
> > >  #define MAX_INTERFACES 2
> > >  #define MAX_INTERFACE_NAME_CHARS 7
> > >  #define MAX_INTERFACES_NAMESPACE_CHARS 10 @@ -48,7 +49,7 @@
> > #define
> > > SOCK_RECONF_CTR 10  #define BATCH_SIZE 64  #define POLL_TMOUT
> > 1000
> > > -#define RECV_TMOUT 3
> > > +#define THREAD_TMOUT 3
> > >  #define DEFAULT_PKT_CNT (4 * 1024)
> > >  #define DEFAULT_UMEM_BUFFERS (DEFAULT_PKT_CNT / 4)  #define
> > UMEM_SIZE
> > > (DEFAULT_UMEM_BUFFERS * XSK_UMEM__DEFAULT_FRAME_SIZE) @@ -
> > 68,7 +69,10
> > > @@ enum test_type {
> > >  	TEST_TYPE_RUN_TO_COMPLETION,
> > >  	TEST_TYPE_RUN_TO_COMPLETION_2K_FRAME,
> > >  	TEST_TYPE_RUN_TO_COMPLETION_SINGLE_PKT,
> > > -	TEST_TYPE_POLL,
> > > +	TEST_TYPE_RX_POLL,
> > > +	TEST_TYPE_TX_POLL,
> > > +	TEST_TYPE_POLL_RXQ_TMOUT,
> > > +	TEST_TYPE_POLL_TXQ_TMOUT,
> > >  	TEST_TYPE_UNALIGNED,
> > >  	TEST_TYPE_ALIGNED_INV_DESC,
> > >  	TEST_TYPE_ALIGNED_INV_DESC_2K_FRAME,
> > > @@ -145,6 +149,8 @@ struct ifobject {
> > >  	bool tx_on;
> > >  	bool rx_on;
> > >  	bool use_poll;
> > > +	bool skip_rx;
> > > +	bool skip_tx;
> > >  	bool busy_poll;
> > >  	bool use_fill_ring;
> > >  	bool release_rx;
> > > --
> > > 2.34.1
> > >

RE: [PATCH bpf-next] selftests: xsk: Update poll test cases

[Koikkara Reeny, Shibin]

> -----Original Message-----
> From: Fijalkowski, Maciej <maciej.fijalkowski@intel.com>
> Sent: Tuesday, July 26, 2022 3:10 PM
> To: Koikkara Reeny, Shibin <shibin.koikkara.reeny@intel.com>
> Cc: bpf@vger.kernel.org; ast@kernel.org; daniel@iogearbox.net;
> netdev@vger.kernel.org; Karlsson, Magnus <magnus.karlsson@intel.com>;
> bjorn@kernel.org; kuba@kernel.org; andrii@kernel.org; Loftus, Ciara
> <ciara.loftus@intel.com>
> Subject: Re: [PATCH bpf-next] selftests: xsk: Update poll test cases
> 
> On Tue, Jul 26, 2022 at 10:43:36AM +0100, Koikkara Reeny, Shibin wrote:
> > > -----Original Message-----
> > > From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > > Sent: Friday, July 22, 2022 3:16 PM
> > > To: Koikkara Reeny, Shibin <shibin.koikkara.reeny@intel.com>
> > > Cc: bpf@vger.kernel.org; ast@kernel.org; daniel@iogearbox.net;
> > > netdev@vger.kernel.org; Karlsson, Magnus
> > > <magnus.karlsson@intel.com>; bjorn@kernel.org; kuba@kernel.org;
> > > andrii@kernel.org; Loftus, Ciara <ciara.loftus@intel.com>
> > > Subject: Re: [PATCH bpf-next] selftests: xsk: Update poll test cases
> > >
> > > On Mon, Jul 18, 2022 at 09:57:12AM +0000, Shibin Koikkara Reeny wrote:
> > > > Poll test case was not testing all the functionality of the poll
> > > > feature in the testsuite. This patch update the poll test case
> > > > with 2 more testcases to check the timeout features.
> > > >
> > > > Poll test case have 4 sub test cases:
> > >
> > > Hi Shibin,
> > >
> > > Kinda not clear with count of added test cases, at first you say you
> > > add 2 more but then you mention something about 4 sub test cases.
> > >
> > > To me these are separate test cases.
> > >
> > Hi Maciej,
> >
> > Will update it in V2
> >
> > > >
> > > > 1. TEST_TYPE_RX_POLL:
> > > > Check if POLLIN function work as expect.
> > > >
> > > > 2. TEST_TYPE_TX_POLL:
> > > > Check if POLLOUT function work as expect.
> > >
> > > From run_pkt_test, I don't see any difference between 1 and 2. Why
> > > split then?
> > >
> >
> >
> > It was done to show which case exactly broke. If RX poll event or TX
> > poll event
> >
> > > >
> > > > 3. TEST_TYPE_POLL_RXQ_EMPTY:
> > >
> > > 3 and 4 don't match with the code here
> > > (TEST_TYPE_POLL_{R,T}XQ_TMOUT)
> > >
> > > > call poll function with parameter POLLIN on empty rx queue will
> > > > cause timeout.If return timeout then test case is pass.
> > > >
> >
> >
> > True but  It was change to RXQ_EMPTY and TXQ_FULL from _TMOUT to
> make
> > it more clearer to what exactly is happening to cause timeout.
> >
> > > > 4. TEST_TYPE_POLL_TXQ_FULL:
> > > > When txq is filled and packets are not cleaned by the kernel then
> > > > if we invoke the poll function with POLLOUT then it should trigger
> > > > timeout.If return timeout then test case is pass.
> > > >
> > > > Signed-off-by: Shibin Koikkara Reeny
> > > > <shibin.koikkara.reeny@intel.com>
> > > > ---
> > > >  tools/testing/selftests/bpf/xskxceiver.c | 173
> > > > +++++++++++++++++------  tools/testing/selftests/bpf/xskxceiver.h
> > > > +++++++++++++++++|
> > > > 10 +-
> > > >  2 files changed, 139 insertions(+), 44 deletions(-)
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/xskxceiver.c
> > > > b/tools/testing/selftests/bpf/xskxceiver.c
> > > > index 74d56d971baf..8ecab3a47c9e 100644
> > > > --- a/tools/testing/selftests/bpf/xskxceiver.c
> > > > +++ b/tools/testing/selftests/bpf/xskxceiver.c
> > > > @@ -424,6 +424,8 @@ static void __test_spec_init(struct test_spec
> > > > *test, struct ifobject *ifobj_tx,
> > > >
> > > >  		ifobj->xsk = &ifobj->xsk_arr[0];
> > > >  		ifobj->use_poll = false;
> > > > +		ifobj->skip_rx = false;
> > > > +		ifobj->skip_tx = false;
> > >
> > > Any chances of trying to avoid these booleans? Not that it's a hard
> > > nack, but the less booleans we spread around in this code the better.
> >
> >
> > Not sure if it is possible but using any other logic will make the
> > code more complex and less readable.
> 
> How did you come with such judgement? You didn't even try the idea that I
> gave to you about having a testapp_validate_traffic() equivalent with a single
> thread.
> 

Hi Maciej,

diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
index 4394788829bf..0b58e026f2a2 100644
--- a/tools/testing/selftests/bpf/xskxceiver.c
+++ b/tools/testing/selftests/bpf/xskxceiver.c
@@ -1317,6 +1317,24 @@ static void *worker_testapp_validate_rx(void *arg)
        pthread_exit(NULL);
 }

+static int testapp_validate_traffic_txq_tmout(struct test_spec *test)
+{
+       struct ifobject *ifobj_tx = test->ifobj_tx;
+       pthread_t t0;
+
+       if (pthread_barrier_init(&barr, NULL, 2))
+               exit_with_error(errno);
+
+       test->current_step++;
+       pkt_stream_reset(ifobj_rx->pkt_stream);
+
+       pthread_create(&t0, NULL, ifobj_tx->func_ptr, test);
+       pthread_join(t0, NULL);
+
+       return !!test->fail;
+}
+

This is what you are suggesting do ?

My point is ifobj_tx->func_ptr calls worker_testapp_validate_tx() ==> send_pkts() ==> __send_pkts().

Normal case when poll timeout happen send_pkts() return TEST_FAILURE which is expected.
Test Case like TEST_TYPE_POLL_TXQ_TMOUT and TEST_TYPE_POLL_RXQ_TMOUT when poll timeout happen
it should return TEST_PASS rather than TEST_FAILURE. How should I let the send_pkts()
to know what timeout type of test is running without a new variable or flag? 
Then boolean skip_rx and skip_tx are both used in the send_pkts() and receive_pkts().

This is why I thought it might be complex but if you have new suggestion I open to try it.

> >
> > >
> > > >  		ifobj->use_fill_ring = true;
> > > >  		ifobj->release_rx = true;
> > > >  		ifobj->pkt_stream = test->pkt_stream_default; @@ -589,6
> > > +591,19 @@
> > > > static struct pkt_stream *pkt_stream_clone(struct xsk_umem_info
> > > *umem,
> > > >  	return pkt_stream_generate(umem, pkt_stream->nb_pkts,
> > > > pkt_stream->pkts[0].len);  }
> > > >
> > > > +static void pkt_stream_invalid(struct test_spec *test, u32
> > > > +nb_pkts,
> > > > +u32 pkt_len) {
> > > > +	struct pkt_stream *pkt_stream;
> > > > +	u32 i;
> > > > +
> > > > +	pkt_stream = pkt_stream_generate(test->ifobj_tx->umem,
> > > nb_pkts, pkt_len);
> > > > +	for (i = 0; i < nb_pkts; i++)
> > > > +		pkt_stream->pkts[i].valid = false;
> > > > +
> > > > +	test->ifobj_tx->pkt_stream = pkt_stream;
> > > > +	test->ifobj_rx->pkt_stream = pkt_stream; }
> > >
> > > Please explain how this work, e.g. why you need to have to have
> > > invalid pkt stream + avoiding launching rx thread and why one of them is
> not enough.
> > >
> > > Personally I think this is not needed. When calling
> > > pkt_stream_generate(), validity of pkt is set based on length of packet vs
> frame size:
> > >
> > > 		if (pkt_len > umem->frame_size)
> > > 			pkt_stream->pkts[i].valid = false;
> > >
> > > so couldn't you use 2k frame size and bigger length of a packet?
> > >
> > This function was introduced for TEST_TYPE_POLL_TXQ_FULL keep the TX
> > full and stop nofying the kernel that there is packet to cleanup.
> > So we are manually setting the packets to invalid. This help to keep
> > the __send_pkts() more generic and reduce the if conditions.
> > ex: xsk_ring_prod__submit() is not needed to be added inside if condition.
> 
> I understand the intend behind it but what I was saying was that you have
> everything ready to be used without a need for introducing new functions.
> You could also try out what I suggested just to see if this makes things
> simpler.
> 

Are you suggesting to do this ?
                test->ifobj_tx->use_poll = true;
-               pkt_stream_invalid(test, 2 * DEFAULT_PKT_CNT, PKT_SIZE);
+               test->ifobj_tx->umem->frame_size = 2048;
+               pkt_stream_replace(test, 2 * DEFAULT_PKT_CNT, 2048);
                testapp_validate_traffic(test);


> >
> > You are right we don't need rx stream but thought it will be good to
> > keep as can be used for other features in future and will be more generic.
> 
> If there are other features that would utilize this then let's introduce this
> then ;)
> 

Got it.

> >
> > > > +
> > > >  static void pkt_stream_replace(struct test_spec *test, u32
> > > > nb_pkts,
> > > > u32 pkt_len)  {
> > > >  	struct pkt_stream *pkt_stream;
> > > > @@ -817,9 +832,9 @@ static int complete_pkts(struct
> > > > xsk_socket_info
> > > *xsk, int batch_size)
> > > >  	return TEST_PASS;
> > > >  }
> > > >
> > > > -static int receive_pkts(struct ifobject *ifobj, struct pollfd
> > > > *fds)
> > > > +static int receive_pkts(struct ifobject *ifobj, struct pollfd
> > > > +*fds, bool skip_tx)
> > > >  {
> > > > -	struct timeval tv_end, tv_now, tv_timeout = {RECV_TMOUT, 0};
> > > > +	struct timeval tv_end, tv_now, tv_timeout = {THREAD_TMOUT, 0};
> > > >  	u32 idx_rx = 0, idx_fq = 0, rcvd, i, pkts_sent = 0;
> > > >  	struct pkt_stream *pkt_stream = ifobj->pkt_stream;
> > > >  	struct xsk_socket_info *xsk = ifobj->xsk; @@ -843,17 +858,28 @@
> > > > static int receive_pkts(struct ifobject *ifobj, struct pollfd *fds)
> > > >  		}
> > > >
> > > >  		kick_rx(xsk);
> > > > +		if (ifobj->use_poll) {
> > > > +			ret = poll(fds, 1, POLL_TMOUT);
> > > > +			if (ret < 0)
> > > > +				exit_with_error(-ret);
> > > > +
> > > > +			if (!ret) {
> > > > +				if (skip_tx)
> > > > +					return TEST_PASS;
> > > > +
> > > > +				ksft_print_msg("ERROR: [%s] Poll timed
> > > out\n", __func__);
> > > > +				return TEST_FAILURE;
> > > >
> > > > -		rcvd = xsk_ring_cons__peek(&xsk->rx, BATCH_SIZE,
> > > &idx_rx);
> > > > -		if (!rcvd) {
> > > > -			if (xsk_ring_prod__needs_wakeup(&umem->fq)) {
> > >
> > > So now we don't check if fq needs to be woken up in non-poll case?
> > > I believe this is still needed so we get to the driver and pick fq
> > > entries. Prove me wrong of course if I'm missing something.
> >
> > xsk_ring_prod__needs_wakeup() ==>  *r->flags &
> XDP_RING_NEED_WAKEUP;
> > This function only check if the flag is set or not and it is not
> > updating or triggering anything. In the original case if flag is set
> > then trigger the poll event and continue.
> > In this patch poll event is called in any case if it enter the if (!rcvd)  is true..
> > We don't check if XDP_RING_NEED_WAKEUP is set or not.
> >
> >
> > >
> > > > -				ret = poll(fds, 1, POLL_TMOUT);
> > > > -				if (ret < 0)
> > > > -					exit_with_error(-ret);
> > > >  			}
> > > > -			continue;
> > > > +
> > > > +			if (!(fds->revents & POLLIN))
> > > > +				continue;
> > > >  		}
> > > >
> > > > +		rcvd = xsk_ring_cons__peek(&xsk->rx, BATCH_SIZE,
> > > &idx_rx);
> > > > +		if (!rcvd)
> > > > +			continue;
> > > > +
> > > >  		if (ifobj->use_fill_ring) {
> > > >  			ret = xsk_ring_prod__reserve(&umem->fq, rcvd,
> > > &idx_fq);
> > > >  			while (ret != rcvd) {
> > > > @@ -863,6 +889,7 @@ static int receive_pkts(struct ifobject
> > > > *ifobj, struct
> > > pollfd *fds)
> > > >  					ret = poll(fds, 1, POLL_TMOUT);
> > > >  					if (ret < 0)
> > > >  						exit_with_error(-ret);
> > > > +					continue;
> > >
> > > Why continue here?
> >
> > You are right it is not needed. Will update in V2 patch. Thanks.
> >
> > >
> > > >  				}
> > > >  				ret = xsk_ring_prod__reserve(&umem->fq,
> > > rcvd, &idx_fq);
> > > >  			}
> > > > @@ -900,13 +927,34 @@ static int receive_pkts(struct ifobject
> > > > *ifobj, struct
> > > pollfd *fds)
> > > >  	return TEST_PASS;
> > > >  }
> > > >
> > > > -static int __send_pkts(struct ifobject *ifobject, u32 *pkt_nb)
> > > > +static int __send_pkts(struct ifobject *ifobject, u32 *pkt_nb,
> > > > +bool
> > > use_poll,
> > > > +		       struct pollfd *fds, bool timeout)
> > > >  {
> > > >  	struct xsk_socket_info *xsk = ifobject->xsk;
> > > > -	u32 i, idx, valid_pkts = 0;
> > > > +	u32 i, idx, ret, valid_pkts = 0;
> > > > +
> > > > +	while (xsk_ring_prod__reserve(&xsk->tx, BATCH_SIZE, &idx) <
> > > BATCH_SIZE) {
> > > > +		if (use_poll) {
> > > > +			ret = poll(fds, 1, POLL_TMOUT);
> > > > +			if (timeout) {
> > > > +				if (ret < 0) {
> > > > +					ksft_print_msg("DEBUG: [%s] Poll
> > > error %d\n",
> > > > +						       __func__, ret);
> > > > +					return TEST_FAILURE;
> > > > +				}
> > > > +				if (ret == 0)
> > > > +					return TEST_PASS;
> > > > +				break;
> > > > +			}
> > > > +			if (ret <= 0) {
> > > > +				ksft_print_msg("DEBUG: [%s] Poll error
> > > %d\n",
> > > > +					       __func__, ret);
> > > > +				return TEST_FAILURE;
> > > > +			}
> > > > +		}
> > > >
> > > > -	while (xsk_ring_prod__reserve(&xsk->tx, BATCH_SIZE, &idx) <
> > > BATCH_SIZE)
> > > >  		complete_pkts(xsk, BATCH_SIZE);
> > > > +	}
> > > >
> > > >  	for (i = 0; i < BATCH_SIZE; i++) {
> > > >  		struct xdp_desc *tx_desc = xsk_ring_prod__tx_desc(&xsk-
> tx, idx
> > > >+  i); @@ -933,11 +981,27 @@ static int __send_pkts(struct ifobject
> > > >*ifobject, u32 *pkt_nb)
> > > >
> > > >  	xsk_ring_prod__submit(&xsk->tx, i);
> > > >  	xsk->outstanding_tx += valid_pkts;
> > > > -	if (complete_pkts(xsk, i))
> > > > -		return TEST_FAILURE;
> > > >
> > > > -	usleep(10);
> > > > -	return TEST_PASS;
> > > > +	if (use_poll) {
> > > > +		ret = poll(fds, 1, POLL_TMOUT);
> > > > +		if (ret <= 0) {
> > > > +			if (ret == 0 && timeout)
> > > > +				return TEST_PASS;
> > > > +
> > > > +			ksft_print_msg("DEBUG: [%s] Poll error %d\n",
> 
> avoid debug prints in upstream patches

I can remove it or I can replace DEBUG as ERROR. What do you suggest? 


> 
> > > __func__, ret);
> > > > +			return TEST_FAILURE;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	if (!timeout) {
> > > > +		if (complete_pkts(xsk, i))
> > > > +			return TEST_FAILURE;
> > > > +
> > > > +		usleep(10);
> > > > +		return TEST_PASS;
> > > > +	}
> > > > +
> > > > +	return TEST_CONTINUE;
> > >
> > > Why do you need this?
> > >
> >
> > __send_pkts is expected to return TEST_PASS or TEST_FAIL to send_pkts
> > function and if returned TEST_PASS then continue sending pkts and exit
> when all the packet are finished.
> > if returned TEST_FAILURE then test failed and return.
> >
> > For TEST_TYPE_POLL_TXQ_TMOUT  TEST_PASS is return value when timout
> > happened and should not sent anymore packets and break. But this will
> > break other test. So needed new return type TEST_CONTINUE to keep
> sending packets.
> >
> > > >  }
> > > >
> > > >  static void wait_for_tx_completion(struct xsk_socket_info *xsk)
> > > > @@
> > > > -948,29 +1012,33 @@ static void wait_for_tx_completion(struct
> > > > xsk_socket_info *xsk)
> > > >
> > > >  static int send_pkts(struct test_spec *test, struct ifobject
> > > > *ifobject)  {
> > > > +	struct timeval tv_end, tv_now, tv_timeout = {THREAD_TMOUT, 0};
> > > > +	bool timeout = test->ifobj_rx->skip_rx;
> > > >  	struct pollfd fds = { };
> > > > -	u32 pkt_cnt = 0;
> > > > +	u32 pkt_cnt = 0, ret;
> > > >
> > > >  	fds.fd = xsk_socket__fd(ifobject->xsk->xsk);
> > > >  	fds.events = POLLOUT;
> > > >
> > > > -	while (pkt_cnt < ifobject->pkt_stream->nb_pkts) {
> > > > -		int err;
> > > > -
> > > > -		if (ifobject->use_poll) {
> > > > -			int ret;
> > > > -
> > > > -			ret = poll(&fds, 1, POLL_TMOUT);
> > > > -			if (ret <= 0)
> > > > -				continue;
> > > > +	ret = gettimeofday(&tv_now, NULL);
> > > > +	if (ret)
> > > > +		exit_with_error(errno);
> > > > +	timeradd(&tv_now, &tv_timeout, &tv_end);
> > >
> > > This logic of timer on Tx side is not mentioned anywhere in the
> > > commit message. Please try your best to describe all of the changes
> > > you're proposing.
> > >
> >
> > Will update in the commit message in V2 patch.
> >
> > > Also, couldn't this be a separate patch?
> > >
> > I prefer to keep it. But if you suggest otherwise I can remove.
> 
> I'm not talking about removing this altogether, pulling this out to separate
> patch would make this one cleaner and reviewers job easier.
> 
Sure. I agree will update in V3 patch.

> >
> > > >
> > > > -			if (!(fds.revents & POLLOUT))
> > > > -				continue;
> > > > +	while (pkt_cnt < ifobject->pkt_stream->nb_pkts) {
> > > > +		ret = gettimeofday(&tv_now, NULL);
> > > > +		if (ret)
> > > > +			exit_with_error(errno);
> > > > +		if (timercmp(&tv_now, &tv_end, >)) {
> > > > +			ksft_print_msg("ERROR: [%s] Send loop timed
> > > out\n", __func__);
> > > > +			return TEST_FAILURE;
> > > >  		}
> > > >
> > > > -		err = __send_pkts(ifobject, &pkt_cnt);
> > > > -		if (err || test->fail)
> > > > +		ret = __send_pkts(ifobject, &pkt_cnt, ifobject->use_poll,
> > > &fds, timeout);
> > > > +		if ((ret || test->fail) && !timeout)
> > > >  			return TEST_FAILURE;
> > > > +		else if (ret == TEST_PASS && timeout)
> > > > +			return ret;
> > > >  	}
> > > >
> > > >  	wait_for_tx_completion(ifobject->xsk);
> > > > @@ -1235,8 +1303,7 @@ static void *worker_testapp_validate_rx(void
> > > > *arg)
> > > >
> > > >  	pthread_barrier_wait(&barr);
> > > >
> > > > -	err = receive_pkts(ifobject, &fds);
> > > > -
> > > > +	err = receive_pkts(ifobject, &fds, test->ifobj_tx->skip_tx);
> > > >  	if (!err && ifobject->validation_func)
> > > >  		err = ifobject->validation_func(ifobject);
> > > >  	if (err) {
> > > > @@ -1265,17 +1332,21 @@ static int testapp_validate_traffic(struct
> > > test_spec *test)
> > > >  	pkts_in_flight = 0;
> > > >
> > > >  	/*Spawn RX thread */
> > > > -	pthread_create(&t0, NULL, ifobj_rx->func_ptr, test);
> > > > -
> > > > -	pthread_barrier_wait(&barr);
> > > > -	if (pthread_barrier_destroy(&barr))
> > > > -		exit_with_error(errno);
> > > > +	if (!ifobj_rx->skip_rx) {
> > > > +		pthread_create(&t0, NULL, ifobj_rx->func_ptr, test);
> > > > +		pthread_barrier_wait(&barr);
> > > > +		if (pthread_barrier_destroy(&barr))
> > > > +			exit_with_error(errno);
> > > > +	}
> > > >
> > > >  	/*Spawn TX thread */
> > > > -	pthread_create(&t1, NULL, ifobj_tx->func_ptr, test);
> > > > +	if (!ifobj_tx->skip_tx) {
> > > > +		pthread_create(&t1, NULL, ifobj_tx->func_ptr, test);
> > > > +		pthread_join(t1, NULL);
> > > > +	}
> > > >
> > > > -	pthread_join(t1, NULL);
> > > > -	pthread_join(t0, NULL);
> > > > +	if (!ifobj_rx->skip_rx)
> > > > +		pthread_join(t0, NULL);
> > >
> > > Have you thought of a testapp_validate_traffic() variant with a
> > > single thread, either Tx or Rx? In this case probably would make
> > > everything clearer in the current pthread code. Also, wouldn't this drop
> the need for skip booleans?
> > >
> >
> > My suggestion will be to reuse the existing functions. If you suggest
> > otherwise I can look into it.
> 
> Existing function wasn't designed for single thread execution which you need
> for your poll test cases. That's why I asked you to discover if having a function
> designed for single threaded tests is worth the hassle.
> 

Still I think i will need a variable to let the send and receive function to see if the timeout is expect or an error. 
I don't know if i am missing something. I am open to the suggestion.

> >
> > > >
> > > >  	return !!test->fail;
> > > >  }
> > > > @@ -1548,10 +1619,28 @@ static void run_pkt_test(struct test_spec
> > > > *test, enum test_mode mode, enum test_
> > > >
> > > >  		pkt_stream_restore_default(test);
> > > >  		break;
> > > > -	case TEST_TYPE_POLL:
> > > > +	case TEST_TYPE_RX_POLL:
> > > > +		test->ifobj_rx->use_poll = true;
> > > > +		test_spec_set_name(test, "POLL_RX");
> > > > +		testapp_validate_traffic(test);
> > > > +		break;
> > > > +	case TEST_TYPE_TX_POLL:
> > > >  		test->ifobj_tx->use_poll = true;
> > > > +		test_spec_set_name(test, "POLL_TX");
> > > > +		testapp_validate_traffic(test);
> > > > +		break;
> > > > +	case TEST_TYPE_POLL_TXQ_TMOUT:
> > > > +		test_spec_set_name(test, "POLL_TXQ_FULL");
> > > > +		test->ifobj_rx->skip_rx = true;
> > > > +		test->ifobj_tx->use_poll = true;
> > > > +		pkt_stream_invalid(test, 2 * DEFAULT_PKT_CNT, PKT_SIZE);
> > > > +		testapp_validate_traffic(test);
> > > > +		pkt_stream_restore_default(test);
> > > > +		break;
> > > > +	case TEST_TYPE_POLL_RXQ_TMOUT:
> > > > +		test_spec_set_name(test, "POLL_RXQ_EMPTY");
> > > > +		test->ifobj_tx->skip_tx = true;
> > > >  		test->ifobj_rx->use_poll = true;
> > > > -		test_spec_set_name(test, "POLL");
> > > >  		testapp_validate_traffic(test);
> > > >  		break;
> > > >  	case TEST_TYPE_ALIGNED_INV_DESC:
> > > > diff --git a/tools/testing/selftests/bpf/xskxceiver.h
> > > > b/tools/testing/selftests/bpf/xskxceiver.h
> > > > index 3d17053f98e5..0db7e0acccb2 100644
> > > > --- a/tools/testing/selftests/bpf/xskxceiver.h
> > > > +++ b/tools/testing/selftests/bpf/xskxceiver.h
> > > > @@ -27,6 +27,7 @@
> > > >
> > > >  #define TEST_PASS 0
> > > >  #define TEST_FAILURE -1
> > > > +#define TEST_CONTINUE 1
> > > >  #define MAX_INTERFACES 2
> > > >  #define MAX_INTERFACE_NAME_CHARS 7  #define
> > > > MAX_INTERFACES_NAMESPACE_CHARS 10 @@ -48,7 +49,7 @@
> > > #define
> > > > SOCK_RECONF_CTR 10  #define BATCH_SIZE 64  #define POLL_TMOUT
> > > 1000
> > > > -#define RECV_TMOUT 3
> > > > +#define THREAD_TMOUT 3
> > > >  #define DEFAULT_PKT_CNT (4 * 1024)  #define
> DEFAULT_UMEM_BUFFERS
> > > > (DEFAULT_PKT_CNT / 4)  #define
> > > UMEM_SIZE
> > > > (DEFAULT_UMEM_BUFFERS * XSK_UMEM__DEFAULT_FRAME_SIZE)
> @@ -
> > > 68,7 +69,10
> > > > @@ enum test_type {
> > > >  	TEST_TYPE_RUN_TO_COMPLETION,
> > > >  	TEST_TYPE_RUN_TO_COMPLETION_2K_FRAME,
> > > >  	TEST_TYPE_RUN_TO_COMPLETION_SINGLE_PKT,
> > > > -	TEST_TYPE_POLL,
> > > > +	TEST_TYPE_RX_POLL,
> > > > +	TEST_TYPE_TX_POLL,
> > > > +	TEST_TYPE_POLL_RXQ_TMOUT,
> > > > +	TEST_TYPE_POLL_TXQ_TMOUT,
> > > >  	TEST_TYPE_UNALIGNED,
> > > >  	TEST_TYPE_ALIGNED_INV_DESC,
> > > >  	TEST_TYPE_ALIGNED_INV_DESC_2K_FRAME,
> > > > @@ -145,6 +149,8 @@ struct ifobject {
> > > >  	bool tx_on;
> > > >  	bool rx_on;
> > > >  	bool use_poll;
> > > > +	bool skip_rx;
> > > > +	bool skip_tx;
> > > >  	bool busy_poll;
> > > >  	bool use_fill_ring;
> > > >  	bool release_rx;
> > > > --
> > > > 2.34.1
> > > >

Re: [PATCH bpf-next] selftests: xsk: Update poll test cases

[Maciej Fijalkowski]

On Wed, Jul 27, 2022 at 11:49:57AM +0100, Koikkara Reeny, Shibin wrote:
> 
> > -----Original Message-----
> > From: Fijalkowski, Maciej <maciej.fijalkowski@intel.com>
> > Sent: Tuesday, July 26, 2022 3:10 PM
> > To: Koikkara Reeny, Shibin <shibin.koikkara.reeny@intel.com>
> > Cc: bpf@vger.kernel.org; ast@kernel.org; daniel@iogearbox.net;
> > netdev@vger.kernel.org; Karlsson, Magnus <magnus.karlsson@intel.com>;
> > bjorn@kernel.org; kuba@kernel.org; andrii@kernel.org; Loftus, Ciara
> > <ciara.loftus@intel.com>
> > Subject: Re: [PATCH bpf-next] selftests: xsk: Update poll test cases
> > 
> > On Tue, Jul 26, 2022 at 10:43:36AM +0100, Koikkara Reeny, Shibin wrote:
> > > > -----Original Message-----
> > > > From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > > > Sent: Friday, July 22, 2022 3:16 PM
> > > > To: Koikkara Reeny, Shibin <shibin.koikkara.reeny@intel.com>
> > > > Cc: bpf@vger.kernel.org; ast@kernel.org; daniel@iogearbox.net;
> > > > netdev@vger.kernel.org; Karlsson, Magnus
> > > > <magnus.karlsson@intel.com>; bjorn@kernel.org; kuba@kernel.org;
> > > > andrii@kernel.org; Loftus, Ciara <ciara.loftus@intel.com>
> > > > Subject: Re: [PATCH bpf-next] selftests: xsk: Update poll test cases
> > > >
> > > > On Mon, Jul 18, 2022 at 09:57:12AM +0000, Shibin Koikkara Reeny wrote:
> > > > > Poll test case was not testing all the functionality of the poll
> > > > > feature in the testsuite. This patch update the poll test case
> > > > > with 2 more testcases to check the timeout features.
> > > > >
> > > > > Poll test case have 4 sub test cases:
> > > >
> > > > Hi Shibin,
> > > >
> > > > Kinda not clear with count of added test cases, at first you say you
> > > > add 2 more but then you mention something about 4 sub test cases.
> > > >
> > > > To me these are separate test cases.
> > > >
> > > Hi Maciej,
> > >
> > > Will update it in V2
> > >
> > > > >
> > > > > 1. TEST_TYPE_RX_POLL:
> > > > > Check if POLLIN function work as expect.
> > > > >
> > > > > 2. TEST_TYPE_TX_POLL:
> > > > > Check if POLLOUT function work as expect.
> > > >
> > > > From run_pkt_test, I don't see any difference between 1 and 2. Why
> > > > split then?
> > > >
> > >
> > >
> > > It was done to show which case exactly broke. If RX poll event or TX
> > > poll event
> > >
> > > > >
> > > > > 3. TEST_TYPE_POLL_RXQ_EMPTY:
> > > >
> > > > 3 and 4 don't match with the code here
> > > > (TEST_TYPE_POLL_{R,T}XQ_TMOUT)
> > > >
> > > > > call poll function with parameter POLLIN on empty rx queue will
> > > > > cause timeout.If return timeout then test case is pass.
> > > > >
> > >
> > >
> > > True but  It was change to RXQ_EMPTY and TXQ_FULL from _TMOUT to
> > make
> > > it more clearer to what exactly is happening to cause timeout.
> > >
> > > > > 4. TEST_TYPE_POLL_TXQ_FULL:
> > > > > When txq is filled and packets are not cleaned by the kernel then
> > > > > if we invoke the poll function with POLLOUT then it should trigger
> > > > > timeout.If return timeout then test case is pass.
> > > > >
> > > > > Signed-off-by: Shibin Koikkara Reeny
> > > > > <shibin.koikkara.reeny@intel.com>
> > > > > ---
> > > > >  tools/testing/selftests/bpf/xskxceiver.c | 173
> > > > > +++++++++++++++++------  tools/testing/selftests/bpf/xskxceiver.h
> > > > > +++++++++++++++++|
> > > > > 10 +-
> > > > >  2 files changed, 139 insertions(+), 44 deletions(-)
> > > > >
> > > > > diff --git a/tools/testing/selftests/bpf/xskxceiver.c
> > > > > b/tools/testing/selftests/bpf/xskxceiver.c
> > > > > index 74d56d971baf..8ecab3a47c9e 100644
> > > > > --- a/tools/testing/selftests/bpf/xskxceiver.c
> > > > > +++ b/tools/testing/selftests/bpf/xskxceiver.c
> > > > > @@ -424,6 +424,8 @@ static void __test_spec_init(struct test_spec
> > > > > *test, struct ifobject *ifobj_tx,
> > > > >
> > > > >  		ifobj->xsk = &ifobj->xsk_arr[0];
> > > > >  		ifobj->use_poll = false;
> > > > > +		ifobj->skip_rx = false;
> > > > > +		ifobj->skip_tx = false;
> > > >
> > > > Any chances of trying to avoid these booleans? Not that it's a hard
> > > > nack, but the less booleans we spread around in this code the better.
> > >
> > >
> > > Not sure if it is possible but using any other logic will make the
> > > code more complex and less readable.
> > 
> > How did you come with such judgement? You didn't even try the idea that I
> > gave to you about having a testapp_validate_traffic() equivalent with a single
> > thread.
> > 
> 
> Hi Maciej,
> 
> diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> index 4394788829bf..0b58e026f2a2 100644
> --- a/tools/testing/selftests/bpf/xskxceiver.c
> +++ b/tools/testing/selftests/bpf/xskxceiver.c
> @@ -1317,6 +1317,24 @@ static void *worker_testapp_validate_rx(void *arg)
>         pthread_exit(NULL);
>  }
> 
> +static int testapp_validate_traffic_txq_tmout(struct test_spec *test)
> +{
> +       struct ifobject *ifobj_tx = test->ifobj_tx;
> +       pthread_t t0;
> +
> +       if (pthread_barrier_init(&barr, NULL, 2))
> +               exit_with_error(errno);
> +
> +       test->current_step++;
> +       pkt_stream_reset(ifobj_rx->pkt_stream);
> +
> +       pthread_create(&t0, NULL, ifobj_tx->func_ptr, test);
> +       pthread_join(t0, NULL);
> +
> +       return !!test->fail;
> +}
> +
> 
> This is what you are suggesting do ?
> 
> My point is ifobj_tx->func_ptr calls worker_testapp_validate_tx() ==> send_pkts() ==> __send_pkts().
> 
> Normal case when poll timeout happen send_pkts() return TEST_FAILURE which is expected.
> Test Case like TEST_TYPE_POLL_TXQ_TMOUT and TEST_TYPE_POLL_RXQ_TMOUT when poll timeout happen
> it should return TEST_PASS rather than TEST_FAILURE. How should I let the send_pkts()
> to know what timeout type of test is running without a new variable or flag? 
> Then boolean skip_rx and skip_tx are both used in the send_pkts() and receive_pkts().
> 
> This is why I thought it might be complex but if you have new suggestion I open to try it.

In such case you could just lookup if test->ifobj_rx has valid umem
pointer or xsk socket. If you didn't spawn rx thread then you know that
resources for Rx are not initialized, thread_common_ops() was not called
there. And you have a pointer to test_spec which allows you to dig up
other ifobject. There are also {r,t}x_on booleans which probably could be
used for your purposes, couldn't they? You are not testing the
bidirectional traffic, you're rather working on a single thread in one
direction.

Do you see what I'm trying to explain?

And sorry if I sounded mean or something in previous message exchanges,
just some harsh stuff on my side that is happening. We're only humans,
after all ;)

> 
> > >
> > > >
> > > > >  		ifobj->use_fill_ring = true;
> > > > >  		ifobj->release_rx = true;
> > > > >  		ifobj->pkt_stream = test->pkt_stream_default; @@ -589,6
> > > > +591,19 @@
> > > > > static struct pkt_stream *pkt_stream_clone(struct xsk_umem_info
> > > > *umem,
> > > > >  	return pkt_stream_generate(umem, pkt_stream->nb_pkts,
> > > > > pkt_stream->pkts[0].len);  }
> > > > >
> > > > > +static void pkt_stream_invalid(struct test_spec *test, u32
> > > > > +nb_pkts,
> > > > > +u32 pkt_len) {
> > > > > +	struct pkt_stream *pkt_stream;
> > > > > +	u32 i;
> > > > > +
> > > > > +	pkt_stream = pkt_stream_generate(test->ifobj_tx->umem,
> > > > nb_pkts, pkt_len);
> > > > > +	for (i = 0; i < nb_pkts; i++)
> > > > > +		pkt_stream->pkts[i].valid = false;
> > > > > +
> > > > > +	test->ifobj_tx->pkt_stream = pkt_stream;
> > > > > +	test->ifobj_rx->pkt_stream = pkt_stream; }
> > > >
> > > > Please explain how this work, e.g. why you need to have to have
> > > > invalid pkt stream + avoiding launching rx thread and why one of them is
> > not enough.
> > > >
> > > > Personally I think this is not needed. When calling
> > > > pkt_stream_generate(), validity of pkt is set based on length of packet vs
> > frame size:
> > > >
> > > > 		if (pkt_len > umem->frame_size)
> > > > 			pkt_stream->pkts[i].valid = false;
> > > >
> > > > so couldn't you use 2k frame size and bigger length of a packet?
> > > >
> > > This function was introduced for TEST_TYPE_POLL_TXQ_FULL keep the TX
> > > full and stop nofying the kernel that there is packet to cleanup.
> > > So we are manually setting the packets to invalid. This help to keep
> > > the __send_pkts() more generic and reduce the if conditions.
> > > ex: xsk_ring_prod__submit() is not needed to be added inside if condition.
> > 
> > I understand the intend behind it but what I was saying was that you have
> > everything ready to be used without a need for introducing new functions.
> > You could also try out what I suggested just to see if this makes things
> > simpler.
> > 
> 
> Are you suggesting to do this ?

Yes

>                 test->ifobj_tx->use_poll = true;
> -               pkt_stream_invalid(test, 2 * DEFAULT_PKT_CNT, PKT_SIZE);
> +               test->ifobj_tx->umem->frame_size = 2048;
> +               pkt_stream_replace(test, 2 * DEFAULT_PKT_CNT, 2048);
>                 testapp_validate_traffic(test);
> 
> 
> > >
> > > You are right we don't need rx stream but thought it will be good to
> > > keep as can be used for other features in future and will be more generic.

Yeah if you really want I'll be ok with pkt_stream_invalid().

> > 

RE: [PATCH bpf-next] selftests: xsk: Update poll test cases

[Koikkara Reeny, Shibin]

> -----Original Message-----
> From: Fijalkowski, Maciej <maciej.fijalkowski@intel.com>
> Sent: Wednesday, July 27, 2022 4:26 PM
> To: Koikkara Reeny, Shibin <shibin.koikkara.reeny@intel.com>
> Cc: bpf@vger.kernel.org; ast@kernel.org; daniel@iogearbox.net;
> netdev@vger.kernel.org; Karlsson, Magnus <magnus.karlsson@intel.com>;
> bjorn@kernel.org; kuba@kernel.org; andrii@kernel.org; Loftus, Ciara
> <ciara.loftus@intel.com>
> Subject: Re: [PATCH bpf-next] selftests: xsk: Update poll test cases
> 
> On Wed, Jul 27, 2022 at 11:49:57AM +0100, Koikkara Reeny, Shibin wrote:
> >
> > > -----Original Message-----
> > > From: Fijalkowski, Maciej <maciej.fijalkowski@intel.com>
> > > Sent: Tuesday, July 26, 2022 3:10 PM
> > > To: Koikkara Reeny, Shibin <shibin.koikkara.reeny@intel.com>
> > > Cc: bpf@vger.kernel.org; ast@kernel.org; daniel@iogearbox.net;
> > > netdev@vger.kernel.org; Karlsson, Magnus
> > > <magnus.karlsson@intel.com>; bjorn@kernel.org; kuba@kernel.org;
> > > andrii@kernel.org; Loftus, Ciara <ciara.loftus@intel.com>
> > > Subject: Re: [PATCH bpf-next] selftests: xsk: Update poll test cases
> > >
> > > On Tue, Jul 26, 2022 at 10:43:36AM +0100, Koikkara Reeny, Shibin wrote:
> > > > > -----Original Message-----
> > > > > From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > > > > Sent: Friday, July 22, 2022 3:16 PM
> > > > > To: Koikkara Reeny, Shibin <shibin.koikkara.reeny@intel.com>
> > > > > Cc: bpf@vger.kernel.org; ast@kernel.org; daniel@iogearbox.net;
> > > > > netdev@vger.kernel.org; Karlsson, Magnus
> > > > > <magnus.karlsson@intel.com>; bjorn@kernel.org; kuba@kernel.org;
> > > > > andrii@kernel.org; Loftus, Ciara <ciara.loftus@intel.com>
> > > > > Subject: Re: [PATCH bpf-next] selftests: xsk: Update poll test
> > > > > cases
> > > > >
> > > > > On Mon, Jul 18, 2022 at 09:57:12AM +0000, Shibin Koikkara Reeny
> wrote:
> > > > > > Poll test case was not testing all the functionality of the
> > > > > > poll feature in the testsuite. This patch update the poll test
> > > > > > case with 2 more testcases to check the timeout features.
> > > > > >
> > > > > > Poll test case have 4 sub test cases:
> > > > >
> > > > > Hi Shibin,
> > > > >
> > > > > Kinda not clear with count of added test cases, at first you say
> > > > > you add 2 more but then you mention something about 4 sub test
> cases.
> > > > >
> > > > > To me these are separate test cases.
> > > > >
> > > > Hi Maciej,
> > > >
> > > > Will update it in V2
> > > >
> > > > > >
> > > > > > 1. TEST_TYPE_RX_POLL:
> > > > > > Check if POLLIN function work as expect.
> > > > > >
> > > > > > 2. TEST_TYPE_TX_POLL:
> > > > > > Check if POLLOUT function work as expect.
> > > > >
> > > > > From run_pkt_test, I don't see any difference between 1 and 2.
> > > > > Why split then?
> > > > >
> > > >
> > > >
> > > > It was done to show which case exactly broke. If RX poll event or
> > > > TX poll event
> > > >
> > > > > >
> > > > > > 3. TEST_TYPE_POLL_RXQ_EMPTY:
> > > > >
> > > > > 3 and 4 don't match with the code here
> > > > > (TEST_TYPE_POLL_{R,T}XQ_TMOUT)
> > > > >
> > > > > > call poll function with parameter POLLIN on empty rx queue
> > > > > > will cause timeout.If return timeout then test case is pass.
> > > > > >
> > > >
> > > >
> > > > True but  It was change to RXQ_EMPTY and TXQ_FULL from _TMOUT to
> > > make
> > > > it more clearer to what exactly is happening to cause timeout.
> > > >
> > > > > > 4. TEST_TYPE_POLL_TXQ_FULL:
> > > > > > When txq is filled and packets are not cleaned by the kernel
> > > > > > then if we invoke the poll function with POLLOUT then it
> > > > > > should trigger timeout.If return timeout then test case is pass.
> > > > > >
> > > > > > Signed-off-by: Shibin Koikkara Reeny
> > > > > > <shibin.koikkara.reeny@intel.com>
> > > > > > ---
> > > > > >  tools/testing/selftests/bpf/xskxceiver.c | 173
> > > > > > +++++++++++++++++------
> > > > > > +++++++++++++++++tools/testing/selftests/bpf/xskxceiver.h
> > > > > > +++++++++++++++++|
> > > > > > 10 +-
> > > > > >  2 files changed, 139 insertions(+), 44 deletions(-)
> > > > > >
> > > > > > diff --git a/tools/testing/selftests/bpf/xskxceiver.c
> > > > > > b/tools/testing/selftests/bpf/xskxceiver.c
> > > > > > index 74d56d971baf..8ecab3a47c9e 100644
> > > > > > --- a/tools/testing/selftests/bpf/xskxceiver.c
> > > > > > +++ b/tools/testing/selftests/bpf/xskxceiver.c
> > > > > > @@ -424,6 +424,8 @@ static void __test_spec_init(struct
> > > > > > test_spec *test, struct ifobject *ifobj_tx,
> > > > > >
> > > > > >  		ifobj->xsk = &ifobj->xsk_arr[0];
> > > > > >  		ifobj->use_poll = false;
> > > > > > +		ifobj->skip_rx = false;
> > > > > > +		ifobj->skip_tx = false;
> > > > >
> > > > > Any chances of trying to avoid these booleans? Not that it's a
> > > > > hard nack, but the less booleans we spread around in this code the
> better.
> > > >
> > > >
> > > > Not sure if it is possible but using any other logic will make the
> > > > code more complex and less readable.
> > >
> > > How did you come with such judgement? You didn't even try the idea
> > > that I gave to you about having a testapp_validate_traffic()
> > > equivalent with a single thread.
> > >
> >
> > Hi Maciej,
> >
> > diff --git a/tools/testing/selftests/bpf/xskxceiver.c
> > b/tools/testing/selftests/bpf/xskxceiver.c
> > index 4394788829bf..0b58e026f2a2 100644
> > --- a/tools/testing/selftests/bpf/xskxceiver.c
> > +++ b/tools/testing/selftests/bpf/xskxceiver.c
> > @@ -1317,6 +1317,24 @@ static void *worker_testapp_validate_rx(void
> *arg)
> >         pthread_exit(NULL);
> >  }
> >
> > +static int testapp_validate_traffic_txq_tmout(struct test_spec *test)
> > +{
> > +       struct ifobject *ifobj_tx = test->ifobj_tx;
> > +       pthread_t t0;
> > +
> > +       if (pthread_barrier_init(&barr, NULL, 2))
> > +               exit_with_error(errno);
> > +
> > +       test->current_step++;
> > +       pkt_stream_reset(ifobj_rx->pkt_stream);
> > +
> > +       pthread_create(&t0, NULL, ifobj_tx->func_ptr, test);
> > +       pthread_join(t0, NULL);
> > +
> > +       return !!test->fail;
> > +}
> > +
> >
> > This is what you are suggesting do ?
> >
> > My point is ifobj_tx->func_ptr calls worker_testapp_validate_tx() ==>
> send_pkts() ==> __send_pkts().
> >
> > Normal case when poll timeout happen send_pkts() return TEST_FAILURE
> which is expected.
> > Test Case like TEST_TYPE_POLL_TXQ_TMOUT and
> TEST_TYPE_POLL_RXQ_TMOUT
> > when poll timeout happen it should return TEST_PASS rather than
> > TEST_FAILURE. How should I let the send_pkts() to know what timeout
> type of test is running without a new variable or flag?
> > Then boolean skip_rx and skip_tx are both used in the send_pkts() and
> receive_pkts().
> >
> > This is why I thought it might be complex but if you have new suggestion I
> open to try it.
> 
> In such case you could just lookup if test->ifobj_rx has valid umem pointer or
> xsk socket. If you didn't spawn rx thread then you know that resources for Rx
> are not initialized, thread_common_ops() was not called there. And you
> have a pointer to test_spec which allows you to dig up other ifobject. There
> are also {r,t}x_on booleans which probably could be used for your purposes,
> couldn't they? You are not testing the bidirectional traffic, you're rather
> working on a single thread in one direction.
> 
> Do you see what I'm trying to explain?
> 

Sure. I have updated it in the Patch v3.

> And sorry if I sounded mean or something in previous message exchanges,
> just some harsh stuff on my side that is happening. We're only humans, after
> all ;)

No worries... 😉

> 
> >
> > > >
> > > > >
> > > > > >  		ifobj->use_fill_ring = true;
> > > > > >  		ifobj->release_rx = true;
> > > > > >  		ifobj->pkt_stream = test->pkt_stream_default; @@
> -589,6
> > > > > +591,19 @@
> > > > > > static struct pkt_stream *pkt_stream_clone(struct
> > > > > > xsk_umem_info
> > > > > *umem,
> > > > > >  	return pkt_stream_generate(umem, pkt_stream->nb_pkts,
> > > > > > pkt_stream->pkts[0].len);  }
> > > > > >
> > > > > > +static void pkt_stream_invalid(struct test_spec *test, u32
> > > > > > +nb_pkts,
> > > > > > +u32 pkt_len) {
> > > > > > +	struct pkt_stream *pkt_stream;
> > > > > > +	u32 i;
> > > > > > +
> > > > > > +	pkt_stream = pkt_stream_generate(test->ifobj_tx->umem,
> > > > > nb_pkts, pkt_len);
> > > > > > +	for (i = 0; i < nb_pkts; i++)
> > > > > > +		pkt_stream->pkts[i].valid = false;
> > > > > > +
> > > > > > +	test->ifobj_tx->pkt_stream = pkt_stream;
> > > > > > +	test->ifobj_rx->pkt_stream = pkt_stream; }
> > > > >
> > > > > Please explain how this work, e.g. why you need to have to have
> > > > > invalid pkt stream + avoiding launching rx thread and why one of
> > > > > them is
> > > not enough.
> > > > >
> > > > > Personally I think this is not needed. When calling
> > > > > pkt_stream_generate(), validity of pkt is set based on length of
> > > > > packet vs
> > > frame size:
> > > > >
> > > > > 		if (pkt_len > umem->frame_size)
> > > > > 			pkt_stream->pkts[i].valid = false;
> > > > >
> > > > > so couldn't you use 2k frame size and bigger length of a packet?
> > > > >
> > > > This function was introduced for TEST_TYPE_POLL_TXQ_FULL keep the
> > > > TX full and stop nofying the kernel that there is packet to cleanup.
> > > > So we are manually setting the packets to invalid. This help to
> > > > keep the __send_pkts() more generic and reduce the if conditions.
> > > > ex: xsk_ring_prod__submit() is not needed to be added inside if
> condition.
> > >
> > > I understand the intend behind it but what I was saying was that you
> > > have everything ready to be used without a need for introducing new
> functions.
> > > You could also try out what I suggested just to see if this makes
> > > things simpler.
> > >
> >
> > Are you suggesting to do this ?
> 
> Yes
> 
> >                 test->ifobj_tx->use_poll = true;
> > -               pkt_stream_invalid(test, 2 * DEFAULT_PKT_CNT, PKT_SIZE);
> > +               test->ifobj_tx->umem->frame_size = 2048;
> > +               pkt_stream_replace(test, 2 * DEFAULT_PKT_CNT, 2048);
> >                 testapp_validate_traffic(test);
> >
> >
> > > >
> > > > You are right we don't need rx stream but thought it will be good
> > > > to keep as can be used for other features in future and will be more
> generic.
> 
> Yeah if you really want I'll be ok with pkt_stream_invalid().

Let's see how it looks. I have updated it in the patch v3.

https://lore.kernel.org/bpf/20220729132337.211443-1-shibin.koikkara.reeny@intel.com/

> 
> > >