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 906D6A04E1; Tue, 22 Sep 2020 08:14:11 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 627101D69A; Tue, 22 Sep 2020 08:14:11 +0200 (CEST) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id C52C81D674 for ; Tue, 22 Sep 2020 08:14:09 +0200 (CEST) IronPort-SDR: h++CmJ5MYyOrHsIdZmKMggNnyyTljjnhKOaF1nf1EPdZHM5o96K27Tm597zoO9rZ8PZoNJmJ3Z nQep4MUC2+1A== X-IronPort-AV: E=McAfee;i="6000,8403,9751"; a="161466802" X-IronPort-AV: E=Sophos;i="5.77,289,1596524400"; d="scan'208,217";a="161466802" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Sep 2020 23:14:08 -0700 IronPort-SDR: OrZvXj5AOx0JdkBFWVzXcr8eU6xuggPq42I/01dWhyGV2Hf9FuJhtILkEs2n/6MBoHXn2vFpOT ScDy5EOW2jMQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.77,289,1596524400"; d="scan'208,217";a="334876026" Received: from fmsmsx604.amr.corp.intel.com ([10.18.126.84]) by fmsmga004.fm.intel.com with ESMTP; 21 Sep 2020 23:14:02 -0700 Received: from shsmsx603.ccr.corp.intel.com (10.109.6.143) by fmsmsx604.amr.corp.intel.com (10.18.126.84) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Mon, 21 Sep 2020 23:14:01 -0700 Received: from shsmsx606.ccr.corp.intel.com (10.109.6.216) by SHSMSX603.ccr.corp.intel.com (10.109.6.143) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Tue, 22 Sep 2020 14:14:00 +0800 Received: from shsmsx606.ccr.corp.intel.com ([10.109.6.216]) by SHSMSX606.ccr.corp.intel.com ([10.109.6.216]) with mapi id 15.01.1713.004; Tue, 22 Sep 2020 14:14:00 +0800 From: "Hu, Jiayu" To: yang_y_yi CC: "dev@dpdk.org" , "thomas@monjalon.net" , "yangyi01@inspur.com" Thread-Topic: Re:Re: [dpdk-dev] [PATCH v6 2/3] gro: add VXLAN UDP/IPv4 GRO support Thread-Index: AQHWkIHePRzYkXkJS0Ks+yvAcnC6w6lz/7cw Date: Tue, 22 Sep 2020 06:14:00 +0000 Message-ID: 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> In-Reply-To: <1f01947d.122e.174b37b5d58.Coremail.yang_y_yi@163.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-reaction: no-action dlp-version: 11.5.1.3 dlp-product: dlpe-windows x-originating-ip: [10.239.127.36] MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.15 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" 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=3D0, frag[1].frag_oft=3D4, frag[2].frag_oft=3D6; 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 flushe= d. Because the timestamp of items[0] is greater than 10, the left two fragment= s 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 suppo= rt Importance: High BTW, start_time is checked for the first packet in a flow, gro_udp4_merge_i= tems(tbl, j) will merge all the packets in this flow once if they can be re= assembled, gro_udp4_merge_items(tbl, j) doesn't check start_time, so this s= till 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 mo= re packets if we don't follow flush_timestamp. So an ideal change can be th= is. 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 =3D tbl->flows[i].start_index; > while (j !=3D INVALID_ARRAY_INDEX) { >- if (tbl->items[j].start_time <=3D flush_timestamp)= { > gro_udp4_merge_items(tbl, j); > out[k++] =3D tbl->items[j].firstseg; > if (tbl->items[j].nb_merged > 1) >@@ -407,12 +406,6 @@ > > if (unlikely(k =3D=3D nb_out)) > return k; >- } else >- /* >- * The left packets in this flow won't be >- * timeout. Go to check other flows. >- */ >- break; > } > } > return k; >