Re: [Qemu-devel] [PATCH] COLO-compare: Add compare_lock aviod comparison conflict

[Hailiang Zhang <zhang.zhanghailiang@huawei.com>]

On 2017/4/20 12:32, Zhang Chen wrote:
> When network traffic heavy, compare_pri_rs_finalize() and
> compare_sec_rs_finalize() have a chance to confilct.
> Both of them call colo_compare_connection() to compare packet,
> But during compare_pri_rs_finalize() comparison, have secondary
> packet come and call compare_sec_rs_finalize(), that packet will be
> handle twice. If packet same, the pkt will be double free.

Interesting, if I'm right, this should not happen, because, all the comparing works
are done in colo compare thread, so there is no chance to access the connect_list
concurrently.  Besides, even both of the packets from primary and secondary arrive
at the same time, it should only be handle once, we will handle it with the later arrived one,
No ?

> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
> ---
>   net/colo-compare.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index 54e6d40..686c1b4 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -79,6 +79,8 @@ typedef struct CompareState {
>        * element type: Connection
>        */
>       GQueue conn_list;
> +    /* compare lock */
> +    QemuMutex compare_lock;
>       /* hashtable to save connection */
>       GHashTable *connection_track_table;
>       /* compare thread, a thread for each NIC */
> @@ -619,7 +621,9 @@ static void compare_pri_rs_finalize(SocketReadState *pri_rs)
>           compare_chr_send(&s->chr_out, pri_rs->buf, pri_rs->packet_len);
>       } else {
>           /* compare connection */
> +        qemu_mutex_lock(&s->compare_lock);
>           g_queue_foreach(&s->conn_list, colo_compare_connection, s);
> +        qemu_mutex_unlock(&s->compare_lock);
>       }
>   }
>   
> @@ -631,7 +635,9 @@ static void compare_sec_rs_finalize(SocketReadState *sec_rs)
>           trace_colo_compare_main("secondary: unsupported packet in");
>       } else {
>           /* compare connection */
> +        qemu_mutex_lock(&s->compare_lock);
>           g_queue_foreach(&s->conn_list, colo_compare_connection, s);
> +        qemu_mutex_unlock(&s->compare_lock);
>       }
>   }
>   
> @@ -702,6 +708,7 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
>       net_socket_rs_init(&s->sec_rs, compare_sec_rs_finalize);
>   
>       g_queue_init(&s->conn_list);
> +    qemu_mutex_init(&s->compare_lock);
>   
>       s->connection_track_table = g_hash_table_new_full(connection_key_hash,
>                                                         connection_key_equal,
> @@ -771,6 +778,7 @@ static void colo_compare_finalize(Object *obj)
>       g_queue_foreach(&s->conn_list, colo_flush_packets, s);
>   
>       g_queue_clear(&s->conn_list);
> +    qemu_mutex_destroy(&s->compare_lock);
>   
>       g_hash_table_destroy(s->connection_track_table);
>       g_free(s->pri_indev);