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
