<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body lang="EN-US" link="blue" vlink="purple" style="word-wrap:break-word"> <div class="WordSection1"> From my experience, inline is used with “static” as a compiler hint.<o:p></o:p> In case of a raw inline, which is used by both insert_new_tcp_item()<o:p></o:p> and delete_tcp_item() now, it seems you need to provide an external<o:p></o:p> one to avoid linking error. But I haven’t used it in this way before.<o:p></o:p> <o:p> </o:p> If inline is the root cause of window compiler errors, defining the two<o:p></o:p> functions as static inline in gro_tcp_internal.h may be a solution.<o:p></o:p> <o:p> </o:p> Thanks,<o:p></o:p> Jiayu<o:p></o:p> <o:p> </o:p> <div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt"> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> 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<o:p></o:p> </div> </div> <o:p> </o:p> <div> <div> <o:p> </o:p> </div> <o:p> </o:p> <div> <div> On Tue, Jun 13, 2023 at 7:56 AM Hu, Jiayu <<a href="mailto:jiayu.hu@intel.com">jiayu.hu@intel.com</a>> wrote:<o:p></o:p> </div> <blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in"> 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.<o:p></o:p> <blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in"> 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. <o:p></o:p> </blockquote> </blockquote> <blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in"> After all comments are addressed, you can add "Reviewed-by: Jiayu Hu <<a href="mailto:jiayu.hu@intel.com" target="_blank">jiayu.hu@intel.com</a>>". Thanks, Jiayu > -----Original Message----- > From: Kumara Parameshwaran <<a href="mailto:kumaraparamesh92@gmail.com" target="_blank">kumaraparamesh92@gmail.com</a>> > Sent: Monday, June 12, 2023 7:31 PM > To: Hu, Jiayu <<a href="mailto:jiayu.hu@intel.com" target="_blank">jiayu.hu@intel.com</a>> > Cc: <a href="mailto:dev@dpdk.org" target="_blank">dev@dpdk.org</a>; Kumara Parameshwaran > <<a href="mailto:kumaraparamesh92@gmail.com" target="_blank">kumaraparamesh92@gmail.com</a>> > 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 <<a href="mailto:kumaraparamesh92@gmail.com" target="_blank">kumaraparamesh92@gmail.com</a>> > --- > 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<o:p></o:p> </blockquote> </div> </div> </div> </div> </body> </html>