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

["Simon Kågström" <simon.kagstrom@netinsight.net>]

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