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 C4D0AA0542; Mon, 29 Aug 2022 12:36:23 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6D9E54069D; Mon, 29 Aug 2022 12:36:23 +0200 (CEST) Received: from out3-smtp.messagingengine.com (out3-smtp.messagingengine.com [66.111.4.27]) by mails.dpdk.org (Postfix) with ESMTP id E6F534003C for ; Mon, 29 Aug 2022 12:36:21 +0200 (CEST) Received: from compute2.internal (compute2.nyi.internal [10.202.2.46]) by mailout.nyi.internal (Postfix) with ESMTP id 8566B5C0113; Mon, 29 Aug 2022 06:36:21 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute2.internal (MEProxy); Mon, 29 Aug 2022 06:36:21 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:cc:content-transfer-encoding:content-type:date:date:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to; s=fm1; t=1661769381; x= 1661855781; bh=TG+F0UsyCQSu/agybI90uzsaqtwoUPq2WBnDMBGsIZE=; b=P xhtlIsX16NCaRiDoZTrI+jIidafmpUqS2HAbqjeTEWJLXtMVnFIgEUvFOHufDyYy +fJn0euuyLNJu3YwznBizoPQimOAxLNrMGDZzGLXY3YtSDTgVJYkXjKYkuYLO8NI PvUtcDCyqClzKM/Qi9Y9Tk1JRollA/ifi9Q0DsSGIw1zy2s7X92MGMjeU2tEDLUX lVPrQ0D0gniIGrytHmsdB26T4T3ZQXQfTT+s2v53u+FqaMHLLaXtqsvMIQ4wNjXP +yyVXlQbnGe+aEQ9DPK7lkt3IorTLeWkPuzCTDlQWHHncd6nBLhRLf+0q4touH64 5ffK91H4Fi0of5ds7OJUQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:date:date:feedback-id:feedback-id:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to:x-me-proxy:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t=1661769381; x= 1661855781; bh=TG+F0UsyCQSu/agybI90uzsaqtwoUPq2WBnDMBGsIZE=; b=t mwNbBylVwfsaaeQdHpMDM2Kx/MbfSGhsFO//8EXQkSJ+zTdzDf23lgXolAULEF2Y HfjMdnZChDxr2pCgJh8dUaGRFB8bCCm0QBp/TKEg2qSk6s0FW01uE/u4uFHKx7+d sdwzdlARPzBjDCd/Rp4SvVwSGNsHlE0aeVWXRY3jpK6eCd0p3lh/hEMLfyuYGLCj dBvMVN+0zin71bD8oIei7aw5oeFjvY/jSoWD8YNu1ijvRXXh5Vadu2TJbEuvFw1A 2GCLCcdmsAGH6GfouERYG0k246NWTl+AxTlvu81ygN30OAyAP2oJryTVXZAcXryD qTqSnr/0swCZgYry9zZaQ== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvfedrvdekuddgfedvucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvvefufffkjghfggfgtgesthhqredttddtjeenucfhrhhomhepvfhhohhm rghsucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenuc ggtffrrghtthgvrhhnpeegtddtleejjeegffekkeektdejvedtheevtdekiedvueeuvdei uddvleevjeeujeenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfh hrohhmpehthhhomhgrshesmhhonhhjrghlohhnrdhnvght X-ME-Proxy: Feedback-ID: i47234305:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Mon, 29 Aug 2022 06:36:19 -0400 (EDT) From: Thomas Monjalon To: "jiayu.hu@intel.com" Cc: kumaraparameshwaran rathinavel , "dev@dpdk.org" , Jun Qiu Subject: Re: [PATCH] gro: fix gro with tcp push flag Date: Mon, 29 Aug 2022 12:36:18 +0200 Message-ID: <4016468.6PsWsQAL7t@thomas> In-Reply-To: References: <20220726061741.21021-1-jun.qiu@jaguarmicro.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="UTF-8" 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 Jiayu, please could you help in this review? 27/07/2022 10:44, Jun Qiu: > I think this delay is tolerable. > Many TCP stacks do not take special care of PUSH packets when receiving t= hem. All received packets with data will trigger Poll events. >=20 > The patch is simple to implement and easy to understand, similar to how t= he kernel stack is handled. >=20 > From: kumaraparameshwaran rathinavel > Sent: Tuesday, July 26, 2022 3:08 PM > To: Jun Qiu > Cc: dev@dpdk.org; jiayu.hu@intel.com; stable@dpdk.org > Subject: Re: [PATCH] gro: fix gro with tcp push flag >=20 > We should do it for the rte_gro_reassemble as well, with timer mode it co= uld lead to more duplicate ACKs. I had a proposal for the enhancement which= would handle both rte_gro_reassemble and rte_gro_reassemble_burst but hav= e not got any response yet. >=20 > I have a custom patch which is working fine for timer mode where there is= no packet reordering, earlier without the patch there were DUP-ACKs and th= is could potentially affect the window scaling. >=20 > On Tue, Jul 26, 2022 at 12:27 PM Jun Qiu > wrote: > May be in rte_gro_reassemble_burst, where no delay is introduced, PUSH pa= ckets can be merged >=20 > =E5=8F=91=E4=BB=B6=E4=BA=BA: kumaraparameshwaran rathinavel > > =E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2022=E5=B9=B47=E6=9C=8826=E6=97=A5 = 14:41 > =E6=94=B6=E4=BB=B6=E4=BA=BA: Jun Qiu > > =E6=8A=84=E9=80=81: dev@dpdk.org; jiayu.hu@intel.com= ; stable@dpdk.org > =E4=B8=BB=E9=A2=98: Re: [PATCH] gro: fix gro with tcp push flag >=20 >=20 >=20 > On Tue, Jul 26, 2022 at 11:48 AM Jun Qiu > wrote: > TCP data packets sometimes carry a PUSH flag. Currently, > only the packets that do not have PUSH flag can be GROed. > The packets that have a PUSH flag cannot be GROed, the packets > that cannot be processed by GRO are placed last. > In this case, the received packets may be out of order. > For example, there are two packets mbuf1 and mbuf2. mbuf1 > contains PUSH flag, mbuf2 does not contain PUSH flag. > After GRO processing, mbuf2 is sent for processing before mbuf1. > This out-of-order will affect TCP processing performance and > lead to unnecessary dup-ACK. >=20 > Referring to the Linux kernel implementation, packets with PUSH > flag can also perform GRO. And if one of the packets containing > PUSH flag, the packets after GRO will carry PUSH flag. >=20 > In case of smaller transfers in which the TCP segment size is not more th= an one MTU, it is a single TCP packet with PSH flag set, so in those cases = we are introducing unwanted delay. I think the better approach would be i= f there are previous packets in the flow and the current packet received ha= s PSH flag then coalesce with the previous packet, if lookup is failure and= the current packet has PSH flag set then deliver it immediately. >=20 > Fixes: 0d2cbe59b719 ("lib/gro: support TCP/IPv4") > Cc: stable@dpdk.org >=20 > Signed-off-by: Jun Qiu > > --- > lib/gro/gro_tcp4.c | 4 ++-- > lib/gro/gro_tcp4.h | 16 +++++++++++++--- > lib/gro/gro_vxlan_tcp4.c | 4 ++-- > 3 files changed, 17 insertions(+), 7 deletions(-) >=20 > diff --git a/lib/gro/gro_tcp4.c b/lib/gro/gro_tcp4.c > index 7498c66141..7849a2bd1d 100644 > --- a/lib/gro/gro_tcp4.c > +++ b/lib/gro/gro_tcp4.c > @@ -220,10 +220,10 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt, > hdr_len =3D pkt->l2_len + pkt->l3_len + pkt->l4_len; >=20 > /* > - * Don't process the packet which has FIN, SYN, RST, PSH, URG, ECE > + * Don't process the packet which has FIN, SYN, RST, URG, ECE > * or CWR set. > */ > - if (tcp_hdr->tcp_flags !=3D RTE_TCP_ACK_FLAG) > + if (tcp_hdr->tcp_flags & (~(RTE_TCP_ACK_FLAG | RTE_TCP_PSH_FLAG))) > return -1; > /* > * Don't process the packet whose payload length is less than or > diff --git a/lib/gro/gro_tcp4.h b/lib/gro/gro_tcp4.h > index 212f97a042..2974faf228 100644 > --- a/lib/gro/gro_tcp4.h > +++ b/lib/gro/gro_tcp4.h > @@ -210,7 +210,8 @@ merge_two_tcp4_packets(struct gro_tcp4_item *item, > uint16_t l2_offset) > { > struct rte_mbuf *pkt_head, *pkt_tail, *lastseg; > - uint16_t hdr_len, l2_len; > + struct rte_tcp_hdr *head_tcp_hdr, *tail_tcp_hdr; > + uint16_t hdr_len, l2_len, l3_offset; >=20 > if (cmp > 0) { > pkt_head =3D item->firstseg; > @@ -221,13 +222,22 @@ merge_two_tcp4_packets(struct gro_tcp4_item *item, > } >=20 > /* check if the IPv4 packet length is greater than the max value = */ > - hdr_len =3D l2_offset + pkt_head->l2_len + pkt_head->l3_len + > - pkt_head->l4_len; > + l3_offset =3D l2_offset + pkt_head->l2_len + pkt_head->l3_len; > + hdr_len =3D l3_offset + pkt_head->l4_len; > l2_len =3D l2_offset > 0 ? pkt_head->outer_l2_len : pkt_head->l2_= len; > if (unlikely(pkt_head->pkt_len - l2_len + pkt_tail->pkt_len - > hdr_len > MAX_IPV4_PKT_LENGTH)) > return 0; >=20 > + /* merge push flag to pkt_head */ > + tail_tcp_hdr =3D rte_pktmbuf_mtod_offset(pkt_tail, > + struct rte_tcp_hdr *, l3_offset); > + if (tail_tcp_hdr->tcp_flags & RTE_TCP_PSH_FLAG) { > + head_tcp_hdr =3D rte_pktmbuf_mtod_offset(pkt_head, > + struct rte_tcp_hdr *, l3_offset); > + head_tcp_hdr->tcp_flags |=3D RTE_TCP_PSH_FLAG; > + } > + > /* remove the packet header for the tail packet */ > rte_pktmbuf_adj(pkt_tail, hdr_len); >=20 > diff --git a/lib/gro/gro_vxlan_tcp4.c b/lib/gro/gro_vxlan_tcp4.c > index 3be4deb7c7..884802af0b 100644 > --- a/lib/gro/gro_vxlan_tcp4.c > +++ b/lib/gro/gro_vxlan_tcp4.c > @@ -326,10 +326,10 @@ gro_vxlan_tcp4_reassemble(struct rte_mbuf *pkt, > tcp_hdr =3D (struct rte_tcp_hdr *)((char *)ipv4_hdr + pkt->l3_len= ); >=20 > /* > - * Don't process the packet which has FIN, SYN, RST, PSH, URG, > + * Don't process the packet which has FIN, SYN, RST, URG, > * ECE or CWR set. > */ > - if (tcp_hdr->tcp_flags !=3D RTE_TCP_ACK_FLAG) > + if (tcp_hdr->tcp_flags & (~(RTE_TCP_ACK_FLAG | RTE_TCP_PSH_FLAG))) > return -1; >=20 > hdr_len =3D pkt->outer_l2_len + pkt->outer_l3_len + pkt->l2_len + > -- > 2.25.1 >=20