From: "Hu, Jiayu" <jiayu.hu@intel.com>
To: Thomas Monjalon <thomas@monjalon.net>
Cc: kumaraparameshwaran rathinavel <kumaraparamesh92@gmail.com>,
"dev@dpdk.org" <dev@dpdk.org>, Jun Qiu <jun.qiu@jaguarmicro.com>
Subject: RE: [PATCH] gro: fix gro with tcp push flag
Date: Mon, 5 Sep 2022 04:02:31 +0000 [thread overview]
Message-ID: <927c323f44f64bbc80b0f18828ddd016@intel.com> (raw)
In-Reply-To: <8bafa976e0b54e6eaeba03cd490f9d86@intel.com>
Hi Qiu,
I cannot find the original mail, and please see replies inline.
> -----Original Message-----
> From: Hu, Jiayu
> Sent: Tuesday, August 30, 2022 4:44 PM
> To: 'Thomas Monjalon' <thomas@monjalon.net>
> Cc: kumaraparameshwaran rathinavel <kumaraparamesh92@gmail.com>;
> dev@dpdk.org; Jun Qiu <jun.qiu@jaguarmicro.com>
> Subject: RE: [PATCH] gro: fix gro with tcp push flag
>
> Sure, I will review it.
>
> > -----Original Message-----
> > From: Thomas Monjalon <thomas@monjalon.net>
> > Sent: Monday, August 29, 2022 6:36 PM
> > To: Hu, Jiayu <jiayu.hu@intel.com>
> > Cc: kumaraparameshwaran rathinavel <kumaraparamesh92@gmail.com>;
> > dev@dpdk.org; Jun Qiu <jun.qiu@jaguarmicro.com>
> > Subject: Re: [PATCH] gro: fix gro with tcp push flag
> >
> > 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
> > them. All received packets with data will trigger Poll events.
> > >
> > > The patch is simple to implement and easy to understand, similar to
> > > how
> > the kernel stack is handled.
> > >
> > > From: kumaraparameshwaran rathinavel
> <kumaraparamesh92@gmail.com>
> > > Sent: Tuesday, July 26, 2022 3:08 PM
> > > To: Jun Qiu <jun.qiu@jaguarmicro.com>
> > > Cc: dev@dpdk.org; jiayu.hu@intel.com; stable@dpdk.org
> > > Subject: Re: [PATCH] gro: fix gro with tcp push flag
> > >
> > > We should do it for the rte_gro_reassemble as well, with timer mode
> > > it
> > could 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 have not got any response yet.
> > >
> > > 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 this could potentially affect the window scaling.
> > >
> > > On Tue, Jul 26, 2022 at 12:27 PM Jun Qiu
> > <jun.qiu@jaguarmicro.com<mailto:jun.qiu@jaguarmicro.com>> wrote:
> > > May be in rte_gro_reassemble_burst, where no delay is introduced,
> > > PUSH packets can be merged
> > >
> > > 发件人: kumaraparameshwaran rathinavel
> > >
> <kumaraparamesh92@gmail.com<mailto:kumaraparamesh92@gmail.com>>
> > > 发送时间: 2022年7月26日 14:41
> > > 收件人: Jun Qiu
> > <jun.qiu@jaguarmicro.com<mailto:jun.qiu@jaguarmicro.com>>
> > > 抄送: dev@dpdk.org<mailto:dev@dpdk.org>;
> > > jiayu.hu@intel.com<mailto:jiayu.hu@intel.com>;
> > > stable@dpdk.org<mailto:stable@dpdk.org>
> > > 主题: Re: [PATCH] gro: fix gro with tcp push flag
> > >
> > >
> > >
> > > On Tue, Jul 26, 2022 at 11:48 AM Jun Qiu
> > <jun.qiu@jaguarmicro.com<mailto:jun.qiu@jaguarmicro.com>> 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.
> > >
> > > 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.
> > >
> > > In case of smaller transfers in which the TCP segment size is not
> > > more than
> > 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 if there are previous packets in the flow and the current
> > packet received has PSH flag then coalesce with the previous packet,
> > if lookup is failure and the current packet has PSH flag set then deliver it
> immediately.
I agree with Kuma. The implementation in the patch will make PSH packet
wait in the gro table. For a PSH packet, we need to try to merge it with
previous packets in the table first. If merge successfully, the merged packet
needs to mark with PSH flag and deliver it to app then; If not, this
PSH packet also needs to be delivered to app immediately.
Thanks,
Jiayu
> > >
> > > Fixes: 0d2cbe59b719 ("lib/gro: support TCP/IPv4")
> > > Cc: stable@dpdk.org<mailto:stable@dpdk.org>
> > >
> > > Signed-off-by: Jun Qiu
> > > <jun.qiu@jaguarmicro.com<mailto:jun.qiu@jaguarmicro.com>>
> > > ---
> > > 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(-)
> > >
> > > 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 = pkt->l2_len + pkt->l3_len + pkt->l4_len;
> > >
> > > /*
> > > - * 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 != 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;
> > >
> > > if (cmp > 0) {
> > > pkt_head = item->firstseg; @@ -221,13 +222,22 @@
> > > merge_two_tcp4_packets(struct gro_tcp4_item *item,
> > > }
> > >
> > > /* check if the IPv4 packet length is greater than the max value */
> > > - hdr_len = l2_offset + pkt_head->l2_len + pkt_head->l3_len +
> > > - pkt_head->l4_len;
> > > + l3_offset = l2_offset + pkt_head->l2_len + pkt_head->l3_len;
> > > + hdr_len = l3_offset + pkt_head->l4_len;
> > > l2_len = 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;
> > >
> > > + /* merge push flag to pkt_head */
> > > + tail_tcp_hdr = 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 = rte_pktmbuf_mtod_offset(pkt_head,
> > > + struct rte_tcp_hdr *, l3_offset);
> > > + head_tcp_hdr->tcp_flags |= RTE_TCP_PSH_FLAG;
> > > + }
> > > +
> > > /* remove the packet header for the tail packet */
> > > rte_pktmbuf_adj(pkt_tail, hdr_len);
> > >
> > > 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 = (struct rte_tcp_hdr *)((char *)ipv4_hdr +
> > > pkt->l3_len);
> > >
> > > /*
> > > - * 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 != RTE_TCP_ACK_FLAG)
> > > + if (tcp_hdr->tcp_flags & (~(RTE_TCP_ACK_FLAG |
> > > + RTE_TCP_PSH_FLAG)))
> > > return -1;
> > >
> > > hdr_len = pkt->outer_l2_len + pkt->outer_l3_len +
> > > pkt->l2_len
> > > +
> > > --
> > > 2.25.1
> > >
> >
> >
> >
> >
next prev parent reply other threads:[~2022-09-05 4:02 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-26 6:17 Jun Qiu
2022-07-26 6:41 ` kumaraparameshwaran rathinavel
2022-07-26 6:57 ` 答复: " Jun Qiu
2022-07-26 7:08 ` kumaraparameshwaran rathinavel
2022-07-27 8:44 ` Jun Qiu
2022-08-29 10:36 ` Thomas Monjalon
2022-08-30 8:43 ` Hu, Jiayu
2022-09-05 4:02 ` Hu, Jiayu [this message]
2022-09-05 7:07 ` Jun Qiu
2022-10-14 1:48 ` [PATCH v2] " Jun Qiu
2022-10-14 2:37 [PATCH] " Jun Qiu
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=927c323f44f64bbc80b0f18828ddd016@intel.com \
--to=jiayu.hu@intel.com \
--cc=dev@dpdk.org \
--cc=jun.qiu@jaguarmicro.com \
--cc=kumaraparamesh92@gmail.com \
--cc=thomas@monjalon.net \
/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).