Jiayu, you didn't get my point. I just show what you want by code, but that isn't a good way. I think current code is most reasonable. Please check my comments in last email, actually, only the first packet of a flow is checked by timestamp, if this flow includes some new items (you're saying), they can be merged, so it isn't the case you're saying.
At 2020-09-22 11:01:26, "Jiayu Hu" <jiayu...@intel.com> wrote: >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" <jiayu...@intel.com> wrote: >> >Hi Yi, >> > >> >Some comments are inline. >> > >> >Thanks, >> >Jiayu >> > >> >> -----Original Message----- >> >> From: yang_y...@163.com <yang_y...@163.com> >> >> Sent: Thursday, September 17, 2020 11:50 AM >> >> To: dev@dpdk.org >> >> Cc: Hu, Jiayu <jiayu...@intel.com>; tho...@monjalon.net; >> >> yangy...@inspur.com; yang_y...@163.com >> >> Subject: [PATCH v6 2/3] gro: add VXLAN UDP/IPv4 GRO support >> >> >> >> From: Yi Yang <yangy...@inspur.com> >> >> >> >> 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 <yangy...@inspur.com> >> >> --- >> >> 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 <rte_ip.h> >> >> #include <rte_udp.h> >> >> +#include <rte_vxlan.h> >> >> >> >> #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 >> >> >> >> >>