Jiayu, is this comment ok?
diff --git a/lib/librte_gro/gro_udp4.c b/lib/librte_gro/gro_udp4.c
index 061e7b0..93e368a 100644
--- a/lib/librte_gro/gro_udp4.c
+++ b/lib/librte_gro/gro_udp4.c
@@ -409,8 +409,10 @@
return k;
} else
/*
- * The left packets in this flow won't be
- * timeout. Go to check other flows.
+ * If start_time of one item is greater than
+ * flush_timestamp, that means the left packets
+ * in this flow won't be flushed this time. Go
+ * to check other flows.
*/
break;
}
At 2020-09-23 10:43:03, "Jiayu Hu" <jiayu...@intel.com> wrote:
>On Wed, Sep 23, 2020 at 10:28:00AM +0800, yang_y_yi wrote:
>> 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.
>
>No, I mean the design in your patch, which stops flushing packets
>once find one whose timestamp is greater than flush_timestamp.
>
>>
>> At 2020-09-23 10:15:12, "Jiayu Hu" <jiayu...@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...@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;
>> >> >>
>> >> >> >
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >>
>> >>
>> >>
>> >>
>> >>
>>
>>
>>
>>
>>
