[PATCH 1/2] event/sw: add selftest for ordered history list

[PATCH 1/2] event/sw: add selftest for ordered history list

[Harry van Haaren]

This commit adds a unit test for an issue identified
where ordered history-list entries are not correctly
cleared when the returned event is of op RELEASE type.

The result of the history-list bug is that a future event
which re-uses that history-list slot, but has an op type
of FORWARD will incorrectly be reordered.

The existing unit-tests did not cover the RELEASE of an
ORDERED queue, and then stress-test the history-list by
iterating HIST_LIST times afterwards.

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 drivers/event/sw/sw_evdev_selftest.c | 132 +++++++++++++++++++++++++++
 1 file changed, 132 insertions(+)

diff --git a/drivers/event/sw/sw_evdev_selftest.c b/drivers/event/sw/sw_evdev_selftest.c
index 3aa8d76ca8..59afa260c6 100644
--- a/drivers/event/sw/sw_evdev_selftest.c
+++ b/drivers/event/sw/sw_evdev_selftest.c
@@ -2959,6 +2959,132 @@ dev_stop_flush(struct test *t) /* test to check we can properly flush events */
 	return -1;
 }
 
+static int
+ordered_atomic_hist_completion(struct test *t)
+{
+	const int rx_enq = 0;
+	int err;
+
+	/* Create instance with 1 atomic QID going to 3 ports + 1 prod port */
+	if (init(t, 2, 2) < 0 ||
+			create_ports(t, 2) < 0 ||
+			create_ordered_qids(t, 1) < 0 ||
+			create_atomic_qids(t, 1) < 0)
+		return -1;
+
+	/* Helpers to identify queues */
+	const uint8_t qid_ordered = t->qid[0];
+	const uint8_t qid_atomic = t->qid[1];
+
+	/* CQ mapping to QID */
+	if (rte_event_port_link(evdev, t->port[1], &t->qid[0], NULL, 1) != 1) {
+		printf("%d: error mapping port 1 qid\n", __LINE__);
+		return -1;
+	}
+	if (rte_event_port_link(evdev, t->port[1], &t->qid[1], NULL, 1) != 1) {
+		printf("%d: error mapping port 1 qid\n", __LINE__);
+		return -1;
+	}
+	if (rte_event_dev_start(evdev) < 0) {
+		printf("%d: Error with start call\n", __LINE__);
+		return -1;
+	}
+
+	/* Enqueue 1x ordered event, to be RELEASE-ed by the worker
+	 * CPU, which may cause hist-list corruption (by not comleting)
+	 */
+	struct rte_event ord_ev = {
+		.op = RTE_EVENT_OP_NEW,
+		.queue_id = qid_ordered,
+		.event_type = RTE_EVENT_TYPE_CPU,
+		.priority = RTE_EVENT_DEV_PRIORITY_NORMAL,
+	};
+	err = rte_event_enqueue_burst(evdev, t->port[rx_enq], &ord_ev, 1);
+	if (err != 1) {
+		printf("%d: Failed to enqueue\n", __LINE__);
+		return -1;
+	}
+
+	/* call the scheduler. This schedules the above event as a single
+	 * event in an ORDERED queue, to the worker.
+	 */
+	rte_service_run_iter_on_app_lcore(t->service_id, 1);
+
+	/* Dequeue ORDERED event 0 from port 1, so that we can then drop */
+	struct rte_event ev;
+	if (!rte_event_dequeue_burst(evdev, t->port[1], &ev, 1, 0)) {
+		printf("%d: failed to dequeue\n", __LINE__);
+		return -1;
+	}
+
+	/* drop the ORDERED event. Here the history list should be completed,
+	 * but might not be if the hist-list bug exists. Call scheduler to make
+	 * it act on the RELEASE that was enqueued.
+	 */
+	rte_event_enqueue_burst(evdev, t->port[1], &release_ev, 1);
+	rte_service_run_iter_on_app_lcore(t->service_id, 1);
+
+	/* Enqueue 1x atomic event, to then FORWARD to trigger atomic hist-list
+	 * completion. If the bug exists, the ORDERED entry may be completed in
+	 * error (aka, using the ORDERED-ROB for the ATOMIC event). This is the
+	 * main focus of this unit test.
+	 */
+	{
+		struct rte_event ev = {
+			.op = RTE_EVENT_OP_NEW,
+			.queue_id = qid_atomic,
+			.event_type = RTE_EVENT_TYPE_CPU,
+			.priority = RTE_EVENT_DEV_PRIORITY_NORMAL,
+			.flow_id = 123,
+		};
+
+		err = rte_event_enqueue_burst(evdev, t->port[rx_enq], &ev, 1);
+		if (err != 1) {
+			printf("%d: Failed to enqueue\n", __LINE__);
+			return -1;
+		}
+	}
+	rte_service_run_iter_on_app_lcore(t->service_id, 1);
+
+	/* Deq ATM event, then forward it for more than HIST_LIST_SIZE times,
+	 * to re-use the history list entry that may be corrupted previously.
+	 */
+	for (int i = 0; i < SW_PORT_HIST_LIST + 2; i++) {
+		if (!rte_event_dequeue_burst(evdev, t->port[1], &ev, 1, 0)) {
+			printf("%d: failed to dequeue, did corrupt ORD hist "
+				"list steal this ATM event?\n", __LINE__);
+			return -1;
+		}
+
+		/* Re-enqueue the ATM event as FWD, trigger hist-list. */
+		ev.op = RTE_EVENT_OP_FORWARD;
+		err = rte_event_enqueue_burst(evdev, t->port[1], &ev, 1);
+		if (err != 1) {
+			printf("%d: Failed to enqueue\n", __LINE__);
+			return -1;
+		}
+
+		rte_service_run_iter_on_app_lcore(t->service_id, 1);
+	}
+
+	/* If HIST-LIST + N count of dequeues succeed above, the hist list
+	 * has not been corrupted. If it is corrupted, the ATM event is pushed
+	 * into the ORDERED-ROB and will not dequeue.
+	 */
+
+	/* release the ATM event that's been forwarded HIST_LIST times */
+	err = rte_event_enqueue_burst(evdev, t->port[1], &release_ev, 1);
+	if (err != 1) {
+		printf("%d: Failed to enqueue\n", __LINE__);
+		return -1;
+	}
+
+	rte_service_run_iter_on_app_lcore(t->service_id, 1);
+
+	cleanup(t);
+	return 0;
+}
+
 static int
 worker_loopback_worker_fn(void *arg)
 {
@@ -3388,6 +3514,12 @@ test_sw_eventdev(void)
 		printf("ERROR - Stop Flush test FAILED.\n");
 		goto test_fail;
 	}
+	printf("*** Running Ordered & Atomic hist-list completion test...\n");
+	ret = ordered_atomic_hist_completion(t);
+	if (ret != 0) {
+		printf("ERROR - Ordered & Atomic hist-list test FAILED.\n");
+		goto test_fail;
+	}
 	if (rte_lcore_count() >= 3) {
 		printf("*** Running Worker loopback test...\n");
 		ret = worker_loopback(t, 0);
-- 
2.34.1

[PATCH 2/2] event/sw: fix ordering corruption with op release

[Harry van Haaren]

This commit changes the logic in the scheduler to always
reset reorder-buffer entries in the QE_FLAG_COMPLETE path,
and not just the QE_FLAG_VALID path.

A release event is a COMPLETE but not VALID (no new event).
As a result, release events previously left the history-list
in an inconsistent state, and future events with op type of
forward could be incorrectly reordered.

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 drivers/event/sw/sw_evdev_scheduler.c | 45 ++++++++++++++++-----------
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/drivers/event/sw/sw_evdev_scheduler.c b/drivers/event/sw/sw_evdev_scheduler.c
index 8bc21944f5..9ee6698525 100644
--- a/drivers/event/sw/sw_evdev_scheduler.c
+++ b/drivers/event/sw/sw_evdev_scheduler.c
@@ -360,10 +360,15 @@ __pull_port_lb(struct sw_evdev *sw, uint32_t port_id, int allow_reorder)
 
 	while (port->pp_buf_count) {
 		const struct rte_event *qe = &port->pp_buf[port->pp_buf_start];
-		struct sw_hist_list_entry *hist_entry = NULL;
 		uint8_t flags = qe->op;
 		const uint16_t eop = !(flags & QE_FLAG_NOT_EOP);
-		int needs_reorder = 0;
+
+		/* rob_entry being NULL or a value is used as the distinction
+		 * between reordering being required (mark ROB as ready) or
+		 * just an Atomic completion.
+		 */
+		struct reorder_buffer_entry *rob_ptr = NULL;
+
 		/* if no-reordering, having PARTIAL == NEW */
 		if (!allow_reorder && !eop)
 			flags = QE_FLAG_VALID;
@@ -386,6 +391,7 @@ __pull_port_lb(struct sw_evdev *sw, uint32_t port_id, int allow_reorder)
 			const uint32_t hist_tail = port->hist_tail &
 					(SW_PORT_HIST_LIST - 1);
 
+			struct sw_hist_list_entry *hist_entry;
 			hist_entry = &port->hist_list[hist_tail];
 			const uint32_t hist_qid = hist_entry->qid;
 			const uint32_t hist_fid = hist_entry->fid;
@@ -396,17 +402,24 @@ __pull_port_lb(struct sw_evdev *sw, uint32_t port_id, int allow_reorder)
 			if (fid->pcount == 0)
 				fid->cq = -1;
 
+			/* Assign current hist-list entry to the rob_entry, to
+			 * allow VALID code below re-use it for checks.
+			 */
+			rob_ptr = hist_entry->rob_entry;
+
+			/* Clear the rob entry in this COMPLETE flag phase, as
+			 * RELEASE events must clear hist-list, but MIGHT NOT
+			 * contain a VALID flag too.
+			 */
+			hist_entry->rob_entry = NULL;
+
 			if (allow_reorder) {
-				/* set reorder ready if an ordered QID */
-				uintptr_t rob_ptr =
-					(uintptr_t)hist_entry->rob_entry;
 				const uintptr_t valid = (rob_ptr != 0);
-				needs_reorder = valid;
-				rob_ptr |=
-					((valid - 1) & (uintptr_t)&dummy_rob);
+				uintptr_t tmp = (uintptr_t)rob_ptr;
+				tmp |= ((valid - 1) & (uintptr_t)&dummy_rob);
 				struct reorder_buffer_entry *tmp_rob_ptr =
-					(struct reorder_buffer_entry *)rob_ptr;
-				tmp_rob_ptr->ready = eop * needs_reorder;
+					(struct reorder_buffer_entry *)tmp;
+				tmp_rob_ptr->ready = eop * valid;
 			}
 
 			port->inflights -= eop;
@@ -415,22 +428,18 @@ __pull_port_lb(struct sw_evdev *sw, uint32_t port_id, int allow_reorder)
 		if (flags & QE_FLAG_VALID) {
 			port->stats.rx_pkts++;
 
-			if (allow_reorder && needs_reorder) {
-				struct reorder_buffer_entry *rob_entry =
-						hist_entry->rob_entry;
-
-				hist_entry->rob_entry = NULL;
+			if (allow_reorder && rob_ptr) {
 				/* Although fragmentation not currently
 				 * supported by eventdev API, we support it
 				 * here. Open: How do we alert the user that
 				 * they've exceeded max frags?
 				 */
-				int num_frag = rob_entry->num_fragments;
+				int num_frag = rob_ptr->num_fragments;
 				if (num_frag == SW_FRAGMENTS_MAX)
 					sw->stats.rx_dropped++;
 				else {
-					int idx = rob_entry->num_fragments++;
-					rob_entry->fragments[idx] = *qe;
+					int idx = rob_ptr->num_fragments++;
+					rob_ptr->fragments[idx] = *qe;
 				}
 				goto end_qe;
 			}
-- 
2.34.1

Re: [PATCH 1/2] event/sw: add selftest for ordered history list

[Bruce Richardson]

On Thu, Aug 31, 2023 at 05:47:35PM +0100, Harry van Haaren wrote:
> This commit adds a unit test for an issue identified
> where ordered history-list entries are not correctly
> cleared when the returned event is of op RELEASE type.
> 
> The result of the history-list bug is that a future event
> which re-uses that history-list slot, but has an op type
> of FORWARD will incorrectly be reordered.
> 
> The existing unit-tests did not cover the RELEASE of an
> ORDERED queue, and then stress-test the history-list by
> iterating HIST_LIST times afterwards.
> 
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> ---
Hi Harry,

having the test first is good for showing up the bug, but won't that mean
that our unit tests are broken until after the second patch is applied? If
so, I suggest reversing the order of the series so the tests always return
clean. [If no V2 necessary, the order can maybe be switched on apply]

/Bruce

Re: [PATCH 2/2] event/sw: fix ordering corruption with op release

[Bruce Richardson]

On Thu, Aug 31, 2023 at 05:47:36PM +0100, Harry van Haaren wrote:
> This commit changes the logic in the scheduler to always
> reset reorder-buffer entries in the QE_FLAG_COMPLETE path,
> and not just the QE_FLAG_VALID path.
> 
> A release event is a COMPLETE but not VALID (no new event).
> As a result, release events previously left the history-list
> in an inconsistent state, and future events with op type of
> forward could be incorrectly reordered.
> 
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> ---
>  drivers/event/sw/sw_evdev_scheduler.c | 45 ++++++++++++++++-----------
>  1 file changed, 27 insertions(+), 18 deletions(-)
> 
Hi Harry,

wondering if this fix might work as well, and offer a simpler alternative.
We can instead zero all unspecified hist-list entries on write.

/Bruce

--- a/drivers/event/sw/sw_evdev_scheduler.c
+++ b/drivers/event/sw/sw_evdev_scheduler.c
@@ -90,8 +90,7 @@ sw_schedule_atomic_to_cq(struct sw_evdev *sw, struct sw_qid * const qid,
                sw->cq_ring_space[cq]--;
 
                int head = (p->hist_head++ & (SW_PORT_HIST_LIST-1));
-               p->hist_list[head].fid = flow_id;
-               p->hist_list[head].qid = qid_id;
+               p->hist_list[head] = (struct sw_hist_list_entry){ .qid = qid_id, .fid = flow_id };
 
                p->stats.tx_pkts++;
                qid->stats.tx_pkts++;
@@ -162,8 +161,8 @@ sw_schedule_parallel_to_cq(struct sw_evdev *sw, struct sw_qid * const qid,
                qid->stats.tx_pkts++;
 
                const int head = (p->hist_head & (SW_PORT_HIST_LIST-1));
-               p->hist_list[head].fid = SW_HASH_FLOWID(qe->flow_id);
-               p->hist_list[head].qid = qid_id;
+               const uint32_t fid = SW_HASH_FLOWID(qe->flow_id);
+               p->hist_list[head] = (struct sw_hist_list_entry){ .qid = qid_id, .fid = fid };
 
                if (keep_order)
                        rob_ring_dequeue(qid->reorder_buffer_freelist,

RE: [PATCH 2/2] event/sw: fix ordering corruption with op release

[Van Haaren, Harry]

> -----Original Message-----
> From: Richardson, Bruce <bruce.richardson@intel.com>
> Sent: Monday, September 4, 2023 5:38 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH 2/2] event/sw: fix ordering corruption with op release

<snip>

> >  drivers/event/sw/sw_evdev_scheduler.c | 45 ++++++++++++++++-----------
> >  1 file changed, 27 insertions(+), 18 deletions(-)
> >
> Hi Harry,
> 
> wondering if this fix might work as well, and offer a simpler alternative.
> We can instead zero all unspecified hist-list entries on write.

Ah, interesting approach. Will investigate and spin a v2 (with order inverted) if this approach proves to be a better fit.

<snip>

[PATCH v2 1/2] event/sw: fix ordering corruption with op release

[Harry van Haaren]

This commit changes the logic in the scheduler to always
reset reorder-buffer (and QID/FID) entries when writing
them. This avoids stale ROB/QID/FID data re-use, which
previously caused ordering issues.

Before this commit, release events left the history-list
in an inconsistent state, and future events with op type of
forward could be incorrectly reordered.

Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>

---

v2:
- Rework fix to simpler suggestion (Bruce)
- Respin patchset to "apply order" (Bruce)

---
 drivers/event/sw/sw_evdev_scheduler.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/event/sw/sw_evdev_scheduler.c b/drivers/event/sw/sw_evdev_scheduler.c
index de6ed21643..21c360770e 100644
--- a/drivers/event/sw/sw_evdev_scheduler.c
+++ b/drivers/event/sw/sw_evdev_scheduler.c
@@ -90,8 +90,10 @@ sw_schedule_atomic_to_cq(struct sw_evdev *sw, struct sw_qid * const qid,
 		sw->cq_ring_space[cq]--;
 
 		int head = (p->hist_head++ & (SW_PORT_HIST_LIST-1));
-		p->hist_list[head].fid = flow_id;
-		p->hist_list[head].qid = qid_id;
+		p->hist_list[head] = (struct sw_hist_list_entry) {
+			.qid = qid_id,
+			.fid = flow_id,
+		};
 
 		p->stats.tx_pkts++;
 		qid->stats.tx_pkts++;
@@ -162,8 +164,13 @@ sw_schedule_parallel_to_cq(struct sw_evdev *sw, struct sw_qid * const qid,
 		qid->stats.tx_pkts++;
 
 		const int head = (p->hist_head & (SW_PORT_HIST_LIST-1));
-		p->hist_list[head].fid = SW_HASH_FLOWID(qe->flow_id);
-		p->hist_list[head].qid = qid_id;
+		const uint32_t fid = SW_HASH_FLOWID(qe->flow_id);
+		p->hist_list[head] = (struct sw_hist_list_entry) {
+			.qid = qid_id,
+			.fid = fid,
+		};
+
+
 
 		if (keep_order)
 			rob_ring_dequeue(qid->reorder_buffer_freelist,
-- 
2.34.1

[PATCH v2 2/2] event/sw: add selftest for ordered history list

[Harry van Haaren]

This commit adds a unit test for an issue identified
where ordered history-list entries are not correctly
cleared when the returned event is of op RELEASE type.

The result of the history-list bug is that a future event
which re-uses that history-list slot, but has an op type
of FORWARD will incorrectly be reordered.

The existing unit-tests did not cover the RELEASE of an
ORDERED queue, and then stress-test the history-list by
iterating HIST_LIST times afterwards.

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 drivers/event/sw/sw_evdev_selftest.c | 132 +++++++++++++++++++++++++++
 1 file changed, 132 insertions(+)

diff --git a/drivers/event/sw/sw_evdev_selftest.c b/drivers/event/sw/sw_evdev_selftest.c
index 3aa8d76ca8..59afa260c6 100644
--- a/drivers/event/sw/sw_evdev_selftest.c
+++ b/drivers/event/sw/sw_evdev_selftest.c
@@ -2959,6 +2959,132 @@ dev_stop_flush(struct test *t) /* test to check we can properly flush events */
 	return -1;
 }
 
+static int
+ordered_atomic_hist_completion(struct test *t)
+{
+	const int rx_enq = 0;
+	int err;
+
+	/* Create instance with 1 atomic QID going to 3 ports + 1 prod port */
+	if (init(t, 2, 2) < 0 ||
+			create_ports(t, 2) < 0 ||
+			create_ordered_qids(t, 1) < 0 ||
+			create_atomic_qids(t, 1) < 0)
+		return -1;
+
+	/* Helpers to identify queues */
+	const uint8_t qid_ordered = t->qid[0];
+	const uint8_t qid_atomic = t->qid[1];
+
+	/* CQ mapping to QID */
+	if (rte_event_port_link(evdev, t->port[1], &t->qid[0], NULL, 1) != 1) {
+		printf("%d: error mapping port 1 qid\n", __LINE__);
+		return -1;
+	}
+	if (rte_event_port_link(evdev, t->port[1], &t->qid[1], NULL, 1) != 1) {
+		printf("%d: error mapping port 1 qid\n", __LINE__);
+		return -1;
+	}
+	if (rte_event_dev_start(evdev) < 0) {
+		printf("%d: Error with start call\n", __LINE__);
+		return -1;
+	}
+
+	/* Enqueue 1x ordered event, to be RELEASE-ed by the worker
+	 * CPU, which may cause hist-list corruption (by not comleting)
+	 */
+	struct rte_event ord_ev = {
+		.op = RTE_EVENT_OP_NEW,
+		.queue_id = qid_ordered,
+		.event_type = RTE_EVENT_TYPE_CPU,
+		.priority = RTE_EVENT_DEV_PRIORITY_NORMAL,
+	};
+	err = rte_event_enqueue_burst(evdev, t->port[rx_enq], &ord_ev, 1);
+	if (err != 1) {
+		printf("%d: Failed to enqueue\n", __LINE__);
+		return -1;
+	}
+
+	/* call the scheduler. This schedules the above event as a single
+	 * event in an ORDERED queue, to the worker.
+	 */
+	rte_service_run_iter_on_app_lcore(t->service_id, 1);
+
+	/* Dequeue ORDERED event 0 from port 1, so that we can then drop */
+	struct rte_event ev;
+	if (!rte_event_dequeue_burst(evdev, t->port[1], &ev, 1, 0)) {
+		printf("%d: failed to dequeue\n", __LINE__);
+		return -1;
+	}
+
+	/* drop the ORDERED event. Here the history list should be completed,
+	 * but might not be if the hist-list bug exists. Call scheduler to make
+	 * it act on the RELEASE that was enqueued.
+	 */
+	rte_event_enqueue_burst(evdev, t->port[1], &release_ev, 1);
+	rte_service_run_iter_on_app_lcore(t->service_id, 1);
+
+	/* Enqueue 1x atomic event, to then FORWARD to trigger atomic hist-list
+	 * completion. If the bug exists, the ORDERED entry may be completed in
+	 * error (aka, using the ORDERED-ROB for the ATOMIC event). This is the
+	 * main focus of this unit test.
+	 */
+	{
+		struct rte_event ev = {
+			.op = RTE_EVENT_OP_NEW,
+			.queue_id = qid_atomic,
+			.event_type = RTE_EVENT_TYPE_CPU,
+			.priority = RTE_EVENT_DEV_PRIORITY_NORMAL,
+			.flow_id = 123,
+		};
+
+		err = rte_event_enqueue_burst(evdev, t->port[rx_enq], &ev, 1);
+		if (err != 1) {
+			printf("%d: Failed to enqueue\n", __LINE__);
+			return -1;
+		}
+	}
+	rte_service_run_iter_on_app_lcore(t->service_id, 1);
+
+	/* Deq ATM event, then forward it for more than HIST_LIST_SIZE times,
+	 * to re-use the history list entry that may be corrupted previously.
+	 */
+	for (int i = 0; i < SW_PORT_HIST_LIST + 2; i++) {
+		if (!rte_event_dequeue_burst(evdev, t->port[1], &ev, 1, 0)) {
+			printf("%d: failed to dequeue, did corrupt ORD hist "
+				"list steal this ATM event?\n", __LINE__);
+			return -1;
+		}
+
+		/* Re-enqueue the ATM event as FWD, trigger hist-list. */
+		ev.op = RTE_EVENT_OP_FORWARD;
+		err = rte_event_enqueue_burst(evdev, t->port[1], &ev, 1);
+		if (err != 1) {
+			printf("%d: Failed to enqueue\n", __LINE__);
+			return -1;
+		}
+
+		rte_service_run_iter_on_app_lcore(t->service_id, 1);
+	}
+
+	/* If HIST-LIST + N count of dequeues succeed above, the hist list
+	 * has not been corrupted. If it is corrupted, the ATM event is pushed
+	 * into the ORDERED-ROB and will not dequeue.
+	 */
+
+	/* release the ATM event that's been forwarded HIST_LIST times */
+	err = rte_event_enqueue_burst(evdev, t->port[1], &release_ev, 1);
+	if (err != 1) {
+		printf("%d: Failed to enqueue\n", __LINE__);
+		return -1;
+	}
+
+	rte_service_run_iter_on_app_lcore(t->service_id, 1);
+
+	cleanup(t);
+	return 0;
+}
+
 static int
 worker_loopback_worker_fn(void *arg)
 {
@@ -3388,6 +3514,12 @@ test_sw_eventdev(void)
 		printf("ERROR - Stop Flush test FAILED.\n");
 		goto test_fail;
 	}
+	printf("*** Running Ordered & Atomic hist-list completion test...\n");
+	ret = ordered_atomic_hist_completion(t);
+	if (ret != 0) {
+		printf("ERROR - Ordered & Atomic hist-list test FAILED.\n");
+		goto test_fail;
+	}
 	if (rte_lcore_count() >= 3) {
 		printf("*** Running Worker loopback test...\n");
 		ret = worker_loopback(t, 0);
-- 
2.34.1

Re: [PATCH v2 1/2] event/sw: fix ordering corruption with op release

[Bruce Richardson]

On Thu, Sep 14, 2023 at 11:58:51AM +0100, Harry van Haaren wrote:
> This commit changes the logic in the scheduler to always
> reset reorder-buffer (and QID/FID) entries when writing
> them. This avoids stale ROB/QID/FID data re-use, which
> previously caused ordering issues.
> 
> Before this commit, release events left the history-list
> in an inconsistent state, and future events with op type of
> forward could be incorrectly reordered.
> 
> Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> 

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Couple of very minor style comments below.

> ---
> 
> v2:
> - Rework fix to simpler suggestion (Bruce)
> - Respin patchset to "apply order" (Bruce)
> 
> ---
>  drivers/event/sw/sw_evdev_scheduler.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/event/sw/sw_evdev_scheduler.c b/drivers/event/sw/sw_evdev_scheduler.c
> index de6ed21643..21c360770e 100644
> --- a/drivers/event/sw/sw_evdev_scheduler.c
> +++ b/drivers/event/sw/sw_evdev_scheduler.c
> @@ -90,8 +90,10 @@ sw_schedule_atomic_to_cq(struct sw_evdev *sw, struct sw_qid * const qid,
>  		sw->cq_ring_space[cq]--;
>  
>  		int head = (p->hist_head++ & (SW_PORT_HIST_LIST-1));
> -		p->hist_list[head].fid = flow_id;
> -		p->hist_list[head].qid = qid_id;
> +		p->hist_list[head] = (struct sw_hist_list_entry) {
> +			.qid = qid_id,
> +			.fid = flow_id,
> +		};
>  
>  		p->stats.tx_pkts++;
>  		qid->stats.tx_pkts++;
> @@ -162,8 +164,13 @@ sw_schedule_parallel_to_cq(struct sw_evdev *sw, struct sw_qid * const qid,
>  		qid->stats.tx_pkts++;
>  
>  		const int head = (p->hist_head & (SW_PORT_HIST_LIST-1));
> -		p->hist_list[head].fid = SW_HASH_FLOWID(qe->flow_id);
> -		p->hist_list[head].qid = qid_id;
> +		const uint32_t fid = SW_HASH_FLOWID(qe->flow_id);
> +		p->hist_list[head] = (struct sw_hist_list_entry) {
> +			.qid = qid_id,
> +			.fid = fid,
> +		};
> +
> +
You're adding some extra whitespace here. Can probably be fixed on apply.

And one final minor nit: if doing a new version, you can remove the
temporary variable for "fid". In my suggestion I only added it because I
had the assignment done on a single line and didn't want to wrap. Since
you've split the temporary struct across multiple lines, the temporary
variable isn't needed as the SW_HASH_FLOWID will fit on the assignment
line.

Re: [PATCH v2 2/2] event/sw: add selftest for ordered history list

[Bruce Richardson]

On Thu, Sep 14, 2023 at 11:58:52AM +0100, Harry van Haaren wrote:
> This commit adds a unit test for an issue identified
> where ordered history-list entries are not correctly
> cleared when the returned event is of op RELEASE type.
> 
> The result of the history-list bug is that a future event
> which re-uses that history-list slot, but has an op type
> of FORWARD will incorrectly be reordered.
> 
> The existing unit-tests did not cover the RELEASE of an
> ORDERED queue, and then stress-test the history-list by
> iterating HIST_LIST times afterwards.
> 
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> ---
>  drivers/event/sw/sw_evdev_selftest.c | 132 +++++++++++++++++++++++++++
>  1 file changed, 132 insertions(+)

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

[PATCH v3 1/2] event/sw: fix ordering corruption with op release

[Harry van Haaren]

This commit changes the logic in the scheduler to always
reset reorder-buffer (and QID/FID) entries when writing
them. This avoids stale ROB/QID/FID data re-use, which
previously caused ordering issues.

Before this commit, release events left the history-list
in an inconsistent state, and future events with op type of
forward could be incorrectly reordered.

There was a partial fix previously committed which is now
being resolved for all cases in a more general way, hence
the two fixlines here.

Fixes: 2e516d18dc01 ("event/sw: fix events mis-identified as needing reorder")
Fixes: 617995dfc5b2 ("event/sw: add scheduling logic")
Cc: stable@dpdk.org

Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

---

v3:
- Fixup whitespace and line wrapping suggestions (Bruce)
- Add Fixes lines (Bruce)
- Cc stable, as this is a functionality bugfix
- Including Ack from v2, as no significant code changes

v2:
- Rework fix to simpler suggestion (Bruce)
- Respin patchset to "apply order" (Bruce)
---
 drivers/event/sw/sw_evdev_scheduler.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/event/sw/sw_evdev_scheduler.c b/drivers/event/sw/sw_evdev_scheduler.c
index de6ed21643..cc652815e4 100644
--- a/drivers/event/sw/sw_evdev_scheduler.c
+++ b/drivers/event/sw/sw_evdev_scheduler.c
@@ -90,8 +90,10 @@ sw_schedule_atomic_to_cq(struct sw_evdev *sw, struct sw_qid * const qid,
 		sw->cq_ring_space[cq]--;
 
 		int head = (p->hist_head++ & (SW_PORT_HIST_LIST-1));
-		p->hist_list[head].fid = flow_id;
-		p->hist_list[head].qid = qid_id;
+		p->hist_list[head] = (struct sw_hist_list_entry) {
+			.qid = qid_id,
+			.fid = flow_id,
+		};
 
 		p->stats.tx_pkts++;
 		qid->stats.tx_pkts++;
@@ -162,8 +164,10 @@ sw_schedule_parallel_to_cq(struct sw_evdev *sw, struct sw_qid * const qid,
 		qid->stats.tx_pkts++;
 
 		const int head = (p->hist_head & (SW_PORT_HIST_LIST-1));
-		p->hist_list[head].fid = SW_HASH_FLOWID(qe->flow_id);
-		p->hist_list[head].qid = qid_id;
+		p->hist_list[head] = (struct sw_hist_list_entry) {
+			.qid = qid_id,
+			.fid = SW_HASH_FLOWID(qe->flow_id),
+		};
 
 		if (keep_order)
 			rob_ring_dequeue(qid->reorder_buffer_freelist,
@@ -419,7 +423,6 @@ __pull_port_lb(struct sw_evdev *sw, uint32_t port_id, int allow_reorder)
 				struct reorder_buffer_entry *rob_entry =
 						hist_entry->rob_entry;
 
-				hist_entry->rob_entry = NULL;
 				/* Although fragmentation not currently
 				 * supported by eventdev API, we support it
 				 * here. Open: How do we alert the user that
-- 
2.34.1

[PATCH v3 2/2] event/sw: add selftest for ordered history list

[Harry van Haaren]

This commit adds a unit test for an issue identified
where ordered history-list entries are not correctly
cleared when the returned event is of op RELEASE type.

The result of the history-list bug is that a future event
which re-uses that history-list slot, but has an op type
of FORWARD will incorrectly be reordered.

The existing unit-tests did not cover the RELEASE of an
ORDERED queue, and then stress-test the history-list by
iterating HIST_LIST times afterwards.

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

---

v3:
- Including Ack from v2
---
 drivers/event/sw/sw_evdev_selftest.c | 132 +++++++++++++++++++++++++++
 1 file changed, 132 insertions(+)

diff --git a/drivers/event/sw/sw_evdev_selftest.c b/drivers/event/sw/sw_evdev_selftest.c
index 3aa8d76ca8..59afa260c6 100644
--- a/drivers/event/sw/sw_evdev_selftest.c
+++ b/drivers/event/sw/sw_evdev_selftest.c
@@ -2959,6 +2959,132 @@ dev_stop_flush(struct test *t) /* test to check we can properly flush events */
 	return -1;
 }
 
+static int
+ordered_atomic_hist_completion(struct test *t)
+{
+	const int rx_enq = 0;
+	int err;
+
+	/* Create instance with 1 atomic QID going to 3 ports + 1 prod port */
+	if (init(t, 2, 2) < 0 ||
+			create_ports(t, 2) < 0 ||
+			create_ordered_qids(t, 1) < 0 ||
+			create_atomic_qids(t, 1) < 0)
+		return -1;
+
+	/* Helpers to identify queues */
+	const uint8_t qid_ordered = t->qid[0];
+	const uint8_t qid_atomic = t->qid[1];
+
+	/* CQ mapping to QID */
+	if (rte_event_port_link(evdev, t->port[1], &t->qid[0], NULL, 1) != 1) {
+		printf("%d: error mapping port 1 qid\n", __LINE__);
+		return -1;
+	}
+	if (rte_event_port_link(evdev, t->port[1], &t->qid[1], NULL, 1) != 1) {
+		printf("%d: error mapping port 1 qid\n", __LINE__);
+		return -1;
+	}
+	if (rte_event_dev_start(evdev) < 0) {
+		printf("%d: Error with start call\n", __LINE__);
+		return -1;
+	}
+
+	/* Enqueue 1x ordered event, to be RELEASE-ed by the worker
+	 * CPU, which may cause hist-list corruption (by not comleting)
+	 */
+	struct rte_event ord_ev = {
+		.op = RTE_EVENT_OP_NEW,
+		.queue_id = qid_ordered,
+		.event_type = RTE_EVENT_TYPE_CPU,
+		.priority = RTE_EVENT_DEV_PRIORITY_NORMAL,
+	};
+	err = rte_event_enqueue_burst(evdev, t->port[rx_enq], &ord_ev, 1);
+	if (err != 1) {
+		printf("%d: Failed to enqueue\n", __LINE__);
+		return -1;
+	}
+
+	/* call the scheduler. This schedules the above event as a single
+	 * event in an ORDERED queue, to the worker.
+	 */
+	rte_service_run_iter_on_app_lcore(t->service_id, 1);
+
+	/* Dequeue ORDERED event 0 from port 1, so that we can then drop */
+	struct rte_event ev;
+	if (!rte_event_dequeue_burst(evdev, t->port[1], &ev, 1, 0)) {
+		printf("%d: failed to dequeue\n", __LINE__);
+		return -1;
+	}
+
+	/* drop the ORDERED event. Here the history list should be completed,
+	 * but might not be if the hist-list bug exists. Call scheduler to make
+	 * it act on the RELEASE that was enqueued.
+	 */
+	rte_event_enqueue_burst(evdev, t->port[1], &release_ev, 1);
+	rte_service_run_iter_on_app_lcore(t->service_id, 1);
+
+	/* Enqueue 1x atomic event, to then FORWARD to trigger atomic hist-list
+	 * completion. If the bug exists, the ORDERED entry may be completed in
+	 * error (aka, using the ORDERED-ROB for the ATOMIC event). This is the
+	 * main focus of this unit test.
+	 */
+	{
+		struct rte_event ev = {
+			.op = RTE_EVENT_OP_NEW,
+			.queue_id = qid_atomic,
+			.event_type = RTE_EVENT_TYPE_CPU,
+			.priority = RTE_EVENT_DEV_PRIORITY_NORMAL,
+			.flow_id = 123,
+		};
+
+		err = rte_event_enqueue_burst(evdev, t->port[rx_enq], &ev, 1);
+		if (err != 1) {
+			printf("%d: Failed to enqueue\n", __LINE__);
+			return -1;
+		}
+	}
+	rte_service_run_iter_on_app_lcore(t->service_id, 1);
+
+	/* Deq ATM event, then forward it for more than HIST_LIST_SIZE times,
+	 * to re-use the history list entry that may be corrupted previously.
+	 */
+	for (int i = 0; i < SW_PORT_HIST_LIST + 2; i++) {
+		if (!rte_event_dequeue_burst(evdev, t->port[1], &ev, 1, 0)) {
+			printf("%d: failed to dequeue, did corrupt ORD hist "
+				"list steal this ATM event?\n", __LINE__);
+			return -1;
+		}
+
+		/* Re-enqueue the ATM event as FWD, trigger hist-list. */
+		ev.op = RTE_EVENT_OP_FORWARD;
+		err = rte_event_enqueue_burst(evdev, t->port[1], &ev, 1);
+		if (err != 1) {
+			printf("%d: Failed to enqueue\n", __LINE__);
+			return -1;
+		}
+
+		rte_service_run_iter_on_app_lcore(t->service_id, 1);
+	}
+
+	/* If HIST-LIST + N count of dequeues succeed above, the hist list
+	 * has not been corrupted. If it is corrupted, the ATM event is pushed
+	 * into the ORDERED-ROB and will not dequeue.
+	 */
+
+	/* release the ATM event that's been forwarded HIST_LIST times */
+	err = rte_event_enqueue_burst(evdev, t->port[1], &release_ev, 1);
+	if (err != 1) {
+		printf("%d: Failed to enqueue\n", __LINE__);
+		return -1;
+	}
+
+	rte_service_run_iter_on_app_lcore(t->service_id, 1);
+
+	cleanup(t);
+	return 0;
+}
+
 static int
 worker_loopback_worker_fn(void *arg)
 {
@@ -3388,6 +3514,12 @@ test_sw_eventdev(void)
 		printf("ERROR - Stop Flush test FAILED.\n");
 		goto test_fail;
 	}
+	printf("*** Running Ordered & Atomic hist-list completion test...\n");
+	ret = ordered_atomic_hist_completion(t);
+	if (ret != 0) {
+		printf("ERROR - Ordered & Atomic hist-list test FAILED.\n");
+		goto test_fail;
+	}
 	if (rte_lcore_count() >= 3) {
 		printf("*** Running Worker loopback test...\n");
 		ret = worker_loopback(t, 0);
-- 
2.34.1

Re: [PATCH v3 1/2] event/sw: fix ordering corruption with op release

[Jerin Jacob]

On Mon, Oct 2, 2023 at 11:36 PM Harry van Haaren
<harry.van.haaren@intel.com> wrote:
>
> This commit changes the logic in the scheduler to always
> reset reorder-buffer (and QID/FID) entries when writing
> them. This avoids stale ROB/QID/FID data re-use, which
> previously caused ordering issues.
>
> Before this commit, release events left the history-list
> in an inconsistent state, and future events with op type of
> forward could be incorrectly reordered.
>
> There was a partial fix previously committed which is now
> being resolved for all cases in a more general way, hence
> the two fixlines here.
>
> Fixes: 2e516d18dc01 ("event/sw: fix events mis-identified as needing reorder")
> Fixes: 617995dfc5b2 ("event/sw: add scheduling logic")
> Cc: stable@dpdk.org
>
> Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>


Series applied to dpdk-next-net-eventdev/for-main. Thanks


>
> ---
>
> v3:
> - Fixup whitespace and line wrapping suggestions (Bruce)
> - Add Fixes lines (Bruce)
> - Cc stable, as this is a functionality bugfix
> - Including Ack from v2, as no significant code changes
>
> v2:
> - Rework fix to simpler suggestion (Bruce)
> - Respin patchset to "apply order" (Bruce)
> ---
>  drivers/event/sw/sw_evdev_scheduler.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/event/sw/sw_evdev_scheduler.c b/drivers/event/sw/sw_evdev_scheduler.c
> index de6ed21643..cc652815e4 100644
> --- a/drivers/event/sw/sw_evdev_scheduler.c
> +++ b/drivers/event/sw/sw_evdev_scheduler.c
> @@ -90,8 +90,10 @@ sw_schedule_atomic_to_cq(struct sw_evdev *sw, struct sw_qid * const qid,
>                 sw->cq_ring_space[cq]--;
>
>                 int head = (p->hist_head++ & (SW_PORT_HIST_LIST-1));
> -               p->hist_list[head].fid = flow_id;
> -               p->hist_list[head].qid = qid_id;
> +               p->hist_list[head] = (struct sw_hist_list_entry) {
> +                       .qid = qid_id,
> +                       .fid = flow_id,
> +               };
>
>                 p->stats.tx_pkts++;
>                 qid->stats.tx_pkts++;
> @@ -162,8 +164,10 @@ sw_schedule_parallel_to_cq(struct sw_evdev *sw, struct sw_qid * const qid,
>                 qid->stats.tx_pkts++;
>
>                 const int head = (p->hist_head & (SW_PORT_HIST_LIST-1));
> -               p->hist_list[head].fid = SW_HASH_FLOWID(qe->flow_id);
> -               p->hist_list[head].qid = qid_id;
> +               p->hist_list[head] = (struct sw_hist_list_entry) {
> +                       .qid = qid_id,
> +                       .fid = SW_HASH_FLOWID(qe->flow_id),
> +               };
>
>                 if (keep_order)
>                         rob_ring_dequeue(qid->reorder_buffer_freelist,
> @@ -419,7 +423,6 @@ __pull_port_lb(struct sw_evdev *sw, uint32_t port_id, int allow_reorder)
>                                 struct reorder_buffer_entry *rob_entry =
>                                                 hist_entry->rob_entry;
>
> -                               hist_entry->rob_entry = NULL;
>                                 /* Although fragmentation not currently
>                                  * supported by eventdev API, we support it
>                                  * here. Open: How do we alert the user that
> --
> 2.34.1
>