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 A34A5A04E1; Tue, 22 Sep 2020 08:46:04 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 5F66A1D6CA; Tue, 22 Sep 2020 08:46:03 +0200 (CEST) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id 4919B1C2A9 for ; Tue, 22 Sep 2020 08:46:01 +0200 (CEST) IronPort-SDR: WLqGh2oHy0LZ2qNuGFfA1bRE03CkJi6ZV4QybdXul5ewR0q6/JFMCP/3yY9xdEPwEqHrZ6m/v4 wgnoVGPfmoow== X-IronPort-AV: E=McAfee;i="6000,8403,9751"; a="140027314" X-IronPort-AV: E=Sophos;i="5.77,289,1596524400"; d="scan'208";a="140027314" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Sep 2020 23:45:59 -0700 IronPort-SDR: sfu7jnJVMUZ2YL351qC9BaPQaqmR1b9sBpufMT+kURfUqwNE9gWJ2iSfkRA27uCr3sFOyVuEgE pON4YHX84mBg== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.77,289,1596524400"; d="scan'208";a="511046863" Received: from npg_dpdk_virtio_jiayuhu_15.sh.intel.com ([10.67.117.43]) by fmsmga005.fm.intel.com with ESMTP; 21 Sep 2020 23:45:58 -0700 Date: Tue, 22 Sep 2020 14:55:46 +0800 From: Jiayu Hu To: yang_y_yi Cc: jiayu.hu@intel.com, dev@dpdk.org, thomas@monjalon.net, yangyi01@inspur.com Message-ID: <20200922065546.GA56452@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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <75421977.3967.174b47b6259.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 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; > > > > > > > > > >