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

[Jason Wang <jasowang@redhat.com>]

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?

Thanks

>
> Thanks,
> Hailiang
>
>
>> Thanks
>>
>>>
>>> Thanks.
>>> Hailiang
>>>
>>>> Thanks
>>>>
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Hailiang
>>>>>
>>>>>> Thought?
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> .
>>>>>>
>>>>>
>>>>
>>>>
>>>> .
>>>>
>>>
>>
>>
>> .
>>
>