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 DF396A04B7; Mon, 21 Sep 2020 09:54:43 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id BC9361D41F; Mon, 21 Sep 2020 09:54:42 +0200 (CEST) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id 0B38D1C1D7 for ; Mon, 21 Sep 2020 09:54:40 +0200 (CEST) IronPort-SDR: PtX9owjFpzI8GfPcVIz8/s7QGdpONwOOX+iE7wFnPtFYUdGb8bZqcT0BDkbRdMpvHTsYEXezRG WCMSvrL/TaDw== X-IronPort-AV: E=McAfee;i="6000,8403,9750"; a="245162729" X-IronPort-AV: E=Sophos;i="5.77,286,1596524400"; d="scan'208";a="245162729" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Sep 2020 00:54:39 -0700 IronPort-SDR: 2Av6XWFVkrpwNrjtl1Uo5aXOEXoo1WXMJ9/MpPWY2crruo4V1yCd+lWEAO0EG8DPw/AJkEpwL2 8u8FWmy+0sKA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.77,286,1596524400"; d="scan'208";a="485335864" Received: from fmsmsx605.amr.corp.intel.com ([10.18.126.85]) by orsmga005.jf.intel.com with ESMTP; 21 Sep 2020 00:54:38 -0700 Received: from shsmsx602.ccr.corp.intel.com (10.109.6.142) by fmsmsx605.amr.corp.intel.com (10.18.126.85) 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 00:54:38 -0700 Received: from shsmsx606.ccr.corp.intel.com (10.109.6.216) by SHSMSX602.ccr.corp.intel.com (10.109.6.142) 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 15:54:36 +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; Mon, 21 Sep 2020 15:54:36 +0800 From: "Hu, Jiayu" To: "yang_y_yi@163.com" , "dev@dpdk.org" CC: "thomas@monjalon.net" , "yangyi01@inspur.com" Thread-Topic: [PATCH v6 2/3] gro: add VXLAN UDP/IPv4 GRO support Thread-Index: AQHWjKWlr/3UxnOt1kagur1wpaFFp6lys1LA Date: Mon, 21 Sep 2020 07:54:36 +0000 Message-ID: References: <20200917034959.194372-1-yang_y_yi@163.com> <20200917034959.194372-3-yang_y_yi@163.com> In-Reply-To: <20200917034959.194372-3-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] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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" Hi Yi, Some comments are inline. Thanks, Jiayu > -----Original Message----- > From: yang_y_yi@163.com > Sent: Thursday, September 17, 2020 11:50 AM > To: dev@dpdk.org > Cc: Hu, Jiayu ; thomas@monjalon.net; > yangyi01@inspur.com; yang_y_yi@163.com > Subject: [PATCH v6 2/3] gro: add VXLAN UDP/IPv4 GRO support >=20 > From: Yi Yang >=20 > VXLAN UDP/IPv4 GRO can help improve VM-to-VM UDP > performance when UFO or GSO is enabled in VM, GRO > must be supported if UFO or GSO is enabled, > otherwise, performance can't get big improvement > if only GSO is there. >=20 > With this enabled in DPDK, OVS DPDK can leverage it > to improve VM-to-VM UDP performance, it will reassemble > VXLAN UDP/IPv4 fragments immediate after they are > received from a physical NIC. It is very helpful in > OVS DPDK VXLAN use case. >=20 > Note: outer IP ID isn't used to check if two packets > are same flow and can be merged because the difference > between outer IP IDs of two packets isn't always +/-1 > in case of OVS DPDK. >=20 > Signed-off-by: Yi Yang > --- > lib/librte_gro/gro_udp4.h | 1 + > lib/librte_gro/gro_vxlan_udp4.c | 542 > ++++++++++++++++++++++++++++++++++++++++ > lib/librte_gro/gro_vxlan_udp4.h | 154 ++++++++++++ > lib/librte_gro/meson.build | 2 +- > lib/librte_gro/rte_gro.c | 115 +++++++-- > lib/librte_gro/rte_gro.h | 3 + > 6 files changed, 790 insertions(+), 27 deletions(-) > create mode 100644 lib/librte_gro/gro_vxlan_udp4.c > create mode 100644 lib/librte_gro/gro_vxlan_udp4.h >=20 > diff --git a/lib/librte_gro/gro_udp4.h b/lib/librte_gro/gro_udp4.h > index 0a078e4..d38b393 100644 > --- a/lib/librte_gro/gro_udp4.h > +++ b/lib/librte_gro/gro_udp4.h > @@ -7,6 +7,7 @@ >=20 > #include > #include > +#include >=20 > #define INVALID_ARRAY_INDEX 0xffffffffUL > #define GRO_UDP4_TBL_MAX_ITEM_NUM (1024UL * 1024UL) > diff --git a/lib/librte_gro/gro_vxlan_udp4.c b/lib/librte_gro/gro_vxlan_u= dp4.c > new file mode 100644 > index 0000000..4eece56 > --- /dev/null > +++ b/lib/librte_gro/gro_vxlan_udp4.c > @@ -0,0 +1,542 @@ > + > +uint16_t > +gro_vxlan_udp4_tbl_timeout_flush(struct gro_vxlan_udp4_tbl *tbl, > + uint64_t flush_timestamp, > + struct rte_mbuf **out, > + uint16_t nb_out) > +{ > + uint16_t k =3D 0; > + uint32_t i, j; > + uint32_t max_flow_num =3D tbl->max_flow_num; > + > + for (i =3D 0; i < max_flow_num; i++) { > + if (unlikely(tbl->flow_num =3D=3D 0)) > + return k; > + > + j =3D tbl->flows[i].start_index; > + while (j !=3D INVALID_ARRAY_INDEX) { > + if (tbl->items[j].inner_item.start_time <=3D > + flush_timestamp) { > + gro_vxlan_udp4_merge_items(tbl, j); > + out[k++] =3D tbl->items[j].inner_item.firstseg; > + if (tbl->items[j].inner_item.nb_merged > 1) > + update_vxlan_header(&(tbl- > >items[j])); > + /* > + * Delete the item and get the next packet > + * index. > + */ > + j =3D delete_item(tbl, j, INVALID_ARRAY_INDEX); > + tbl->flows[i].start_index =3D j; > + if (j =3D=3D INVALID_ARRAY_INDEX) > + tbl->flow_num--; > + > + if (unlikely(k =3D=3D nb_out)) > + return k; > + } else > + /* > + * The left packets in the flow won't be > + * timeout. Go to check other flows. > + */ > + break; 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. > + } > + } > + return k; > +} > diff --git a/lib/librte_gro/gro_vxlan_udp4.h > b/lib/librte_gro/gro_vxlan_udp4.h > new file mode 100644 > index 0000000..6a42fb3 > --- /dev/null > +++ b/lib/librte_gro/gro_vxlan_udp4.h > @@ -0,0 +1,154 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2020 Inspur Corporation > + */ > + > +#ifndef _GRO_VXLAN_UDP4_H_ > +#define _GRO_VXLAN_UDP4_H_ > + > +#include "gro_udp4.h" > + > +#define GRO_VXLAN_UDP4_TBL_MAX_ITEM_NUM (1024UL * 1024UL) > + > +/* Header fields representing a VxLAN flow */ > +struct vxlan_udp4_flow_key { > + struct udp4_flow_key inner_key; > + struct rte_vxlan_hdr vxlan_hdr; > + > + struct rte_ether_addr outer_eth_saddr; > + struct rte_ether_addr outer_eth_daddr; > + > + uint32_t outer_ip_src_addr; > + uint32_t outer_ip_dst_addr; > + > + /* Note: It is unnecessary to save outer_src_port here because it can > + * be different for VxLAN UDP fragments from the same flow. > + */ > + uint16_t outer_dst_port; > + The above empty line is unnecessary. > +}; > + > + > +struct gro_vxlan_udp4_item { > + struct gro_udp4_item inner_item; > + /* Note: VXLAN UDP/IPv4 GRO needn't check outer_ip_id because > + * the difference between outer_ip_ids of two received packets > + * isn't always +/-1 in case of OVS DPDK. So no outer_ip_id > + * and outer_is_atomic fields here. > + */ It seems the above comments for outer IP ID is enough, and no need to highlight in the commit log again. How do you think? > +}; > + > diff --git a/lib/librte_gro/rte_gro.c b/lib/librte_gro/rte_gro.c > index f623230..db990cf 100644 > --- a/lib/librte_gro/rte_gro.c > +++ b/lib/librte_gro/rte_gro.c > @@ -11,6 +11,7 @@ > #include "gro_tcp4.h" > #include "gro_udp4.h" > #include "gro_vxlan_tcp4.h" > +#include "gro_vxlan_udp4.h" >=20 > /* > * GRO context structure. It keeps the table structures, which are > @@ -137,19 +148,27 @@ struct gro_ctx { > struct gro_udp4_item udp_items[RTE_GRO_MAX_BURST_ITEM_NUM] > =3D {{0} }; >=20 > /* Allocate a reassembly table for VXLAN TCP GRO */ > - struct gro_vxlan_tcp4_tbl vxlan_tbl; > - struct gro_vxlan_tcp4_flow > vxlan_flows[RTE_GRO_MAX_BURST_ITEM_NUM]; > - struct gro_vxlan_tcp4_item > vxlan_items[RTE_GRO_MAX_BURST_ITEM_NUM] > + struct gro_vxlan_tcp4_tbl vxlan_tcp_tbl; > + struct gro_vxlan_tcp4_flow > vxlan_tcp_flows[RTE_GRO_MAX_BURST_ITEM_NUM]; > + struct gro_vxlan_tcp4_item > vxlan_tcp_items[RTE_GRO_MAX_BURST_ITEM_NUM] > =3D {{{0}, 0, 0} }; >=20 > + /* Allocate a reassembly table for VXLAN UDP GRO */ > + struct gro_vxlan_udp4_tbl vxlan_udp_tbl; > + struct gro_vxlan_udp4_flow > vxlan_udp_flows[RTE_GRO_MAX_BURST_ITEM_NUM]; > + struct gro_vxlan_udp4_item > vxlan_udp_items[RTE_GRO_MAX_BURST_ITEM_NUM] > + =3D {{{0}} }; > + > struct rte_mbuf *unprocess_pkts[nb_pkts]; > uint32_t item_num; > int32_t ret; > uint16_t i, unprocess_num =3D 0, nb_after_gro =3D nb_pkts; > - uint8_t do_tcp4_gro =3D 0, do_vxlan_gro =3D 0, do_udp4_gro =3D 0; > + uint8_t do_tcp4_gro =3D 0, do_vxlan_tcp_gro =3D 0, do_udp4_gro =3D 0, > + do_vxlan_udp_gro =3D 0; >=20 > if (unlikely((param->gro_types & (RTE_GRO_IPV4_VXLAN_TCP_IPV4 | > RTE_GRO_TCP_IPV4 | > + RTE_GRO_IPV4_VXLAN_UDP_IPV4 | > RTE_GRO_UDP_IPV4)) =3D=3D 0)) > return nb_pkts; >=20 > @@ -160,15 +179,28 @@ struct gro_ctx { >=20 > if (param->gro_types & RTE_GRO_IPV4_VXLAN_TCP_IPV4) { > for (i =3D 0; i < item_num; i++) > - vxlan_flows[i].start_index =3D INVALID_ARRAY_INDEX; > - > - vxlan_tbl.flows =3D vxlan_flows; > - vxlan_tbl.items =3D vxlan_items; > - vxlan_tbl.flow_num =3D 0; > - vxlan_tbl.item_num =3D 0; > - vxlan_tbl.max_flow_num =3D item_num; > - vxlan_tbl.max_item_num =3D item_num; > - do_vxlan_gro =3D 1; > + vxlan_tcp_flows[i].start_index =3D > INVALID_ARRAY_INDEX; > + > + vxlan_tcp_tbl.flows =3D vxlan_tcp_flows; > + vxlan_tcp_tbl.items =3D vxlan_tcp_items; > + vxlan_tcp_tbl.flow_num =3D 0; > + vxlan_tcp_tbl.item_num =3D 0; > + vxlan_tcp_tbl.max_flow_num =3D item_num; > + vxlan_tcp_tbl.max_item_num =3D item_num; > + do_vxlan_tcp_gro =3D 1; > + } > + > + if (param->gro_types & RTE_GRO_IPV4_VXLAN_UDP_IPV4) { > + for (i =3D 0; i < item_num; i++) > + vxlan_udp_flows[i].start_index =3D > INVALID_ARRAY_INDEX; > + > + vxlan_udp_tbl.flows =3D vxlan_udp_flows; > + vxlan_udp_tbl.items =3D vxlan_udp_items; > + vxlan_udp_tbl.flow_num =3D 0; > + vxlan_udp_tbl.item_num =3D 0; > + vxlan_udp_tbl.max_flow_num =3D item_num; > + vxlan_udp_tbl.max_item_num =3D item_num; > + do_vxlan_udp_gro =3D 1; > } >=20 > if (param->gro_types & RTE_GRO_TCP_IPV4) { > @@ -204,9 +236,18 @@ struct gro_ctx { > * will be flushed from the tables. > */ > if (IS_IPV4_VXLAN_TCP4_PKT(pkts[i]->packet_type) && > - do_vxlan_gro) { > + do_vxlan_tcp_gro) { > ret =3D gro_vxlan_tcp4_reassemble(pkts[i], > - &vxlan_tbl, 0); > + &vxlan_tcp_tbl, 0); > + if (ret > 0) > + /* Merge successfully */ > + nb_after_gro--; > + else if (ret < 0) > + unprocess_pkts[unprocess_num++] =3D pkts[i]; > + } else if (IS_IPV4_VXLAN_UDP4_PKT(pkts[i]->packet_type) && > + do_vxlan_udp_gro) { > + ret =3D gro_vxlan_udp4_reassemble(pkts[i], > + &vxlan_udp_tbl, 0); > if (ret > 0) > /* Merge successfully */ > nb_after_gro--; > @@ -236,11 +277,17 @@ struct gro_ctx { > || (unprocess_num < nb_pkts)) { > i =3D 0; > /* Flush all packets from the tables */ > - if (do_vxlan_gro) { > - i =3D gro_vxlan_tcp4_tbl_timeout_flush(&vxlan_tbl, > + if (do_vxlan_tcp_gro) { > + i =3D gro_vxlan_tcp4_tbl_timeout_flush(&vxlan_tcp_tbl, > 0, pkts, nb_pkts); > } >=20 > + if (do_vxlan_udp_gro) { > + i +=3D > gro_vxlan_udp4_tbl_timeout_flush(&vxlan_udp_tbl, > + 0, &pkts[i], nb_pkts - i); > + > + } > + > if (do_tcp4_gro) { > i +=3D gro_tcp4_tbl_timeout_flush(&tcp_tbl, 0, > &pkts[i], nb_pkts - i); > @@ -269,33 +316,42 @@ struct gro_ctx { > { > struct rte_mbuf *unprocess_pkts[nb_pkts]; > struct gro_ctx *gro_ctx =3D ctx; > - void *tcp_tbl, *udp_tbl, *vxlan_tbl; > + void *tcp_tbl, *udp_tbl, *vxlan_tcp_tbl, *vxlan_udp_tbl; > uint64_t current_time; > uint16_t i, unprocess_num =3D 0; > - uint8_t do_tcp4_gro, do_vxlan_gro, do_udp4_gro; > + uint8_t do_tcp4_gro, do_vxlan_tcp_gro, do_udp4_gro, > do_vxlan_udp_gro; >=20 > if (unlikely((gro_ctx->gro_types & (RTE_GRO_IPV4_VXLAN_TCP_IPV4 > | > RTE_GRO_TCP_IPV4 | > + RTE_GRO_IPV4_VXLAN_UDP_IPV4 | > RTE_GRO_UDP_IPV4)) =3D=3D 0)) > return nb_pkts; >=20 > tcp_tbl =3D gro_ctx->tbls[RTE_GRO_TCP_IPV4_INDEX]; > - vxlan_tbl =3D gro_ctx->tbls[RTE_GRO_IPV4_VXLAN_TCP_IPV4_INDEX]; > + vxlan_tcp_tbl =3D gro_ctx- > >tbls[RTE_GRO_IPV4_VXLAN_TCP_IPV4_INDEX]; > udp_tbl =3D gro_ctx->tbls[RTE_GRO_UDP_IPV4_INDEX]; > + vxlan_udp_tbl =3D gro_ctx- > >tbls[RTE_GRO_IPV4_VXLAN_UDP_IPV4_INDEX]; >=20 > do_tcp4_gro =3D (gro_ctx->gro_types & RTE_GRO_TCP_IPV4) =3D=3D > RTE_GRO_TCP_IPV4; > - do_vxlan_gro =3D (gro_ctx->gro_types & > RTE_GRO_IPV4_VXLAN_TCP_IPV4) =3D=3D > + do_vxlan_tcp_gro =3D (gro_ctx->gro_types & > RTE_GRO_IPV4_VXLAN_TCP_IPV4) =3D=3D > RTE_GRO_IPV4_VXLAN_TCP_IPV4; > do_udp4_gro =3D (gro_ctx->gro_types & RTE_GRO_UDP_IPV4) =3D=3D > RTE_GRO_UDP_IPV4; > + do_vxlan_udp_gro =3D (gro_ctx->gro_types & > RTE_GRO_IPV4_VXLAN_UDP_IPV4) =3D=3D > + RTE_GRO_IPV4_VXLAN_UDP_IPV4; >=20 > current_time =3D rte_rdtsc(); >=20 > for (i =3D 0; i < nb_pkts; i++) { > if (IS_IPV4_VXLAN_TCP4_PKT(pkts[i]->packet_type) && > - do_vxlan_gro) { > - if (gro_vxlan_tcp4_reassemble(pkts[i], vxlan_tbl, > + do_vxlan_tcp_gro) { > + if (gro_vxlan_tcp4_reassemble(pkts[i], vxlan_tcp_tbl, > + current_time) < 0) > + unprocess_pkts[unprocess_num++] =3D pkts[i]; > + } else if (IS_IPV4_VXLAN_UDP4_PKT(pkts[i]->packet_type) && > + do_vxlan_udp_gro) { > + if (gro_vxlan_udp4_reassemble(pkts[i], > vxlan_udp_tbl, > current_time) < 0) > unprocess_pkts[unprocess_num++] =3D pkts[i]; > } else if (IS_IPV4_TCP_PKT(pkts[i]->packet_type) && > @@ -341,6 +397,13 @@ struct gro_ctx { > left_nb_out =3D max_nb_out - num; > } >=20 > + if ((gro_types & RTE_GRO_IPV4_VXLAN_UDP_IPV4) && max_nb_out > > 0) { Max_nb_out is read-only and not updated. So it cannot check if 'out' array has enough space. Need to use left_nb_out instead. This issue also existed in UDP/IPv4 GRO, and please correct them both. > + num +=3D gro_vxlan_udp4_tbl_timeout_flush(gro_ctx->tbls[ > + RTE_GRO_IPV4_VXLAN_UDP_IPV4_INDEX], > + flush_timestamp, &out[num], left_nb_out); > + left_nb_out =3D max_nb_out - num; > + } > + > /* If no available space in 'out', stop flushing. */ > if ((gro_types & RTE_GRO_TCP_IPV4) && max_nb_out > 0) { > num +=3D gro_tcp4_tbl_timeout_flush( > diff --git a/lib/librte_gro/rte_gro.h b/lib/librte_gro/rte_gro.h > index 470f3ed..9f9ed49 100644 > --- a/lib/librte_gro/rte_gro.h > +++ b/lib/librte_gro/rte_gro.h > @@ -35,6 +35,9 @@ > #define RTE_GRO_UDP_IPV4_INDEX 2 > #define RTE_GRO_UDP_IPV4 (1ULL << RTE_GRO_UDP_IPV4_INDEX) > /**< UDP/IPv4 GRO flag */ > +#define RTE_GRO_IPV4_VXLAN_UDP_IPV4_INDEX 3 > +#define RTE_GRO_IPV4_VXLAN_UDP_IPV4 (1ULL << > RTE_GRO_IPV4_VXLAN_UDP_IPV4_INDEX) > +/**< VxLAN UDP/IPv4 GRO flag. */ >=20 > /** > * Structure used to create GRO context objects or used to pass > -- > 1.8.3.1