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

[Jason Wang]

On 2017年04月20日 14:36, Zhang Chen wrote:
> On 04/20/2017 02:20 PM, Hailiang Zhang wrote:
> > 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 ?
>
> No, In my test often trigger this bug, you can use udp server and 
> client test it.
>
> address@hidden:colo_compare_main : packet same and release 
> packet
> address@hidden:colo_compare_main : packet same and release 
> packet
> *** glibc detected *** 
> /home/zhangchen/qemu-colo-apr14/x86_64-softmmu/qemu-system-x86_64: 
> double free or corruption (out): 0x0000555556a75210 ***
> ======= Backtrace: =========
> /lib64/libc.so.6(+0x76628)[0x7ffff53d6628]
> /lib64/libc.so.6(cfree+0x6c)[0x7ffff53db5cc]
>
>
> Thanks
> Zhang Chen

I agree that you should check whether or not they are running in the 
same thread.

Thanks

> > > Signed-off-by: Zhang Chen <address@hidden>
> > > ---
> > >   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);
> >
> >
> >
> >
> > .