DPDK patches and discussions
 help / color / mirror / Atom feed
From: kumaraparameshwaran rathinavel <kumaraparamesh92@gmail.com>
To: Jun Qiu <jun.qiu@jaguarmicro.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	"jiayu.hu@intel.com" <jiayu.hu@intel.com>,
	 "stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [PATCH] gro: fix gro with tcp push flag
Date: Tue, 26 Jul 2022 12:38:21 +0530	[thread overview]
Message-ID: <CANxNyasVd9nUv=C1F9Y+GP8BXVjksyAwrjthrO105ntu8p-R_g@mail.gmail.com> (raw)
In-Reply-To: <KL1PR0601MB4530942B32414C97701CF28AF7949@KL1PR0601MB4530.apcprd06.prod.outlook.com>

[-- Attachment #1: Type: text/plain, Size: 5920 bytes --]

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> wrote:

> May be in rte_gro_reassemble_burst, where no delay is introduced, PUSH
> packets can be merged
>
>
>
> *发件人:* kumaraparameshwaran rathinavel <kumaraparamesh92@gmail.com>
> *发送时间:* 2022年7月26日 14:41
> *收件人:* Jun Qiu <jun.qiu@jaguarmicro.com>
> *抄送:* dev@dpdk.org; jiayu.hu@intel.com; 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> 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.
>
>
>
> Fixes: 0d2cbe59b719 ("lib/gro: support TCP/IPv4")
> Cc: stable@dpdk.org
>
> Signed-off-by: Jun Qiu <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
>
>

[-- Attachment #2: Type: text/html, Size: 9578 bytes --]

  reply	other threads:[~2022-07-26  7:08 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 [this message]
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
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='CANxNyasVd9nUv=C1F9Y+GP8BXVjksyAwrjthrO105ntu8p-R_g@mail.gmail.com' \
    --to=kumaraparamesh92@gmail.com \
    --cc=dev@dpdk.org \
    --cc=jiayu.hu@intel.com \
    --cc=jun.qiu@jaguarmicro.com \
    --cc=stable@dpdk.org \
    /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).