From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Gonzalez Monroy, Sergio" Subject: Re: [PATCH] rte_reorder: Allow sequence numbers > 0 as starting point Date: Thu, 28 May 2015 10:36:18 +0100 Message-ID: <5566E192.7020708@intel.com> References: <20150520130205.03ed30be@miho> <5566C87B.8010703@intel.com> <5566CEBD.7040403@netinsight.net> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Cc: dev@dpdk.org To: =?UTF-8?B?U2ltb24gS8OlZ3N0csO2bQ==?= Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 3326E2EDA for ; Thu, 28 May 2015 11:36:21 +0200 (CEST) In-Reply-To: <5566CEBD.7040403@netinsight.net> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 28/05/2015 09:15, Simon K=C3=A5gstr=C3=B6m 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 =3D &b->order_buf; >>> + if (!b->is_initialized) { >>> + b->min_seqn =3D mbuf->seqn; >>> + >>> + b->is_initialized =3D 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 no= t >> 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. No= w >> 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=20 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 patc= h > actually improves the documentation without touching it :-) Fair enough :) Sergio > // Simon