Re: [PATCH v9] gro: fix reordering of packets in GRO layer

[胡嘉瑜 <hujiayu.hu@foxmail.com>]

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


在 2023/12/9 上午2:17, Kumara Parameshwaran 写道:
> 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) {

It is more likely to find a match flow. So better to put the below logic 
to the else statement.

> -		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 {

"else" is not needed.

>   			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)) {
Add a space beween RTE_TCP_ACK_FLAG and '|'.
> +			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 {

It is better to check the "invalid" flags, like SYN, RST and URG, at the 
beginning of

gro_tcp4_reassemble(), as the packet can be returned earlier.

Thanks,

Jiayu

>   			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;
>   }
>   
>   /*

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