[Qemu-devel] [PATCH V4 0/3] Optimize COLO-compare performance

[Qemu-devel] [PATCH V4 0/3] Optimize COLO-compare performance

[Zhang Chen]

In this serise, we do a lot of job to optimize COLO net performance.
Mainly focus on TCP protocol.

V4:
 - Remove the old patch1.

V3:
 - Rebase on upstream.
 - Remove origin p2.
 - Move the "checkpoint_time_ms" to CompareState,
   in order to aviod multi colo-compare instance conflict.
 - Add "TODO comments" for reset s->checkpoint_time_ms.
 - Add a new patch fix comments and scheme.

V2:
 - Rename p2's subject.


Zhang Chen (3):
  net/colo-compare.c: Optimize unpredictable tcp options comparison
  net/colo-compare.c: Adjust net queue pop order for performance
  net/colo-compare.c: Fix comments and scheme

 net/colo-compare.c | 62 +++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 22 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH V4 1/3] net/colo-compare.c: Optimize unpredictable tcp options comparison

[Zhang Chen]

When network is busy, some tcp options(like sack) will unpredictable
occur in primary side or secondary side. it will make packet size
not same, but the two packet's payload is identical. colo just
care about packet payload, so we skip the option field.

Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
---
 net/colo-compare.c | 39 +++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index ca67c68..f6bda41 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -186,7 +186,10 @@ static int packet_enqueue(CompareState *s, int mode)
  * return:    0  means packet same
  *            > 0 || < 0 means packet different
  */
-static int colo_packet_compare_common(Packet *ppkt, Packet *spkt, int offset)
+static int colo_packet_compare_common(Packet *ppkt,
+                                      Packet *spkt,
+                                      int poffset,
+                                      int soffset)
 {
     if (trace_event_get_state(TRACE_COLO_COMPARE_MISCOMPARE)) {
         char pri_ip_src[20], pri_ip_dst[20], sec_ip_src[20], sec_ip_dst[20];
@@ -201,12 +204,13 @@ static int colo_packet_compare_common(Packet *ppkt, Packet *spkt, int offset)
                                    sec_ip_src, sec_ip_dst);
     }
 
-    offset = ppkt->vnet_hdr_len + offset;
+    poffset = ppkt->vnet_hdr_len + poffset;
+    soffset = ppkt->vnet_hdr_len + soffset;
 
-    if (ppkt->size == spkt->size) {
-        return memcmp(ppkt->data + offset,
-                      spkt->data + offset,
-                      spkt->size - offset);
+    if (ppkt->size == spkt->size || poffset != soffset) {
+        return memcmp(ppkt->data + poffset,
+                      spkt->data + soffset,
+                      spkt->size - soffset);
     } else {
         trace_colo_compare_main("Net packet size are not the same");
         return -1;
@@ -263,13 +267,22 @@ static int colo_packet_compare_tcp(Packet *spkt, Packet *ppkt)
      * so we just need skip this field.
      */
     if (ptcp->th_off > 5) {
-        ptrdiff_t tcp_offset;
+        ptrdiff_t ptcp_offset, stcp_offset;
 
-        tcp_offset = ppkt->transport_header - (uint8_t *)ppkt->data
-                     + (ptcp->th_off * 4) - ppkt->vnet_hdr_len;
-        res = colo_packet_compare_common(ppkt, spkt, tcp_offset);
+        ptcp_offset = ppkt->transport_header - (uint8_t *)ppkt->data
+                      + (ptcp->th_off * 4) - ppkt->vnet_hdr_len;
+        stcp_offset = spkt->transport_header - (uint8_t *)spkt->data
+                      + (stcp->th_off * 4) - spkt->vnet_hdr_len;
+
+        /*
+         * When network is busy, some tcp options(like sack) will unpredictable
+         * occur in primary side or secondary side. it will make packet size
+         * not same, but the two packet's payload is identical. colo just
+         * care about packet payload, so we skip the option field.
+         */
+        res = colo_packet_compare_common(ppkt, spkt, ptcp_offset, stcp_offset);
     } else if (ptcp->th_sum == stcp->th_sum) {
-        res = colo_packet_compare_common(ppkt, spkt, ETH_HLEN);
+        res = colo_packet_compare_common(ppkt, spkt, ETH_HLEN, ETH_HLEN);
     } else {
         res = -1;
     }
@@ -329,6 +342,7 @@ static int colo_packet_compare_udp(Packet *spkt, Packet *ppkt)
      * the ip payload here.
      */
     ret = colo_packet_compare_common(ppkt, spkt,
+                                     network_header_length + ETH_HLEN,
                                      network_header_length + ETH_HLEN);
 
     if (ret) {
@@ -366,6 +380,7 @@ static int colo_packet_compare_icmp(Packet *spkt, Packet *ppkt)
      * the ip payload here.
      */
     if (colo_packet_compare_common(ppkt, spkt,
+                                   network_header_length + ETH_HLEN,
                                    network_header_length + ETH_HLEN)) {
         trace_colo_compare_icmp_miscompare("primary pkt size",
                                            ppkt->size);
@@ -403,7 +418,7 @@ static int colo_packet_compare_other(Packet *spkt, Packet *ppkt)
                                    sec_ip_src, sec_ip_dst);
     }
 
-    return colo_packet_compare_common(ppkt, spkt, 0);
+    return colo_packet_compare_common(ppkt, spkt, 0, 0);
 }
 
 static int colo_old_packet_check_one(Packet *pkt, int64_t *check_time)
-- 
2.7.4

[Qemu-devel] [PATCH V4 2/3] net/colo-compare.c: Adjust net queue pop order for performance

[Zhang Chen]

The packet_enqueue() use g_queue_push_tail() to
enqueue net packet, so it is more efficent way use
g_queue_pop_head() to get packet for compare.
That will improve the success rate of comparison.
In my test the performance of ftp put 1000M file
will increase 10%

Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
---
 net/colo-compare.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index f6bda41..a8ccac7 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -483,7 +483,7 @@ static void colo_compare_connection(void *opaque, void *user_data)
 
     while (!g_queue_is_empty(&conn->primary_list) &&
            !g_queue_is_empty(&conn->secondary_list)) {
-        pkt = g_queue_pop_tail(&conn->primary_list);
+        pkt = g_queue_pop_head(&conn->primary_list);
         switch (conn->ip_proto) {
         case IPPROTO_TCP:
             result = g_queue_find_custom(&conn->secondary_list,
@@ -521,7 +521,7 @@ static void colo_compare_connection(void *opaque, void *user_data)
              * until next comparison.
              */
             trace_colo_compare_main("packet different");
-            g_queue_push_tail(&conn->primary_list, pkt);
+            g_queue_push_head(&conn->primary_list, pkt);
             /* TODO: colo_notify_checkpoint();*/
             break;
         }
-- 
2.7.4

[Qemu-devel] [PATCH V4 3/3] net/colo-compare.c: Fix comments and scheme

[Zhang Chen]

Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
---
 net/colo-compare.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index a8ccac7..e8af8bb 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -44,7 +44,7 @@
   + CompareState ++
   |               |
   +---------------+   +---------------+         +---------------+
-  |conn list      +--->conn           +--------->conn           |
+  |   conn list   + - >      conn     + ------- >      conn     + -- > .......
   +---------------+   +---------------+         +---------------+
   |               |     |           |             |          |
   +---------------+ +---v----+  +---v----+    +---v----+ +---v----+
@@ -75,14 +75,14 @@ typedef struct CompareState {
     SocketReadState sec_rs;
     bool vnet_hdr;
 
-    /* connection list: the connections belonged to this NIC could be found
-     * in this list.
-     * element type: Connection
+    /*
+     * Record the connection that through the NIC
+     * Element type: Connection
      */
     GQueue conn_list;
-    /* hashtable to save connection */
+    /* Record the connection without repetition */
     GHashTable *connection_track_table;
-    /* compare thread, a thread for each NIC */
+    /* This thread just do packet compare job */
     QemuThread thread;
 
     GMainContext *worker_context;
@@ -444,8 +444,11 @@ static int colo_old_packet_check_one_conn(Connection *conn,
                                  (GCompareFunc)colo_old_packet_check_one);
 
     if (result) {
-        /* do checkpoint will flush old packet */
-        /* TODO: colo_notify_checkpoint();*/
+        /* Do checkpoint will flush old packet */
+        /*
+         * TODO: Notify colo frame to do checkpoint.
+         * colo_compare_inconsistent_notify();
+         */
         return 0;
     }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH V4 0/3] Optimize COLO-compare performance

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1503305719-2512-1-git-send-email-zhangchen.fnst@cn.fujitsu.com
Subject: [Qemu-devel] [PATCH V4 0/3] Optimize COLO-compare performance

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
37052358b5 net/colo-compare.c: Fix comments and scheme
46824a6565 net/colo-compare.c: Adjust net queue pop order for performance
acea048383 net/colo-compare.c: Optimize unpredictable tcp options comparison

=== OUTPUT BEGIN ===
Checking PATCH 1/3: net/colo-compare.c: Optimize unpredictable tcp options comparison...
Checking PATCH 2/3: net/colo-compare.c: Adjust net queue pop order for performance...
Checking PATCH 3/3: net/colo-compare.c: Fix comments and scheme...
ERROR: space prohibited after that '-' (ctx:WxW)
#18: FILE: net/colo-compare.c:47:
+  |   conn list   + - >      conn     + ------- >      conn     + -- > .......
                     ^

ERROR: space prohibited after that '-' (ctx:OxW)
#18: FILE: net/colo-compare.c:47:
+  |   conn list   + - >      conn     + ------- >      conn     + -- > .......
                                               ^

ERROR: space required one side of that '--' (ctx:WxW)
#18: FILE: net/colo-compare.c:47:
+  |   conn list   + - >      conn     + ------- >      conn     + -- > .......
                                                                   ^

total: 3 errors, 0 warnings, 40 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH V4 0/3] Optimize COLO-compare performance

[Zhang Chen]

Hi~ Jason.

Have any comments for this series?

Thanks

Zhang Chen


On 08/21/2017 04:55 PM, Zhang Chen wrote:
> In this serise, we do a lot of job to optimize COLO net performance.
> Mainly focus on TCP protocol.
>
> V4:
>   - Remove the old patch1.
>
> V3:
>   - Rebase on upstream.
>   - Remove origin p2.
>   - Move the "checkpoint_time_ms" to CompareState,
>     in order to aviod multi colo-compare instance conflict.
>   - Add "TODO comments" for reset s->checkpoint_time_ms.
>   - Add a new patch fix comments and scheme.
>
> V2:
>   - Rename p2's subject.
>
>
> Zhang Chen (3):
>    net/colo-compare.c: Optimize unpredictable tcp options comparison
>    net/colo-compare.c: Adjust net queue pop order for performance
>    net/colo-compare.c: Fix comments and scheme
>
>   net/colo-compare.c | 62 +++++++++++++++++++++++++++++++++++-------------------
>   1 file changed, 40 insertions(+), 22 deletions(-)
>

-- 
Thanks
Zhang Chen

Re: [Qemu-devel] [PATCH V4 0/3] Optimize COLO-compare performance

[Jason Wang]

On 2017年08月22日 15:16, no-reply@patchew.org wrote:
> Hi,
>
> This series seems to have some coding style problems. See output below for
> more information:
>
> Type: series
> Message-id: 1503305719-2512-1-git-send-email-zhangchen.fnst@cn.fujitsu.com
> Subject: [Qemu-devel] [PATCH V4 0/3] Optimize COLO-compare performance
>
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
>
> BASE=base
> n=1
> total=$(git log --oneline $BASE.. | wc -l)
> failed=0
>
> git config --local diff.renamelimit 0
> git config --local diff.renames True
>
> commits="$(git log --format=%H --reverse $BASE..)"
> for c in $commits; do
>      echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
>      if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
>          failed=1
>          echo
>      fi
>      n=$((n+1))
> done
>
> exit $failed
> === TEST SCRIPT END ===
>
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> Switched to a new branch 'test'
> 37052358b5 net/colo-compare.c: Fix comments and scheme
> 46824a6565 net/colo-compare.c: Adjust net queue pop order for performance
> acea048383 net/colo-compare.c: Optimize unpredictable tcp options comparison
>
> === OUTPUT BEGIN ===
> Checking PATCH 1/3: net/colo-compare.c: Optimize unpredictable tcp options comparison...
> Checking PATCH 2/3: net/colo-compare.c: Adjust net queue pop order for performance...
> Checking PATCH 3/3: net/colo-compare.c: Fix comments and scheme...
> ERROR: space prohibited after that '-' (ctx:WxW)
> #18: FILE: net/colo-compare.c:47:
> +  |   conn list   + - >      conn     + ------- >      conn     + -- > .......
>                       ^
>
> ERROR: space prohibited after that '-' (ctx:OxW)
> #18: FILE: net/colo-compare.c:47:
> +  |   conn list   + - >      conn     + ------- >      conn     + -- > .......
>                                                 ^
>
> ERROR: space required one side of that '--' (ctx:WxW)
> #18: FILE: net/colo-compare.c:47:
> +  |   conn list   + - >      conn     + ------- >      conn     + -- > .......
>                                                                     ^
>
> total: 3 errors, 0 warnings, 40 lines checked
>
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> === OUTPUT END ===
>
> Test command exited with code: 1
>
>
> ---
> Email generated automatically by Patchew [http://patchew.org/].
> Please send your feedback to patchew-devel@freelists.org

Fam, this looks like a false positive since it was actually an ascii 
graph inside a comment?

Thanks

Re: [Qemu-devel] [PATCH V4 0/3] Optimize COLO-compare performance

[Fam Zheng]

On Tue, 08/29 17:01, Jason Wang wrote:
> 
> 
> On 2017年08月22日 15:16, no-reply@patchew.org wrote:
> > Hi,
> > 
> > This series seems to have some coding style problems. See output below for
> > more information:
> > 
> > Type: series
> > Message-id: 1503305719-2512-1-git-send-email-zhangchen.fnst@cn.fujitsu.com
> > Subject: [Qemu-devel] [PATCH V4 0/3] Optimize COLO-compare performance
> > 
> > === TEST SCRIPT BEGIN ===
> > #!/bin/bash
> > 
> > BASE=base
> > n=1
> > total=$(git log --oneline $BASE.. | wc -l)
> > failed=0
> > 
> > git config --local diff.renamelimit 0
> > git config --local diff.renames True
> > 
> > commits="$(git log --format=%H --reverse $BASE..)"
> > for c in $commits; do
> >      echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
> >      if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
> >          failed=1
> >          echo
> >      fi
> >      n=$((n+1))
> > done
> > 
> > exit $failed
> > === TEST SCRIPT END ===
> > 
> > Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> > Switched to a new branch 'test'
> > 37052358b5 net/colo-compare.c: Fix comments and scheme
> > 46824a6565 net/colo-compare.c: Adjust net queue pop order for performance
> > acea048383 net/colo-compare.c: Optimize unpredictable tcp options comparison
> > 
> > === OUTPUT BEGIN ===
> > Checking PATCH 1/3: net/colo-compare.c: Optimize unpredictable tcp options comparison...
> > Checking PATCH 2/3: net/colo-compare.c: Adjust net queue pop order for performance...
> > Checking PATCH 3/3: net/colo-compare.c: Fix comments and scheme...
> > ERROR: space prohibited after that '-' (ctx:WxW)
> > #18: FILE: net/colo-compare.c:47:
> > +  |   conn list   + - >      conn     + ------- >      conn     + -- > .......
> >                       ^
> > 
> > ERROR: space prohibited after that '-' (ctx:OxW)
> > #18: FILE: net/colo-compare.c:47:
> > +  |   conn list   + - >      conn     + ------- >      conn     + -- > .......
> >                                                 ^
> > 
> > ERROR: space required one side of that '--' (ctx:WxW)
> > #18: FILE: net/colo-compare.c:47:
> > +  |   conn list   + - >      conn     + ------- >      conn     + -- > .......
> >                                                                     ^
> > 
> > total: 3 errors, 0 warnings, 40 lines checked
> > 
> > Your patch has style problems, please review.  If any of these errors
> > are false positives report them to the maintainer, see
> > CHECKPATCH in MAINTAINERS.
> > 
> > === OUTPUT END ===
> > 
> > Test command exited with code: 1
> > 
> > 
> > ---
> > Email generated automatically by Patchew [http://patchew.org/].
> > Please send your feedback to patchew-devel@freelists.org
> 
> Fam, this looks like a false positive since it was actually an ascii graph
> inside a comment?
> 

Yes, let's just ignore this report.

Fam

Re: [Qemu-devel] [PATCH V4 0/3] Optimize COLO-compare performance

[Jason Wang]

On 2017年08月29日 18:45, Fam Zheng wrote:
> On Tue, 08/29 17:01, Jason Wang wrote:
>>
>> On 2017年08月22日 15:16, no-reply@patchew.org wrote:
>>> Hi,
>>>
>>> This series seems to have some coding style problems. See output below for
>>> more information:
>>>
>>> Type: series
>>> Message-id: 1503305719-2512-1-git-send-email-zhangchen.fnst@cn.fujitsu.com
>>> Subject: [Qemu-devel] [PATCH V4 0/3] Optimize COLO-compare performance
>>>
>>> === TEST SCRIPT BEGIN ===
>>> #!/bin/bash
>>>
>>> BASE=base
>>> n=1
>>> total=$(git log --oneline $BASE.. | wc -l)
>>> failed=0
>>>
>>> git config --local diff.renamelimit 0
>>> git config --local diff.renames True
>>>
>>> commits="$(git log --format=%H --reverse $BASE..)"
>>> for c in $commits; do
>>>       echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
>>>       if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
>>>           failed=1
>>>           echo
>>>       fi
>>>       n=$((n+1))
>>> done
>>>
>>> exit $failed
>>> === TEST SCRIPT END ===
>>>
>>> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
>>> Switched to a new branch 'test'
>>> 37052358b5 net/colo-compare.c: Fix comments and scheme
>>> 46824a6565 net/colo-compare.c: Adjust net queue pop order for performance
>>> acea048383 net/colo-compare.c: Optimize unpredictable tcp options comparison
>>>
>>> === OUTPUT BEGIN ===
>>> Checking PATCH 1/3: net/colo-compare.c: Optimize unpredictable tcp options comparison...
>>> Checking PATCH 2/3: net/colo-compare.c: Adjust net queue pop order for performance...
>>> Checking PATCH 3/3: net/colo-compare.c: Fix comments and scheme...
>>> ERROR: space prohibited after that '-' (ctx:WxW)
>>> #18: FILE: net/colo-compare.c:47:
>>> +  |   conn list   + - >      conn     + ------- >      conn     + -- > .......
>>>                        ^
>>>
>>> ERROR: space prohibited after that '-' (ctx:OxW)
>>> #18: FILE: net/colo-compare.c:47:
>>> +  |   conn list   + - >      conn     + ------- >      conn     + -- > .......
>>>                                                  ^
>>>
>>> ERROR: space required one side of that '--' (ctx:WxW)
>>> #18: FILE: net/colo-compare.c:47:
>>> +  |   conn list   + - >      conn     + ------- >      conn     + -- > .......
>>>                                                                      ^
>>>
>>> total: 3 errors, 0 warnings, 40 lines checked
>>>
>>> Your patch has style problems, please review.  If any of these errors
>>> are false positives report them to the maintainer, see
>>> CHECKPATCH in MAINTAINERS.
>>>
>>> === OUTPUT END ===
>>>
>>> Test command exited with code: 1
>>>
>>>
>>> ---
>>> Email generated automatically by Patchew [http://patchew.org/].
>>> Please send your feedback to patchew-devel@freelists.org
>> Fam, this looks like a false positive since it was actually an ascii graph
>> inside a comment?
>>
> Yes, let's just ignore this report.
>
> Fam

Thanks.

I've queued this series.

Re: [Qemu-devel] [PATCH V4 1/3] net/colo-compare.c: Optimize unpredictable tcp options comparison

[Dou Liyang]

Hi chen,

At 08/21/2017 04:55 PM, Zhang Chen wrote:
> When network is busy, some tcp options(like sack) will unpredictable
> occur in primary side or secondary side. it will make packet size
> not same, but the two packet's payload is identical. colo just
> care about packet payload, so we skip the option field.
>
> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
> ---
>  net/colo-compare.c | 39 +++++++++++++++++++++++++++------------
>  1 file changed, 27 insertions(+), 12 deletions(-)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index ca67c68..f6bda41 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -186,7 +186,10 @@ static int packet_enqueue(CompareState *s, int mode)
>   * return:    0  means packet same
>   *            > 0 || < 0 means packet different
>   */
> -static int colo_packet_compare_common(Packet *ppkt, Packet *spkt, int offset)
> +static int colo_packet_compare_common(Packet *ppkt,
> +                                      Packet *spkt,
> +                                      int poffset,
> +                                      int soffset)
>  {
>      if (trace_event_get_state(TRACE_COLO_COMPARE_MISCOMPARE)) {
>          char pri_ip_src[20], pri_ip_dst[20], sec_ip_src[20], sec_ip_dst[20];
> @@ -201,12 +204,13 @@ static int colo_packet_compare_common(Packet *ppkt, Packet *spkt, int offset)
>                                     sec_ip_src, sec_ip_dst);
>      }
>
> -    offset = ppkt->vnet_hdr_len + offset;
> +    poffset = ppkt->vnet_hdr_len + poffset;
> +    soffset = ppkt->vnet_hdr_len + soffset;
>
> -    if (ppkt->size == spkt->size) {
> -        return memcmp(ppkt->data + offset,
> -                      spkt->data + offset,
> -                      spkt->size - offset);
> +    if (ppkt->size == spkt->size || poffset != soffset) {
> +        return memcmp(ppkt->data + poffset,
> +                      spkt->data + soffset,
> +                      spkt->size - soffset);

Here maybe a mistake,

I guess you should make sure

(ppkt->size - poffset) == (spkt->size - soffset) is true in case of
(poffset != soffset) at the same time.

Because, if sack make the header not equal, it is also possible that
the data is not equal at the same time.

>      } else {
>          trace_colo_compare_main("Net packet size are not the same");
>          return -1;
> @@ -263,13 +267,22 @@ static int colo_packet_compare_tcp(Packet *spkt, Packet *ppkt)
>       * so we just need skip this field.
>       */
>      if (ptcp->th_off > 5) {
> -        ptrdiff_t tcp_offset;
> +        ptrdiff_t ptcp_offset, stcp_offset;
>
> -        tcp_offset = ppkt->transport_header - (uint8_t *)ppkt->data
> -                     + (ptcp->th_off * 4) - ppkt->vnet_hdr_len;
> -        res = colo_packet_compare_common(ppkt, spkt, tcp_offset);
> +        ptcp_offset = ppkt->transport_header - (uint8_t *)ppkt->data
> +                      + (ptcp->th_off * 4) - ppkt->vnet_hdr_len;
> +        stcp_offset = spkt->transport_header - (uint8_t *)spkt->data
> +                      + (stcp->th_off * 4) - spkt->vnet_hdr_len;
> +
> +        /*
> +         * When network is busy, some tcp options(like sack) will unpredictable
> +         * occur in primary side or secondary side. it will make packet size
> +         * not same, but the two packet's payload is identical. colo just
> +         * care about packet payload, so we skip the option field.
> +         */
> +        res = colo_packet_compare_common(ppkt, spkt, ptcp_offset, stcp_offset);

we add a new parameter in xxx_common() just for xxx_tcp().
In my opinion, it seems not good.

Can we split the tcp related code out from xxx_common, and make the 
xxx_common function just do like what its name says.


>      } else if (ptcp->th_sum == stcp->th_sum) {
> -        res = colo_packet_compare_common(ppkt, spkt, ETH_HLEN);
> +        res = colo_packet_compare_common(ppkt, spkt, ETH_HLEN, ETH_HLEN);
>      } else {
>          res = -1;
>      }
> @@ -329,6 +342,7 @@ static int colo_packet_compare_udp(Packet *spkt, Packet *ppkt)
>       * the ip payload here.
>       */
>      ret = colo_packet_compare_common(ppkt, spkt,
> +                                     network_header_length + ETH_HLEN,
>                                       network_header_length + ETH_HLEN);

ditto

>
>      if (ret) {
> @@ -366,6 +380,7 @@ static int colo_packet_compare_icmp(Packet *spkt, Packet *ppkt)
>       * the ip payload here.
>       */
>      if (colo_packet_compare_common(ppkt, spkt,
> +                                   network_header_length + ETH_HLEN,
>                                     network_header_length + ETH_HLEN)) {

ditto

>          trace_colo_compare_icmp_miscompare("primary pkt size",
>                                             ppkt->size);
> @@ -403,7 +418,7 @@ static int colo_packet_compare_other(Packet *spkt, Packet *ppkt)
>                                     sec_ip_src, sec_ip_dst);
>      }
>
> -    return colo_packet_compare_common(ppkt, spkt, 0);
> +    return colo_packet_compare_common(ppkt, spkt, 0, 0);

ditto

Thanks,
	dou.

>  }
>
>  static int colo_old_packet_check_one(Packet *pkt, int64_t *check_time)
>

Re: [Qemu-devel] [PATCH V4 1/3] net/colo-compare.c: Optimize unpredictable tcp options comparison

[Zhang Chen]

On 09/01/2017 05:35 PM, Dou Liyang wrote:
> Hi chen,
>
> At 08/21/2017 04:55 PM, Zhang Chen wrote:
>> When network is busy, some tcp options(like sack) will unpredictable
>> occur in primary side or secondary side. it will make packet size
>> not same, but the two packet's payload is identical. colo just
>> care about packet payload, so we skip the option field.
>>
>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>> ---
>>  net/colo-compare.c | 39 +++++++++++++++++++++++++++------------
>>  1 file changed, 27 insertions(+), 12 deletions(-)
>>
>> diff --git a/net/colo-compare.c b/net/colo-compare.c
>> index ca67c68..f6bda41 100644
>> --- a/net/colo-compare.c
>> +++ b/net/colo-compare.c
>> @@ -186,7 +186,10 @@ static int packet_enqueue(CompareState *s, int 
>> mode)
>>   * return:    0  means packet same
>>   *            > 0 || < 0 means packet different
>>   */
>> -static int colo_packet_compare_common(Packet *ppkt, Packet *spkt, 
>> int offset)
>> +static int colo_packet_compare_common(Packet *ppkt,
>> +                                      Packet *spkt,
>> +                                      int poffset,
>> +                                      int soffset)
>>  {
>>      if (trace_event_get_state(TRACE_COLO_COMPARE_MISCOMPARE)) {
>>          char pri_ip_src[20], pri_ip_dst[20], sec_ip_src[20], 
>> sec_ip_dst[20];
>> @@ -201,12 +204,13 @@ static int colo_packet_compare_common(Packet 
>> *ppkt, Packet *spkt, int offset)
>>                                     sec_ip_src, sec_ip_dst);
>>      }
>>
>> -    offset = ppkt->vnet_hdr_len + offset;
>> +    poffset = ppkt->vnet_hdr_len + poffset;
>> +    soffset = ppkt->vnet_hdr_len + soffset;
>>
>> -    if (ppkt->size == spkt->size) {
>> -        return memcmp(ppkt->data + offset,
>> -                      spkt->data + offset,
>> -                      spkt->size - offset);
>> +    if (ppkt->size == spkt->size || poffset != soffset) {
>> +        return memcmp(ppkt->data + poffset,
>> +                      spkt->data + soffset,
>> +                      spkt->size - soffset);
>
> Here maybe a mistake,
>
> I guess you should make sure
>
> (ppkt->size - poffset) == (spkt->size - soffset) is true in case of
> (poffset != soffset) at the same time.
>
> Because, if sack make the header not equal, it is also possible that
> the data is not equal at the same time.

Good catch! In rare situation will miss a different packet, I will fix 
it in next version.

>
>>      } else {
>>          trace_colo_compare_main("Net packet size are not the same");
>>          return -1;
>> @@ -263,13 +267,22 @@ static int colo_packet_compare_tcp(Packet 
>> *spkt, Packet *ppkt)
>>       * so we just need skip this field.
>>       */
>>      if (ptcp->th_off > 5) {
>> -        ptrdiff_t tcp_offset;
>> +        ptrdiff_t ptcp_offset, stcp_offset;
>>
>> -        tcp_offset = ppkt->transport_header - (uint8_t *)ppkt->data
>> -                     + (ptcp->th_off * 4) - ppkt->vnet_hdr_len;
>> -        res = colo_packet_compare_common(ppkt, spkt, tcp_offset);
>> +        ptcp_offset = ppkt->transport_header - (uint8_t *)ppkt->data
>> +                      + (ptcp->th_off * 4) - ppkt->vnet_hdr_len;
>> +        stcp_offset = spkt->transport_header - (uint8_t *)spkt->data
>> +                      + (stcp->th_off * 4) - spkt->vnet_hdr_len;
>> +
>> +        /*
>> +         * When network is busy, some tcp options(like sack) will 
>> unpredictable
>> +         * occur in primary side or secondary side. it will make 
>> packet size
>> +         * not same, but the two packet's payload is identical. colo 
>> just
>> +         * care about packet payload, so we skip the option field.
>> +         */
>> +        res = colo_packet_compare_common(ppkt, spkt, ptcp_offset, 
>> stcp_offset);
>
> we add a new parameter in xxx_common() just for xxx_tcp().
> In my opinion, it seems not good.
>
> Can we split the tcp related code out from xxx_common, and make the 
> xxx_common function just do like what its name says.

We introduce this parameter for all protocol, just tcp is the first user.
We should have the ability of compare all kinds of packet.
Maybe we must use different offset for other protocol except tcp in the 
future.

Thanks
Zhang Chen

>
>
>>      } else if (ptcp->th_sum == stcp->th_sum) {
>> -        res = colo_packet_compare_common(ppkt, spkt, ETH_HLEN);
>> +        res = colo_packet_compare_common(ppkt, spkt, ETH_HLEN, 
>> ETH_HLEN);
>>      } else {
>>          res = -1;
>>      }
>> @@ -329,6 +342,7 @@ static int colo_packet_compare_udp(Packet *spkt, 
>> Packet *ppkt)
>>       * the ip payload here.
>>       */
>>      ret = colo_packet_compare_common(ppkt, spkt,
>> +                                     network_header_length + ETH_HLEN,
>>                                       network_header_length + ETH_HLEN);
>
> ditto
>
>>
>>      if (ret) {
>> @@ -366,6 +380,7 @@ static int colo_packet_compare_icmp(Packet *spkt, 
>> Packet *ppkt)
>>       * the ip payload here.
>>       */
>>      if (colo_packet_compare_common(ppkt, spkt,
>> +                                   network_header_length + ETH_HLEN,
>>                                     network_header_length + ETH_HLEN)) {
>
> ditto
>
>> trace_colo_compare_icmp_miscompare("primary pkt size",
>>                                             ppkt->size);
>> @@ -403,7 +418,7 @@ static int colo_packet_compare_other(Packet 
>> *spkt, Packet *ppkt)
>>                                     sec_ip_src, sec_ip_dst);
>>      }
>>
>> -    return colo_packet_compare_common(ppkt, spkt, 0);
>> +    return colo_packet_compare_common(ppkt, spkt, 0, 0);
>
> ditto
>
> Thanks,
>     dou.
>
>>  }
>>
>>  static int colo_old_packet_check_one(Packet *pkt, int64_t *check_time)
>>
>
>
>

-- 
Thanks
Zhang Chen

Re: [Qemu-devel] [PATCH V4 1/3] net/colo-compare.c: Optimize unpredictable tcp options comparison

[Dou Liyang]

Hi Chen,

At 09/02/2017 12:02 AM, Zhang Chen wrote:
>
>
> On 09/01/2017 05:35 PM, Dou Liyang wrote:
>> Hi chen,
>>
>> At 08/21/2017 04:55 PM, Zhang Chen wrote:
>>> When network is busy, some tcp options(like sack) will unpredictable
>>> occur in primary side or secondary side. it will make packet size
>>> not same, but the two packet's payload is identical. colo just
>>> care about packet payload, so we skip the option field.
>>>
>>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>>> ---
>>>  net/colo-compare.c | 39 +++++++++++++++++++++++++++------------
>>>  1 file changed, 27 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/net/colo-compare.c b/net/colo-compare.c
>>> index ca67c68..f6bda41 100644
>>> --- a/net/colo-compare.c
>>> +++ b/net/colo-compare.c
>>> @@ -186,7 +186,10 @@ static int packet_enqueue(CompareState *s, int
>>> mode)
>>>   * return:    0  means packet same
>>>   *            > 0 || < 0 means packet different
>>>   */
>>> -static int colo_packet_compare_common(Packet *ppkt, Packet *spkt,
>>> int offset)
>>> +static int colo_packet_compare_common(Packet *ppkt,
>>> +                                      Packet *spkt,
>>> +                                      int poffset,
>>> +                                      int soffset)
>>>  {
>>>      if (trace_event_get_state(TRACE_COLO_COMPARE_MISCOMPARE)) {
>>>          char pri_ip_src[20], pri_ip_dst[20], sec_ip_src[20],
>>> sec_ip_dst[20];
>>> @@ -201,12 +204,13 @@ static int colo_packet_compare_common(Packet
>>> *ppkt, Packet *spkt, int offset)
>>>                                     sec_ip_src, sec_ip_dst);
>>>      }
>>>
>>> -    offset = ppkt->vnet_hdr_len + offset;
>>> +    poffset = ppkt->vnet_hdr_len + poffset;
>>> +    soffset = ppkt->vnet_hdr_len + soffset;
>>>
>>> -    if (ppkt->size == spkt->size) {
>>> -        return memcmp(ppkt->data + offset,
>>> -                      spkt->data + offset,
>>> -                      spkt->size - offset);
>>> +    if (ppkt->size == spkt->size || poffset != soffset) {
>>> +        return memcmp(ppkt->data + poffset,
>>> +                      spkt->data + soffset,
>>> +                      spkt->size - soffset);
>>
>> Here maybe a mistake,
>>
>> I guess you should make sure
>>
>> (ppkt->size - poffset) == (spkt->size - soffset) is true in case of
>> (poffset != soffset) at the same time.
>>
>> Because, if sack make the header not equal, it is also possible that
>> the data is not equal at the same time.
>
> Good catch! In rare situation will miss a different packet, I will fix
> it in next version.
>

Thank you very much!

>>
>>>      } else {
>>>          trace_colo_compare_main("Net packet size are not the same");
>>>          return -1;
>>> @@ -263,13 +267,22 @@ static int colo_packet_compare_tcp(Packet
>>> *spkt, Packet *ppkt)
>>>       * so we just need skip this field.
>>>       */
>>>      if (ptcp->th_off > 5) {
>>> -        ptrdiff_t tcp_offset;
>>> +        ptrdiff_t ptcp_offset, stcp_offset;
>>>
>>> -        tcp_offset = ppkt->transport_header - (uint8_t *)ppkt->data
>>> -                     + (ptcp->th_off * 4) - ppkt->vnet_hdr_len;
>>> -        res = colo_packet_compare_common(ppkt, spkt, tcp_offset);
>>> +        ptcp_offset = ppkt->transport_header - (uint8_t *)ppkt->data
>>> +                      + (ptcp->th_off * 4) - ppkt->vnet_hdr_len;
>>> +        stcp_offset = spkt->transport_header - (uint8_t *)spkt->data
>>> +                      + (stcp->th_off * 4) - spkt->vnet_hdr_len;
>>> +
>>> +        /*
>>> +         * When network is busy, some tcp options(like sack) will
>>> unpredictable
>>> +         * occur in primary side or secondary side. it will make
>>> packet size
>>> +         * not same, but the two packet's payload is identical. colo
>>> just
>>> +         * care about packet payload, so we skip the option field.
>>> +         */
>>> +        res = colo_packet_compare_common(ppkt, spkt, ptcp_offset,
>>> stcp_offset);
>>
>> we add a new parameter in xxx_common() just for xxx_tcp().
>> In my opinion, it seems not good.
>>
>> Can we split the tcp related code out from xxx_common, and make the
>> xxx_common function just do like what its name says.
>
> We introduce this parameter for all protocol, just tcp is the first user.
> We should have the ability of compare all kinds of packet.
> Maybe we must use different offset for other protocol except tcp in the
> future.
>

Got it,

Thanks,
	dou.

> Thanks
> Zhang Chen
>
>>
>>
>>>      } else if (ptcp->th_sum == stcp->th_sum) {
>>> -        res = colo_packet_compare_common(ppkt, spkt, ETH_HLEN);
>>> +        res = colo_packet_compare_common(ppkt, spkt, ETH_HLEN,
>>> ETH_HLEN);
>>>      } else {
>>>          res = -1;
>>>      }
>>> @@ -329,6 +342,7 @@ static int colo_packet_compare_udp(Packet *spkt,
>>> Packet *ppkt)
>>>       * the ip payload here.
>>>       */
>>>      ret = colo_packet_compare_common(ppkt, spkt,
>>> +                                     network_header_length + ETH_HLEN,
>>>                                       network_header_length + ETH_HLEN);
>>
>> ditto
>>
>>>
>>>      if (ret) {
>>> @@ -366,6 +380,7 @@ static int colo_packet_compare_icmp(Packet *spkt,
>>> Packet *ppkt)
>>>       * the ip payload here.
>>>       */
>>>      if (colo_packet_compare_common(ppkt, spkt,
>>> +                                   network_header_length + ETH_HLEN,
>>>                                     network_header_length + ETH_HLEN)) {
>>
>> ditto
>>
>>> trace_colo_compare_icmp_miscompare("primary pkt size",
>>>                                             ppkt->size);
>>> @@ -403,7 +418,7 @@ static int colo_packet_compare_other(Packet
>>> *spkt, Packet *ppkt)
>>>                                     sec_ip_src, sec_ip_dst);
>>>      }
>>>
>>> -    return colo_packet_compare_common(ppkt, spkt, 0);
>>> +    return colo_packet_compare_common(ppkt, spkt, 0, 0);
>>
>> ditto
>>
>> Thanks,
>>     dou.
>>
>>>  }
>>>
>>>  static int colo_old_packet_check_one(Packet *pkt, int64_t *check_time)
>>>
>>
>>
>>
>