Fragments of a flow are sorted by frag_oft, but they may have different timestamp. For example, there are three fragments, whose frag_oft is: frag[0].frag_oft=0, frag[1].frag_oft=4, frag[2].frag_oft=6; and they are fragments of one UDP packet but are not neighbors. In the first RX burst, host receives frag[1] and calls rte_gro_reassemble(), and we assume the timestamp of frag[1] is 10; in the second RX burst, host receives frag[0] and also call rte_gro_reassemble(), and timestamp of frag[0] is 11; the third time, host receives frag[2] and timestamp of frag[2] is 12. The three fragments are stored in three items of a UDP GRO table: items[0]: frag[0], timestamp is 11 items[1]: frag[1], timestamp is 10 items[2]: frag[2], timestamp is 12 Now we want to flush packets whose timestamp is less than or equal to 10. frag[1] should be returned, but in your code, no packets will be flushed. Because the timestamp of items[0] is greater than 10, the left two fragments will not be checked. This is what I want to say.
From: yang_y_yi <yang_y...@163.com> Sent: Tuesday, September 22, 2020 9:44 AM To: Hu, Jiayu <jiayu...@intel.com> Cc: dev@dpdk.org; tho...@monjalon.net; yangy...@inspur.com Subject: Re:Re: [dpdk-dev] [PATCH v6 2/3] gro: add VXLAN UDP/IPv4 GRO support Importance: High BTW, start_time is checked for the first packet in a flow, gro_udp4_merge_items(tbl, j) will merge all the packets in this flow once if they can be reassembled, gro_udp4_merge_items(tbl, j) doesn't check start_time, so this still can let some new items in this flow have chance to be merged. At 2020-09-22 09:29:38, "yang_y_yi" <yang_y...@163.com<mailto:yang_y...@163.com>> wrote: >Thanks Jiayu, I have fixed other comments except this one: > > > >>The items of a flow are ordered by frag_oft, and start_time >>of these items is not always in ascending order. Therefore, >>you cannot skip checking the items after the item whose >>start_time is greater than flush_timestamp. This issue also >>exists in UDP/IPv4 GRO, and need to correct them both. > > >I think the issue here is if we should strictly follow flush_timestamp, it is >possible there are new items in items chain. we have chance to merge more >packets if we don't follow flush_timestamp. So an ideal change can be this. >But is it acceptible if we don't use flush_timestamp? It can flush some >packets in advance therefore miss next merge window. Maybe current way is most >resonable and a tradeoff between two exterem cases. > > > > > >diff --git a/lib/librte_gro/gro_udp4.c b/lib/librte_gro/gro_udp4.c >index 061e7b0..ffa35a2 100644 >--- a/lib/librte_gro/gro_udp4.c >+++ b/lib/librte_gro/gro_udp4.c >@@ -391,7 +391,6 @@ > > j = tbl->flows[i].start_index; > while (j != INVALID_ARRAY_INDEX) { >- if (tbl->items[j].start_time <= flush_timestamp) { > gro_udp4_merge_items(tbl, j); > out[k++] = tbl->items[j].firstseg; > if (tbl->items[j].nb_merged > 1) >@@ -407,12 +406,6 @@ > > if (unlikely(k == nb_out)) > return k; >- } else >- /* >- * The left packets in this flow won't be >- * timeout. Go to check other flows. >- */ >- break; > } > } > return k; >
