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

From: "Zhang, Chen" <chen.zhang@intel.com>
To: Derek Su <dereksu@qnap.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Cc: "jasowang@redhat.com" <jasowang@redhat.com>,
	"jwsu1986@gmail.com" <jwsu1986@gmail.com>,
	"chyang@qnap.com" <chyang@qnap.com>,
	"lizhijian@cn.fujitsu.com" <lizhijian@cn.fujitsu.com>,
	"ctcheng@qnap.com" <ctcheng@qnap.com>
Subject: RE: [PATCH v4 2/2] net/colo-compare.c: handling of the full primary or secondary queue
Date: Tue, 31 Mar 2020 01:15:30 +0000	[thread overview]
Message-ID: <302c2946a968453fa3c77b17a14f0328@intel.com> (raw)
In-Reply-To: <20200328124646.7778-3-dereksu@qnap.com>

> 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