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

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

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 ?

Thanks,
Hailiang


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