[dpdk-dev] 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 <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(+)
  

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
  

Thanks for the review, Sergio!

On 2015-05-28 09:49, Gonzalez Monroy, Sergio wrote:
>> @@ -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.

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?

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

> 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
  

On 28/05/2015 09:15, Simon Kågström wrote:
> Thanks for the review, Sergio!
>
> On 2015-05-28 09:49, Gonzalez Monroy, Sergio wrote:
>>> @@ -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.
> 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?
Yes, I agree.
>> 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.
As you said, it would not make much difference from the stream point of 
view.
> But perhaps you are right about this issue, I'm not sure.
>
>> 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 :-)
Fair enough :)

Sergio
> // Simon
  

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

Acked-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>
  

2015-05-29 17:37, Gonzalez Monroy, Sergio:
> 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.
> >
> > 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>
> 
> Acked-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>

Applied, thanks