From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <sergio.gonzalez.monroy@intel.com>
Received: from mga09.intel.com (mga09.intel.com [134.134.136.24])
by dpdk.org (Postfix) with ESMTP id 3326E2EDA
for <dev@dpdk.org>; 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" <sergio.gonzalez.monroy@intel.com>
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==?= <simon.kagstrom@netinsight.net>
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 <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=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