[2/2] event/sw: fix ordering corruption with op release
Checks
Commit Message
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(-)
Comments
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,
> -----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>
@@ -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;
}