rte_reorder: Allow sequence numbers > 0 as starting point

We use sequence numbers from a generator which has potentially started long before the receiver. Therefore, the first number will typically be > 0. The rte_reorder code will not work in this case, since the packet is seen as outside of the buffer.

The patch instead records the first sequence number inserted as the starting point.

Signed-off-by Simon Kagstrom <[email protected]>

Signed-off-by Johan Faltstrom <[email protected]>

---
  lib/librte_reorder/rte_reorder.c | 8 ++++++++
  1 file changed, 8 insertions(+)

lib/librte_reorder/rte_reorder.c +2 -0

index dc0e806..4d6449e 100644
@@ -73,6 +73,8 @@ struct rte_reorder_buffer {
  	unsigned int memsize; /**< memory area size of reorder buffer */
  	struct cir_buffer ready_buf; /**< temp buffer for dequeued entries */
  	struct cir_buffer order_buf; /**< buffer used to reorder entries */
+
+	int is_initialized;
  } __rte_cache_aligned;
 
  static void

@@ -325,6 +327,12 @@ rte_reorder_insert(struct rte_reorder_buffer *b, struct rte_mbuf *mbuf)
  	uint32_t offset, position;
  	struct cir_buffer *order_buf = &b->order_buf;

Quoted 8 lines

lib/librte_reorder/rte_reorder.c +3 -0

@@ -325,6 +327,12 @@ rte_reorder_insert(struct rte_reorder_buffer *b,
 struct rte_mbuf *mbuf)
      uint32_t offset, position;
      struct cir_buffer *order_buf = &b->order_buf;
  +    if (!b->is_initialized) {
+        b->min_seqn = mbuf->seqn;
+
+        b->is_initialized = 1;

lib/librte_reorder/rte_reorder.c rte_reorder_insert(struct rte_reorder_buffer *b, struct rte_mbuf *mbuf) · line 338

Quoted 5 lines

lib/librte_reorder/rte_reorder.c +2 -0

+    }
+
      /*
       * calculate the offset from the head pointer we need to go.
       * The subtraction takes care of the sequence number wrapping.

So my first impression was, why do this in insert instead of init? I guess the goal was trying to avoid changing the API, but would it not be worth it? after all is a one time thing only.

About the implementation, packets being inserted could be out of order, so the first packet inserted may not be the first in your sequence. Now what happens with that packet would be app specific so probably is not a big deal but what about initializing min_seqn to something like (mbuf->seqn - b->size/2) ? That would give enough room for packets out of order.

You should also update the documentation regarding rte_reorder_insert.

Thanks for the review, Sergio!

Quoted 8 lines · Sergio

Quoted 8 lines

lib/librte_reorder/rte_reorder.c +3 -0

@@ -325,6 +327,12 @@ rte_reorder_insert(struct rte_reorder_buffer *b,
 struct rte_mbuf *mbuf)
       uint32_t offset, position;
       struct cir_buffer *order_buf = &b->order_buf;
   +    if (!b->is_initialized) {
+        b->min_seqn = mbuf->seqn;
+
+        b->is_initialized = 1;

Quoted 8 lines

lib/librte_reorder/rte_reorder.c rte_reorder_insert(struct rte_reorder_buffer *b, struct rte_mbuf *mbuf) · line 338

Quoted 5 lines

lib/librte_reorder/rte_reorder.c +2 -0

+    }
+
       /*
        * calculate the offset from the head pointer we need to go.
        * The subtraction takes care of the sequence number wrapping.

So my first impression was, why do this in insert instead of init? I guess the goal was trying to avoid changing the API, but would it not be worth it? after all is a one time thing only.

We don't know the first sequence number until the first insert, so I think it has to be there. Alternatively, there could be an API to set the minimum sequence number, but I think that would instead make the application uglier, and isn't that also just exposing library implementation details in the API?

Quoted 6 lines

About the implementation, packets being inserted could be out of order, so the first packet inserted may not be the first in your sequence. Now what happens with that packet would be app specific so probably is not a big deal but what about initializing min_seqn to something like (mbuf->seqn - b->size/2) ? That would give enough room for packets out of order.

I thought about that, but you will always miss some packets if you have an active stream at start anyway, so in the end I removed that part.

But perhaps you are right about this issue, I'm not sure.

Quoted 1 line

You should also update the documentation regarding rte_reorder_insert.

Actually, the rte_reorder.h file says nothing about the (current) limitation of the first seq number having to be 0, so I think this patch actually improves the documentation without touching it :-)

// Simon

We use sequence numbers from a generator which has potentially started long before the receiver. Therefore, the first number will typically be > 0. The rte_reorder code will not work in this case, since the packet is seen as outside of the buffer.

The patch instead records the first sequence number inserted as the starting point.

Signed-off-by Simon Kagstrom <[email protected]>

Signed-off-by Johan Faltstrom <[email protected]>

---
  lib/librte_reorder/rte_reorder.c | 8 ++++++++
  1 file changed, 8 insertions(+)

lib/librte_reorder/rte_reorder.c +2 -0

index dc0e806..4d6449e 100644
@@ -73,6 +73,8 @@ struct rte_reorder_buffer {
  	unsigned int memsize; /**< memory area size of reorder buffer */
  	struct cir_buffer ready_buf; /**< temp buffer for dequeued entries */
  	struct cir_buffer order_buf; /**< buffer used to reorder entries */
+
+	int is_initialized;
  } __rte_cache_aligned;
 
  static void

@@ -325,6 +327,12 @@ rte_reorder_insert(struct rte_reorder_buffer *b, struct rte_mbuf *mbuf)
  	uint32_t offset, position;
  	struct cir_buffer *order_buf = &b->order_buf;

Quoted 5 lines

We use sequence numbers from a generator which has potentially started long before the receiver. Therefore, the first number will typically be > 0. The rte_reorder code will not work in this case, since the packet is seen as outside of the buffer.

Quoted 5 lines

The patch instead records the first sequence number inserted as the starting point.

Signed-off-by Simon Kagstrom <[email protected]>

Signed-off-by Johan Faltstrom <[email protected]>

Acked-by Sergio Gonzalez Monroy <[email protected]>