From: "Hu, Jiayu" <jiayu.hu@intel.com>
To: kumaraparameshwaran rathinavel <kumaraparamesh92@gmail.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: RE: [PATCH v7] gro : ipv6 changes to support GRO for TCP/ipv6
Date: Wed, 14 Jun 2023 04:56:46 +0000 [thread overview]
Message-ID: <CY5PR11MB648741DBBF4B9AB43B3C3B1A925AA@CY5PR11MB6487.namprd11.prod.outlook.com> (raw)
In-Reply-To: <CANxNyatXxgiANx+o9L7CrqzruYzM0jvCP=ECNcZk2czfmaS9dQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 16620 bytes --]
From my experience, inline is used with “static” as a compiler hint.
In case of a raw inline, which is used by both insert_new_tcp_item()
and delete_tcp_item() now, it seems you need to provide an external
one to avoid linking error. But I haven’t used it in this way before.
If inline is the root cause of window compiler errors, defining the two
functions as static inline in gro_tcp_internal.h may be a solution.
Thanks,
Jiayu
From: kumaraparameshwaran rathinavel <kumaraparamesh92@gmail.com>
Sent: Wednesday, June 14, 2023 11:43 AM
To: Hu, Jiayu <jiayu.hu@intel.com>
Cc: dev@dpdk.org
Subject: Re: [PATCH v7] gro : ipv6 changes to support GRO for TCP/ipv6
On Tue, Jun 13, 2023 at 7:56 AM Hu, Jiayu <jiayu.hu@intel.com<mailto:jiayu.hu@intel.com>> wrote:
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.
Thanks Jiayu. I see some linking errors for windows. It was a for inesert_new_tcp_item which is now in *.c file. Earlier this was inert_new_item in *.h file. The error is that the insert_new_tcp_item function is undefined. Was there a reason why it was in a header file earlier ? Just wanted to check with you if you had encountered this before. Will try it on a windows setup as well.
After all comments are addressed, you can add "Reviewed-by: Jiayu Hu <jiayu.hu@intel.com<mailto:jiayu.hu@intel.com>>".
Thanks,
Jiayu
> -----Original Message-----
> From: Kumara Parameshwaran <kumaraparamesh92@gmail.com<mailto:kumaraparamesh92@gmail.com>>
> Sent: Monday, June 12, 2023 7:31 PM
> To: Hu, Jiayu <jiayu.hu@intel.com<mailto:jiayu.hu@intel.com>>
> Cc: dev@dpdk.org<mailto:dev@dpdk.org>; Kumara Parameshwaran
> <kumaraparamesh92@gmail.com<mailto: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<mailto: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
[-- Attachment #2: Type: text/html, Size: 30227 bytes --]
next prev parent reply other threads:[~2023-06-14 4:56 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
2023-06-14 3:43 ` kumaraparameshwaran rathinavel
2023-06-14 4:56 ` Hu, Jiayu [this message]
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=CY5PR11MB648741DBBF4B9AB43B3C3B1A925AA@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).