DPDK patches and discussions
 help / color / mirror / Atom feed
From: yang_y_yi  <yang_y_yi@163.com>
To: "Jiayu Hu" <jiayu.hu@intel.com>
Cc: dev@dpdk.org, thomas@monjalon.net, yangyi01@inspur.com
Subject: Re: [dpdk-dev] [PATCH v6 2/3] gro: add VXLAN UDP/IPv4 GRO support
Date: Wed, 23 Sep 2020 10:28:00 +0800 (CST)
Message-ID: <56bb5989.20af.174b8ca00c3.Coremail.yang_y_yi@163.com> (raw)
In-Reply-To: <20200923021511.GA83995@NPG_DPDK_VIRTIO_jiayuhu_15.sh.intel.com>

Thanks Jiayu, do you mean not comparing timestamp and flush all the packets in a flow as I showed code, right? If so, we shouldn't provide argument flush_timestamp. But this will result in very bad issues, rte_gro_timeout_flush will be called after rte_gro_reassemble every time, so the result may be you can't reassemble out any original UDP packet because every UDP fragments will be flushed very soon, no chance to reassemble.

At 2020-09-23 10:15:12, "Jiayu Hu" <jiayu.hu@intel.com> wrote:
>On Tue, Sep 22, 2020 at 03:38:29PM +0800, yang_y_yi wrote:
>> The problem is timestamp of which one item in a flow we should use as timestamp
>> reference base for this flow, for merge (reassemble), only the first packet can
>> trigger merge of all the packets, merge is forward not backward, if you
>> traverse the whole linked list in this flow to get the oldest timestamp and
>> compare it with flushtime, that will be not worthy, in the worst case (say I
>> send a 64K UDP packet and MTU is 1450), you will have 46 items to check to get
>> the oldest timestamp by linked list.
>
>OK, I got the point. I agree to flush packets without strictly
>obeying timestamp. But you need to change the comment in the
>code and clarify the design for both UDP and VxLAN GRO patch.
>Current comment "The left packets in ..." is not appropriate
>for your design.
>
>> 
>> 
>> I'm not sure what inconsistentcy you're saying mean.
>> 
>> At 2020-09-22 14:55:46, "Jiayu Hu" <jiayu.hu@intel.com> wrote:
>> >On Tue, Sep 22, 2020 at 02:23:39PM +0800, yang_y_yi wrote:
>> >> Not a question, in next flush, they will be flushed, we have to check timestamp
>> >> in the first time unless we don't strictly follow this time limitation.
>> >
>> >who will check the timestamp? I did't get the point.
>> >
>> >IMO, this will cause inconsistency of the rte_gro_timeout_flush().
>> >BTW, what stops you to traverse all items and check timestamp
>> >before flush them out?
>> >
>> >>
>> >> At 2020-09-22 14:14:00, "Hu, Jiayu" <jiayu.hu@intel.com> wrote:
>> >>
>> >>     Fragments of a flow are sorted by frag_oft, but they may have different
>> >>
>> >>     timestamp. For example, there are three fragments, whose frag_oft is:
>> >>
>> >>     frag[0].frag_oft=0, frag[1].frag_oft=4, frag[2].frag_oft=6; and they are
>> >>
>> >>     fragments of one UDP packet but are not neighbors. In the first RX burst,
>> >>
>> >>     host receives frag[1] and calls rte_gro_reassemble(), and we assume the
>> >>
>> >>     timestamp of frag[1] is 10; in the second RX burst, host receives frag[0]
>> >>
>> >>     and also call rte_gro_reassemble(), and timestamp of frag[0] is 11; the
>> >>
>> >>     third time, host receives frag[2] and timestamp of frag[2] is 12. The three
>> >>
>> >>     fragments are stored in three items of a UDP GRO table:
>> >>
>> >>     items[0]: frag[0], timestamp is 11
>> >>
>> >>     items[1]: frag[1], timestamp is 10
>> >>
>> >>     items[2]: frag[2], timestamp is 12
>> >>
>> >>     Now we want to flush packets whose timestamp is less than or equal to
>> >>
>> >>     10. frag[1] should be returned, but in your code, no packets will be
>> >>     flushed.
>> >>
>> >>     Because the timestamp of items[0] is greater than 10, the left two
>> >>     fragments
>> >>
>> >>     will not be checked. This is what I want to say.
>> >>
>> >>
>> >>
>> >>     From: yang_y_yi <yang_y_yi@163.com>
>> >>     Sent: Tuesday, September 22, 2020 9:44 AM
>> >>     To: Hu, Jiayu <jiayu.hu@intel.com>
>> >>     Cc: dev@dpdk.org; thomas@monjalon.net; yangyi01@inspur.com
>> >>     Subject: Re:Re: [dpdk-dev] [PATCH v6 2/3] gro: add VXLAN UDP/IPv4 GRO
>> >>     support
>> >>     Importance: High
>> >>
>> >>
>> >>
>> >>     BTW, start_time is checked for the first packet in a flow,
>> >>     gro_udp4_merge_items(tbl, j) will merge all the packets in this flow once
>> >>     if they can be reassembled, gro_udp4_merge_items(tbl, j) doesn't check
>> >>     start_time, so this still can let some new items in this flow have chance
>> >>     to be merged.
>> >>
>> >>     At 2020-09-22 09:29:38, "yang_y_yi" <yang_y_yi@163.com> wrote:
>> >>
>> >>     >Thanks Jiayu, I have fixed other comments except this one:
>> >>
>> >>     >
>> >>
>> >>     >
>> >>
>> >>     >
>> >>
>> >>     >>The items of a flow are ordered by frag_oft, and start_time
>> >>
>> >>     >>of these items is not always in ascending order. Therefore,
>> >>
>> >>     >>you cannot skip checking the items after the item whose
>> >>
>> >>     >>start_time is greater than flush_timestamp. This issue also
>> >>
>> >>     >>exists in UDP/IPv4 GRO, and need to correct them both.
>> >>
>> >>     >
>> >>
>> >>     >
>> >>
>> >>     >I think the issue here is if we should strictly follow flush_timestamp, it is possible there are new items in items chain. we have chance to merge more packets if we don't follow flush_timestamp. So an ideal change can be this. But is it acceptible if we don't use flush_timestamp? It can flush some packets in advance therefore miss next merge window. Maybe current way is most resonable and a tradeoff between two exterem cases.
>> >>
>> >>     >
>> >>
>> >>     >
>> >>
>> >>     >
>> >>
>> >>     >
>> >>
>> >>     >
>> >>
>> >>     >diff --git a/lib/librte_gro/gro_udp4.c b/lib/librte_gro/gro_udp4.c
>> >>
>> >>     >index 061e7b0..ffa35a2 100644
>> >>
>> >>     >--- a/lib/librte_gro/gro_udp4.c
>> >>
>> >>     >+++ b/lib/librte_gro/gro_udp4.c
>> >>
>> >>     >@@ -391,7 +391,6 @@
>> >>
>> >>     >
>> >>
>> >>     >                j = tbl->flows[i].start_index;
>> >>
>> >>     >                while (j != INVALID_ARRAY_INDEX) {
>> >>
>> >>     >-                       if (tbl->items[j].start_time <= flush_timestamp) {
>> >>
>> >>     >                                gro_udp4_merge_items(tbl, j);
>> >>
>> >>     >                                out[k++] = tbl->items[j].firstseg;
>> >>
>> >>     >                                if (tbl->items[j].nb_merged > 1)
>> >>
>> >>     >@@ -407,12 +406,6 @@
>> >>
>> >>     >
>> >>
>> >>     >                                if (unlikely(k == nb_out))
>> >>
>> >>     >                                        return k;
>> >>
>> >>     >-                       } else
>> >>
>> >>     >-                               /*
>> >>
>> >>     >-                                * The left packets in this flow won't be
>> >>
>> >>     >-                                * timeout. Go to check other flows.
>> >>
>> >>     >-                                */
>> >>
>> >>     >-                               break;
>> >>
>> >>     >                }
>> >>
>> >>     >        }
>> >>
>> >>     >        return k;
>> >>
>> >>     >
>> >>
>> >>
>> >>
>> >>
>> >>
>> >>
>> >>
>> 
>> 
>> 
>>  
>> 

  reply	other threads:[~2020-09-23  2:28 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-17  3:49 [dpdk-dev] [PATCH v6 0/3] gro: add UDP/IPv4 GRO and " yang_y_yi
2020-09-17  3:49 ` [dpdk-dev] [PATCH v6 1/3] gro: add " yang_y_yi
2020-09-21  6:21   ` Hu, Jiayu
2020-09-17  3:49 ` [dpdk-dev] [PATCH v6 2/3] gro: add VXLAN " yang_y_yi
2020-09-21  7:54   ` Hu, Jiayu
2020-09-22  1:29     ` yang_y_yi
2020-09-22  1:44       ` yang_y_yi
2020-09-22  6:14         ` Hu, Jiayu
2020-09-22  6:23           ` yang_y_yi
2020-09-22  6:55             ` Jiayu Hu
2020-09-22  7:38               ` yang_y_yi
2020-09-23  2:15                 ` Jiayu Hu
2020-09-23  2:28                   ` yang_y_yi [this message]
2020-09-23  2:43                     ` Jiayu Hu
2020-09-24  2:41                       ` yang_y_yi
2020-09-22  3:01       ` Jiayu Hu
2020-09-22  3:00         ` yang_y_yi
2020-09-17  3:49 ` [dpdk-dev] [PATCH v6 3/3] doc: update prog_guide and rel_notes for GRO yang_y_yi

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=56bb5989.20af.174b8ca00c3.Coremail.yang_y_yi@163.com \
    --to=yang_y_yi@163.com \
    --cc=dev@dpdk.org \
    --cc=jiayu.hu@intel.com \
    --cc=thomas@monjalon.net \
    --cc=yangyi01@inspur.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

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git