[Qemu-devel] [PATCH V3 0/4] Optimize COLO-compare performance

[Qemu-devel] [PATCH V3 0/4] 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.

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 (4):
  net/colo-compare.c: Add checkpoint min period to optimize performance
  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 | 83 +++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 60 insertions(+), 23 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH V3 1/4] net/colo-compare.c: Add checkpoint min period to optimize performance

[Zhang Chen]

If colo-compare find out the first different packet that means
the following packet almost is different. we needn't do a lot
of checkpoint in this time, so we set the no-need-checkpoint
peroid, default just set 3 second.

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

diff --git a/net/colo-compare.c b/net/colo-compare.c
index ca67c68..5313e74 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -40,6 +40,9 @@
 /* TODO: Should be configurable */
 #define REGULAR_PACKET_CHECK_MS 3000
 
+/* TODO: Should be configurable */
+#define CHECKPOINT_MIN_TIME 3000
+
 /*
   + CompareState ++
   |               |
@@ -75,6 +78,9 @@ typedef struct CompareState {
     SocketReadState sec_rs;
     bool vnet_hdr;
 
+    /* Record the last checkpoint time */
+    int64_t checkpoint_time_ms;
+
     /* connection list: the connections belonged to this NIC could be found
      * in this list.
      * element type: Connection
@@ -507,7 +513,19 @@ static void colo_compare_connection(void *opaque, void *user_data)
              */
             trace_colo_compare_main("packet different");
             g_queue_push_tail(&conn->primary_list, pkt);
-            /* TODO: colo_notify_checkpoint();*/
+
+            if (pkt->creation_ms - s->checkpoint_time_ms >
+                CHECKPOINT_MIN_TIME) {
+                /*
+                 * TODO: Notify colo frame to do checkpoint.
+                 * colo_compare_inconsistent_notify();
+                 *
+                 * TODO: Reset s->checkpoint_time_ms after finish
+                 * checkpoint(when colo-compare get notify from colo-frame,
+                 * in another independent patch).
+                 */
+                s->checkpoint_time_ms = pkt->creation_ms;
+            }
             break;
         }
     }
@@ -803,6 +821,7 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
                        colo_compare_thread, s,
                        QEMU_THREAD_JOINABLE);
     compare_id++;
+    s->checkpoint_time_ms = 0;
 
     return;
 }
-- 
2.7.4

[Qemu-devel] [PATCH V3 2/4] 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 5313e74..6aa5ea6 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -192,7 +192,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];
@@ -207,12 +210,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;
@@ -269,13 +273,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;
     }
@@ -335,6 +348,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) {
@@ -372,6 +386,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);
@@ -409,7 +424,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 V3 3/4] 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 6aa5ea6..58c3250 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -489,7 +489,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,
@@ -527,7 +527,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);
 
             if (pkt->creation_ms - s->checkpoint_time_ms >
                 CHECKPOINT_MIN_TIME) {
-- 
2.7.4

[Qemu-devel] [PATCH V3 4/4] 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 58c3250..d10abb2 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -47,7 +47,7 @@
   + CompareState ++
   |               |
   +---------------+   +---------------+         +---------------+
-  |conn list      +--->conn           +--------->conn           |
+  |conn list      +--->conn           +--------->conn           |---> .......
   +---------------+   +---------------+         +---------------+
   |               |     |           |             |          |
   +---------------+ +---v----+  +---v----+    +---v----+ +---v----+
@@ -81,14 +81,14 @@ typedef struct CompareState {
     /* Record the last checkpoint time */
     int64_t checkpoint_time_ms;
 
-    /* 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;
@@ -450,8 +450,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 V3 0/4] Optimize COLO-compare performance

[no-reply]

Hi,

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

Subject: [Qemu-devel] [PATCH V3 0/4] Optimize COLO-compare performance
Message-id: 1500449399-19107-1-git-send-email-zhangchen.fnst@cn.fujitsu.com
Type: series

=== 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
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/1500448182-21376-1-git-send-email-armbru@redhat.com -> patchew/1500448182-21376-1-git-send-email-armbru@redhat.com
 * [new tag]         patchew/20170719095033.20482-1-david@redhat.com -> patchew/20170719095033.20482-1-david@redhat.com
Switched to a new branch 'test'
fcb0d9f net/colo-compare.c: Fix comments and scheme
1b4531a net/colo-compare.c: Adjust net queue pop order for performance
8433e86 net/colo-compare.c: Optimize unpredictable tcp options comparison
384fa35 net/colo-compare.c: Add checkpoint min period to optimize performance

=== OUTPUT BEGIN ===
Checking PATCH 1/4: net/colo-compare.c: Add checkpoint min period to optimize performance...
Checking PATCH 2/4: net/colo-compare.c: Optimize unpredictable tcp options comparison...
Checking PATCH 3/4: net/colo-compare.c: Adjust net queue pop order for performance...
Checking PATCH 4/4: net/colo-compare.c: Fix comments and scheme...
ERROR: spaces required around that '|' (ctx:ExV)
#18: FILE: net/colo-compare.c:50:
+  |conn list      +--->conn           +--------->conn           |---> .......
   ^

ERROR: spaces required around that '+' (ctx:WxO)
#18: FILE: net/colo-compare.c:50:
+  |conn list      +--->conn           +--------->conn           |---> .......
                   ^

ERROR: spaces required around that '+' (ctx:WxO)
#18: FILE: net/colo-compare.c:50:
+  |conn list      +--->conn           +--------->conn           |---> .......
                                       ^

ERROR: spaces required around that '|' (ctx:WxO)
#18: FILE: net/colo-compare.c:50:
+  |conn list      +--->conn           +--------->conn           |---> .......
                                                                 ^

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

total: 5 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 V3 0/4] Optimize COLO-compare performance

[Zhang Chen]

Hi~~ All~

No one review this series for a long time.

Ping...


Thanks

Zhang Chen


On 07/19/2017 03:29 PM, Zhang Chen wrote:
> In this serise, we do a lot of job to optimize COLO net performance.
> Mainly focus on TCP protocol.
>
> 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 (4):
>    net/colo-compare.c: Add checkpoint min period to optimize performance
>    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 | 83 +++++++++++++++++++++++++++++++++++++++---------------
>   1 file changed, 60 insertions(+), 23 deletions(-)
>

-- 
Thanks
Zhang Chen

Re: [Qemu-devel] [PATCH V3 0/4] Optimize COLO-compare performance

[Jason Wang]

On 2017年08月21日 09:45, Zhang Chen wrote:
> Hi~~ All~
>
> No one review this series for a long time.
>
> Ping...
>
>

Looks ok to me. But I don't like patch 1 since it adds more TODO 
especially for a non-exist function colo_notify_checkpoint().

I tend to have it after colo_notify_checkpoint() is implemented or merged.

Thanks

Re: [Qemu-devel] [PATCH V3 0/4] Optimize COLO-compare performance

[Zhang Chen]

On 08/21/2017 11:55 AM, Jason Wang wrote:
>
>
> On 2017年08月21日 09:45, Zhang Chen wrote:
>> Hi~~ All~
>>
>> No one review this series for a long time.
>>
>> Ping...
>>
>>
>
> Looks ok to me. But I don't like patch 1 since it adds more TODO 
> especially for a non-exist function colo_notify_checkpoint().
>
> I tend to have it after colo_notify_checkpoint() is implemented or 
> merged.
>

I got your point, I will send the V4 without patch1.

Thanks
Zhang Chen

> Thanks
>
>
>
>

-- 
Thanks
Zhang Chen