From: "Hu, Jiayu" <jiayu.hu@intel.com>
To: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: RE: [PATCH v7] gro : ipv6 changes to support GRO for TCP/ipv6
Date: Tue, 13 Jun 2023 02:25:52 +0000 [thread overview]
Message-ID: <CY5PR11MB6487E281291E2F2C108818379255A@CY5PR11MB6487.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20230612113110.72345-1-kumaraparamesh92@gmail.com>
Hi Kumara,
Overall, the patch looks good to me. But you need to update the doc and there
are some minor changes needed to be made. Please see replies inline.
After all comments are addressed, you can add "Reviewed-by: Jiayu Hu <jiayu.hu@intel.com>".
Thanks,
Jiayu
> -----Original Message-----
> From: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
> Sent: Monday, June 12, 2023 7:31 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>
> Cc: dev@dpdk.org; Kumara Parameshwaran
> <kumaraparamesh92@gmail.com>
> Subject: [PATCH v7] gro : ipv6 changes to support GRO for TCP/ipv6
>
> The patch adds GRO support for TCP/ipv6 packets. This does not include the
> support for vxlan, udp ipv6 packets.
>
> Signed-off-by: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
> ---
> v1:
> * Changes to support GRO for TCP/ipv6 packets. This does not
> include
> vxlan changes.
> * The GRO is performed only for ipv6 packets that does not contain
> extension headers.
> * The logic for the TCP coalescing remains the same, in ipv6 header
> the source address, destination address, flow label, version fields
> are expected to be the same.
> * Re-organised the code to reuse certain tcp functions for both ipv4
> and
> ipv6 flows.
> v2:
> * Fix comments in gro_tcp6.h header file.
>
> v3:
> * Adderess review comments to fix code duplication for v4 and v6
>
> v4:
> * Addresses review comments for v3, do not use callbacks
>
> v5:
> * Address review comments
>
> v6:
> * Fix warning and coding style issues
>
> v7:
> * Fix build compilation issue
> lib/gro/gro_tcp4.c | 178 ++++++-------------------
> lib/gro/gro_tcp4.h | 170 +----------------------
> lib/gro/gro_tcp6.c | 267 +++++++++++++++++++++++++++++++++++++
> lib/gro/gro_tcp6.h | 161 ++++++++++++++++++++++
> lib/gro/gro_tcp_internal.c | 128 ++++++++++++++++++
> lib/gro/gro_tcp_internal.h | 212 +++++++++++++++++++++++++++++
> lib/gro/gro_vxlan_tcp4.c | 23 ++--
> lib/gro/gro_vxlan_tcp4.h | 3 +-
> lib/gro/meson.build | 2 +
> lib/gro/rte_gro.c | 83 ++++++++++--
> lib/gro/rte_gro.h | 3 +
> 11 files changed, 897 insertions(+), 333 deletions(-) create mode 100644
> lib/gro/gro_tcp6.c create mode 100644 lib/gro/gro_tcp6.h create mode
> 100644 lib/gro/gro_tcp_internal.c create mode 100644
> lib/gro/gro_tcp_internal.h
>
> diff --git a/lib/gro/gro_tcp4.c b/lib/gro/gro_tcp4.c index
> 0014096e63..42fee78f30 100644
> --- a/lib/gro/gro_tcp4.c
> +++ b/lib/gro/gro_tcp4.c
> -
> int32_t
> gro_tcp4_reassemble(struct rte_mbuf *pkt,
> struct gro_tcp4_tbl *tbl,
> @@ -202,9 +122,8 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
> uint8_t is_atomic;
>
> struct tcp4_flow_key key;
> - uint32_t cur_idx, prev_idx, item_idx;
> + uint32_t item_idx;
> uint32_t i, max_flow_num, remaining_flow_num;
> - int cmp;
> uint8_t find;
>
> /*
> @@ -216,7 +135,7 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
>
> eth_hdr = rte_pktmbuf_mtod(pkt, struct rte_ether_hdr *);
> ipv4_hdr = (struct rte_ipv4_hdr *)((char *)eth_hdr + pkt->l2_len);
> - tcp_hdr = (struct rte_tcp_hdr *)((char *)ipv4_hdr + pkt->l3_len);
> + tcp_hdr = rte_pktmbuf_mtod_offset(pkt, struct rte_tcp_hdr *,
> +pkt->l2_len + pkt->l3_len);
No need to change the method to obtain tcp_hdr.
> hdr_len = pkt->l2_len + pkt->l3_len + pkt->l4_len;
>
> /*
> @@ -230,7 +149,6 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
> ip_tlen = rte_be_to_cpu_16(ipv4_hdr->total_length);
> if (pkt->pkt_len > (uint32_t)(ip_tlen + pkt->l2_len))
> rte_pktmbuf_trim(pkt, pkt->pkt_len - ip_tlen - pkt->l2_len);
> -
No need to delete the blank line.
> /*
> * Don't process the packet whose payload length is less than or
> * equal to 0.
> @@ -239,6 +157,13 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
> if (tcp_dl <= 0)
> return -1;
>
> + rte_ether_addr_copy(&(eth_hdr->src_addr),
> &(key.cmn_key.eth_saddr));
> + rte_ether_addr_copy(&(eth_hdr->dst_addr),
> &(key.cmn_key.eth_daddr));
> + key.ip_src_addr = ipv4_hdr->src_addr;
> + key.ip_dst_addr = ipv4_hdr->dst_addr;
> + key.cmn_key.src_port = tcp_hdr->src_port;
> + key.cmn_key.dst_port = tcp_hdr->dst_port;
> + key.cmn_key.recv_ack = tcp_hdr->recv_ack;
Add a blank line between key.cmn_key.recv_ack assignment and annotation.
> /*
> * Save IPv4 ID for the packet whose DF bit is 0. For the packet
> * whose DF bit is 1, IPv4 ID is ignored.
> @@ -246,15 +171,6 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
> frag_off = rte_be_to_cpu_16(ipv4_hdr->fragment_offset);
> is_atomic = (frag_off & RTE_IPV4_HDR_DF_FLAG) ==
> RTE_IPV4_HDR_DF_FLAG;
> ip_id = is_atomic ? 0 : rte_be_to_cpu_16(ipv4_hdr->packet_id);
> - sent_seq = rte_be_to_cpu_32(tcp_hdr->sent_seq);
> -
> - rte_ether_addr_copy(&(eth_hdr->src_addr), &(key.eth_saddr));
> - rte_ether_addr_copy(&(eth_hdr->dst_addr), &(key.eth_daddr));
> - key.ip_src_addr = ipv4_hdr->src_addr;
> - key.ip_dst_addr = ipv4_hdr->dst_addr;
> - key.src_port = tcp_hdr->src_port;
> - key.dst_port = tcp_hdr->dst_port;
> - key.recv_ack = tcp_hdr->recv_ack;
>
> /* Search for a matched flow. */
> max_flow_num = tbl->max_flow_num;
> @@ -270,63 +186,44 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
> }
> }
>
> - /*
> - * Fail to find a matched flow. Insert a new flow and store the
> - * packet into the flow.
> - */
> if (find == 0) {
> - item_idx = insert_new_item(tbl, pkt, start_time,
> - INVALID_ARRAY_INDEX, sent_seq, ip_id,
> - is_atomic);
> + sent_seq = rte_be_to_cpu_32(tcp_hdr->sent_seq);
> + item_idx = insert_new_tcp_item(pkt, tbl->items, &tbl-
> >item_num,
> + tbl->max_item_num,
> start_time,
> + INVALID_ARRAY_INDEX,
> sent_seq, ip_id,
> + is_atomic);
> if (item_idx == INVALID_ARRAY_INDEX)
> return -1;
> if (insert_new_flow(tbl, &key, item_idx) ==
> - INVALID_ARRAY_INDEX) {
> + INVALID_ARRAY_INDEX) {
> /*
> * Fail to insert a new flow, so delete the
> * stored packet.
> - */
> - delete_item(tbl, item_idx, INVALID_ARRAY_INDEX);
> + */
> + delete_tcp_item(tbl->items, item_idx, &tbl-
> >item_num,
> +INVALID_ARRAY_INDEX);
> return -1;
> }
> return 0;
> }
>
> - /*
> - * Check all packets in the flow and try to find a neighbor for
> - * the input packet.
> - */
> - cur_idx = tbl->flows[i].start_index;
> - prev_idx = cur_idx;
> - do {
> - cmp = check_seq_option(&(tbl->items[cur_idx]), tcp_hdr,
> - sent_seq, ip_id, pkt->l4_len, tcp_dl, 0,
> - is_atomic);
> - if (cmp) {
> - if (merge_two_tcp4_packets(&(tbl->items[cur_idx]),
> - pkt, cmp, sent_seq, ip_id, 0))
> - return 1;
> - /*
> - * Fail to merge the two packets, as the packet
> - * length is greater than the max value. Store
> - * the packet into the flow.
> - */
> - if (insert_new_item(tbl, pkt, start_time, cur_idx,
> - sent_seq, ip_id, is_atomic) ==
> - INVALID_ARRAY_INDEX)
> - return -1;
> - return 0;
> - }
> - prev_idx = cur_idx;
> - cur_idx = tbl->items[cur_idx].next_pkt_idx;
> - } while (cur_idx != INVALID_ARRAY_INDEX);
> + return process_tcp_item(pkt, tcp_hdr, tcp_dl, tbl->items, tbl-
> >flows[i].start_index,
> + &tbl->item_num, tbl-
> >max_item_num,
> + ip_id, is_atomic, start_time);
> +}
>
> - /* Fail to find a neighbor, so store the packet into the flow. */
> - if (insert_new_item(tbl, pkt, start_time, prev_idx, sent_seq,
> - ip_id, is_atomic) == INVALID_ARRAY_INDEX)
> - return -1;
> +/*
> + * update the packet length for the flushed packet.
> + */
> +static inline void
> +update_header(struct gro_tcp_item *item) {
> + struct rte_ipv4_hdr *ipv4_hdr;
> + struct rte_mbuf *pkt = item->firstseg;
>
> - return 0;
> + ipv4_hdr = (struct rte_ipv4_hdr *)(rte_pktmbuf_mtod(pkt, char *) +
> + pkt->l2_len);
> + ipv4_hdr->total_length = rte_cpu_to_be_16(pkt->pkt_len -
> + pkt->l2_len);
> }
>
> uint16_t
> @@ -353,7 +250,8 @@ gro_tcp4_tbl_timeout_flush(struct gro_tcp4_tbl *tbl,
> * Delete the packet and get the next
> * packet in the flow.
> */
> - j = delete_item(tbl, j, INVALID_ARRAY_INDEX);
> + j = delete_tcp_item(tbl->items, j,
> + &tbl->item_num,
> INVALID_ARRAY_INDEX);
> tbl->flows[i].start_index = j;
> if (j == INVALID_ARRAY_INDEX)
> tbl->flow_num--;
> diff --git a/lib/gro/gro_tcp4.h b/lib/gro/gro_tcp4.h index
> 212f97a042..c0154afa24 100644
> --- a/lib/gro/gro_tcp4.h
> +++ b/lib/gro/gro_tcp4.h
> @@ -5,32 +5,15 @@
> #ifndef _GRO_TCP4_H_
> #define _GRO_TCP4_H_
>
> -#include <rte_tcp.h>
> +#include <gro_tcp_internal.h>
Use "#include "gro_tcp_internal.h"".
>
> -#define INVALID_ARRAY_INDEX 0xffffffffUL #define
> GRO_TCP4_TBL_MAX_ITEM_NUM (1024UL * 1024UL)
>
> diff --git a/lib/gro/gro_tcp6.c b/lib/gro/gro_tcp6.c new file mode 100644
> index 0000000000..0ea73741c1
> --- /dev/null
> +++ b/lib/gro/gro_tcp6.c
> @@ -0,0 +1,267 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2017 Intel Corporation
> + */
The license header is incorrect, and please update. Same and similar issue happens
in the new added files, like gro_tcp_internal.h.
> +
> diff --git a/lib/gro/gro_tcp6.h b/lib/gro/gro_tcp6.h new file mode 100644
> index 0000000000..cc02b0004a
> --- /dev/null
> +++ b/lib/gro/gro_tcp6.h
> @@ -0,0 +1,161 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2017 Intel Corporation
> + */
Ditto.
> +
> +#ifndef _GRO_TCP6_H_
> +#define _GRO_TCP6_H_
> +
> +#include <gro_tcp_internal.h>
Use "#include "gro_tcp_internal.h"".
> +
> +#define INVALID_ARRAY_INDEX 0xffffffffUL #define
Duplicate definition for INVALID_ARRAY_INDEX in both gro_tcp6.h
and gro_tcp_internal.h.
> +GRO_TCP6_TBL_MAX_ITEM_NUM (1024UL * 1024UL)
> +
> +/* Header fields representing a TCP/IPv6 flow */ struct tcp6_flow_key {
> + struct cmn_tcp_key cmn_key;
> + uint8_t src_addr[16];
> + uint8_t dst_addr[16];
> + rte_be32_t vtc_flow;
> +};
> +
> diff --git a/lib/gro/gro_tcp_internal.c b/lib/gro/gro_tcp_internal.c new file
> mode 100644 index 0000000000..5a21bca7f8
> --- /dev/null
> +++ b/lib/gro/gro_tcp_internal.c
> @@ -0,0 +1,128 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2017 Intel Corporation
> + */
Update license header.
> +#include <rte_malloc.h>
> +#include <rte_mbuf.h>
> +#include <rte_ethdev.h>
> +
> +#include "gro_tcp_internal.h"
> +
> diff --git a/lib/gro/gro_tcp_internal.h b/lib/gro/gro_tcp_internal.h new file
> mode 100644 index 0000000000..072b7aea13
> --- /dev/null
> +++ b/lib/gro/gro_tcp_internal.h
> @@ -0,0 +1,212 @@
> +#ifndef _GRO_TCP_H_
> +#define _GRO_TCP_H_
> +
Add license header in this file.
> +#define INVALID_ARRAY_INDEX 0xffffffffUL
> +
> +#include <rte_tcp.h>
> +
> +/*
> + * The max length of a IPv4 packet, which includes the length of the L3
> + * header, the L4 header and the data payload.
> + */
> +#define MAX_IP_PKT_LENGTH UINT16_MAX
> +
> +/* The maximum TCP header length */
> +#define MAX_TCP_HLEN 60
> +#define INVALID_TCP_HDRLEN(len) \
> + (((len) < sizeof(struct rte_tcp_hdr)) || ((len) > MAX_TCP_HLEN))
> +
> +/*
> + * Check if two TCP/IPv4 packets are neighbors.
> + */
This function is not only for TCP4, so use "Check if two TCP packets are neighbors".
> +static inline int
> +check_seq_option(struct gro_tcp_item *item,
> + struct rte_tcp_hdr *tcph,
> + uint32_t sent_seq,
> + uint16_t ip_id,
> + uint16_t tcp_hl,
> + uint16_t tcp_dl,
> + uint16_t l2_offset,
> + uint8_t is_atomic)
> +{
> + struct rte_mbuf *pkt_orig = item->firstseg;
> + char *iph_orig;
> + struct rte_tcp_hdr *tcph_orig;
> + uint16_t len, tcp_hl_orig;
> +
> + iph_orig = (char *)(rte_pktmbuf_mtod(pkt_orig, char *) +
> + l2_offset + pkt_orig->l2_len);
> + tcph_orig = (struct rte_tcp_hdr *)(iph_orig + pkt_orig->l3_len);
> + tcp_hl_orig = pkt_orig->l4_len;
> +
> + /* Check if TCP option fields equal */
> + len = RTE_MAX(tcp_hl, tcp_hl_orig) - sizeof(struct rte_tcp_hdr);
> + if ((tcp_hl != tcp_hl_orig) || ((len > 0) &&
> + (memcmp(tcph + 1, tcph_orig + 1,
> + len) != 0)))
> + return 0;
> +
> + /* Don't merge packets whose DF bits are different */
> + if (unlikely(item->is_atomic ^ is_atomic))
> + return 0;
> +
> + /* check if the two packets are neighbors */
> + len = pkt_orig->pkt_len - l2_offset - pkt_orig->l2_len -
> + pkt_orig->l3_len - tcp_hl_orig;
> + if ((sent_seq == item->sent_seq + len) && (is_atomic ||
> + (ip_id == item->l3.ip_id + 1)))
> + /* append the new packet */
> + return 1;
> + else if ((sent_seq + tcp_dl == item->sent_seq) && (is_atomic ||
> + (ip_id + item->nb_merged == item->l3.ip_id)))
> + /* pre-pend the new packet */
> + return -1;
> +
> + return 0;
> +}
> +
> +static inline int
> +is_common_tcp_key(struct cmn_tcp_key *k1, struct cmn_tcp_key *k2) {
Rename it to "is_same_common_tcp_key()"?
> + return (!memcmp(k1, k2, sizeof(struct cmn_tcp_key))); }
> +
> +#endif
next prev parent reply other threads:[~2023-06-13 2:26 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-20 18:07 [PATCH] " Kumara Parameshwaran
2022-10-20 18:14 ` [PATCH v2] " Kumara Parameshwaran
2023-05-12 2:47 ` Hu, Jiayu
2023-05-16 9:28 ` kumaraparameshwaran rathinavel
2023-05-25 11:22 ` kumaraparameshwaran rathinavel
2023-05-31 8:20 ` Hu, Jiayu
2023-06-02 6:02 ` [PATCH v3] gro : ipv6-gro review comments to reduce code duplication across v4 and v6 Kumara Parameshwaran
2023-06-02 6:34 ` [PATCH v3] gro : ipv6 changes to support GRO for TCP/ipv6 Kumara Parameshwaran
2023-06-06 4:35 ` Hu, Jiayu
2023-06-06 9:31 ` kumaraparameshwaran rathinavel
2023-06-06 14:58 ` [PATCH v4] " Kumara Parameshwaran
2023-06-08 4:05 ` Hu, Jiayu
2023-06-08 16:52 ` kumaraparameshwaran rathinavel
2023-06-09 1:04 ` Hu, Jiayu
2023-06-12 11:05 ` [PATCH v5] " Kumara Parameshwaran
2023-06-12 11:23 ` [PATCH v6] " Kumara Parameshwaran
2023-06-12 11:31 ` [PATCH v7] " Kumara Parameshwaran
2023-06-12 12:04 ` kumaraparameshwaran rathinavel
2023-06-13 2:25 ` Hu, Jiayu [this message]
2023-06-14 3:43 ` kumaraparameshwaran rathinavel
2023-06-14 4:56 ` Hu, Jiayu
2023-06-15 5:40 ` [PATCH v8] " Kumara Parameshwaran
2023-06-15 6:20 ` [PATCH v9] " Kumara Parameshwaran
2023-06-15 6:30 ` kumaraparameshwaran rathinavel
2023-06-15 8:01 ` Hu, Jiayu
2023-06-15 9:16 ` kumaraparameshwaran rathinavel
2023-06-19 13:30 ` Thomas Monjalon
2023-06-21 8:25 ` [PATCH v10 1/2] gro : refactor IPv4 to add GRO support for IPv6 Kumara Parameshwaran
2023-06-21 8:25 ` [PATCH v10 2/2] gro : add support for IPv6 GRO Kumara Parameshwaran
2023-06-21 8:38 ` [PATCH v11 1/2] gro : refactor IPv4 to add GRO support for IPv6 Kumara Parameshwaran
2023-06-21 8:38 ` [PATCH v11 2/2] gro : add support for IPv6 GRO Kumara Parameshwaran
2023-06-27 15:47 ` Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CY5PR11MB6487E281291E2F2C108818379255A@CY5PR11MB6487.namprd11.prod.outlook.com \
--to=jiayu.hu@intel.com \
--cc=dev@dpdk.org \
--cc=kumaraparamesh92@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).