From: kumaraparameshwaran rathinavel <kumaraparamesh92@gmail.com>
To: hujiayu.hu@foxmail.com
Cc: dev@dpdk.org, Kumara Parameshwaran <krathinavel@microsoft.com>
Subject: Re: [PATCH v9] gro: fix reordering of packets in GRO layer
Date: Thu, 18 Jan 2024 01:43:28 +0530 [thread overview]
Message-ID: <CANxNyasq_r3JF3zXYagpy_H_DLayfZFAm7HQNs-Set6DD+_5ag@mail.gmail.com> (raw)
In-Reply-To: <20240117201014.425906-1-kumaraparamesh92@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 8861 bytes --]
On Thu, Jan 18, 2024 at 1:40 AM Kumara Parameshwaran <
kumaraparamesh92@gmail.com> wrote:
> In the current implementation when a packet is received with
> special TCP flag(s) set, only that packet is delivered out of order.
> There could be already coalesced packets in the GRO table
> belonging to the same flow but not delivered.
> This fix makes sure that the entire segment is delivered with the
> special flag(s) set which is how the Linux GRO is also implemented
>
> Signed-off-by: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
> Co-authored-by: Kumara Parameshwaran <krathinavel@microsoft.com>
> ---
> 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 avoid
> 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
> reordering. With the patch
> copy the unprocess packets first and then the packets from the GRO
> table.
>
> Testing done
> The csum test-pmd was modified to support the following
> GET request of 10MB from client to server via test-pmd (static arp
> 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/RST flag.
>
> v3:
> Fix warnings.
>
> v4:
> Rebase with master.
>
> v5:
> Adding co-author email
> v6:
> Address review comments from the maintainer to restructure the
> code
> and handle only special flags PSH,FIN
>
> v7:
> Fix warnings and errors
>
> v8:
> Fix warnings and errors
>
> v9:
> Fix commit message
>
> lib/gro/gro_tcp.h | 11 ++++++++
> lib/gro/gro_tcp4.c | 67 +++++++++++++++++++++++++++++-----------------
> 2 files changed, 54 insertions(+), 24 deletions(-)
>
> diff --git a/lib/gro/gro_tcp.h b/lib/gro/gro_tcp.h
> index d926c4b8cc..137a03bc96 100644
> --- a/lib/gro/gro_tcp.h
> +++ b/lib/gro/gro_tcp.h
> @@ -187,4 +187,15 @@ is_same_common_tcp_key(struct cmn_tcp_key *k1, struct
> cmn_tcp_key *k2)
> return (!memcmp(k1, k2, sizeof(struct cmn_tcp_key)));
> }
>
> +static inline void
> +update_tcp_hdr_flags(struct rte_tcp_hdr *tcp_hdr, struct rte_mbuf *pkt)
> +{
> + struct rte_tcp_hdr *merged_tcp_hdr;
> +
> + merged_tcp_hdr = rte_pktmbuf_mtod_offset(pkt, struct rte_tcp_hdr
> *, pkt->l2_len +
> + pkt->l3_len);
> + merged_tcp_hdr->tcp_flags |= tcp_hdr->tcp_flags;
> +
> +}
> +
> #endif
> diff --git a/lib/gro/gro_tcp4.c b/lib/gro/gro_tcp4.c
> index 6645de592b..8af5a8d8a9 100644
> --- a/lib/gro/gro_tcp4.c
> +++ b/lib/gro/gro_tcp4.c
> @@ -126,6 +126,7 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
> uint32_t item_idx;
> uint32_t i, max_flow_num, remaining_flow_num;
> uint8_t find;
> + uint32_t item_start_idx;
>
> /*
> * Don't process the packet whose TCP header length is greater
> @@ -139,13 +140,6 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
> tcp_hdr = (struct rte_tcp_hdr *)((char *)ipv4_hdr + pkt->l3_len);
> hdr_len = pkt->l2_len + pkt->l3_len + pkt->l4_len;
>
> - /*
> - * Don't process the packet which has FIN, SYN, RST, PSH, URG, ECE
> - * or CWR set.
> - */
> - if (tcp_hdr->tcp_flags != RTE_TCP_ACK_FLAG)
> - return -1;
> -
> /* trim the tail padding bytes */
> ip_tlen = rte_be_to_cpu_16(ipv4_hdr->total_length);
> if (pkt->pkt_len > (uint32_t)(ip_tlen + pkt->l2_len))
> @@ -183,6 +177,7 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
> if (tbl->flows[i].start_index != INVALID_ARRAY_INDEX) {
> if (is_same_tcp4_flow(tbl->flows[i].key, key)) {
> find = 1;
> + item_start_idx = tbl->flows[i].start_index;
> break;
> }
> remaining_flow_num--;
> @@ -190,28 +185,52 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
> }
>
> if (find == 0) {
> - 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)
> + /*
> + * Add new flow to the table only if contains ACK flag
> with data.
> + * Do not add any packets with additional tcp flags to the
> GRO table
> + */
> + if (tcp_hdr->tcp_flags == RTE_TCP_ACK_FLAG) {
> + 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) {
> + /*
> + * Fail to insert a new flow, so delete the
> + * stored packet.
> + */
> + delete_tcp_item(tbl->items, item_idx,
> &tbl->item_num,
> + INVALID_ARRAY_INDEX);
> + return -1;
> + }
> + return 0;
> + } else {
> return -1;
> - if (insert_new_flow(tbl, &key, item_idx) ==
> - INVALID_ARRAY_INDEX) {
> - /*
> - * Fail to insert a new flow, so delete the
> - * stored packet.
> - */
> - delete_tcp_item(tbl->items, item_idx,
> &tbl->item_num, INVALID_ARRAY_INDEX);
> + }
> + } else {
> + /*
> + * Any packet with additional flags like PSH,FIN should be
> processed
> + * and flushed immediately.
> + * Hence marking the start time to 0, so that the packets
> will be flushed
> + * immediately in timer mode.
> + */
> + if (tcp_hdr->tcp_flags &
> (RTE_TCP_ACK_FLAG|RTE_TCP_PSH_FLAG|RTE_TCP_FIN_FLAG)) {
> + if (tcp_hdr->tcp_flags != RTE_TCP_ACK_FLAG)
> + tbl->items[item_start_idx].start_time = 0;
> + 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);
> + } else {
> return -1;
> }
> - return 0;
> }
>
> - 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);
> + return -1;
> }
>
> /*
> --
> 2.34.1
> >> Please ignore.
>
[-- Attachment #2: Type: text/html, Size: 10993 bytes --]
next prev parent reply other threads:[~2024-01-17 20:13 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20240117195228.423261-1-kumaraparamesh92@gmail.co>
2024-01-17 20:10 ` Kumara Parameshwaran
2024-01-17 20:13 ` kumaraparameshwaran rathinavel [this message]
2022-11-01 7:05 [PATCH v5] gro : fix reordering of packets in GRO library Kumara Parameshwaran
2023-12-08 18:17 ` [PATCH v9] gro: fix reordering of packets in GRO layer Kumara Parameshwaran
2024-01-04 15:49 ` 胡嘉瑜
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=CANxNyasq_r3JF3zXYagpy_H_DLayfZFAm7HQNs-Set6DD+_5ag@mail.gmail.com \
--to=kumaraparamesh92@gmail.com \
--cc=dev@dpdk.org \
--cc=hujiayu.hu@foxmail.com \
--cc=krathinavel@microsoft.com \
/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).