From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 708F842D12; Wed, 21 Jun 2023 10:47:32 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id F1EE7406BC; Wed, 21 Jun 2023 10:47:31 +0200 (CEST) Received: from mail-yb1-f179.google.com (mail-yb1-f179.google.com [209.85.219.179]) by mails.dpdk.org (Postfix) with ESMTP id 0877C4068E for ; Wed, 21 Jun 2023 10:47:29 +0200 (CEST) Received: by mail-yb1-f179.google.com with SMTP id 3f1490d57ef6-bfee679b7efso489638276.0 for ; Wed, 21 Jun 2023 01:47:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1687337249; x=1689929249; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=RQK3ZyJBak32DvQEANGd/5PLH80/5eznGpmhofIYbqA=; b=n7msGbwrJWs8+MdQImrBOnyBHgpjc84S+LfCXlwedjEMhU456KBat/oeuGgDme22jK Kd7/sT9V6IdxAH6m2a91AR/4KxIWT9sMi6ApV/A03YPRu++5OzJJN/DF/Ju4lH33NqeB YcBf1K2XJKU7R19VW5F/sXAtov/ZEXGXgnM2bvyEbLmnOeIMk76qUkjpLJs9iAVjaepk NyI1t/0279g5qJPlwBa+NVYOxZ6dUMWUWkRZe6ba2UnCBJlKS+70EM44MGaa7yMTSS+2 NWP193ozuxifUkOu7o5I/08j3WOm0oUTER8OCkqfbTug2vBhm8uwlDe55YEUfq6mdwFp LRcA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687337249; x=1689929249; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=RQK3ZyJBak32DvQEANGd/5PLH80/5eznGpmhofIYbqA=; b=b4N9TrCFeX5s9nsYNTpJRBO3nPRuyrcvZFYXAXIjsMT3v5fmd80++tWB37eTiJQ5yT 6SfwjabiTwNCdpXrwx2+vA/PzkyPy/J4zXC4ps4DzhdsOBHxWjQxVI69DKZqZbyUI3AT cSBXa1f06wTYP/liW8IDWuxWqNSQdEvrWAT766wYzsfH6eDS5EYozygMZziI2WUQ67lA R3a/oIADBoEUKsQZPAGlUQKrL0mcLHq3+9sDLwi/EZNoNKmBdjIfG2aiR48gyQzF1rcP zipUZSb80Cb9RSBugMCcMeOWTR636OP24wn482ZgAO0hiw/xSVQUDoPwFk/n/Seq6z2A aPEQ== X-Gm-Message-State: AC+VfDy5QQPJFa7vVALKyTpKC0NfUtayeQCwBC7Uy/RYfKRFsGIg5W7E JxduH/w0gW8JnmQXdMbCiJdPFlBF1bkUOFowvfg= X-Google-Smtp-Source: ACHHUZ7sZJCGYbpei8RmfxW80z97xIsG/otW7urGB7kHqnlXQ+F9gWY2x+FOM5oiO50hqPuFFyta2eL2S4d46ZtL9fw= X-Received: by 2002:a81:4f54:0:b0:56f:f565:c037 with SMTP id d81-20020a814f54000000b0056ff565c037mr15692349ywb.7.1687337249235; Wed, 21 Jun 2023 01:47:29 -0700 (PDT) MIME-Version: 1.0 References: <20221028095114.17210-1-kumaraparmesh92@gmail.com> <20221101070557.58808-1-kumaraparmesh92@gmail.com> In-Reply-To: From: kumaraparameshwaran rathinavel Date: Wed, 21 Jun 2023 14:17:18 +0530 Message-ID: Subject: Re: [PATCH v5] gro : fix reordering of packets in GRO library To: "Hu, Jiayu" Cc: "dev@dpdk.org" , Kumara Parameshwaran , "thomas@monjalon.net" Content-Type: multipart/alternative; boundary="0000000000005b684805fe9fd122" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --0000000000005b684805fe9fd122 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Jiayu, Thanks for the comments. I have replied inline below. Will address the review comments, but I think this would be a good patch to have in general. Please let me know your thoughts. On Tue, Jun 20, 2023 at 1:06=E2=80=AFPM Hu, Jiayu wrot= e: > Hi Kumara, > > Please see replies inline. > > Thanks, > Jiayu > > > -----Original Message----- > > From: Kumara Parameshwaran > > Sent: Tuesday, November 1, 2022 3:06 PM > > To: Hu, Jiayu > > Cc: dev@dpdk.org; Kumara Parameshwaran > > ; Kumara Parameshwaran > > > > Subject: [PATCH v5] gro : fix reordering of packets in GRO library > > > > From: Kumara Parameshwaran > > > > When a TCP packet contains flags like PSH it is returned immediately to > the > > application though there might be packets of the same flow in the GRO > table. > > If PSH flag is set on a segment packets up to the segment should be > delivered > > immediately. But the current implementation delivers the last arrived > packet > > with PSH flag set causing re-ordering > > > > With this patch, if a packet does not contain only ACK flag and if ther= e > are no > > previous packets for the flow the packet would be returned immediately, > > else will be merged with the previous segment and the flag on the last > > segment will be set on the entire segment. > > This is the behaviour with linux stack as well. > > > > Signed-off-by: Kumara Parameshwaran > > Co-authored-by: Kumara Parameshwaran > > --- > > v1: > > If the received packet is not a pure ACK packet, we check if > > there are any previous packets in the flow, if present we indulge > > the received packet also in the coalescing logic and update the > flags > > of the last recived packet to the entire segment which would avoi= d > > re-ordering. > > > > Lets say a case where P1(PSH), P2(ACK), P3(ACK) are received in > > burst mode, > > P1 contains PSH flag and since it does not contain any prior > packets in > > the flow > > we copy it to unprocess_packets and P2(ACK) and P3(ACK) are > > merged together. > > In the existing case the P2,P3 would be delivered as single > segment > > first and the > > unprocess_packets will be copied later which will cause reorderin= g. > > With the patch > > copy the unprocess packets first and then the packets from the GR= O > > table. > > > > Testing done > > The csum test-pmd was modifited to support the following > > GET request of 10MB from client to server via test-pmd (static ar= p > > entries added in client > > and server). Enable GRO and TSO in test-pmd where the packets > > recived from the client mac > > would be sent to server mac and vice versa. > > In above testing, without the patch the client observerd > re-ordering > > of 25 packets > > and with the patch there were no packet re-ordering observerd. > > > > v2: > > Fix warnings in commit and comment. > > Do not consider packet as candidate to merge if it contains SYN/R= ST > > flag. > > > > v3: > > Fix warnings. > > > > v4: > > Rebase with master. > > > > v5: > > Adding co-author email > > > > lib/gro/gro_tcp4.c | 45 +++++++++++++++++++++++++++++++++++++-------- > > lib/gro/rte_gro.c | 18 +++++++++--------- > > 2 files changed, 46 insertions(+), 17 deletions(-) > > > > diff --git a/lib/gro/gro_tcp4.c b/lib/gro/gro_tcp4.c index > > 0014096e63..7363c5d540 100644 > > --- a/lib/gro/gro_tcp4.c > > +++ b/lib/gro/gro_tcp4.c > > @@ -188,6 +188,19 @@ update_header(struct gro_tcp4_item *item) > > pkt->l2_len); > > } > > > > +static inline void > > +update_tcp_hdr_flags(struct rte_tcp_hdr *tcp_hdr, struct rte_mbuf *pkt= ) > > +{ > > + struct rte_ether_hdr *eth_hdr; > > + struct rte_ipv4_hdr *ipv4_hdr; > > + struct rte_tcp_hdr *merged_tcp_hdr; > > + > > + eth_hdr =3D rte_pktmbuf_mtod(pkt, struct rte_ether_hdr *); > > + ipv4_hdr =3D (struct rte_ipv4_hdr *)((char *)eth_hdr + pkt->l2_le= n); > > + merged_tcp_hdr =3D (struct rte_tcp_hdr *)((char *)ipv4_hdr + pkt- > > >l3_len); > > + merged_tcp_hdr->tcp_flags |=3D tcp_hdr->tcp_flags; } > > The Linux kernel updates the TCP flag via "tcp_flag_word(th2) |=3D flags = & > (TCP_FLAG_FIN | TCP_FLAG_PSH)", > which only adds FIN and PSH at most to the merge packet. > >> Sure, will change it to add only FIN and PSH. >> > > > + > > int32_t > > gro_tcp4_reassemble(struct rte_mbuf *pkt, > > struct gro_tcp4_tbl *tbl, > > @@ -206,6 +219,7 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt, > > uint32_t i, max_flow_num, remaining_flow_num; > > int cmp; > > uint8_t find; > > + uint32_t start_idx; > > > > /* > > * Don't process the packet whose TCP header length is greater @@= - > > 219,13 +233,6 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt, > > tcp_hdr =3D (struct rte_tcp_hdr *)((char *)ipv4_hdr + pkt->l3_len= ); > > hdr_len =3D pkt->l2_len + pkt->l3_len + pkt->l4_len; > > > > - /* > > - * Don't process the packet which has FIN, SYN, RST, PSH, URG, EC= E > > - * or CWR set. > > - */ > > - if (tcp_hdr->tcp_flags !=3D RTE_TCP_ACK_FLAG) > > - return -1; > > - > > /* trim the tail padding bytes */ > > ip_tlen =3D rte_be_to_cpu_16(ipv4_hdr->total_length); > > if (pkt->pkt_len > (uint32_t)(ip_tlen + pkt->l2_len)) @@ -264,12 > > +271,30 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt, > > if (tbl->flows[i].start_index !=3D INVALID_ARRAY_INDEX) { > > if (is_same_tcp4_flow(tbl->flows[i].key, key)) { > > find =3D 1; > > + start_idx =3D tbl->flows[i].start_index; > > break; > > } > > remaining_flow_num--; > > } > > } > > > > + if (tcp_hdr->tcp_flags !=3D RTE_TCP_ACK_FLAG) { > > + /* > > + * Check and try merging the current TCP segment with the > > previous > > + * TCP segment if the TCP header does not contain RST and > > SYN flag > > + * There are cases where the last segment is sent with > > FIN|PSH|ACK > > + * which should also be considered for merging with > previous > > segments. > > + */ > > + if (find && !(tcp_hdr->tcp_flags & > > (RTE_TCP_RST_FLAG|RTE_TCP_SYN_FLAG))) > > + /* > > + * Since PSH flag is set, start time will be set > to 0 so it > > will be flushed > > + * immediately. > > + */ > > + tbl->items[start_idx].start_time =3D 0; > > + else > > + return -1; > > + } > > The nested if-else check is not straightforward, and it's hard to read th= e > condition-action of > different combinations of flag bits. In addition, are all flag bits > considered like Linux kernel? > >> Not all bits are considered. Will fix to make sure that all bits are >> considered. Will try to reorganise the code better. Let me know if you h= ave >> any other suggestions here. >> > > > + > > /* > > * Fail to find a matched flow. Insert a new flow and store the > > * packet into the flow. > > @@ -304,8 +329,12 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt, > > is_atomic); > > if (cmp) { > > if (merge_two_tcp4_packets(&(tbl->items[cur_idx])= , > > - pkt, cmp, sent_seq, ip_id= , > 0)) > > + pkt, cmp, sent_seq, ip_id= , > 0)) > > { > > + if (tbl->items[cur_idx].start_time =3D=3D= 0) > > + update_tcp_hdr_flags(tcp_hdr, tbl= - > > >items[cur_idx].firstseg); > > return 1; > > + } > > + > > /* > > * Fail to merge the two packets, as the packet > > * length is greater than the max value. Store > diff --git > > a/lib/gro/rte_gro.c b/lib/gro/rte_gro.c index e35399fd42..87c5502dce > > 100644 > > --- a/lib/gro/rte_gro.c > > +++ b/lib/gro/rte_gro.c > > @@ -283,10 +283,17 @@ rte_gro_reassemble_burst(struct rte_mbuf **pkts, > > if ((nb_after_gro < nb_pkts) > > || (unprocess_num < nb_pkts)) { > > i =3D 0; > > + /* Copy unprocessed packets */ > > + if (unprocess_num > 0) { > > + memcpy(&pkts[i], unprocess_pkts, > > + sizeof(struct rte_mbuf *) * > > + unprocess_num); > > + i =3D unprocess_num; > > + } > > Why copy unprocess pkts first? This is for avoiding out-of-order? > > Thanks, > Jiayu > > /* Flush all packets from the tables */ > > if (do_vxlan_tcp_gro) { > > - i =3D > gro_vxlan_tcp4_tbl_timeout_flush(&vxlan_tcp_tbl, > > - 0, pkts, nb_pkts); > > + i +=3D > > gro_vxlan_tcp4_tbl_timeout_flush(&vxlan_tcp_tbl, > > + 0, &pkts[i], nb_pkts - i); > > } > > > > if (do_vxlan_udp_gro) { > > @@ -304,13 +311,6 @@ rte_gro_reassemble_burst(struct rte_mbuf **pkts, > > i +=3D gro_udp4_tbl_timeout_flush(&udp_tbl, 0, > > &pkts[i], nb_pkts - i); > > } > > - /* Copy unprocessed packets */ > > - if (unprocess_num > 0) { > > - memcpy(&pkts[i], unprocess_pkts, > > - sizeof(struct rte_mbuf *) * > > - unprocess_num); > > - } > > - nb_after_gro =3D i + unprocess_num; > > } > > > > return nb_after_gro; > > -- > > 2.25.1 > > --0000000000005b684805fe9fd122 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Jiayu,

T= hanks for the comments. I have replied inline below. Will address the revie= w comments, but I think this would be a good patch to have in general. Plea= se let me know your thoughts.

On Tue, Jun 20, 2023 at 1:06=E2=80= =AFPM Hu, Jiayu <jiayu.hu@intel.co= m> wrote:
Hi Kumara,

Please see replies inline.

Thanks,
Jiayu

> -----Original Message-----
> From: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
> Sent: Tuesday, November 1, 2022 3:06 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>
> Cc: dev@dpdk.org= ; Kumara Parameshwaran
> <ku= maraparamesh92@gmail.com>; Kumara Parameshwaran
> <kpara= meshwar@vmware.com>
> Subject: [PATCH v5] gro : fix reordering of packets in GRO library
>
> From: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
>
> When a TCP packet contains flags like PSH it is returned immediately t= o the
> application though there might be packets of the same flow in the GRO = table.
> If PSH flag is set on a segment packets up to the segment should be de= livered
> immediately. But the current implementation delivers the last arrived = packet
> with PSH flag set causing re-ordering
>
> With this patch, if a packet does not contain only ACK flag and if the= re are no
> previous packets for the flow the packet would be returned immediately= ,
> else will be merged with the previous segment and the flag on the last=
> segment will be set on the entire segment.
> This is the behaviour with linux stack as well.
>
> Signed-off-by: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
> Co-authored-by: Kumara Parameshwaran <kparameshwar@vmware.com>
> ---
> v1:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0If the received packet is not a pure ACK pac= ket, we check if
>=C2=A0 =C2=A0 =C2=A0 =C2=A0there are any previous packets in the flow, = if present we indulge
>=C2=A0 =C2=A0 =C2=A0 =C2=A0the received packet also in the coalescing l= ogic and update the flags
>=C2=A0 =C2=A0 =C2=A0 =C2=A0of the last recived packet to the entire seg= ment which would avoid
>=C2=A0 =C2=A0 =C2=A0 =C2=A0re-ordering.
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0Lets say a case where P1(PSH), P2(ACK), P3(A= CK)=C2=A0 are received in
> burst mode,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0P1 contains PSH flag and since it does not c= ontain any prior packets in
> the flow
>=C2=A0 =C2=A0 =C2=A0 =C2=A0we copy it to unprocess_packets and P2(ACK) = and P3(ACK) are
> merged together.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0In the existing case the=C2=A0 P2,P3 would b= e delivered as single segment
> first and the
>=C2=A0 =C2=A0 =C2=A0 =C2=A0unprocess_packets will be copied later which= will cause reordering.
> With the patch
>=C2=A0 =C2=A0 =C2=A0 =C2=A0copy the unprocess packets first and then th= e packets from the GRO
> table.
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0Testing done
>=C2=A0 =C2=A0 =C2=A0 =C2=A0The csum test-pmd was modifited to support t= he following
>=C2=A0 =C2=A0 =C2=A0 =C2=A0GET request of 10MB from client to server vi= a test-pmd (static arp
> entries added in client
>=C2=A0 =C2=A0 =C2=A0 =C2=A0and server). Enable GRO and TSO in test-pmd = where the packets
> recived from the client mac
>=C2=A0 =C2=A0 =C2=A0 =C2=A0would be sent to server mac and vice versa.<= br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0In above testing, without the patch the clie= nt observerd re-ordering
> of 25 packets
>=C2=A0 =C2=A0 =C2=A0 =C2=A0and with the patch there were no packet re-o= rdering observerd.
>
> v2:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0Fix warnings in commit and comment.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0Do not consider packet as candidate to merge= if it contains SYN/RST
> flag.
>
> v3:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0Fix warnings.
>
> v4:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0Rebase with master.
>
> v5:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0Adding co-author email
>
>=C2=A0 lib/gro/gro_tcp4.c | 45 +++++++++++++++++++++++++++++++++++++---= -----
>=C2=A0 lib/gro/rte_gro.c=C2=A0 | 18 +++++++++---------
>=C2=A0 2 files changed, 46 insertions(+), 17 deletions(-)
>
> diff --git a/lib/gro/gro_tcp4.c b/lib/gro/gro_tcp4.c index
> 0014096e63..7363c5d540 100644
> --- a/lib/gro/gro_tcp4.c
> +++ b/lib/gro/gro_tcp4.c
> @@ -188,6 +188,19 @@ update_header(struct gro_tcp4_item *item)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0pkt->l2_len);
>=C2=A0 }
>
> +static inline void
> +update_tcp_hdr_flags(struct rte_tcp_hdr *tcp_hdr, struct rte_mbuf *pk= t)
> +{
> +=C2=A0 =C2=A0 =C2=A0struct rte_ether_hdr *eth_hdr;
> +=C2=A0 =C2=A0 =C2=A0struct rte_ipv4_hdr *ipv4_hdr;
> +=C2=A0 =C2=A0 =C2=A0struct rte_tcp_hdr *merged_tcp_hdr;
> +
> +=C2=A0 =C2=A0 =C2=A0eth_hdr =3D rte_pktmbuf_mtod(pkt, struct rte_ethe= r_hdr *);
> +=C2=A0 =C2=A0 =C2=A0ipv4_hdr =3D (struct rte_ipv4_hdr *)((char *)eth_= hdr + pkt->l2_len);
> +=C2=A0 =C2=A0 =C2=A0merged_tcp_hdr =3D (struct rte_tcp_hdr *)((char *= )ipv4_hdr + pkt-
> >l3_len);
> +=C2=A0 =C2=A0 =C2=A0merged_tcp_hdr->tcp_flags |=3D tcp_hdr->tcp= _flags; }

The Linux kernel updates the TCP flag via "tcp_flag_word(th2) |=3D fla= gs & (TCP_FLAG_FIN | TCP_FLAG_PSH)",
which only adds FIN and PSH at most to the merge packet.
Sure, will change it to add only FIN and= PSH.

> +
>=C2=A0 int32_t
>=C2=A0 gro_tcp4_reassemble(struct rte_mbuf *pkt,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct gro_tcp4_= tbl *tbl,
> @@ -206,6 +219,7 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0uint32_t i, max_flow_num, remaining_flow_num= ;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0int cmp;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0uint8_t find;
> +=C2=A0 =C2=A0 =C2=A0uint32_t start_idx;
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0/*
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 * Don't process the packet whose TCP he= ader length is greater @@ -
> 219,13 +233,6 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0tcp_hdr =3D (struct rte_tcp_hdr *)((char *)i= pv4_hdr + pkt->l3_len);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0hdr_len =3D pkt->l2_len + pkt->l3_len = + pkt->l4_len;
>
> -=C2=A0 =C2=A0 =C2=A0/*
> -=C2=A0 =C2=A0 =C2=A0 * Don't process the packet which has FIN, SY= N, RST, PSH, URG, ECE
> -=C2=A0 =C2=A0 =C2=A0 * or CWR set.
> -=C2=A0 =C2=A0 =C2=A0 */
> -=C2=A0 =C2=A0 =C2=A0if (tcp_hdr->tcp_flags !=3D RTE_TCP_ACK_FLAG)<= br> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -1;
> -
>=C2=A0 =C2=A0 =C2=A0 =C2=A0/* trim the tail padding bytes */
>=C2=A0 =C2=A0 =C2=A0 =C2=A0ip_tlen =3D rte_be_to_cpu_16(ipv4_hdr->to= tal_length);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (pkt->pkt_len > (uint32_t)(ip_tlen = + pkt->l2_len)) @@ -264,12
> +271,30 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (tbl->flow= s[i].start_index !=3D INVALID_ARRAY_INDEX) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0if (is_same_tcp4_flow(tbl->flows[i].key, key)) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0find =3D 1;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0start_idx =3D tbl->flows[i].start_ind= ex;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0}
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0remaining_flow_num--;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
>=C2=A0 =C2=A0 =C2=A0 =C2=A0}
>
> +=C2=A0 =C2=A0 =C2=A0if (tcp_hdr->tcp_flags !=3D RTE_TCP_ACK_FLAG) = {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/*
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * Check and try merg= ing the current TCP segment with the
> previous
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * TCP segment if the= TCP header does not contain RST and
> SYN flag
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * There are cases wh= ere the last segment is sent with
> FIN|PSH|ACK
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * which should also = be considered for merging with previous
> segments.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (find && != (tcp_hdr->tcp_flags &
> (RTE_TCP_RST_FLAG|RTE_TCP_SYN_FLAG)))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0/*
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 * Since PSH flag is set, start time will be set to 0 so it
> will be flushed
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 * immediately.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0tbl->items[start_idx].start_time =3D 0;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0else
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return -1;
> +=C2=A0 =C2=A0 =C2=A0}

The nested if-else check is not straightforward, and it's hard to read = the condition-action of
different combinations of flag bits. In addition, are all flag bits conside= red like Linux kernel?
Not all bits are considered. Will fix to make sure that all bits are consi= dered. Will try to reorganise the code better. Let me know if you have any = other suggestions here.

> +
>=C2=A0 =C2=A0 =C2=A0 =C2=A0/*
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 * Fail to find a matched flow. Insert a new= flow and store the
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 * packet into the flow.
> @@ -304,8 +329,12 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0is_atomic);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (cmp) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0if (merge_two_tcp4_packets(&(tbl->items[cur_idx]),
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0pkt, cmp, sent_seq, ip_id, 0))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0pkt, cmp, sent_seq, ip_id, 0))
> {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (tbl->items[cur_idx].start_time = =3D=3D 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0update_tcp_h= dr_flags(tcp_hdr, tbl-
> >items[cur_idx].firstseg);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 1;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0}
> +
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0/*
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 * Fail to merge the two packets, as the packet
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 * length is greater than the max value. Store diff --git
> a/lib/gro/rte_gro.c b/lib/gro/rte_gro.c index e35399fd42..87c5502dce > 100644
> --- a/lib/gro/rte_gro.c
> +++ b/lib/gro/rte_gro.c
> @@ -283,10 +283,17 @@ rte_gro_reassemble_burst(struct rte_mbuf **pkts,=
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if ((nb_after_gro < nb_pkts)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 || (unprocess_n= um < nb_pkts)) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0i =3D 0;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Copy unprocessed p= ackets */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (unprocess_num >= ; 0) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0memcpy(&pkts[i], unprocess_pkts,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0sizeof(struc= t rte_mbuf *) *
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0unprocess_nu= m);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0i =3D unprocess_num;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}

Why copy unprocess pkts first? This is for avoiding out-of-order?

Thanks,
Jiayu
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Flush all pac= kets from the tables */
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (do_vxlan_tcp= _gro) {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0i =3D gro_vxlan_tcp4_tbl_timeout_flush(&vxlan_tcp_tbl,
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A00, pkts, nb_= pkts);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0i +=3D
> gro_vxlan_tcp4_tbl_timeout_flush(&vxlan_tcp_tbl,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A00, &pkts= [i], nb_pkts - i);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (do_vxlan_udp= _gro) {
> @@ -304,13 +311,6 @@ rte_gro_reassemble_burst(struct rte_mbuf **pkts,<= br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0i +=3D gro_udp4_tbl_timeout_flush(&udp_tbl, 0,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0&p= kts[i], nb_pkts - i);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Copy unprocessed p= ackets */
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (unprocess_num >= ; 0) {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0memcpy(&pkts[i], unprocess_pkts,
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0sizeof(struc= t rte_mbuf *) *
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0unprocess_nu= m);
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0nb_after_gro =3D i + = unprocess_num;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0}
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0return nb_after_gro;
> --
> 2.25.1

--0000000000005b684805fe9fd122--