From mboxrd@z Thu Jan 1 00:00:00 1970 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) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga102.jf.intel.com with ESMTP; 28 May 2015 02:36:21 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,512,1427785200"; d="scan'208";a="701475881" Received: from smonroyx-mobl.ger.corp.intel.com (HELO [10.237.220.42]) ([10.237.220.42]) by orsmga001.jf.intel.com with ESMTP; 28 May 2015 02:36:19 -0700 Message-ID: <5566E192.7020708@intel.com> Date: Thu, 28 May 2015 10:36:18 +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: =?UTF-8?B?U2ltb24gS8OlZ3N0csO2bQ==?= References: <20150520130205.03ed30be@miho> <5566C87B.8010703@intel.com> <5566CEBD.7040403@netinsight.net> In-Reply-To: <5566CEBD.7040403@netinsight.net> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 09:36:21 -0000 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