Re: [Qemu-ppc] [QEMU PATCH v8 2/3] migration: migrate QTAILQ

[Jianjun Duan]

On 10/26/2016 10:09 AM, Dr. David Alan Gilbert wrote:
> * Jianjun Duan (address@hidden) wrote:
>>
>>
>> On 10/26/2016 09:53 AM, Dr. David Alan Gilbert wrote:
>>> * Jianjun Duan (address@hidden) wrote:
>>>>
>>>>
>>>> On 10/26/2016 09:29 AM, Dr. David Alan Gilbert wrote:
>>>>> * Jianjun Duan (address@hidden) wrote:
>>>>>> Currently we cannot directly transfer a QTAILQ instance because of the
>>>>>> limitation in the migration code. Here we introduce an approach to
>>>>>> transfer such structures. We created VMStateInfo vmstate_info_qtailq
>>>>>> for QTAILQ. Similar VMStateInfo can be created for other data structures
>>>>>> such as list.
>>>>>>
>>>>>> This approach will be used to transfer pending_events and ccs_list in 
>>>>>> spapr
>>>>>> state.
>>>>>>
>>>>>> We also create some macros in qemu/queue.h to access a QTAILQ using 
>>>>>> pointer
>>>>>> arithmetic. This ensures that we do not depend on the implementation
>>>>>> details about QTAILQ in the migration code.
>>>>>>
>>>>>> Signed-off-by: Jianjun Duan <address@hidden>
>>>>>> ---
>>>>>>  include/migration/vmstate.h | 20 ++++++++++++++
>>>>>>  include/qemu/queue.h        | 46 +++++++++++++++++++++++++++++++
>>>>>>  migration/trace-events      |  4 +++
>>>>>>  migration/vmstate.c         | 67 
>>>>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>>>>  4 files changed, 137 insertions(+)
>>>>>>
>>>>>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>>>>>> index d0e37b5..318a6f1 100644
>>>>>> --- a/include/migration/vmstate.h
>>>>>> +++ b/include/migration/vmstate.h
>>>>>> @@ -251,6 +251,7 @@ extern const VMStateInfo vmstate_info_timer;
>>>>>>  extern const VMStateInfo vmstate_info_buffer;
>>>>>>  extern const VMStateInfo vmstate_info_unused_buffer;
>>>>>>  extern const VMStateInfo vmstate_info_bitmap;
>>>>>> +extern const VMStateInfo vmstate_info_qtailq;
>>>>>>  
>>>>>>  #define type_check_2darray(t1,t2,n,m) ((t1(*)[n][m])0 - (t2*)0)
>>>>>>  #define type_check_array(t1,t2,n) ((t1(*)[n])0 - (t2*)0)
>>>>>> @@ -662,6 +663,25 @@ extern const VMStateInfo vmstate_info_bitmap;
>>>>>>      .offset       = offsetof(_state, _field),                        \
>>>>>>  }
>>>>>>  
>>>>>> +/* For QTAILQ that need customized handling.
>>>>>> + * Target QTAILQ needs be properly initialized.
>>>>>> + * _type: type of QTAILQ element
>>>>>> + * _next: name of QTAILQ entry field in QTAILQ element
>>>>>> + * _vmsd: VMSD for QTAILQ element
>>>>>> + * size: size of QTAILQ element
>>>>>> + * start: offset of QTAILQ entry in QTAILQ element
>>>>>> + */
>>>>>> +#define VMSTATE_QTAILQ_V(_field, _state, _version, _vmsd, _type, _next) 
>>>>>>  \
>>>>>> +{                                                                       
>>>>>>  \
>>>>>> +    .name         = (stringify(_field)),                                
>>>>>>  \
>>>>>> +    .version_id   = (_version),                                         
>>>>>>  \
>>>>>> +    .vmsd         = &(_vmsd),                                           
>>>>>>  \
>>>>>> +    .size         = sizeof(_type),                                      
>>>>>>  \
>>>>>> +    .info         = &vmstate_info_qtailq,                               
>>>>>>  \
>>>>>> +    .offset       = offsetof(_state, _field),                           
>>>>>>  \
>>>>>> +    .start        = offsetof(_type, _next),                             
>>>>>>  \
>>>>>> +}
>>>>>> +
>>>>>>  /* _f : field name
>>>>>>     _f_n : num of elements field_name
>>>>>>     _n : num of elements
>>>>>> diff --git a/include/qemu/queue.h b/include/qemu/queue.h
>>>>>> index 342073f..e9378fa 100644
>>>>>> --- a/include/qemu/queue.h
>>>>>> +++ b/include/qemu/queue.h
>>>>>> @@ -438,4 +438,50 @@ struct {                                            
>>>>>>                     \
>>>>>>  #define QTAILQ_PREV(elm, headname, field) \
>>>>>>          (*(((struct headname *)((elm)->field.tqe_prev))->tqh_last))
>>>>>>  
>>>>>> +#define RAW_FIELD(base, offset)                                         
>>>>>>        \
>>>>>> +        ((char *) (base) + offset)
>>>>>> +
>>>>>> +/*
>>>>>> + * Offsets of layout of a tail queue head.
>>>>>> + */
>>>>>> +#define QTAILQ_FIRST_OFFSET 0
>>>>>> +#define QTAILQ_LAST_OFFSET (sizeof(void *))
>>>>>
>>>>> OK, you might want to add some asserts somewhere in a .c just to catch if
>>>>> any of these offsets change.
>>>>>
>>>> if the layout of QTAILQ changes, the version id for the vmsd should also
>>>> be changed. It will break migration between different versions. However
>>>> the operation doesn't depend on these values.
>>>
>>> No, changing layout of QTAILQ doesn't need to change the version id of vmsd;
>>> it's an internal change.  But if someone does make the change and forgets
>>> to update your OFFSET macros it'll cause memory corruption.
>>> You could catch that with an assert (possibly build time).
>>>
>> If we use these constant I would agree an assertion is necessary. By
>> using a macro we leave the door open for change. and if someone changes
>> the layout, he or she better change the macros and the version id. If an
>> assertion is added, then that assertion needs to be changed to reflect
>> the change, then in the unlikely situations we could have several
>> version of layout/macro/assertions floating around. That is too much. SO
>> version id is the best guard here.
> 
> Version id's are irrelevant here.
> The macros are irrelevant here.
> I'm talking about a potential mismatch between the definition of 
> QTAILQ_LAST_OFFSET
> and the definition of Q_TAILQ_HEAD.
> 
> Dave
> 
I suppose anyone who changes the layout should also change the macro and
version ID of the relevant vmsd. Similar issue was discussed before as
the early version tried to generate all these offsets based on the
element and head type. You can see in version 2 discussion.


Thanks,
Jianjun
>>
>> Thanks,
>> Jianjun
>>
>>>>>> + * Raw access of elements of a tail queue
>>>>>> + */
>>>>>> +#define QTAILQ_RAW_FIRST(head)                                          
>>>>>>        \
>>>>>> +        (*((void **) (RAW_FIELD(head,  QTAILQ_FIRST_OFFSET))))
>>>>>> +#define QTAILQ_RAW_LAST(head)                                           
>>>>>>        \
>>>>>> +        (*((void ***) (RAW_FIELD(head,  QTAILQ_LAST_OFFSET))))
>>>>>> +
>>>>>> +/*
>>>>>> + * Offsets of layout of a tail queue element.
>>>>>> + */
>>>>>> +#define QTAILQ_NEXT_OFFSET 0
>>>>>> +#define QTAILQ_PREV_OFFSET (sizeof(void *))
>>>>>> +
>>>>>> +/*
>>>>>> + * Raw access of elements of a tail entry
>>>>>> + */
>>>>>> +#define QTAILQ_RAW_NEXT(elm, entry)                                     
>>>>>>        \
>>>>>> +        (*((void **) (RAW_FIELD(elm, entry + QTAILQ_NEXT_OFFSET))))
>>>>>> +#define QTAILQ_RAW_PREV(elm, entry)                                     
>>>>>>        \
>>>>>> +        (*((void ***) (RAW_FIELD(elm, entry + QTAILQ_PREV_OFFSET))))
>>>>>> +/*
>>>>>> + * Tail queue tranversal using pointer arithmetic.
>>>>>> + */
>>>>>> +#define QTAILQ_RAW_FOREACH(elm, head, entry)                            
>>>>>>        \
>>>>>> +        for ((elm) = QTAILQ_RAW_FIRST(head);                            
>>>>>>        \
>>>>>> +             (elm);                                                     
>>>>>>        \
>>>>>> +             (elm) = QTAILQ_RAW_NEXT(elm, entry))
>>>>>> +/*
>>>>>> + * Tail queue insertion using pointer arithmetic.
>>>>>> + */
>>>>>> +#define QTAILQ_RAW_INSERT_TAIL(head, elm, entry) do {                   
>>>>>>        \
>>>>>> +        QTAILQ_RAW_NEXT(elm, entry) = NULL;                             
>>>>>>        \
>>>>>> +        QTAILQ_RAW_PREV(elm, entry) = QTAILQ_RAW_LAST(head);            
>>>>>>        \
>>>>>> +        *QTAILQ_RAW_LAST(head) = (elm);                                 
>>>>>>        \
>>>>>> +        QTAILQ_RAW_LAST(head) = &QTAILQ_RAW_NEXT(elm, entry);           
>>>>>>        \
>>>>>> +} while (/*CONSTCOND*/0)
>>>>>> +
>>>>>>  #endif /* QEMU_SYS_QUEUE_H */
>>>>>> diff --git a/migration/trace-events b/migration/trace-events
>>>>>> index dfee75a..9a6ec59 100644
>>>>>> --- a/migration/trace-events
>>>>>> +++ b/migration/trace-events
>>>>>> @@ -52,6 +52,10 @@ vmstate_n_elems(const char *name, int n_elems) "%s: 
>>>>>> %d"
>>>>>>  vmstate_subsection_load(const char *parent) "%s"
>>>>>>  vmstate_subsection_load_bad(const char *parent,  const char *sub, const 
>>>>>> char *sub2) "%s: %s/%s"
>>>>>>  vmstate_subsection_load_good(const char *parent) "%s"
>>>>>> +get_qtailq(const char *name, int version_id) "%s v%d"
>>>>>> +get_qtailq_end(const char *name, const char *reason, int val) "%s %s/%d"
>>>>>> +put_qtailq(const char *name, int version_id) "%s v%d"
>>>>>> +put_qtailq_end(const char *name, const char *reason) "%s %s"
>>>>>>  
>>>>>>  # migration/qemu-file.c
>>>>>>  qemu_file_fclose(void) ""
>>>>>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>>>>>> index d188afa..fcf808e 100644
>>>>>> --- a/migration/vmstate.c
>>>>>> +++ b/migration/vmstate.c
>>>>>> @@ -5,6 +5,7 @@
>>>>>>  #include "migration/vmstate.h"
>>>>>>  #include "qemu/bitops.h"
>>>>>>  #include "qemu/error-report.h"
>>>>>> +#include "qemu/queue.h"
>>>>>>  #include "trace.h"
>>>>>>  #include "migration/qjson.h"
>>>>>>  
>>>>>> @@ -948,3 +949,69 @@ const VMStateInfo vmstate_info_bitmap = {
>>>>>>      .get = get_bitmap,
>>>>>>      .put = put_bitmap,
>>>>>>  };
>>>>>> +
>>>>>> +/* get for QTAILQ
>>>>>> + * meta data about the QTAILQ is encoded in a VMStateField structure
>>>>>> + */
>>>>>> +static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size,
>>>>>> +                      VMStateField *field)
>>>>>> +{
>>>>>> +    int ret = 0;
>>>>>> +    const VMStateDescription *vmsd = field->vmsd;
>>>>>> +    /* size of a QTAILQ element */
>>>>>> +    size_t size = field->size;
>>>>>> +    /* offset of the QTAILQ entry in a QTAILQ element */
>>>>>> +    size_t entry_offset = field->start;
>>>>>> +    int version_id = field->version_id;
>>>>>> +    void *elm;
>>>>>> +
>>>>>> +    trace_get_qtailq(vmsd->name, version_id);
>>>>>> +    if (version_id > vmsd->version_id) {
>>>>>> +        error_report("%s %s",  vmsd->name, "too new");
>>>>>> +        trace_get_qtailq_end(vmsd->name, "too new", -EINVAL);
>>>>>> +
>>>>>> +        return -EINVAL;
>>>>>> +    }
>>>>>> +    if (version_id < vmsd->minimum_version_id) {
>>>>>> +        error_report("%s %s",  vmsd->name, "too old");
>>>>>> +        trace_get_qtailq_end(vmsd->name, "too old", -EINVAL);
>>>>>> +        return -EINVAL;
>>>>>> +    }
>>>>>> +
>>>>>> +    while (qemu_get_byte(f)) {
>>>>>> +        elm =  g_malloc(size);
>>>>>> +        ret = vmstate_load_state(f, vmsd, elm, version_id);
>>>>>> +        if (ret) {
>>>>>> +            return ret;
>>>>>
>>>>> You could just make that a break since you're returning ret
>>>>> (that's otherwise all 0).  Still, not important, but could
>>>>> be tidied if you need to regenerate the patch.
>>>>> (and you could remove the 2nd space after elm =).
>>>
>>>> By returning it, the trace file will miss "get_qtailq_end', that
>>>> is useful debug information.
>>>> You are right about the space.
>>>
>>> But that's what you're doing now, you're missing the trace call
>>> and returning 'ret' that will always be 0 if it gets there.
>>>
>>> Dave
>>>
>>>>
>>>> Thanks,
>>>> Jianjun
>>>>
>>>>>
>>>>>> +        }
>>>>>> +        QTAILQ_RAW_INSERT_TAIL(pv, elm, entry_offset);
>>>>>> +    }
>>>>>> +
>>>>>> +    trace_get_qtailq_end(vmsd->name, "end", ret);
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>> +/* put for QTAILQ */
>>>>>> +static void put_qtailq(QEMUFile *f, void *pv, size_t unused_size,
>>>>>> +                       VMStateField *field, QJSON *vmdesc)
>>>>>> +{
>>>>>> +    const VMStateDescription *vmsd = field->vmsd;
>>>>>> +    /* offset of the QTAILQ entry in a QTAILQ element*/
>>>>>> +    size_t entry_offset = field->start;
>>>>>> +    void *elm;
>>>>>> +
>>>>>> +    trace_put_qtailq(vmsd->name, vmsd->version_id);
>>>>>> +
>>>>>> +    QTAILQ_RAW_FOREACH(elm, pv, entry_offset) {
>>>>>> +        qemu_put_byte(f, true);
>>>>>> +        vmstate_save_state(f, vmsd, elm, vmdesc);
>>>>>> +    }
>>>>>> +    qemu_put_byte(f, false);
>>>>>> +
>>>>>> +    trace_put_qtailq_end(vmsd->name, "end");
>>>>>> +}
>>>>>> +const VMStateInfo vmstate_info_qtailq = {
>>>>>> +    .name = "qtailq",
>>>>>> +    .get  = get_qtailq,
>>>>>> +    .put  = put_qtailq,
>>>>>> +};
>>>>>
>>>>> Reviewed-by: Dr. David Alan Gilbert <address@hidden>
>>>>>
>>>>>> -- 
>>>>>> 1.9.1
>>>>>>
>>>>> --
>>>>> Dr. David Alan Gilbert / address@hidden / Manchester, UK
>>>>>
>>>>
>>> --
>>> Dr. David Alan Gilbert / address@hidden / Manchester, UK
>>>
>>
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK
>