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
