From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 2B3FA3237 for ; Thu, 28 May 2015 09:49:18 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga103.fm.intel.com with ESMTP; 28 May 2015 00:49:17 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,511,1427785200"; d="scan'208";a="736599191" Received: from smonroyx-mobl.ger.corp.intel.com (HELO [10.237.220.42]) ([10.237.220.42]) by orsmga002.jf.intel.com with ESMTP; 28 May 2015 00:49:16 -0700 Message-ID: <5566C87B.8010703@intel.com> Date: Thu, 28 May 2015 08:49:15 +0100 From: "Gonzalez Monroy, Sergio" User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Simon Kagstrom References: <20150520130205.03ed30be@miho> In-Reply-To: <20150520130205.03ed30be@miho> Content-Type: text/plain; charset=UTF-8; format=flowed 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 07:49:18 -0000 Sorry for the delay :) On 20/05/2015 12:02, Simon Kagstrom wrote: > We use sequence numbers from a generator which has potentially started > long before the receiver. Therefore, the first number will typically > be > 0. The rte_reorder code will not work in this case, since the > packet is seen as outside of the buffer. Yep, that is a flaw in the current implementation. > The patch instead records the first sequence number inserted as the > starting point. > > Signed-off-by: Simon Kagstrom > Signed-off-by: Johan Faltstrom > --- > lib/librte_reorder/rte_reorder.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/lib/librte_reorder/rte_reorder.c b/lib/librte_reorder/rte_reorder.c > index dc0e806..4d6449e 100644 > --- a/lib/librte_reorder/rte_reorder.c > +++ b/lib/librte_reorder/rte_reorder.c > @@ -73,6 +73,8 @@ struct rte_reorder_buffer { > unsigned int memsize; /**< memory area size of reorder buffer */ > struct cir_buffer ready_buf; /**< temp buffer for dequeued entries */ > struct cir_buffer order_buf; /**< buffer used to reorder entries */ > + > + int is_initialized; > } __rte_cache_aligned; > > static void > @@ -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. 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. You should also update the documentation regarding rte_reorder_insert. Thanks, Sergio