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