From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ernst.netinsight.se (ernst.netinsight.se [194.16.221.21]) by dpdk.org (Postfix) with SMTP id 5DC483237 for ; Thu, 28 May 2015 10:16:02 +0200 (CEST) Received: from [10.100.1.152] (unverified [10.100.1.152]) by ernst.netinsight.se (EMWAC SMTPRS 0.83) with SMTP id ; Thu, 28 May 2015 10:15:57 +0200 Message-ID: <5566CEBD.7040403@netinsight.net> Date: Thu, 28 May 2015 10:15:57 +0200 From: =?UTF-8?B?U2ltb24gS8OlZ3N0csO2bQ==?= User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: "Gonzalez Monroy, Sergio" References: <20150520130205.03ed30be@miho> <5566C87B.8010703@intel.com> In-Reply-To: <5566C87B.8010703@intel.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH] rte_reorder: Allow sequence numbers > 0 as starting point X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 28 May 2015 08:16:02 -0000 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