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

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

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<zhang.zhanghailiang@huawei.com>
>>>>>>>>>
>>>>>>>>> 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.
>
>
> .
>