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