Re: [Qemu-devel] [Qemu-ppc] [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ

[Jianjun Duan <duanj@linux.vnet.ibm.com>]

On 06/07/2016 07:43 AM, Dr. David Alan Gilbert wrote:
> * Jianjun Duan (duanj@linux.vnet.ibm.com) wrote:
>> Hi Sascha,
>>
>> On 06/02/2016 08:01 AM, Sascha Silbe wrote:
>>> Dear Jianjun,
>>>
>>> Jianjun Duan <duanj@linux.vnet.ibm.com> writes:
>>>
>>> [include/migration/vmstate.h]
>>>> @@ -185,6 +185,8 @@ enum VMStateFlags {
>>>>       * to determine the number of entries in the array. Only valid in
>>>>       * combination with one of VMS_VARRAY*. */
>>>>      VMS_MULTIPLY_ELEMENTS = 0x4000,
>>>> +    /* For fields which need customized handling, such as QTAILQ in queue.h*/
>>>> +    VMS_CSTM            = 0x8000,
>>>
>>> Can you describe (in the comment) how this customised handling is
>>> performed, please? I.e. describe what exactly happens if the flag is
>>> set, from the point of view of an API consumer.
>>
>> I will add more comments. When this flag is set VMStateInfo.get/put will
>> be used in vmstate_load/save_state instead of recursive call. And the
>> user should implement VMStateInfo.get/put to handle the concerned data
>> structure.
>>> Also, why do you need this flag at all? The only change I can see is
>>> that you pass additional information to VMStateInfo.get() / .put(),
>>> using NULL if it's not set. Why don't you just always pass the
>>> additional information? If the additional information is not needed by
>>> get() / put() the parameter will be unused anyway.
>> You can do it without creating this flag. Instead just to check if info
>> is set in the field. However I think it is more readable and more robust
>> to check this flag in vmstate_load/get_state.
> 
> Apologies for not getting around to this sooner; I was on holiday when you first
> sent it.
> 
> But:
>   a) Is there a reason to use the 'bool' between each element; can't you count the
>      length of the queue, send the length and then send the contents?  In that case
>      it should look a lot more like an ARRAY case on the wire.
>   b) I think you should really try and split it into two parts:
>     1) A VMSTATE_ARRAY_CUSTOM (?) - so it's an array of elements but we've got a special
>        way of allocating/counting/reading the elements
>     2) A version of that which is for a QTAILQ.
>        It's important that whatever ends up on the migration stream doesn't reflect
>        that it happens to be implemented as a QTAILQ; so if you decide to change
>        it later the migration compatibility doesn't break.

There are certainly more than one way to do this. I have thought about
the way you suggested and decided not to do it that way. We either need
to track its size, which means new element in the structure containing
the queue, or go through the queue and count it. I don't want to add an
element unless it is necessary. Counting and dumping the queue will go
through the queue twice. Current solution only goes through the queue
once. Besides, the queue is recursive and current solution is a natural
one.
>   c) Use new trace_ names for get_qtailq rather than misusing trace_vmstate_load_state*
>   d) Add a trace_ for put_qtailq as well - that way when it goes horribly wrong we can
>      turn the tracing on on both sides :-)
>   e) Is there anyway to make QTAILQ_RAW_INSERT_TAIL any more readable?
>      I don't think I understand why you can't use the standard QTAILQ macros.
I agree with this one. Need to make the trace more specific.
>   f) You might need to fix up Amit's scripts/vmstate-static-checker.py
Will check it.
> 
> Dave
> 
>>> Sascha
>>>
>>
>> Thanks,
>> Jianjun
>>
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

Thanks,
Jianjun