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
