Re: [Qemu-devel] [PATCH 1/3] colo-compare: reconstruct the mutex lock usage

[Hailiang Zhang]

On 2017/2/7 17:21, Jason Wang wrote:
> On 2017年02月07日 16:19, Hailiang Zhang wrote:
> > On 2017/2/7 15:57, Jason Wang wrote:
> > > On 2017年02月07日 15:54, Hailiang Zhang wrote:
> > > > Hi Jason,
> > > >
> > > > On 2017/2/6 20:53, Jason Wang wrote:
> > > > > On 2017年02月06日 19:11, Hailiang Zhang wrote:
> > > > > > On 2017/2/6 17:35, Jason Wang wrote:
> > > > > > > On 2017年02月06日 16:13, Hailiang Zhang wrote:
> > > > > > > > On 2017/2/3 11:47, Jason Wang wrote:
> > > > > > > > > On 2017年01月24日 22:05, zhanghailiang wrote:
> > > > > > > > > > The original 'timer_check_lock' mutex lock of struct CompareState
> > > > > > > > > > is used to protect the 'conn_list' queue and its child queues
> > > > > > > > > > which
> > > > > > > > > > are 'primary_list' and 'secondary_list', which is a little abused
> > > > > > > > > > and confusing
> > > > > > > > > >
> > > > > > > > > > To make it clearer, we rename 'timer_check_lock' to
> > > > > > > > > > 'conn_list_lock'
> > > > > > > > > > which is used to protect 'conn_list' queue, use another
> > > > > > > > > > 'conn_lock'
> > > > > > > > > > to protect 'primary_list' and 'secondary_list'.
> > > > > > > > > >
> > > > > > > > > > Besides, fix some missing places which need these mutex lock.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: zhanghailiang<address@hidden>
> > > > > > > > >
> > > > > > > > > Instead of sticking to such kind of mutex, I think it's time to
> > > > > > > > > make
> > > > > > > > > colo timer run in colo thread (there's a TODO in the code).
> > > > > > > >
> > > > > > > > Er, it seems that, we still need these mutex locks even we make
> > > > > > > > colo
> > > > > > > > timer and colo thread run in the same thread, because we may access
> > > > > > > > the connect/primary/secondary list from colo (migratioin) thread
> > > > > > > > concurrently.
> > > > > > >
> > > > > > > Just make sure I understand the issue, why need it access the list?
> > > > > > >
> > > > > > > > Besides, it seems to be a little complex to make colo timer run in
> > > > > > > > colo
> > > > > > > > compare thread, and it is not this series' goal.
> > > > > > >
> > > > > > > Seems not by just looking at how it was implemented in main loop,
> > > > > > > but
> > > > > > > maybe I was wrong.
> > > > > > >
> > > > > > > > This series is preparing
> > > > > > > > work for integrating COLO compare with COLO frame and it is
> > > > > > > > prerequisite.
> > > > > > > >
> > > > > > > > So, we may consider implementing it later in another series.
> > > > > > > > Zhang Chen, what's your opinion ?
> > > > > > >
> > > > > > > The problem is this patch make things even worse, it introduces one
> > > > > > > more
> > > > > > > mutex.
> > > > > >
> > > > > > Hmm, for most cases, we need to get these two locks at the same time.
> > > > > > We can use one lock to protect conn_list/primary_list/secondary_list,
> > > > > > (We need to get this lock before operate all these lists, as you can
> > > > > > see
> > > > > > in patch 2, while do checkpoint, we may operate these lists in
> > > > > > COLO checkpoint thread concurrently.)
> > > > > >
> > > > > > But for the original codes, we didn't got timer_check_lock in
> > > > > > packet_enqueue() while operate conn_list/primary_list/secondary_list,
> > > > > > and didn't got this lock in colo_compare_connection while operate
> > > > > > secondary_list either.
> > > > > >
> > > > > > So, is it OK to use the conn_lock instead of timer_check_lock, and
> > > > > > add the lock where it is need ?
> > > > >
> > > > > I'd like to know if timer were run in colo thread (this looks as
> > > > > simple
> > > > > as a g_timeout_source_new() followed by a g_source_attach()), why
> > > > > do we
> > > > > still need a mutex. And if we need it now but plan to remove it in the
> > > > > future, I'd like to use big lock to simplify the codes.
> > > >
> > > > After investigated your above suggestion, I think it works by using
> > > > g_timeout_source_new() and g_source_attach(), but I'm not sure
> > > > if it is a good idea to use the big lock to protect the possible
> > > > concurrent cases which seem to only happen between COLO migration
> > > > thread and COLO compare thread, any further suggestions ?
> > >
> > > I think I need first understand why migration thread need to access the
> > > list?
> >
> > Er, sorry to confuse you here, to be exactly, it is COLO checkpoint
> > thread,
> > we reuse the migration thread to realize checkpoint process for COLO,
> > Because qemu enters into COLO state after a complete migration, so we
> > reuse it.
> >
> > While do checkpoint, we need to release all the buffered packets that
> > have not
> > yet been compared, so we need to access the list in COLO checkpoint
> > thread.

Hi Jason,

> I think the better way is notify the comparing thread and let it do the
> releasing. You probably need similar mechanism to notify from comparing
> thread to checkpoint thread.

It seems that there is no available APIs in glib to notify a thread which
are run coroutine to do something (idle source ?). What about using anonymous 
pipe
as the GPollFD to communicate between colo comparing thread and colo thread ?

Any ideas ?

Hailiang


> Thanks
>
> > Thanks.
>
>
> .