[PATCH v4 0/2] COLO: handling of the full primary or secondary queue

[PATCH v4 0/2] COLO: handling of the full primary or secondary queue

[Derek Su]

The series is to handle the full primary or secondary queue in colo-compare.

(1) fix the "pkt" memory leak in packet_enqueue().
    Reproduce steps:
    1. Setup PVM and SVM both with NIC e1000 by the steps descripted
       in the wiki qemu/COLO
    2. Run "iperf3 -s" in PVM
    3. Run "iperf3 -c <PVM-IP> -t 7200" in client

    The memory usage of qemu-system-x86_64 increases as the PVM's QMP 
    shows "qemu-system-x86_64: colo compare secondary queue size too big,
    drop packet".

(2) The pervious handling of the full primary or queue is only dropping
    the packet. If there are lots of clients to the guest VM,
    the "drop" will lead to the lost of the networking connection
    until next checkpoint, and therefore decrease the response of service.

    This patch drops the packet firstly. Then, send all queued primary
    packets, remove all queued secondary packets (flush packets) and
    do checkpoint.

Please help to review, thanks.

V4:
 - Remove redundant flush of packets

V3:
 - handling of the full primary or secondary queue according to the
   suggestion from Zhang Chen
V2:
 - Fix incorrect patch format

Derek Su (2):
  net/colo-compare.c: Fix memory leak in packet_enqueue()
  net/colo-compare.c: handling of the full primary or secondary queue

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

-- 
2.17.1

[PATCH v4 1/2] net/colo-compare.c: Fix memory leak in packet_enqueue()

[Derek Su]

The patch is to fix the "pkt" memory leak in packet_enqueue().
The allocated "pkt" needs to be freed if the colo compare
primary or secondary queue is too big.

Signed-off-by: Derek Su <dereksu@qnap.com>
---
 net/colo-compare.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index 7ee17f2cf8..cdd87b2aa8 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -120,6 +120,10 @@ enum {
     SECONDARY_IN,
 };
 
+static const char *colo_mode[] = {
+    [PRIMARY_IN] = "primary",
+    [SECONDARY_IN] = "secondary",
+};
 
 static int compare_chr_send(CompareState *s,
                             const uint8_t *buf,
@@ -215,6 +219,7 @@ static int packet_enqueue(CompareState *s, int mode, Connection **con)
     ConnectionKey key;
     Packet *pkt = NULL;
     Connection *conn;
+    int ret;
 
     if (mode == PRIMARY_IN) {
         pkt = packet_new(s->pri_rs.buf,
@@ -243,16 +248,18 @@ static int packet_enqueue(CompareState *s, int mode, Connection **con)
     }
 
     if (mode == PRIMARY_IN) {
-        if (!colo_insert_packet(&conn->primary_list, pkt, &conn->pack)) {
-            error_report("colo compare primary queue size too big,"
-                         "drop packet");
-        }
+        ret = colo_insert_packet(&conn->primary_list, pkt, &conn->pack);
     } else {
-        if (!colo_insert_packet(&conn->secondary_list, pkt, &conn->sack)) {
-            error_report("colo compare secondary queue size too big,"
-                         "drop packet");
-        }
+        ret = colo_insert_packet(&conn->secondary_list, pkt, &conn->sack);
     }
+
+    if (!ret) {
+        error_report("colo compare %s queue size too big,"
+                     "drop packet", colo_mode[mode]);
+        packet_destroy(pkt, NULL);
+        pkt = NULL;
+    }
+
     *con = conn;
 
     return 0;
-- 
2.17.1

RE: [PATCH v4 1/2] net/colo-compare.c: Fix memory leak in packet_enqueue()

[Zhang, Chen]

> -----Original Message-----
> From: Derek Su <dereksu@qnap.com>
> Sent: Saturday, March 28, 2020 8:47 PM
> To: qemu-devel@nongnu.org
> Cc: Zhang, Chen <chen.zhang@intel.com>; lizhijian@cn.fujitsu.com;
> jasowang@redhat.com; ctcheng@qnap.com; chyang@qnap.com;
> jwsu1986@gmail.com; Derek Su <dereksu@qnap.com>
> Subject: [PATCH v4 1/2] net/colo-compare.c: Fix memory leak in
> packet_enqueue()
> 
> The patch is to fix the "pkt" memory leak in packet_enqueue().
> The allocated "pkt" needs to be freed if the colo compare primary or
> secondary queue is too big.
> 
> Signed-off-by: Derek Su <dereksu@qnap.com>

Reviewed-by: Zhang Chen <chen.zhang@intel.com>

> ---
>  net/colo-compare.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/net/colo-compare.c b/net/colo-compare.c index
> 7ee17f2cf8..cdd87b2aa8 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -120,6 +120,10 @@ enum {
>      SECONDARY_IN,
>  };
> 
> +static const char *colo_mode[] = {
> +    [PRIMARY_IN] = "primary",
> +    [SECONDARY_IN] = "secondary",
> +};
> 
>  static int compare_chr_send(CompareState *s,
>                              const uint8_t *buf, @@ -215,6 +219,7 @@ static int
> packet_enqueue(CompareState *s, int mode, Connection **con)
>      ConnectionKey key;
>      Packet *pkt = NULL;
>      Connection *conn;
> +    int ret;
> 
>      if (mode == PRIMARY_IN) {
>          pkt = packet_new(s->pri_rs.buf, @@ -243,16 +248,18 @@ static int
> packet_enqueue(CompareState *s, int mode, Connection **con)
>      }
> 
>      if (mode == PRIMARY_IN) {
> -        if (!colo_insert_packet(&conn->primary_list, pkt, &conn->pack)) {
> -            error_report("colo compare primary queue size too big,"
> -                         "drop packet");
> -        }
> +        ret = colo_insert_packet(&conn->primary_list, pkt,
> + &conn->pack);
>      } else {
> -        if (!colo_insert_packet(&conn->secondary_list, pkt, &conn->sack)) {
> -            error_report("colo compare secondary queue size too big,"
> -                         "drop packet");
> -        }
> +        ret = colo_insert_packet(&conn->secondary_list, pkt,
> + &conn->sack);
>      }
> +
> +    if (!ret) {
> +        error_report("colo compare %s queue size too big,"
> +                     "drop packet", colo_mode[mode]);
> +        packet_destroy(pkt, NULL);
> +        pkt = NULL;
> +    }
> +
>      *con = conn;
> 
>      return 0;
> --
> 2.17.1

Re: [PATCH v4 1/2] net/colo-compare.c: Fix memory leak in packet_enqueue()

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 2310 bytes --]

On Sat, 28 Mar 2020 20:46:45 +0800
Derek Su <dereksu@qnap.com> wrote:

> The patch is to fix the "pkt" memory leak in packet_enqueue().
> The allocated "pkt" needs to be freed if the colo compare
> primary or secondary queue is too big.
> 
> Signed-off-by: Derek Su <dereksu@qnap.com>

Looks good and works well in my tests.
Reviewed-by: Lukas Straub <lukasstraub2@web.de>
Tested-by: Lukas Straub <lukasstraub2@web.de>

Regards,
Lukas Straub

> ---
>  net/colo-compare.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index 7ee17f2cf8..cdd87b2aa8 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -120,6 +120,10 @@ enum {
>      SECONDARY_IN,
>  };
>  
> +static const char *colo_mode[] = {
> +    [PRIMARY_IN] = "primary",
> +    [SECONDARY_IN] = "secondary",
> +};
>  
>  static int compare_chr_send(CompareState *s,
>                              const uint8_t *buf,
> @@ -215,6 +219,7 @@ static int packet_enqueue(CompareState *s, int mode, Connection **con)
>      ConnectionKey key;
>      Packet *pkt = NULL;
>      Connection *conn;
> +    int ret;
>  
>      if (mode == PRIMARY_IN) {
>          pkt = packet_new(s->pri_rs.buf,
> @@ -243,16 +248,18 @@ static int packet_enqueue(CompareState *s, int mode, Connection **con)
>      }
>  
>      if (mode == PRIMARY_IN) {
> -        if (!colo_insert_packet(&conn->primary_list, pkt, &conn->pack)) {
> -            error_report("colo compare primary queue size too big,"
> -                         "drop packet");
> -        }
> +        ret = colo_insert_packet(&conn->primary_list, pkt, &conn->pack);
>      } else {
> -        if (!colo_insert_packet(&conn->secondary_list, pkt, &conn->sack)) {
> -            error_report("colo compare secondary queue size too big,"
> -                         "drop packet");
> -        }
> +        ret = colo_insert_packet(&conn->secondary_list, pkt, &conn->sack);
>      }
> +
> +    if (!ret) {
> +        error_report("colo compare %s queue size too big,"
> +                     "drop packet", colo_mode[mode]);
> +        packet_destroy(pkt, NULL);
> +        pkt = NULL;
> +    }
> +
>      *con = conn;
>  
>      return 0;


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v4 2/2] net/colo-compare.c: handling of the full primary or secondary queue

[Derek Su]

The pervious handling of the full primary or queue is only dropping
the packet. If there are lots of clients to the guest VM,
the "drop" will lead to the lost of the networking connection
until next checkpoint.

To address the issue, this patch drops the packet firstly.
Then, do checkpoint and flush packets.

Signed-off-by: Derek Su <dereksu@qnap.com>
---
 net/colo-compare.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index cdd87b2aa8..fe8779cf2d 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -125,6 +125,12 @@ static const char *colo_mode[] = {
     [SECONDARY_IN] = "secondary",
 };
 
+enum {
+    QUEUE_INSERT_ERR = -1,
+    QUEUE_INSERT_OK = 0,
+    QUEUE_INSERT_FULL = 1,
+};
+
 static int compare_chr_send(CompareState *s,
                             const uint8_t *buf,
                             uint32_t size,
@@ -211,8 +217,10 @@ static int colo_insert_packet(GQueue *queue, Packet *pkt, uint32_t *max_ack)
 }
 
 /*
- * Return 0 on success, if return -1 means the pkt
- * is unsupported(arp and ipv6) and will be sent later
+ * Return QUEUE_INSERT_OK on success.
+ * If return QUEUE_INSERT_FULL means list is full, and
+ * QUEUE_INSERT_ERR means the pkt is unsupported(arp and ipv6) and
+ * will be sent later
  */
 static int packet_enqueue(CompareState *s, int mode, Connection **con)
 {
@@ -234,7 +242,7 @@ static int packet_enqueue(CompareState *s, int mode, Connection **con)
     if (parse_packet_early(pkt)) {
         packet_destroy(pkt, NULL);
         pkt = NULL;
-        return -1;
+        return QUEUE_INSERT_ERR;
     }
     fill_connection_key(pkt, &key);
 
@@ -258,11 +266,12 @@ static int packet_enqueue(CompareState *s, int mode, Connection **con)
                      "drop packet", colo_mode[mode]);
         packet_destroy(pkt, NULL);
         pkt = NULL;
+        return QUEUE_INSERT_FULL;
     }
 
     *con = conn;
 
-    return 0;
+    return QUEUE_INSERT_OK;
 }
 
 static inline bool after(uint32_t seq1, uint32_t seq2)
@@ -995,17 +1004,21 @@ static void compare_pri_rs_finalize(SocketReadState *pri_rs)
 {
     CompareState *s = container_of(pri_rs, CompareState, pri_rs);
     Connection *conn = NULL;
+    int ret;
 
-    if (packet_enqueue(s, PRIMARY_IN, &conn)) {
+    ret = packet_enqueue(s, PRIMARY_IN, &conn);
+    if (ret == QUEUE_INSERT_OK) {
+        /* compare packet in the specified connection */
+        colo_compare_connection(conn, s);
+    } else if (ret == QUEUE_INSERT_FULL) {
+        colo_compare_inconsistency_notify(s);
+    } else {
         trace_colo_compare_main("primary: unsupported packet in");
         compare_chr_send(s,
                          pri_rs->buf,
                          pri_rs->packet_len,
                          pri_rs->vnet_hdr_len,
                          false);
-    } else {
-        /* compare packet in the specified connection */
-        colo_compare_connection(conn, s);
     }
 }
 
@@ -1013,12 +1026,16 @@ static void compare_sec_rs_finalize(SocketReadState *sec_rs)
 {
     CompareState *s = container_of(sec_rs, CompareState, sec_rs);
     Connection *conn = NULL;
+    int ret;
 
-    if (packet_enqueue(s, SECONDARY_IN, &conn)) {
-        trace_colo_compare_main("secondary: unsupported packet in");
-    } else {
+    ret = packet_enqueue(s, SECONDARY_IN, &conn);
+    if (ret == QUEUE_INSERT_OK) {
         /* compare packet in the specified connection */
         colo_compare_connection(conn, s);
+    } else if (ret == QUEUE_INSERT_FULL) {
+        colo_compare_inconsistency_notify(s);
+    } else {
+        trace_colo_compare_main("secondary: unsupported packet in");
     }
 }
 
-- 
2.17.1

RE: [PATCH v4 2/2] net/colo-compare.c: handling of the full primary or secondary queue

[Zhang, Chen]

> Subject: [PATCH v4 2/2] net/colo-compare.c: handling of the full primary or
> secondary queue
> 
> The pervious handling of the full primary or queue is only dropping the
> packet. If there are lots of clients to the guest VM, the "drop" will lead to the
> lost of the networking connection until next checkpoint.
> 
> To address the issue, this patch drops the packet firstly.
> Then, do checkpoint and flush packets.
> 
> Signed-off-by: Derek Su <dereksu@qnap.com>

Looks good for me.
Reviewed-by: Zhang Chen <chen.zhang@intel.com>

Thanks
Zhang Chen

> ---
>  net/colo-compare.c | 39 ++++++++++++++++++++++++++++-----------
>  1 file changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/net/colo-compare.c b/net/colo-compare.c index
> cdd87b2aa8..fe8779cf2d 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -125,6 +125,12 @@ static const char *colo_mode[] = {
>      [SECONDARY_IN] = "secondary",
>  };
> 
> +enum {
> +    QUEUE_INSERT_ERR = -1,
> +    QUEUE_INSERT_OK = 0,
> +    QUEUE_INSERT_FULL = 1,
> +};
> +
>  static int compare_chr_send(CompareState *s,
>                              const uint8_t *buf,
>                              uint32_t size, @@ -211,8 +217,10 @@ static int
> colo_insert_packet(GQueue *queue, Packet *pkt, uint32_t *max_ack)  }
> 
>  /*
> - * Return 0 on success, if return -1 means the pkt
> - * is unsupported(arp and ipv6) and will be sent later
> + * Return QUEUE_INSERT_OK on success.
> + * If return QUEUE_INSERT_FULL means list is full, and
> + * QUEUE_INSERT_ERR means the pkt is unsupported(arp and ipv6) and
> + * will be sent later
>   */
>  static int packet_enqueue(CompareState *s, int mode, Connection **con)
> { @@ -234,7 +242,7 @@ static int packet_enqueue(CompareState *s, int
> mode, Connection **con)
>      if (parse_packet_early(pkt)) {
>          packet_destroy(pkt, NULL);
>          pkt = NULL;
> -        return -1;
> +        return QUEUE_INSERT_ERR;
>      }
>      fill_connection_key(pkt, &key);
> 
> @@ -258,11 +266,12 @@ static int packet_enqueue(CompareState *s, int
> mode, Connection **con)
>                       "drop packet", colo_mode[mode]);
>          packet_destroy(pkt, NULL);
>          pkt = NULL;
> +        return QUEUE_INSERT_FULL;
>      }
> 
>      *con = conn;
> 
> -    return 0;
> +    return QUEUE_INSERT_OK;
>  }
> 
>  static inline bool after(uint32_t seq1, uint32_t seq2) @@ -995,17 +1004,21
> @@ static void compare_pri_rs_finalize(SocketReadState *pri_rs)  {
>      CompareState *s = container_of(pri_rs, CompareState, pri_rs);
>      Connection *conn = NULL;
> +    int ret;
> 
> -    if (packet_enqueue(s, PRIMARY_IN, &conn)) {
> +    ret = packet_enqueue(s, PRIMARY_IN, &conn);
> +    if (ret == QUEUE_INSERT_OK) {
> +        /* compare packet in the specified connection */
> +        colo_compare_connection(conn, s);
> +    } else if (ret == QUEUE_INSERT_FULL) {
> +        colo_compare_inconsistency_notify(s);
> +    } else {
>          trace_colo_compare_main("primary: unsupported packet in");
>          compare_chr_send(s,
>                           pri_rs->buf,
>                           pri_rs->packet_len,
>                           pri_rs->vnet_hdr_len,
>                           false);
> -    } else {
> -        /* compare packet in the specified connection */
> -        colo_compare_connection(conn, s);
>      }
>  }
> 
> @@ -1013,12 +1026,16 @@ static void
> compare_sec_rs_finalize(SocketReadState *sec_rs)  {
>      CompareState *s = container_of(sec_rs, CompareState, sec_rs);
>      Connection *conn = NULL;
> +    int ret;
> 
> -    if (packet_enqueue(s, SECONDARY_IN, &conn)) {
> -        trace_colo_compare_main("secondary: unsupported packet in");
> -    } else {
> +    ret = packet_enqueue(s, SECONDARY_IN, &conn);
> +    if (ret == QUEUE_INSERT_OK) {
>          /* compare packet in the specified connection */
>          colo_compare_connection(conn, s);
> +    } else if (ret == QUEUE_INSERT_FULL) {
> +        colo_compare_inconsistency_notify(s);
> +    } else {
> +        trace_colo_compare_main("secondary: unsupported packet in");
>      }
>  }
> 
> --
> 2.17.1

Re: [PATCH v4 2/2] net/colo-compare.c: handling of the full primary or secondary queue

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 4303 bytes --]

On Sat, 28 Mar 2020 20:46:46 +0800
Derek Su <dereksu@qnap.com> wrote:

> The pervious handling of the full primary or queue is only dropping
> the packet. If there are lots of clients to the guest VM,
> the "drop" will lead to the lost of the networking connection
> until next checkpoint.
> 
> To address the issue, this patch drops the packet firstly.
> Then, do checkpoint and flush packets.
> 
> Signed-off-by: Derek Su <dereksu@qnap.com>

Looks good and works well in my tests.
Reviewed-by: Lukas Straub <lukasstraub2@web.de>
Tested-by: Lukas Straub <lukasstraub2@web.de>

Regards,
Lukas Straub

> ---
>  net/colo-compare.c | 39 ++++++++++++++++++++++++++++-----------
>  1 file changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index cdd87b2aa8..fe8779cf2d 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -125,6 +125,12 @@ static const char *colo_mode[] = {
>      [SECONDARY_IN] = "secondary",
>  };
>  
> +enum {
> +    QUEUE_INSERT_ERR = -1,
> +    QUEUE_INSERT_OK = 0,
> +    QUEUE_INSERT_FULL = 1,
> +};
> +
>  static int compare_chr_send(CompareState *s,
>                              const uint8_t *buf,
>                              uint32_t size,
> @@ -211,8 +217,10 @@ static int colo_insert_packet(GQueue *queue, Packet *pkt, uint32_t *max_ack)
>  }
>  
>  /*
> - * Return 0 on success, if return -1 means the pkt
> - * is unsupported(arp and ipv6) and will be sent later
> + * Return QUEUE_INSERT_OK on success.
> + * If return QUEUE_INSERT_FULL means list is full, and
> + * QUEUE_INSERT_ERR means the pkt is unsupported(arp and ipv6) and
> + * will be sent later
>   */
>  static int packet_enqueue(CompareState *s, int mode, Connection **con)
>  {
> @@ -234,7 +242,7 @@ static int packet_enqueue(CompareState *s, int mode, Connection **con)
>      if (parse_packet_early(pkt)) {
>          packet_destroy(pkt, NULL);
>          pkt = NULL;
> -        return -1;
> +        return QUEUE_INSERT_ERR;
>      }
>      fill_connection_key(pkt, &key);
>  
> @@ -258,11 +266,12 @@ static int packet_enqueue(CompareState *s, int mode, Connection **con)
>                       "drop packet", colo_mode[mode]);
>          packet_destroy(pkt, NULL);
>          pkt = NULL;
> +        return QUEUE_INSERT_FULL;
>      }
>  
>      *con = conn;
>  
> -    return 0;
> +    return QUEUE_INSERT_OK;
>  }
>  
>  static inline bool after(uint32_t seq1, uint32_t seq2)
> @@ -995,17 +1004,21 @@ static void compare_pri_rs_finalize(SocketReadState *pri_rs)
>  {
>      CompareState *s = container_of(pri_rs, CompareState, pri_rs);
>      Connection *conn = NULL;
> +    int ret;
>  
> -    if (packet_enqueue(s, PRIMARY_IN, &conn)) {
> +    ret = packet_enqueue(s, PRIMARY_IN, &conn);
> +    if (ret == QUEUE_INSERT_OK) {
> +        /* compare packet in the specified connection */
> +        colo_compare_connection(conn, s);
> +    } else if (ret == QUEUE_INSERT_FULL) {
> +        colo_compare_inconsistency_notify(s);
> +    } else {
>          trace_colo_compare_main("primary: unsupported packet in");
>          compare_chr_send(s,
>                           pri_rs->buf,
>                           pri_rs->packet_len,
>                           pri_rs->vnet_hdr_len,
>                           false);
> -    } else {
> -        /* compare packet in the specified connection */
> -        colo_compare_connection(conn, s);
>      }
>  }
>  
> @@ -1013,12 +1026,16 @@ static void compare_sec_rs_finalize(SocketReadState *sec_rs)
>  {
>      CompareState *s = container_of(sec_rs, CompareState, sec_rs);
>      Connection *conn = NULL;
> +    int ret;
>  
> -    if (packet_enqueue(s, SECONDARY_IN, &conn)) {
> -        trace_colo_compare_main("secondary: unsupported packet in");
> -    } else {
> +    ret = packet_enqueue(s, SECONDARY_IN, &conn);
> +    if (ret == QUEUE_INSERT_OK) {
>          /* compare packet in the specified connection */
>          colo_compare_connection(conn, s);
> +    } else if (ret == QUEUE_INSERT_FULL) {
> +        colo_compare_inconsistency_notify(s);
> +    } else {
> +        trace_colo_compare_main("secondary: unsupported packet in");
>      }
>  }
>  


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 2/2] net/colo-compare.c: handling of the full primary or secondary queue

[Lukas Straub]

[-- Attachment #1.1: Type: text/plain, Size: 1170 bytes --]

On Sat, 28 Mar 2020 20:46:46 +0800
Derek Su <dereksu@qnap.com> wrote:

> The pervious handling of the full primary or queue is only dropping
> the packet. If there are lots of clients to the guest VM,
> the "drop" will lead to the lost of the networking connection
> until next checkpoint.
> 
> To address the issue, this patch drops the packet firstly.
> Then, do checkpoint and flush packets.
> 
> Signed-off-by: Derek Su <dereksu@qnap.com>

Hello,
I had a look at this again and did some benchmarking.
First just qemu 5.0-rc1 with my bugfixes ( https://lists.nongnu.org/archive/html/qemu-devel/2020-04/msg01432.html )
Then qemu 5.0-rc1 with my bugfixes and this patch series.

This commit hurts performance too much:
Client-to-server bandwidth falls from ~45.9 Mbit/s to 22.9 Mbit/s.
Server-to-client bandwidth falls from ~6.3 Mbit/s to just ~674 Kbit/s.
Average latency rises from ~197ms to ~397ms.

Meanwhile the packet loss without this commit is negligible,
only 1-2 ping packets got lost during each test run.

Instead I think we should just turn the error message
into a trace so it doesn't flood the logs.

Regards,
Lukas Straub

[-- Attachment #1.2: qemu-benchmark-summary.txt --]
[-- Type: text/plain, Size: 8864 bytes --]

5.0-rc1 with bugfixes:

*** iperf -c 192.168.178.65 ***

[  3] local 192.168.178.68 port 33418 connected with 192.168.178.65 port 5001                           64 bytes from 192.168.178.65: icmp_seq=53 ttl=64 time=2.57 ms
[ ID] Interval       Transfer     Bandwidth                                                             64 bytes from 192.168.178.65: icmp_seq=54 ttl=64 time=142 ms
[  3]  0.0-10.2 sec  60.1 MBytes  49.4 Mbits/sec                                                        64 bytes from 192.168.178.65: icmp_seq=55 ttl=64 time=2.64 ms

[  3] Sent 892 datagrams                                                                                rtt min/avg/max/mdev = 1.920/235.514/1810.653/411.394 ms, pipe 4

[  3] local 192.168.178.68 port 33420 connected with 192.168.178.65 port 5001                           64 bytes from 192.168.178.65: icmp_seq=55 ttl=64 time=24.3 ms
[ ID] Interval       Transfer     Bandwidth                                                             64 bytes from 192.168.178.65: icmp_seq=56 ttl=64 time=505 ms
[  3]  0.0-10.3 sec  51.0 MBytes  41.7 Mbits/sec                                                        64 bytes from 192.168.178.65: icmp_seq=57 ttl=64 time=11.7 ms

[  3] Sent 892 datagrams                                                                                rtt min/avg/max/mdev = 1.962/218.713/1365.510/344.164 ms, pipe 3

[  3] local 192.168.178.68 port 33422 connected with 192.168.178.65 port 5001                           64 bytes from 192.168.178.65: icmp_seq=54 ttl=64 time=49.2 ms
[ ID] Interval       Transfer     Bandwidth                                                             64 bytes from 192.168.178.65: icmp_seq=55 ttl=64 time=587 ms
[  3]  0.0-10.8 sec  60.1 MBytes  46.6 Mbits/sec                                                        64 bytes from 192.168.178.65: icmp_seq=56 ttl=64 time=167 ms

[  3] Sent 892 datagrams                                                                                rtt min/avg/max/mdev = 1.573/276.375/1384.085/362.027 ms, pipe 3


*** iperf -c 192.168.178.65 -d ***

[  4] local 192.168.178.68 port 33424 connected with 192.168.178.65 port 5001                           64 bytes from 192.168.178.65: icmp_seq=60 ttl=64 time=2.27 ms
[  5] local 192.168.178.68 port 5001 connected with 192.168.178.65 port 48440                           ^C
[ ID] Interval       Transfer     Bandwidth                                                             --- 192.168.178.65 ping statistics ---
[  4]  0.0-10.1 sec  60.8 MBytes  50.4 Mbits/sec                                                        60 packets transmitted, 59 received, 1.66667% packet loss, time 29595ms
[  5]  0.0-11.7 sec  11.2 MBytes  8.05 Mbits/sec                                                        rtt min/avg/max/mdev = 1.785/254.258/1352.916/395.070 ms, pipe 3

[  5] local 192.168.178.68 port 33426 connected with 192.168.178.65 port 5001                           64 bytes from 192.168.178.65: icmp_seq=59 ttl=64 time=2.98 ms
[  4] local 192.168.178.68 port 5001 connected with 192.168.178.65 port 48442                           ^C
[ ID] Interval       Transfer     Bandwidth                                                             --- 192.168.178.65 ping statistics ---
[  5]  0.0-10.3 sec  58.6 MBytes  47.6 Mbits/sec                                                        59 packets transmitted, 59 received, 0% packet loss, time 29088ms
[  4]  0.0-11.9 sec  13.8 MBytes  9.77 Mbits/sec                                                        rtt min/avg/max/mdev = 2.026/207.507/1518.013/331.972 ms, pipe 4

[  5] local 192.168.178.68 port 33432 connected with 192.168.178.65 port 5001                           64 bytes from 192.168.178.65: icmp_seq=93 ttl=64 time=12.6 ms
[  4] local 192.168.178.68 port 5001 connected with 192.168.178.65 port 48446                           ^C
[ ID] Interval       Transfer     Bandwidth                                                             --- 192.168.178.65 ping statistics ---
[  5]  0.0-10.2 sec  59.4 MBytes  49.0 Mbits/sec                                                        93 packets transmitted, 92 received, 1.07527% packet loss, time 46137ms
[  4]  0.0-31.0 sec  4.41 MBytes  1.19 Mbits/sec                                                        rtt min/avg/max/mdev = 2.107/131.212/1405.670/287.854 ms, pipe 3


5.0-rc1 with bugfixes and checkpoint on queue full:

*** iperf -c 192.168.178.65 ***

[  3] local 192.168.178.68 port 33402 connected with 192.168.178.65 port 5001                           64 bytes from 192.168.178.65: icmp_seq=52 ttl=64 time=1329 ms
[ ID] Interval       Transfer     Bandwidth                                                             64 bytes from 192.168.178.65: icmp_seq=53 ttl=64 time=828 ms
[  3]  0.0-10.1 sec  35.9 MBytes  29.9 Mbits/sec                                                        64 bytes from 192.168.178.65: icmp_seq=54 ttl=64 time=619 ms

[  3] Sent 892 datagrams                                                                                rtt min/avg/max/mdev = 2.183/788.828/3329.045/958.683 ms, pipe 7

[  3] local 192.168.178.68 port 33404 connected with 192.168.178.65 port 5001                           64 bytes from 192.168.178.65: icmp_seq=57 ttl=64 time=2.58 ms
[ ID] Interval       Transfer     Bandwidth                                                             64 bytes from 192.168.178.65: icmp_seq=58 ttl=64 time=322 ms
[  3]  0.0-11.5 sec  36.5 MBytes  26.6 Mbits/sec                                                        64 bytes from 192.168.178.65: icmp_seq=59 ttl=64 time=10.2 ms

[  3] Sent 892 datagrams                                                                                rtt min/avg/max/mdev = 1.962/610.122/3303.544/907.708 ms, pipe 7

[  3] local 192.168.178.68 port 33406 connected with 192.168.178.65 port 5001                           64 bytes from 192.168.178.65: icmp_seq=55 ttl=64 time=2.60 ms
[ ID] Interval       Transfer     Bandwidth                                                             64 bytes from 192.168.178.65: icmp_seq=56 ttl=64 time=112 ms
[  3]  0.0-11.1 sec  16.1 MBytes  12.2 Mbits/sec                                                        64 bytes from 192.168.178.65: icmp_seq=57 ttl=64 time=118 ms

[  3] Sent 892 datagrams                                                                                rtt min/avg/max/mdev = 2.088/593.139/2734.770/857.021 ms, pipe 6

*** iperf -c 192.168.178.65 -d ***

[  5] local 192.168.178.68 port 33408 connected with 192.168.178.65 port 5001                           64 bytes from 192.168.178.65: icmp_seq=75 ttl=64 time=2.64 ms
[  4] local 192.168.178.68 port 5001 connected with 192.168.178.65 port 47492                           ^C
[ ID] Interval       Transfer     Bandwidth                                                             --- 192.168.178.65 ping statistics ---
[  5]  0.0-10.3 sec  51.2 MBytes  41.9 Mbits/sec                                                        75 packets transmitted, 75 received, 0% packet loss, time 37122ms
[  4]  0.0-25.0 sec  1.00 MBytes   336 Kbits/sec                                                        rtt min/avg/max/mdev = 1.830/563.652/1996.992/651.117 ms, pipe 4

[  4] local 192.168.178.68 port 33410 connected with 192.168.178.65 port 5001                           64 bytes from 192.168.178.65: icmp_seq=127 ttl=64 time=2.41 ms
[  5] local 192.168.178.68 port 5001 connected with 192.168.178.65 port 47494                           ^C
[ ID] Interval       Transfer     Bandwidth                                                             --- 192.168.178.65 ping statistics ---
[  4]  0.0-10.1 sec  39.0 MBytes  32.4 Mbits/sec                                                        127 packets transmitted, 126 received, 0.787402% packet loss, time 63205ms
[  5]  0.0-50.8 sec  5.01 MBytes   826 Kbits/sec                                                        rtt min/avg/max/mdev = 1.791/239.200/2105.867/513.022 ms, pipe 5

[  4] local 192.168.178.68 port 33414 connected with 192.168.178.65 port 5001                           64 bytes from 192.168.178.65: icmp_seq=90 ttl=64 time=69.4 ms
[  5] local 192.168.178.68 port 5001 connected with 192.168.178.65 port 47496                           ^C
[ ID] Interval       Transfer     Bandwidth                                                             --- 192.168.178.65 ping statistics ---
[  4]  0.0-10.5 sec  46.1 MBytes  37.0 Mbits/sec                                                        90 packets transmitted, 90 received, 0% packet loss, time 44643ms
[  5]  0.0-31.8 sec  3.26 MBytes   861 Kbits/sec                                                        rtt min/avg/max/mdev = 1.909/391.216/1878.484/645.897 ms, pipe 4

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH v4 2/2] net/colo-compare.c: handling of the full primary or secondary queue

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Thursday, April 9, 2020 3:19 AM
> To: Derek Su <dereksu@qnap.com>
> Cc: qemu-devel@nongnu.org; lizhijian@cn.fujitsu.com; chyang@qnap.com;
> jasowang@redhat.com; ctcheng@qnap.com; Zhang, Chen
> <chen.zhang@intel.com>; jwsu1986@gmail.com
> Subject: Re: [PATCH v4 2/2] net/colo-compare.c: handling of the full primary
> or secondary queue
> 
> On Sat, 28 Mar 2020 20:46:46 +0800
> Derek Su <dereksu@qnap.com> wrote:
> 
> > The pervious handling of the full primary or queue is only dropping
> > the packet. If there are lots of clients to the guest VM, the "drop"
> > will lead to the lost of the networking connection until next
> > checkpoint.
> >
> > To address the issue, this patch drops the packet firstly.
> > Then, do checkpoint and flush packets.
> >
> > Signed-off-by: Derek Su <dereksu@qnap.com>
> 
> Hello,
> I had a look at this again and did some benchmarking.
> First just qemu 5.0-rc1 with my bugfixes
> ( https://lists.nongnu.org/archive/html/qemu-devel/2020-
> 04/msg01432.html ) Then qemu 5.0-rc1 with my bugfixes and this patch
> series.
> 
> This commit hurts performance too much:
> Client-to-server bandwidth falls from ~45.9 Mbit/s to 22.9 Mbit/s.
> Server-to-client bandwidth falls from ~6.3 Mbit/s to just ~674 Kbit/s.
> Average latency rises from ~197ms to ~397ms.
> 
> Meanwhile the packet loss without this commit is negligible, only 1-2 ping
> packets got lost during each test run.
> 
> Instead I think we should just turn the error message into a trace so it
> doesn't flood the logs.

We re-test this patch, Lukas is right.
Sorry for the original idea, looks like it did not show better performance in the test.
Agree with Lukas's comments. Derek, can you please change it?

Thanks
Zhang Chen


> 
> Regards,
> Lukas Straub

Re: [PATCH v4 2/2] net/colo-compare.c: handling of the full primary or secondary queue

[Derek Su]

Hello, Zhang and Lukas

Sure, after my re-test, the performance is hurt. Will update it later.

By the way, could I also move the "error_report("colo compare
primary/secondary queue size too big, drop packet");" to trace?
The use of error_report is a little strange and make a flood in log.

May I  also make "MAX_QUEUE_SIZE"  be user-configurable in this series?

Thanks,
Derek Su




Zhang, Chen <chen.zhang@intel.com> 於 2020年4月9日 週四 下午2:59寫道：
>
>
>
> > -----Original Message-----
> > From: Lukas Straub <lukasstraub2@web.de>
> > Sent: Thursday, April 9, 2020 3:19 AM
> > To: Derek Su <dereksu@qnap.com>
> > Cc: qemu-devel@nongnu.org; lizhijian@cn.fujitsu.com; chyang@qnap.com;
> > jasowang@redhat.com; ctcheng@qnap.com; Zhang, Chen
> > <chen.zhang@intel.com>; jwsu1986@gmail.com
> > Subject: Re: [PATCH v4 2/2] net/colo-compare.c: handling of the full primary
> > or secondary queue
> >
> > On Sat, 28 Mar 2020 20:46:46 +0800
> > Derek Su <dereksu@qnap.com> wrote:
> >
> > > The pervious handling of the full primary or queue is only dropping
> > > the packet. If there are lots of clients to the guest VM, the "drop"
> > > will lead to the lost of the networking connection until next
> > > checkpoint.
> > >
> > > To address the issue, this patch drops the packet firstly.
> > > Then, do checkpoint and flush packets.
> > >
> > > Signed-off-by: Derek Su <dereksu@qnap.com>
> >
> > Hello,
> > I had a look at this again and did some benchmarking.
> > First just qemu 5.0-rc1 with my bugfixes
> > ( https://lists.nongnu.org/archive/html/qemu-devel/2020-
> > 04/msg01432.html ) Then qemu 5.0-rc1 with my bugfixes and this patch
> > series.
> >
> > This commit hurts performance too much:
> > Client-to-server bandwidth falls from ~45.9 Mbit/s to 22.9 Mbit/s.
> > Server-to-client bandwidth falls from ~6.3 Mbit/s to just ~674 Kbit/s.
> > Average latency rises from ~197ms to ~397ms.
> >
> > Meanwhile the packet loss without this commit is negligible, only 1-2 ping
> > packets got lost during each test run.
> >
> > Instead I think we should just turn the error message into a trace so it
> > doesn't flood the logs.
>
> We re-test this patch, Lukas is right.
> Sorry for the original idea, looks like it did not show better performance in the test.
> Agree with Lukas's comments. Derek, can you please change it?
>
> Thanks
> Zhang Chen
>
>
> >
> > Regards,
> > Lukas Straub

RE: [PATCH v4 2/2] net/colo-compare.c: handling of the full primary or secondary queue

[Zhang, Chen]

> -----Original Message-----
> From: Derek Su <jwsu1986@gmail.com>
> Sent: Thursday, April 9, 2020 3:11 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Lukas Straub <lukasstraub2@web.de>; Derek Su <dereksu@qnap.com>;
> qemu-devel@nongnu.org; lizhijian@cn.fujitsu.com; chyang@qnap.com;
> jasowang@redhat.com; ctcheng@qnap.com
> Subject: Re: [PATCH v4 2/2] net/colo-compare.c: handling of the full primary
> or secondary queue
> 
> Hello, Zhang and Lukas
> 
> Sure, after my re-test, the performance is hurt. Will update it later.
> 
> By the way, could I also move the "error_report("colo compare
> primary/secondary queue size too big, drop packet");" to trace?

Yes.

> The use of error_report is a little strange and make a flood in log.
> 
> May I  also make "MAX_QUEUE_SIZE"  be user-configurable in this series?

I have this patch internal and will send it later.

Thanks
Zhang Chen

> 
> Thanks,
> Derek Su
> 
> 
> 
> 
> Zhang, Chen <chen.zhang@intel.com> 於 2020年4月9日 週四 下午2:59
> 寫道：
> >
> >
> >
> > > -----Original Message-----
> > > From: Lukas Straub <lukasstraub2@web.de>
> > > Sent: Thursday, April 9, 2020 3:19 AM
> > > To: Derek Su <dereksu@qnap.com>
> > > Cc: qemu-devel@nongnu.org; lizhijian@cn.fujitsu.com;
> > > chyang@qnap.com; jasowang@redhat.com; ctcheng@qnap.com; Zhang,
> Chen
> > > <chen.zhang@intel.com>; jwsu1986@gmail.com
> > > Subject: Re: [PATCH v4 2/2] net/colo-compare.c: handling of the full
> > > primary or secondary queue
> > >
> > > On Sat, 28 Mar 2020 20:46:46 +0800
> > > Derek Su <dereksu@qnap.com> wrote:
> > >
> > > > The pervious handling of the full primary or queue is only
> > > > dropping the packet. If there are lots of clients to the guest VM, the
> "drop"
> > > > will lead to the lost of the networking connection until next
> > > > checkpoint.
> > > >
> > > > To address the issue, this patch drops the packet firstly.
> > > > Then, do checkpoint and flush packets.
> > > >
> > > > Signed-off-by: Derek Su <dereksu@qnap.com>
> > >
> > > Hello,
> > > I had a look at this again and did some benchmarking.
> > > First just qemu 5.0-rc1 with my bugfixes (
> > > https://lists.nongnu.org/archive/html/qemu-devel/2020-
> > > 04/msg01432.html ) Then qemu 5.0-rc1 with my bugfixes and this patch
> > > series.
> > >
> > > This commit hurts performance too much:
> > > Client-to-server bandwidth falls from ~45.9 Mbit/s to 22.9 Mbit/s.
> > > Server-to-client bandwidth falls from ~6.3 Mbit/s to just ~674 Kbit/s.
> > > Average latency rises from ~197ms to ~397ms.
> > >
> > > Meanwhile the packet loss without this commit is negligible, only
> > > 1-2 ping packets got lost during each test run.
> > >
> > > Instead I think we should just turn the error message into a trace
> > > so it doesn't flood the logs.
> >
> > We re-test this patch, Lukas is right.
> > Sorry for the original idea, looks like it did not show better performance in
> the test.
> > Agree with Lukas's comments. Derek, can you please change it?
> >
> > Thanks
> > Zhang Chen
> >
> >
> > >
> > > Regards,
> > > Lukas Straub