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

["Zhang, Chen" <chen.zhang@intel.com>]

> -----Original Message-----
> From: Derek Su <jwsu1986@gmail.com>
> Sent: Monday, March 23, 2020 1:48 AM
> To: qemu-devel@nongnu.org
> Cc: Zhang, Chen <chen.zhang@intel.com>; lizhijian@cn.fujitsu.com;
> jasowang@redhat.com; dereksu@qnap.com
> Subject: [PATCH v2 1/1] 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.

Hi Derek,

Thank you for the patch.
I re-think this issue in a big view, looks just free the pkg is not enough in this situation.
The root cause is network is too busy to compare, So, better choice is notify COLO frame
to do a checkpoint and clean up all the network queue. This work maybe decrease
COLO network performance but seams better than drop lots of pkg.

Thanks
Zhang Chen 

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