Re: [Qemu-ppc] [QEMU PATCH v5 4/6] migration: migrate QTAILQ

[Paolo Bonzini]

On 05/10/2016 18:56, 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. In our approach such a structure is tagged
>> with VMS_LINKED. We then modified vmstate_save_state and vmstate_load_state
>> so that when VMS_LINKED is encountered, put and get from VMStateInfo are
>> called respectively. 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.
> 
> I think we're going to need a way to have a more flexible
> loops; and thus my choice here wouldn't be to use the .get/.put together
> with the VMSD; but I think we'll end up needing a new
> data structure, maybe a VMStateLoop *loop in VMStateField.

Or just realize that we already have a Turing-complete programming
language at our disposal, and it's called C. :)

> Specifically, your format of QTAILQ is perfectly reasonable - a
> byte before each entry which is 1 to indicate there's an entry or 0
> to indicate termination, but there are lots of other variants, e.g.
> 
>    a) put_scsi_requests uses that byte to hold a flag, so it's 0,1,2
>       0 still means terminate but 1 or 2 set a flag in the structure.

Seriously, the way I'd implement (a) is to make the QTAILQ reader peek
for the next byte.  If 0, eat it and exit the loop.  Otherwise, go ahead
with vmstate_load_state which will parse the byte again.

Likewise, on saving let the VMState write a non-zero byte at the
beginning, and then write a zero at the end.

Yes, it's sickening but that's what you do to honor backwards compatibility.

The other possibility is just to bump the version and make the SCSI
request flag a separate byte after the "is there another entry" byte.

Paolo

>    b) slirp_state_load also uses a null byte termination but not off a QTAILQ
>       (although I think it could be flipped for one) (it uses '42' for the
>       non-0 value, but looks like it could become 1)
> 
>    c) virtio_blk also rolls it's own linked list but again with the 0/1 byte
> 
>   Now how would I modify your QTAILQ load/store to do (a) without copying the 
> whole
> thing?
> 
> Dave
> 
>>
>> Signed-off-by: Jianjun Duan <address@hidden>
>> ---
>>  include/migration/vmstate.h | 26 ++++++++++++++++++
>>  include/qemu/queue.h        | 32 ++++++++++++++++++++++
>>  migration/trace-events      |  4 +++
>>  migration/vmstate.c         | 66 
>> +++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 128 insertions(+)
>>
>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>> index 459dd4a..e60c994 100644
>> --- a/include/migration/vmstate.h
>> +++ b/include/migration/vmstate.h
>> @@ -186,6 +186,12 @@ 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.
>> +     * When this flag is set in VMStateField, info->get/put will
>> +     * be used in vmstate_load/save_state instead of recursive call.
>> +     * User should implement set info to handle the concerned data 
>> structure.
>> +     */
>> +    VMS_LINKED            = 0x8000,
>>  };
>>  
>>  struct VMStateField {
>> @@ -246,6 +252,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)
>> @@ -657,6 +664,25 @@ extern const VMStateInfo vmstate_info_bitmap;
>>      .offset       = offsetof(_state, _field),                        \
>>  }
>>  
>> +/* For QTAILQ that need customized handling
>> + * _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,                                \
>> +    .flags        = VMS_LINKED,                                          \
>> +    .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..12c3f80 100644
>> --- a/include/qemu/queue.h
>> +++ b/include/qemu/queue.h
>> @@ -438,4 +438,36 @@ struct {                                                
>>                 \
>>  #define QTAILQ_PREV(elm, headname, field) \
>>          (*(((struct headname *)((elm)->field.tqe_prev))->tqh_last))
>>  
>> +/*
>> + * Offsets of layout of a tail queue head.
>> + */
>> +#define QTAILQ_FIRST_OFFSET 0
>> +#define QTAILQ_LAST_OFFSET (sizeof(void *))
>> +
>> +/*
>> + * Offsets of layout of a tail queue element.
>> + */
>> +#define QTAILQ_NEXT_OFFSET 0
>> +#define QTAILQ_PREV_OFFSET (sizeof(void *))
>> +
>> +/*
>> + * Tail queue tranversal using pointer arithmetic.
>> + */
>> +#define QTAILQ_RAW_FOREACH(elm, head, entry)                                
>>    \
>> +        for ((elm) = *((void **) ((char *) (head) + QTAILQ_FIRST_OFFSET));  
>>    \
>> +             (elm);                                                         
>>    \
>> +             (elm) =                                                        
>>    \
>> +                 *((void **) ((char *) (elm) + (entry) + 
>> QTAILQ_NEXT_OFFSET)))
>> +/*
>> + * Tail queue insertion using pointer arithmetic.
>> + */
>> +#define QTAILQ_RAW_INSERT_TAIL(head, elm, entry) do {                       
>>    \
>> +        *((void **) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET)) = 
>> NULL;   \
>> +        *((void **) ((char *) (elm) + (entry) + QTAILQ_PREV_OFFSET)) =      
>>    \
>> +            *((void **) ((char *) (head) +QTAILQ_LAST_OFFSET));             
>>    \
>> +        **((void ***)((char *) (head) +QTAILQ_LAST_OFFSET)) = (elm);        
>>    \
>> +        *((void **) ((char *) (head) + QTAILQ_LAST_OFFSET)) =               
>>    \
>> +            (void *) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET);       
>>    \
>> +} 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 66802cb..192db8a 100644
>> --- a/migration/vmstate.c
>> +++ b/migration/vmstate.c
>> @@ -5,7 +5,9 @@
>>  #include "migration/vmstate.h"
>>  #include "qemu/bitops.h"
>>  #include "qemu/error-report.h"
>> +#include "qemu/queue.h"
>>  #include "trace.h"
>> +#include "migration/qjson.h"
>>  
>>  static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription 
>> *vmsd,
>>                                      void *opaque, QJSON *vmdesc);
>> @@ -121,6 +123,8 @@ int vmstate_load_state(QEMUFile *f, const 
>> VMStateDescription *vmsd,
>>                  if (field->flags & VMS_STRUCT) {
>>                      ret = vmstate_load_state(f, field->vmsd, addr,
>>                                               field->vmsd->version_id);
>> +                } else if (field->flags & VMS_LINKED) {
>> +                    ret = field->info->get(f, addr, size, field);
>>                  } else {
>>                      ret = field->info->get(f, addr, size, NULL);
>>  
>> @@ -193,6 +197,8 @@ static const char *vmfield_get_type_name(VMStateField 
>> *field)
>>  
>>      if (field->flags & VMS_STRUCT) {
>>          type = "struct";
>> +    } else if (field->flags & VMS_LINKED) {
>> +        type = "linked";
>>      } else if (field->info->name) {
>>          type = field->info->name;
>>      }
>> @@ -327,6 +333,8 @@ void vmstate_save_state(QEMUFile *f, const 
>> VMStateDescription *vmsd,
>>                  }
>>                  if (field->flags & VMS_STRUCT) {
>>                      vmstate_save_state(f, field->vmsd, addr, vmdesc_loop);
>> +                } else if  (field->flags & VMS_LINKED) {
>> +                    field->info->put(f, addr, size, field, vmdesc_loop);
>>                  } else {
>>                      field->info->put(f, addr, size, NULL, NULL);
>>                  }
>> @@ -939,3 +947,61 @@ const VMStateInfo vmstate_info_bitmap = {
>>      .get = get_bitmap,
>>      .put = put_bitmap,
>>  };
>> +
>> +/*get for QTAILQ */
>> +static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size,
>> +                      VMStateField *field)
>> +{
>> +    int ret = 0;
>> +    const VMStateDescription *vmsd = field->vmsd;
>> +    size_t size = field->size;
>> +    size_t entry = field->start;
>> +    int version_id = field->version_id;
>> +    void *elm;
>> +
>> +    trace_get_qtailq(vmsd->name, version_id);
>> +    if (version_id > vmsd->version_id) {
>> +        trace_get_qtailq_end(vmsd->name, "too new", -EINVAL);
> 
> Can you make those error_report's please - if it fails we want to
> see why in the log.
> 
> Dave
> 
>> +        return -EINVAL;
>> +    }
>> +    if (version_id < vmsd->minimum_version_id) {
>> +        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;
>> +        }
>> +        QTAILQ_RAW_INSERT_TAIL(pv, elm, entry);
>> +    }
>> +
>> +    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;
>> +    size_t entry = field->start;
>> +    void *elm;
>> +
>> +    trace_put_qtailq(vmsd->name, vmsd->version_id);
>> +
>> +    QTAILQ_RAW_FOREACH(elm, pv, entry) {
>> +        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,
>> +};
>> -- 
>> 1.9.1
>>
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK
> 
>