From: Jiayu Hu <jiayu.hu@intel.com>
To: yang_y_yi <yang_y_yi@163.com>
Cc: dev@dpdk.org, thomas@monjalon.net, yangyi01@inspur.com,
jiayu.hu@intel.com
Subject: Re: [dpdk-dev] [PATCH v6 2/3] gro: add VXLAN UDP/IPv4 GRO support
Date: Wed, 23 Sep 2020 10:43:03 +0800 [thread overview]
Message-ID: <20200923024303.GA91241@NPG_DPDK_VIRTIO_jiayuhu_15.sh.intel.com> (raw)
In-Reply-To: <56bb5989.20af.174b8ca00c3.Coremail.yang_y_yi@163.com>
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.hu@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@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@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@163.com>
> >> >> Sent: Tuesday, September 22, 2020 9:44 AM
> >> >> To: Hu, Jiayu <jiayu.hu@intel.com>
> >> >> Cc: dev@dpdk.org; thomas@monjalon.net; yangyi01@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@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;
> >> >>
> >> >> >
> >> >>
> >> >>
> >> >>
> >> >>
> >> >>
> >> >>
> >> >>
> >>
> >>
> >>
> >>
> >>
>
>
>
>
>
next prev parent reply other threads:[~2020-09-23 2:33 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-17 3:49 [dpdk-dev] [PATCH v6 0/3] gro: add UDP/IPv4 GRO and " yang_y_yi
2020-09-17 3:49 ` [dpdk-dev] [PATCH v6 1/3] gro: add " yang_y_yi
2020-09-21 6:21 ` Hu, Jiayu
2020-09-17 3:49 ` [dpdk-dev] [PATCH v6 2/3] gro: add VXLAN " yang_y_yi
2020-09-21 7:54 ` Hu, Jiayu
2020-09-22 1:29 ` yang_y_yi
2020-09-22 1:44 ` yang_y_yi
2020-09-22 6:14 ` Hu, Jiayu
2020-09-22 6:23 ` yang_y_yi
2020-09-22 6:55 ` Jiayu Hu
2020-09-22 7:38 ` yang_y_yi
2020-09-23 2:15 ` Jiayu Hu
2020-09-23 2:28 ` yang_y_yi
2020-09-23 2:43 ` Jiayu Hu [this message]
2020-09-24 2:41 ` yang_y_yi
2020-09-22 3:01 ` Jiayu Hu
2020-09-22 3:00 ` yang_y_yi
2020-09-17 3:49 ` [dpdk-dev] [PATCH v6 3/3] doc: update prog_guide and rel_notes for GRO yang_y_yi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200923024303.GA91241@NPG_DPDK_VIRTIO_jiayuhu_15.sh.intel.com \
--to=jiayu.hu@intel.com \
--cc=dev@dpdk.org \
--cc=thomas@monjalon.net \
--cc=yang_y_yi@163.com \
--cc=yangyi01@inspur.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).