[PATCH v5 0/1] COLO: handling of the full primary or secondary queue

[PATCH v5 0/1] 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.

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".

Please help to review, thanks.

V5:
 - Replace the error_report of full queue with a trace event
 - Remove handling of the full primary or secondary queue which hurt
   network throughput too much

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 (1):
  colo-compare: Fix memory leak in packet_enqueue()

 net/colo-compare.c | 23 +++++++++++++++--------
 net/trace-events   |  1 +
 2 files changed, 16 insertions(+), 8 deletions(-)

-- 
2.17.1

[PATCH v5 1/1] colo-compare: 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.

Replace the error_report of full queue with a trace event.

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

diff --git a/net/colo-compare.c b/net/colo-compare.c
index 10c0239f9d..035e11d4d3 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -122,6 +122,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,
@@ -217,6 +221,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,
@@ -245,16 +250,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) {
+        trace_colo_compare_drop_packet(colo_mode[mode],
+            "queue size too big, drop packet");
+        packet_destroy(pkt, NULL);
+        pkt = NULL;
+    }
+
     *con = conn;
 
     return 0;
diff --git a/net/trace-events b/net/trace-events
index 02c13fd0ba..fa49c71533 100644
--- a/net/trace-events
+++ b/net/trace-events
@@ -12,6 +12,7 @@ colo_proxy_main(const char *chr) ": %s"
 
 # colo-compare.c
 colo_compare_main(const char *chr) ": %s"
+colo_compare_drop_packet(const char *queue, const char *chr) ": %s: %s"
 colo_compare_udp_miscompare(const char *sta, int size) ": %s = %d"
 colo_compare_icmp_miscompare(const char *sta, int size) ": %s = %d"
 colo_compare_ip_info(int psize, const char *sta, const char *stb, int ssize, const char *stc, const char *std) "ppkt size = %d, ip_src = %s, ip_dst = %s, spkt size = %d, ip_src = %s, ip_dst = %s"
-- 
2.17.1

RE: [PATCH v5 1/1] colo-compare: Fix memory leak in packet_enqueue()

[Zhang, Chen]

> -----Original Message-----
> From: Derek Su <dereksu@qnap.com>
> Sent: Friday, April 10, 2020 10:01 AM
> 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 v5 1/1] colo-compare: 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.
> 
> Replace the error_report of full queue with a trace event.
> 

Looks good for me. Thanks your contribution.

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

Thanks
Zhang Chen

> Signed-off-by: Derek Su <dereksu@qnap.com>
> ---
>  net/colo-compare.c | 23 +++++++++++++++--------
>  net/trace-events   |  1 +
>  2 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/net/colo-compare.c b/net/colo-compare.c index
> 10c0239f9d..035e11d4d3 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -122,6 +122,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, @@ -217,6 +221,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, @@ -245,16 +250,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) {
> +        trace_colo_compare_drop_packet(colo_mode[mode],
> +            "queue size too big, drop packet");
> +        packet_destroy(pkt, NULL);
> +        pkt = NULL;
> +    }
> +
>      *con = conn;
> 
>      return 0;
> diff --git a/net/trace-events b/net/trace-events index
> 02c13fd0ba..fa49c71533 100644
> --- a/net/trace-events
> +++ b/net/trace-events
> @@ -12,6 +12,7 @@ colo_proxy_main(const char *chr) ": %s"
> 
>  # colo-compare.c
>  colo_compare_main(const char *chr) ": %s"
> +colo_compare_drop_packet(const char *queue, const char *chr) ": %s: %s"
>  colo_compare_udp_miscompare(const char *sta, int size) ": %s = %d"
>  colo_compare_icmp_miscompare(const char *sta, int size) ": %s = %d"
>  colo_compare_ip_info(int psize, const char *sta, const char *stb, int ssize,
> const char *stc, const char *std) "ppkt size = %d, ip_src = %s, ip_dst = %s,
> spkt size = %d, ip_src = %s, ip_dst = %s"
> --
> 2.17.1

Re: [PATCH v5 1/1] colo-compare: Fix memory leak in packet_enqueue()

[Lukas Straub]

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

On Fri, 10 Apr 2020 10:00:56 +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.
> 
> Replace the error_report of full queue with a trace event.
> 
> Signed-off-by: Derek Su <dereksu@qnap.com>

Looks good now 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 +++++++++++++++--------
>  net/trace-events   |  1 +
>  2 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index 10c0239f9d..035e11d4d3 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -122,6 +122,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,
> @@ -217,6 +221,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,
> @@ -245,16 +250,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) {
> +        trace_colo_compare_drop_packet(colo_mode[mode],
> +            "queue size too big, drop packet");
> +        packet_destroy(pkt, NULL);
> +        pkt = NULL;
> +    }
> +
>      *con = conn;
>  
>      return 0;
> diff --git a/net/trace-events b/net/trace-events
> index 02c13fd0ba..fa49c71533 100644
> --- a/net/trace-events
> +++ b/net/trace-events
> @@ -12,6 +12,7 @@ colo_proxy_main(const char *chr) ": %s"
>  
>  # colo-compare.c
>  colo_compare_main(const char *chr) ": %s"
> +colo_compare_drop_packet(const char *queue, const char *chr) ": %s: %s"
>  colo_compare_udp_miscompare(const char *sta, int size) ": %s = %d"
>  colo_compare_icmp_miscompare(const char *sta, int size) ": %s = %d"
>  colo_compare_ip_info(int psize, const char *sta, const char *stb, int ssize, const char *stc, const char *std) "ppkt size = %d, ip_src = %s, ip_dst = %s, spkt size = %d, ip_src = %s, ip_dst = %s"


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

RE: [PATCH v5 1/1] colo-compare: Fix memory leak in packet_enqueue()

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Sunday, April 26, 2020 7:25 PM
> 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 v5 1/1] colo-compare: Fix memory leak in
> packet_enqueue()
> 
> On Fri, 10 Apr 2020 10:00:56 +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.
> >
> > Replace the error_report of full queue with a trace event.
> >
> > Signed-off-by: Derek Su <dereksu@qnap.com>
> 
> Looks good now and works well in my tests.
> Reviewed-by: Lukas Straub <lukasstraub2@web.de>
> Tested-by: Lukas Straub <lukasstraub2@web.de>
> 

I will queue this patch to COLO branch, then submit to Jason.

Thanks
Zhang Chen

> Regards,
> Lukas Straub
> 
> > ---
> >  net/colo-compare.c | 23 +++++++++++++++--------
> >  net/trace-events   |  1 +
> >  2 files changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > 10c0239f9d..035e11d4d3 100644
> > --- a/net/colo-compare.c
> > +++ b/net/colo-compare.c
> > @@ -122,6 +122,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, @@ -217,6 +221,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, @@ -245,16 +250,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) {
> > +        trace_colo_compare_drop_packet(colo_mode[mode],
> > +            "queue size too big, drop packet");
> > +        packet_destroy(pkt, NULL);
> > +        pkt = NULL;
> > +    }
> > +
> >      *con = conn;
> >
> >      return 0;
> > diff --git a/net/trace-events b/net/trace-events index
> > 02c13fd0ba..fa49c71533 100644
> > --- a/net/trace-events
> > +++ b/net/trace-events
> > @@ -12,6 +12,7 @@ colo_proxy_main(const char *chr) ": %s"
> >
> >  # colo-compare.c
> >  colo_compare_main(const char *chr) ": %s"
> > +colo_compare_drop_packet(const char *queue, const char *chr)
> ": %s: %s"
> >  colo_compare_udp_miscompare(const char *sta, int size) ": %s = %d"
> >  colo_compare_icmp_miscompare(const char *sta, int size) ": %s = %d"
> >  colo_compare_ip_info(int psize, const char *sta, const char *stb, int ssize,
> const char *stc, const char *std) "ppkt size = %d, ip_src = %s, ip_dst = %s,
> spkt size = %d, ip_src = %s, ip_dst = %s"