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 C410BA04DE; Tue, 22 Sep 2020 04:51:40 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 490081C1D5; Tue, 22 Sep 2020 04:51:40 +0200 (CEST) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id F24EA1C121 for ; Tue, 22 Sep 2020 04:51:38 +0200 (CEST) IronPort-SDR: xhOyZGU63uqIxIVeKsQL0uFYf9IQ3bws/uduSxRl+0C408hMNk3RO46F8bFZQNrP14rlGE0J3Z iIJ1/k26yzlA== X-IronPort-AV: E=McAfee;i="6000,8403,9751"; a="178580525" X-IronPort-AV: E=Sophos;i="5.77,288,1596524400"; d="scan'208";a="178580525" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Sep 2020 19:51:38 -0700 IronPort-SDR: 7E7f7AKyWlvARvfbgg/ea74XBu729iYoeF7Qg3dfqx8M/IEV95V9XG1mucrZBy9AAMv2TO3sbg YbypEEkdXz8g== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.77,288,1596524400"; d="scan'208";a="510983860" Received: from npg_dpdk_virtio_jiayuhu_15.sh.intel.com ([10.67.117.43]) by fmsmga005.fm.intel.com with ESMTP; 21 Sep 2020 19:51:33 -0700 Date: Tue, 22 Sep 2020 11:01:26 +0800 From: Jiayu Hu To: yang_y_yi Cc: thomas@monjalon.net, dev@dpdk.org, jiayu.hu@intel.com, yangyi01@inspur.com Message-ID: <20200922030126.GA44813@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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <37c12b46.f2a.174b36e34c1.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 09:29:38AM +0800, 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. You cannot simplily delete the code, as rte_gro_timeout_flush() is designed to flush packets according to cycles. If users want to flush all packets from the table, they can set 'timeout_cycles' in rte_gro_timeout_flush() to 0. rte_gro_reassemble_burst() uses this way to return all packets of a burst to users after GRO. Thanks, Jiayu > > > 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; > > > > > At 2020-09-21 15:54:36, "Hu, Jiayu" wrote: > >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 > >> > >> From: Yi Yang > >> > >> 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. > >> > >> 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. > >> > >> 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. > >> > >> 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 > >> > >> 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 @@ > >> > >> #include > >> #include > >> +#include > >> > >> #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_udp4.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 = 0; > >> + uint32_t i, j; > >> + uint32_t max_flow_num = tbl->max_flow_num; > >> + > >> + for (i = 0; i < max_flow_num; i++) { > >> + if (unlikely(tbl->flow_num == 0)) > >> + return k; > >> + > >> + j = tbl->flows[i].start_index; > >> + while (j != INVALID_ARRAY_INDEX) { > >> + if (tbl->items[j].inner_item.start_time <= > >> + flush_timestamp) { > >> + gro_vxlan_udp4_merge_items(tbl, j); > >> + out[k++] = 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 = delete_item(tbl, j, INVALID_ARRAY_INDEX); > >> + tbl->flows[i].start_index = j; > >> + if (j == INVALID_ARRAY_INDEX) > >> + tbl->flow_num--; > >> + > >> + if (unlikely(k == 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" > >> > >> /* > >> * 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] > >> = {{0} }; > >> > >> /* 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] > >> = {{{0}, 0, 0} }; > >> > >> + /* 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] > >> + = {{{0}} }; > >> + > >> struct rte_mbuf *unprocess_pkts[nb_pkts]; > >> uint32_t item_num; > >> int32_t ret; > >> uint16_t i, unprocess_num = 0, nb_after_gro = nb_pkts; > >> - uint8_t do_tcp4_gro = 0, do_vxlan_gro = 0, do_udp4_gro = 0; > >> + uint8_t do_tcp4_gro = 0, do_vxlan_tcp_gro = 0, do_udp4_gro = 0, > >> + do_vxlan_udp_gro = 0; > >> > >> 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)) == 0)) > >> return nb_pkts; > >> > >> @@ -160,15 +179,28 @@ struct gro_ctx { > >> > >> if (param->gro_types & RTE_GRO_IPV4_VXLAN_TCP_IPV4) { > >> for (i = 0; i < item_num; i++) > >> - vxlan_flows[i].start_index = INVALID_ARRAY_INDEX; > >> - > >> - vxlan_tbl.flows = vxlan_flows; > >> - vxlan_tbl.items = vxlan_items; > >> - vxlan_tbl.flow_num = 0; > >> - vxlan_tbl.item_num = 0; > >> - vxlan_tbl.max_flow_num = item_num; > >> - vxlan_tbl.max_item_num = item_num; > >> - do_vxlan_gro = 1; > >> + vxlan_tcp_flows[i].start_index = > >> INVALID_ARRAY_INDEX; > >> + > >> + vxlan_tcp_tbl.flows = vxlan_tcp_flows; > >> + vxlan_tcp_tbl.items = vxlan_tcp_items; > >> + vxlan_tcp_tbl.flow_num = 0; > >> + vxlan_tcp_tbl.item_num = 0; > >> + vxlan_tcp_tbl.max_flow_num = item_num; > >> + vxlan_tcp_tbl.max_item_num = item_num; > >> + do_vxlan_tcp_gro = 1; > >> + } > >> + > >> + if (param->gro_types & RTE_GRO_IPV4_VXLAN_UDP_IPV4) { > >> + for (i = 0; i < item_num; i++) > >> + vxlan_udp_flows[i].start_index = > >> INVALID_ARRAY_INDEX; > >> + > >> + vxlan_udp_tbl.flows = vxlan_udp_flows; > >> + vxlan_udp_tbl.items = vxlan_udp_items; > >> + vxlan_udp_tbl.flow_num = 0; > >> + vxlan_udp_tbl.item_num = 0; > >> + vxlan_udp_tbl.max_flow_num = item_num; > >> + vxlan_udp_tbl.max_item_num = item_num; > >> + do_vxlan_udp_gro = 1; > >> } > >> > >> 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 = 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++] = pkts[i]; > >> + } else if (IS_IPV4_VXLAN_UDP4_PKT(pkts[i]->packet_type) && > >> + do_vxlan_udp_gro) { > >> + ret = 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 = 0; > >> /* Flush all packets from the tables */ > >> - if (do_vxlan_gro) { > >> - i = gro_vxlan_tcp4_tbl_timeout_flush(&vxlan_tbl, > >> + if (do_vxlan_tcp_gro) { > >> + i = gro_vxlan_tcp4_tbl_timeout_flush(&vxlan_tcp_tbl, > >> 0, pkts, nb_pkts); > >> } > >> > >> + if (do_vxlan_udp_gro) { > >> + i += > >> gro_vxlan_udp4_tbl_timeout_flush(&vxlan_udp_tbl, > >> + 0, &pkts[i], nb_pkts - i); > >> + > >> + } > >> + > >> if (do_tcp4_gro) { > >> i += 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 = 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 = 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; > >> > >> 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)) == 0)) > >> return nb_pkts; > >> > >> tcp_tbl = gro_ctx->tbls[RTE_GRO_TCP_IPV4_INDEX]; > >> - vxlan_tbl = gro_ctx->tbls[RTE_GRO_IPV4_VXLAN_TCP_IPV4_INDEX]; > >> + vxlan_tcp_tbl = gro_ctx- > >> >tbls[RTE_GRO_IPV4_VXLAN_TCP_IPV4_INDEX]; > >> udp_tbl = gro_ctx->tbls[RTE_GRO_UDP_IPV4_INDEX]; > >> + vxlan_udp_tbl = gro_ctx- > >> >tbls[RTE_GRO_IPV4_VXLAN_UDP_IPV4_INDEX]; > >> > >> do_tcp4_gro = (gro_ctx->gro_types & RTE_GRO_TCP_IPV4) == > >> RTE_GRO_TCP_IPV4; > >> - do_vxlan_gro = (gro_ctx->gro_types & > >> RTE_GRO_IPV4_VXLAN_TCP_IPV4) == > >> + do_vxlan_tcp_gro = (gro_ctx->gro_types & > >> RTE_GRO_IPV4_VXLAN_TCP_IPV4) == > >> RTE_GRO_IPV4_VXLAN_TCP_IPV4; > >> do_udp4_gro = (gro_ctx->gro_types & RTE_GRO_UDP_IPV4) == > >> RTE_GRO_UDP_IPV4; > >> + do_vxlan_udp_gro = (gro_ctx->gro_types & > >> RTE_GRO_IPV4_VXLAN_UDP_IPV4) == > >> + RTE_GRO_IPV4_VXLAN_UDP_IPV4; > >> > >> current_time = rte_rdtsc(); > >> > >> for (i = 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++] = 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++] = pkts[i]; > >> } else if (IS_IPV4_TCP_PKT(pkts[i]->packet_type) && > >> @@ -341,6 +397,13 @@ struct gro_ctx { > >> left_nb_out = max_nb_out - num; > >> } > >> > >> + 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 += 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 = max_nb_out - num; > >> + } > >> + > >> /* If no available space in 'out', stop flushing. */ > >> if ((gro_types & RTE_GRO_TCP_IPV4) && max_nb_out > 0) { > >> num += 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. */ > >> > >> /** > >> * Structure used to create GRO context objects or used to pass > >> -- > >> 1.8.3.1 > > > > >