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

[Jason Wang <jasowang@redhat.com>]

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.

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.

Thanks

>
> Thanks.