The problem is timestamp of which one item in a flow we should use as timestamp
reference base for this flow, for merge (reassemble), only the first packet can
trigger merge of all the packets, merge is forward not backward, if you
traverse the whole linked list in this flow to get the oldest timestamp and
compare it with flushtime, that will be not worthy, in the worst case (say I
send a 64K UDP packet and MTU is 1450), you will have 46 items to check to get
the oldest timestamp by linked list.
I'm not sure what inconsistentcy you're saying mean.
At 2020-09-22 14:55:46, "Jiayu Hu" <jiayu...@intel.com> wrote:
>On Tue, Sep 22, 2020 at 02:23:39PM +0800, yang_y_yi wrote:
>> Not a question, in next flush, they will be flushed, we have to check
>> timestamp
>> in the first time unless we don't strictly follow this time limitation.
>
>who will check the timestamp? I did't get the point.
>
>IMO, this will cause inconsistency of the rte_gro_timeout_flush().
>BTW, what stops you to traverse all items and check timestamp
>before flush them out?
>
>>
>> At 2020-09-22 14:14:00, "Hu, Jiayu" <jiayu...@intel.com> wrote:
>>
>> 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> 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;
>>
>> >
>>
>>
>>
>>
>>
>>
>>
