On Tue, Jun 13, 2023 at 7:56 AM Hu, Jiayu 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>". > > Thanks, > Jiayu > > > -----Original Message----- > > From: Kumara Parameshwaran > > Sent: Monday, June 12, 2023 7:31 PM > > To: Hu, Jiayu > > Cc: dev@dpdk.org; Kumara Parameshwaran > > > > 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 > > --- > > 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 > > +#include > > 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 > > 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 > > +#include > > +#include > > + > > +#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 > > + > > +/* > > + * 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 >