DPDK patches and discussions
 help / color / mirror / Atom feed
From: yang_y_yi  <yang_y_yi@163.com>
To: "Jiayu Hu" <jiayu.hu@intel.com>
Cc: dev@dpdk.org, thomas@monjalon.net, yangyi01@inspur.com
Subject: Re: [dpdk-dev] [PATCH v6 2/3] gro: add VXLAN UDP/IPv4 GRO support
Date: Thu, 24 Sep 2020 10:41:28 +0800 (CST)	[thread overview]
Message-ID: <14c28531.1ee6.174bdfcb27b.Coremail.yang_y_yi@163.com> (raw)
In-Reply-To: <20200923024303.GA91241@NPG_DPDK_VIRTIO_jiayuhu_15.sh.intel.com>

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.hu@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.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;
>> >> >>
>> >> >>     >
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >>
>> >>
>> >>
>> >>
>> >>
>> 
>> 
>> 
>>  
>> 

  reply	other threads:[~2020-09-24  2:41 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
2020-09-24  2:41                       ` yang_y_yi [this message]
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=14c28531.1ee6.174bdfcb27b.Coremail.yang_y_yi@163.com \
    --to=yang_y_yi@163.com \
    --cc=dev@dpdk.org \
    --cc=jiayu.hu@intel.com \
    --cc=thomas@monjalon.net \
    --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).