[dpdk-dev] [PATCH v6 2/3] gro: add VXLAN UDP/IPv4 GRO support

[yang_y_yi]

Thanks Jiayu, do you mean not comparing timestamp and flush all the packets in a flow as I showed code, right? If so, we shouldn't provide argument flush_timestamp. But this will result in very bad issues, rte_gro_timeout_flush will be called after rte_gro_reassemble every time, so the result may be you can't reassemble out any original UDP packet because every UDP fragments will be flushed very soon, no chance to reassemble.

At 2020-09-23 10:15:12, "Jiayu Hu" <jiayu.hu at intel.com> wrote:
>On Tue, Sep 22, 2020 at 03:38:29PM +0800, yang_y_yi wrote:
>> 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.
>
>OK, I got the point. I agree to flush packets without strictly
>obeying timestamp. But you need to change the comment in the
>code and clarify the design for both UDP and VxLAN GRO patch.
>Current comment "The left packets in ..." is not appropriate
>for your design.
>
>> 
>> 
>> I'm not sure what inconsistentcy you're saying mean.
>> 
>> At 2020-09-22 14:55:46, "Jiayu Hu" <jiayu.hu at 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.hu at 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_yi at 163.com>
>> >>     Sent: Tuesday, September 22, 2020 9:44 AM
>> >>     To: Hu, Jiayu <jiayu.hu at intel.com>
>> >>     Cc: dev at dpdk.org; thomas at monjalon.net; yangyi01 at 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_yi at 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;
>> >>
>> >>     >
>> >>
>> >>
>> >>
>> >>
>> >>
>> >>
>> >>
>> 
>> 
>> 
>>  
>>