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

[Qemu-devel] [PATCH V5 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.

V5:
 - Fix bug in colo_packet_compare_common().
 - Fix patch3 ascii graph style.

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 | 103 +++++++++++++++++++++++++++++++----------------------
 1 file changed, 61 insertions(+), 42 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH V5 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 | 40 ++++++++++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 12 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index ca67c68..18a9ebf 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,14 @@ 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 ||
+        ppkt->size - poffset == spkt->size - 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 +268,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 +343,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 +381,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 +419,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 V5 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 18a9ebf..8dd76e4 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -484,7 +484,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,
@@ -522,7 +522,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 V5 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 | 59 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 31 insertions(+), 28 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index 8dd76e4..c9a7174 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -41,27 +41,27 @@
 #define REGULAR_PACKET_CHECK_MS 3000
 
 /*
-  + CompareState ++
-  |               |
-  +---------------+   +---------------+         +---------------+
-  |conn list      +--->conn           +--------->conn           |
-  +---------------+   +---------------+         +---------------+
-  |               |     |           |             |          |
-  +---------------+ +---v----+  +---v----+    +---v----+ +---v----+
-                    |primary |  |secondary    |primary | |secondary
-                    |packet  |  |packet  +    |packet  | |packet  +
-                    +--------+  +--------+    +--------+ +--------+
-                        |           |             |          |
-                    +---v----+  +---v----+    +---v----+ +---v----+
-                    |primary |  |secondary    |primary | |secondary
-                    |packet  |  |packet  +    |packet  | |packet  +
-                    +--------+  +--------+    +--------+ +--------+
-                        |           |             |          |
-                    +---v----+  +---v----+    +---v----+ +---v----+
-                    |primary |  |secondary    |primary | |secondary
-                    |packet  |  |packet  +    |packet  | |packet  +
-                    +--------+  +--------+    +--------+ +--------+
-*/
+ *  + CompareState ++
+ *  |               |
+ *  +---------------+   +---------------+         +---------------+
+ *  |   conn list   + - >      conn     + ------- >      conn     + -- > ......
+ *  +---------------+   +---------------+         +---------------+
+ *  |               |     |           |             |          |
+ *  +---------------+ +---v----+  +---v----+    +---v----+ +---v----+
+ *                    |primary |  |secondary    |primary | |secondary
+ *                    |packet  |  |packet  +    |packet  | |packet  +
+ *                    +--------+  +--------+    +--------+ +--------+
+ *                        |           |             |          |
+ *                    +---v----+  +---v----+    +---v----+ +---v----+
+ *                    |primary |  |secondary    |primary | |secondary
+ *                    |packet  |  |packet  +    |packet  | |packet  +
+ *                    +--------+  +--------+    +--------+ +--------+
+ *                        |           |             |          |
+ *                    +---v----+  +---v----+    +---v----+ +---v----+
+ *                    |primary |  |secondary    |primary | |secondary
+ *                    |packet  |  |packet  +    |packet  | |packet  +
+ *                    +--------+  +--------+    +--------+ +--------+
+ */
 typedef struct CompareState {
     Object parent;
 
@@ -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;
@@ -445,8 +445,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 V5 1/3] net/colo-compare.c: Optimize unpredictable tcp options comparison

[Dou Liyang]

Hi Chen,

At 09/04/2017 02:14 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 | 40 ++++++++++++++++++++++++++++------------
>  1 file changed, 28 insertions(+), 12 deletions(-)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index ca67c68..18a9ebf 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,14 @@ 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 ||
> +        ppkt->size - poffset == spkt->size - soffset) {

This logic has a problem:

ppkt->data
    |-----------|---------rangeP---------|
                |                    \   |
            poffset                 ppkt->size
                  \    equal range     \
spkt->data        \                    \
    |---------------|------rangeS--------|
                    |                    |
                 soffset            spkt->size

See the above picture, (ppkt->size == spkt->size) is true,
if  [soffset, spkt->size] == [poffset, poffset+(spkt->size- soffset) is
also ture, the code will return 0, but actually, they are not equal.

Please use following code instead,

if (ppkt->size - poffset == spkt->size - soffset)

I am a new boy in COLO, let's see what we actually want to compare,
If I am wrong, please correct me. :-)

ppkt->data
    |-----------|---------rangeP---------|
                |                        |
            poffset                 ppkt->size

spkt->data
    |----|----------rangeS---------|
         |                         |
      soffset                 spkt->size

The data in rangeP and rangeS is what we want to compare.
So, we just need care about the rangeX's size and head pointer,
not the whole size.


Thanks,
	dou.

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

[Zhang Chen]

On 09/05/2017 10:11 AM, Dou Liyang wrote:
> Hi Chen,
>
> At 09/04/2017 02:14 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 | 40 ++++++++++++++++++++++++++++------------
>>  1 file changed, 28 insertions(+), 12 deletions(-)
>>
>> diff --git a/net/colo-compare.c b/net/colo-compare.c
>> index ca67c68..18a9ebf 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,14 @@ 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 ||
>> +        ppkt->size - poffset == spkt->size - soffset) {
>
> This logic has a problem:
>
> ppkt->data
>    |-----------|---------rangeP---------|
>                |                    \   |
>            poffset                 ppkt->size
>                  \    equal range     \
> spkt->data        \                    \
>    |---------------|------rangeS--------|
>                    |                    |
>                 soffset            spkt->size
>
> See the above picture, (ppkt->size == spkt->size) is true,
> if  [soffset, spkt->size] == [poffset, poffset+(spkt->size- soffset) is
> also ture, the code will return 0, but actually, they are not equal.
>
> Please use following code instead,
>
> if (ppkt->size - poffset == spkt->size - soffset)
>
> I am a new boy in COLO, let's see what we actually want to compare,
> If I am wrong, please correct me. :-)
>
> ppkt->data
>    |-----------|---------rangeP---------|
>                |                        |
>            poffset                 ppkt->size
>
> spkt->data
>    |----|----------rangeS---------|
>         |                         |
>      soffset                 spkt->size
>
> The data in rangeP and rangeS is what we want to compare.
> So, we just need care about the rangeX's size and head pointer,
> not the whole size.

Yes, I have already considered this rare situation, but for packet 
comparing efficiency
I ignored it. As you know, every packet will be compared in COLO FT, 
most of packet
have the same offset, in this time use pkt->size is ok. the other packet 
with different
offset always have a different pkt->size. But we need cover all 
situation firstly,
I will update the V6 later.

Thanks
Zhang Chen

>
>
> Thanks,
>     dou.
> .
>

-- 
Thanks
Zhang Chen