From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 9FEE8A04B5; Wed, 23 Sep 2020 04:05:24 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id F1A201DB57; Wed, 23 Sep 2020 04:05:23 +0200 (CEST) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 74F991DB53 for ; Wed, 23 Sep 2020 04:05:22 +0200 (CEST) IronPort-SDR: cbZpiAg1PR4Sdm+EyYttZIjp1hYWqp32a3DZ8ec/lX5Z5dahrdaULEhVFtQyLbONCeQkbU8pZf 2apo0O6ilOEw== X-IronPort-AV: E=McAfee;i="6000,8403,9752"; a="148419541" X-IronPort-AV: E=Sophos;i="5.77,292,1596524400"; d="scan'208";a="148419541" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Sep 2020 19:05:21 -0700 IronPort-SDR: uAY6iu5s6DSXA5PI+RX/OfmMFLuy72kgzmEi5PoXnkzh7dFAhOF+yIR/JPIWdvu9mVMCoGfOFU kjlzgDgbO8dw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.77,292,1596524400"; d="scan'208";a="309717757" Received: from npg_dpdk_virtio_jiayuhu_15.sh.intel.com ([10.67.117.43]) by orsmga006.jf.intel.com with ESMTP; 22 Sep 2020 19:05:19 -0700 Date: Wed, 23 Sep 2020 10:15:12 +0800 From: Jiayu Hu To: yang_y_yi Cc: dev@dpdk.org, thomas@monjalon.net, yangyi01@inspur.com, jiayu.hu@intel.com Message-ID: <20200923021511.GA83995@NPG_DPDK_VIRTIO_jiayuhu_15.sh.intel.com> References: <20200917034959.194372-1-yang_y_yi@163.com> <20200917034959.194372-3-yang_y_yi@163.com> <37c12b46.f2a.174b36e34c1.Coremail.yang_y_yi@163.com> <1f01947d.122e.174b37b5d58.Coremail.yang_y_yi@163.com> <75421977.3967.174b47b6259.Coremail.yang_y_yi@163.com> <20200922065546.GA56452@NPG_DPDK_VIRTIO_jiayuhu_15.sh.intel.com> <7b4fc638.486f.174b4bfe6c7.Coremail.yang_y_yi@163.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <7b4fc638.486f.174b4bfe6c7.Coremail.yang_y_yi@163.com> User-Agent: Mutt/1.7.1 (2016-10-04) Subject: Re: [dpdk-dev] [PATCH v6 2/3] gro: add VXLAN UDP/IPv4 GRO support X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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" 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" 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 > >> Sent: Tuesday, September 22, 2020 9:44 AM > >> To: Hu, Jiayu > >> 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" 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; > >> > >> > > >> > >> > >> > >> > >> > >> > >> > > > > >