Re: [dpdk-dev] [PATCH] rte_reorder: Allow sequence numbers > 0 as starting point

["Gonzalez Monroy, Sergio" <sergio.gonzalez.monroy@intel.com>]

Sorry for the delay :)

On 20/05/2015 12:02, Simon Kagstrom wrote:
> 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.
Yep, that is a flaw in the current implementation.
> The patch instead records the first sequence number inserted as the
> starting point.
>
> Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
> Signed-off-by: Johan Faltstrom <johan.faltstrom@netinsight.net>
> ---
>   lib/librte_reorder/rte_reorder.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/lib/librte_reorder/rte_reorder.c b/lib/librte_reorder/rte_reorder.c
> index dc0e806..4d6449e 100644
> --- a/lib/librte_reorder/rte_reorder.c
> +++ b/lib/librte_reorder/rte_reorder.c
> @@ -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;
>   
> +	if (!b->is_initialized) {
> +	    b->min_seqn = mbuf->seqn;
> +
> +	    b->is_initialized = 1;
> +	}
> +
>   	/*
>   	 * 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,
Sergio