DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Simon Kågström" <simon.kagstrom@netinsight.net>
To: "Gonzalez Monroy, Sergio" <sergio.gonzalez.monroy@intel.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] rte_reorder: Allow sequence numbers > 0 as starting point
Date: Thu, 28 May 2015 10:15:57 +0200	[thread overview]
Message-ID: <5566CEBD.7040403@netinsight.net> (raw)
In-Reply-To: <5566C87B.8010703@intel.com>

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

  reply	other threads:[~2015-05-28  8:16 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-05-20 11:02 Simon Kagstrom
2015-05-28  7:49 ` Gonzalez Monroy, Sergio
2015-05-28  8:15   ` Simon Kågström [this message]
2015-05-28  9:36     ` Gonzalez Monroy, Sergio
2015-05-29 16:37 ` Gonzalez Monroy, Sergio
2015-06-22 20:28   ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5566CEBD.7040403@netinsight.net \
    --to=simon.kagstrom@netinsight.net \
    --cc=dev@dpdk.org \
    --cc=sergio.gonzalez.monroy@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).