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

[Jianjun Duan]

I will try to explain my design rationale in details here.

1 QTAILQ should only be accessed using the interfaces defined in
queue.h. Its structs should not be directly used. So I created
interfaces in queue.h to query about its layout. If the implementation
is changed, these interfaces should be changed accordingly. Code using
these interfaces should not break.

2 Based on point 1, vmstate_load_state/vmstate_put_state in vmstate.c
doesn't exactly know the structs of QTAILAQ head and entry. So pointer
arithmetic is needed to put/get a QTAILQ. To do it, we need those 6
parameters to be passed in. So it is not redundant if we only want to
only use the interfaces.

3 At this moment, vmstate_load_state/vmstate_put_state couldn't handle a
queue, or a list, or another recursive structure. To make it
extensible, I think a metadata is needed. The idea is for any
structure which needs special handling, customized metadata/put/get
should provide enough flexibility to hack around.

There are two issues I tried to address. One is the recursive queue,
another is working around of hiding of the QTAILQ implementation
details. It seems we need to agree on how the latter should be
addressed.

I will give it a try to fix those 35 calling sites of VMStateInfo.

Thanks,
Jianjun

On 05/25/2016 10:51 AM, Paolo Bonzini wrote:
>>>> +    /*
>>>> +     * Following 3 fields are for VMStateField which needs customized
>>>> handling,
>>>> +     * such as QTAILQ in qemu/queue.h, lists, and tree.
>>>> +     */
>>>> +    const void *meta_data;
>>>> +    int (*extend_get)(QEMUFile *f, const void *metadata, void *opaque);
>>>> +    void (*extend_put)(QEMUFile *f, const void *metadata, void *opaque,
>>>> +                       QJSON *vmdesc);
>>>>  } VMStateField;
>>>
>>> Do not add these two function pointers to VMStateField, instead add
>>> QJSON* and VMStateField* arguments as needed to VMStateInfo's get and put.
>>
>> That is definitely the ideal way. However, VMStateInfo's get/put are
>> already used extensively. If I change them, I need change all the
>> calling sites of them. Not very sure about whether it will be welcomed.
> 
> Sure, don't be worried. :)  True, there are quite a few VMStateInfos (about
> 35) but the changes are simple.
> 
>>>> +#define VMSTATE_QTAILQ_METADATA(_field, _state, _type, _next, _vmsd) { \
>>>> +    .first = QTAILQ_FIRST_OFFSET(typeof_field(_state, _field)),        \
>>>> +    .last =  QTAILQ_LAST_OFFSET(typeof_field(_state, _field)),         \
>>>> +    .next = QTAILQ_NEXT_OFFSET(_type, _next),                          \
>>>> +    .prev = QTAILQ_PREV_OFFSET(_type, _next),                          \
>>>> +    .entry = offsetof(_type, _next),                                   \
>>>> +    .size = sizeof(_type),                                             \
>>>> +    .vmsd = &(_vmsd),                                                  \
>>>> +}
>>>
>>> .last and .prev are unnecessary, since they come just after .first and
>>> .next (and their use is hidden behind QTAILQ_RAW_*).  .first and .next
>                                                           ^^^^^^^^^^^^^^^^
> 
> Actually, .first and .next are always 0.  I misread your changes to 
> qemu/queue.h.
> See below.
> 
>>> can be placed in .offset and .num_offset respectively.  So you don't
>>> really need an extra metadata struct.
>>>
>>> If you prefer you could have something like
>>>
>>>     union {
>>>         size_t num_offset;    /* VMS_VARRAY */
>>>         size_t size_offset;   /* VMS_VBUFFER */
>>>         size_t next_offset;   /* VMS_TAILQ */
>>>     } offsets;
>>
>> Actually I explored the approach in which I use a VMSD to encode all the
>> information. But a VMSD describes actual memory layout. Interpreting it
>> another way could be confusing.
>>
>> One of the assumption about QTAILQ is that we can only use the
>> interfaces defined in queue.h to access it. I intend not to depend on
>> its actually layout. From this perspective, these 6 parameters are
>> needed.
> 
> You are already adding QTAILQ_RAW_*, aren't you?  Those interfaces
> would need to know about the layout, and you are passing redundant
> information:
> 
> - .next_offset should always be 0
> - .prev_offset should always be sizeof(void *)
> - .first_offset should always be 0
> - .last_offset should always be sizeof(void *)
> 
> so you only need head and entry, which you can store in .offset and
> .num_offset.  The .vmsd field you have in ->metadata can be stored
> in VMStateField's .vmsd, and likewise for .size (which can be stored
> in VMStateField's .vmsd).
> 
> Thanks,
> 
> Paolo
>