-----Original Message-----
From: Fijalkowski, Maciej <maciej.fijalkowski@intel.com>
Sent: Friday, July 29, 2022 3:55 PM
To: Koikkara Reeny, Shibin <redacted>
Cc: bpf@vger.kernel.org; ast@kernel.org; daniel@iogearbox.net;
netdev@vger.kernel.org; Karlsson, Magnus [off-list ref];
bjorn@kernel.org; kuba@kernel.org; andrii@kernel.org; Loftus, Ciara
[off-list ref]
Subject: Re: [PATCH bpf-next v3] selftests: xsk: Update poll test cases

On Fri, Jul 29, 2022 at 01:23:37PM +0000, Shibin Koikkara Reeny wrote:

quoted

Poll test case was not testing all the functionality of the poll
feature in the testsuite. This patch update the poll test case which
contain 2 testcases to test the RX and the TX poll functionality and
additional
2 more testcases to check the timeout features of the poll event.

Poll testsuite have 4 test cases:

1. TEST_TYPE_RX_POLL:
Check if RX path POLLIN function work as expect. TX path can use any
method to sent the traffic.

2. TEST_TYPE_TX_POLL:
Check if TX path POLLOUT function work as expect. RX path can use any
method to receive the traffic.

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.

v1:
https://lore.kernel.org/bpf/20220718095712.588513-1-shibin.koikkara.re
eny@intel.com/
v2:
https://lore.kernel.org/bpf/20220726101723.250746-1-shibin.koikkara.re
eny@intel.com/

Changes in v2:
 * Updated the commit message
 * fixed the while loop flow in receive_pkts function.
Changes in v3:
 * Introduced single thread validation function.
 * Removed pkt_stream_invalid().
 * Updated TEST_TYPE_POLL_TXQ_FULL testcase to create invalid frame.
 * Removed timer from send_pkts().

Hmm, so timer is not needed for tx side? Is it okay to remove it altogether? I
only meant to pull it out to preceding patch.

Yes timer is not needed. It was introduced for the poll tx timeout but the logic has changed so we don't need it.

quoted

 * Removed boolean variable skip_rx and skip_tx

Signed-off-by: Shibin Koikkara Reeny <redacted>
---
 tools/testing/selftests/bpf/xskxceiver.c | 155 +++++++++++++++++------
 tools/testing/selftests/bpf/xskxceiver.h |   8 +-
 2 files changed, 125 insertions(+), 38 deletions(-)

diff --git a/tools/testing/selftests/bpf/xskxceiver.c

b/tools/testing/selftests/bpf/xskxceiver.c
index 74d56d971baf..32ba6464f29f 100644

--- a/tools/testing/selftests/bpf/xskxceiver.c
+++ b/tools/testing/selftests/bpf/xskxceiver.c

@@ -817,9 +817,9 @@ static int complete_pkts(struct xsk_socket_info

*xsk, int batch_size)

quoted

 	return TEST_PASS;
 }

-static int receive_pkts(struct ifobject *ifobj, struct pollfd *fds)
+static int receive_pkts(struct test_spec *test, struct ifobject
+*ifobj, struct pollfd *fds)

Nit : I think that we could skip passing ifobj explicitly if we're passing
test_spec itself and just work on

	struct ifobject *ifobj = test->ifobj_rx;

within the function.

Will update in V4 patch.
 

quoted

 {
-	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 +843,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 (!test->ifobj_tx->umem->umem)
+					return TEST_PASS;
+
+				ksft_print_msg("ERROR: [%s] Poll timed

out\n", __func__);

quoted

+				return TEST_FAILURE;

-		rcvd = xsk_ring_cons__peek(&xsk->rx, BATCH_SIZE,

&idx_rx);

quoted

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

quoted

+		if (!rcvd)
+			continue;
+
 		if (ifobj->use_fill_ring) {
 			ret = xsk_ring_prod__reserve(&umem->fq, rcvd,

&idx_fq);

quoted

 			while (ret != rcvd) {

@@ -900,13 +911,34 @@ static int receive_pkts(struct ifobject *ifobj, struct

pollfd *fds)

quoted

 	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,

quoted

+		       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) {

quoted

+		if (use_poll) {
+			ret = poll(fds, 1, POLL_TMOUT);
+			if (timeout) {
+				if (ret < 0) {
+					ksft_print_msg("ERROR: [%s] Poll

error %d\n",

quoted

+						       __func__, ret);
+					return TEST_FAILURE;
+				}
+				if (ret == 0)
+					return TEST_PASS;
+				break;
+			}
+			if (ret <= 0) {
+				ksft_print_msg("ERROR: [%s] Poll error

%d\n",

quoted

+					       __func__, ret);
+				return TEST_FAILURE;
+			}
+		}

-	while (xsk_ring_prod__reserve(&xsk->tx, BATCH_SIZE, &idx) <

BATCH_SIZE)

quoted

 		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 +965,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("ERROR: [%s] Poll error %d\n",

__func__, ret);

quoted

+			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 +996,19 @@ static void wait_for_tx_completion(struct
xsk_socket_info *xsk)

 static int send_pkts(struct test_spec *test, struct ifobject
*ifobject)  {
+	bool timeout = (!test->ifobj_rx->umem->umem) ? true : false;

normally instead of such ternary operator to test if some ptr is NULL or not
we would do:

static bool is_umem_valid(struct ifobject *ifobj) {
	return !!ifobj->umem->umem;
}

	bool timeout = !is_umem_valid(test->ifobj_rx);

but I think this can stay as is.

I think it is a good suggestion. It is more readable.
Will update in the patch V4.

quoted

 	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;
-
-			if (!(fds.revents & POLLOUT))
-				continue;
-		}
-
-		err = __send_pkts(ifobject, &pkt_cnt);
-		if (err || test->fail)
+		ret = __send_pkts(ifobject, &pkt_cnt, ifobject->use_poll,

&fds,

quoted

+timeout);

could you avoid passing ifobject->use_poll explicitly?

Will update in patch v4.

quoted

+		if ((ret || test->fail) && !timeout)
 			return TEST_FAILURE;
+		else if (ret == TEST_PASS && timeout)
+			return ret;
 	}

 	wait_for_tx_completion(ifobject->xsk);

@@ -1235,7 +1273,7 @@ static void *worker_testapp_validate_rx(void

*arg)

 	pthread_barrier_wait(&barr);

-	err = receive_pkts(ifobject, &fds);
+	err = receive_pkts(test, ifobject, &fds);

 	if (!err && ifobject->validation_func)
 		err = ifobject->validation_func(ifobject);

@@ -1251,6 +1289,33 @@ static void *worker_testapp_validate_rx(void

*arg)

quoted

 	pthread_exit(NULL);
 }

+static int testapp_validate_traffic_single_thread(struct test_spec *test,

struct ifobject *ifobj,

quoted

+						  enum test_type type)
+{
+	pthread_t t0;
+
+	if (pthread_barrier_init(&barr, NULL, 2))
+		exit_with_error(errno);
+
+	test->current_step++;
+	if (type  == TEST_TYPE_POLL_RXQ_TMOUT)
+		pkt_stream_reset(ifobj->pkt_stream);
+	pkts_in_flight = 0;
+
+	/*Spawn thread */
+	pthread_create(&t0, NULL, ifobj->func_ptr, test);
+
+	if (type  != TEST_TYPE_POLL_TXQ_TMOUT)

nit: double space before !=

quoted

+		pthread_barrier_wait(&barr);
+
+	if (pthread_barrier_destroy(&barr))
+		exit_with_error(errno);
+
+	pthread_join(t0, NULL);
+
+	return !!test->fail;
+}

I like this better as it serves its purpose and testapp_validate_traffic() stays
cleaner. Thanks!

Your welcome.

quoted

+
 static int testapp_validate_traffic(struct test_spec *test)  {
 	struct ifobject *ifobj_tx = test->ifobj_tx; @@ -1548,12 +1613,30 @@
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:
-		test->ifobj_tx->use_poll = true;
+	case TEST_TYPE_RX_POLL:
 		test->ifobj_rx->use_poll = true;
-		test_spec_set_name(test, "POLL");
+		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_tx->use_poll = true;
+		/* create invalid frame by set umem frame_size and pkt

length equal to 2048 */

quoted

+		test->ifobj_tx->umem->frame_size = 2048;
+		pkt_stream_replace(test, 2 * DEFAULT_PKT_CNT, 2048);
+		testapp_validate_traffic_single_thread(test, test->ifobj_tx,

type);

quoted

+		pkt_stream_restore_default(test);
+		break;
+	case TEST_TYPE_POLL_RXQ_TMOUT:
+		test_spec_set_name(test, "POLL_RXQ_EMPTY");
+		test->ifobj_rx->use_poll = true;
+		testapp_validate_traffic_single_thread(test, test->ifobj_rx,

type);

quoted

+		break;
 	case TEST_TYPE_ALIGNED_INV_DESC:
 		test_spec_set_name(test, "ALIGNED_INV_DESC");
 		testapp_invalid_desc(test);

diff --git a/tools/testing/selftests/bpf/xskxceiver.h

b/tools/testing/selftests/bpf/xskxceiver.h
index 3d17053f98e5..ee97576757a9 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

quoted

SOCK_RECONF_CTR 10  #define BATCH_SIZE 64  #define POLL_TMOUT

1000

quoted

-#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

quoted

(DEFAULT_UMEM_BUFFERS * XSK_UMEM__DEFAULT_FRAME_SIZE) @@ -

68,7 +69,10

quoted

@@ 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,
--

2.34.1