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;
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
