Re: [Qemu-devel] [very-WIP 1/7] migration: Add VMSTATE_WITH_TMP

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Jianjun Duan (duanj@linux.vnet.ibm.com) wrote:
> 
> 
> On 10/17/2016 11:52 AM, Dr. David Alan Gilbert wrote:
> > * Jianjun Duan (duanj@linux.vnet.ibm.com) wrote:
> >>
> >>
> >> On 10/16/2016 08:31 PM, David Gibson wrote:
> >>> On Tue, Oct 11, 2016 at 06:18:30PM +0100, Dr. David Alan Gilbert (git) wrote:
> >>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >>>>
> >>>> VMSTATE_WITH_TMP is for handling structures where some calculation
> >>>> or rearrangement of the data needs to be performed before the data
> >>>> hits the wire.
> >>>> For example,  where the value on the wire is an offset from a
> >>>> non-migrated base, but the data in the structure is the actual pointer.
> >>>>
> >>>> To use it, a temporary type is created and a vmsd used on that type.
> >>>> The first element of the type must be 'parent' a pointer back to the
> >>>> type of the main structure.  VMSTATE_WITH_TMP takes care of allocating
> >>>> and freeing the temporary before running the child vmsd.
> >>>>
> >>>> The post_load/pre_save on the child vmsd can copy things from the parent
> >>>> to the temporary using the parent pointer and do any other calculations
> >>>> needed; it can then use normal VMSD entries to do the actual data
> >>>> storage without having to fiddle around with qemu_get_*/qemu_put_*
> >>>>
> >> If customized put/get can do transformation and dumping/loading data
> >> to/from the parent structure, you don't have to go through
> >> pre_save/post_load, and may get rid of parent pointer.
> > 
> > Yes but I'd rather try and get rid of the customized put/get from
> > every device, because then people start using qemu_put/qemu_get in them all.
> > 
> Then customized handling need to happen in pre_save/post_load. I think
> you need a way to pass TMP pointer around?

But then why is that better than having the parent pointer?

Dave

> 
> Thanks,
> Jianjun
> > Dave
> > 
> >>
> >> Thanks,
> >> Jianjun
> >>
> >>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >>>
> >>> The requirement for the parent pointer is a little clunky, but I don't
> >>> quickly see a better way, and it is compile-time verified.  As noted
> >>> elsewhere I think this is a really useful approach which could allow a
> >>> bunch of internal state cleanups while preserving migration.
> >>>
> >>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> >>>
> >>>> ---
> >>>>  include/migration/vmstate.h | 20 ++++++++++++++++++++
> >>>>  migration/vmstate.c         | 38 ++++++++++++++++++++++++++++++++++++++
> >>>>  2 files changed, 58 insertions(+)
> >>>>
> >>>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> >>>> index 9500da1..efb0e90 100644
> >>>> --- a/include/migration/vmstate.h
> >>>> +++ b/include/migration/vmstate.h
> >>>> @@ -259,6 +259,7 @@ extern const VMStateInfo vmstate_info_cpudouble;
> >>>>  extern const VMStateInfo vmstate_info_timer;
> >>>>  extern const VMStateInfo vmstate_info_buffer;
> >>>>  extern const VMStateInfo vmstate_info_unused_buffer;
> >>>> +extern const VMStateInfo vmstate_info_tmp;
> >>>>  extern const VMStateInfo vmstate_info_bitmap;
> >>>>  extern const VMStateInfo vmstate_info_qtailq;
> >>>>  
> >>>> @@ -651,6 +652,25 @@ extern const VMStateInfo vmstate_info_qtailq;
> >>>>      .offset     = offsetof(_state, _field),                          \
> >>>>  }
> >>>>  
> >>>> +/* Allocate a temporary of type 'tmp_type', set tmp->parent to _state
> >>>> + * and execute the vmsd on the temporary.  Note that we're working with
> >>>> + * the whole of _state here, not a field within it.
> >>>> + * We compile time check that:
> >>>> + *    That _tmp_type contains a 'parent' member that's a pointer to the
> >>>> + *        '_state' type
> >>>> + *    That the pointer is right at the start of _tmp_type.
> >>>> + */
> >>>> +#define VMSTATE_WITH_TMP(_state, _tmp_type, _vmsd) {                 \
> >>>> +    .name         = "tmp",                                           \
> >>>> +    .size         = sizeof(_tmp_type) +                              \
> >>>> +                    QEMU_BUILD_BUG_EXPR(offsetof(_tmp_type, parent) != 0) + \
> >>>> +                    type_check_pointer(_state,                       \
> >>>> +                        typeof_field(_tmp_type, parent)),            \
> >>>> +    .vmsd         = &(_vmsd),                                        \
> >>>> +    .info         = &vmstate_info_tmp,                               \
> >>>> +    .flags        = VMS_LINKED,                                      \
> >>>> +}
> >>>> +
> >>>>  #define VMSTATE_UNUSED_BUFFER(_test, _version, _size) {              \
> >>>>      .name         = "unused",                                        \
> >>>>      .field_exists = (_test),                                         \
> >>>> diff --git a/migration/vmstate.c b/migration/vmstate.c
> >>>> index 2157997..f2563c5 100644
> >>>> --- a/migration/vmstate.c
> >>>> +++ b/migration/vmstate.c
> >>>> @@ -925,6 +925,44 @@ const VMStateInfo vmstate_info_unused_buffer = {
> >>>>      .put  = put_unused_buffer,
> >>>>  };
> >>>>  
> >>>> +/* vmstate_info_tmp, see VMSTATE_WITH_TMP, the idea is that we allocate
> >>>> + * a temporary buffer and the pre_load/pre_save methods in the child vmsd
> >>>> + * copy stuff from the parent into the child and do calculations to fill
> >>>> + * in fields that don't really exist in the parent but need to be in the
> >>>> + * stream.
> >>>> + */
> >>>> +static int get_tmp(QEMUFile *f, void *pv, size_t size, VMStateField *field)
> >>>> +{
> >>>> +    int ret;
> >>>> +    const VMStateDescription *vmsd = field->vmsd;
> >>>> +    int version_id = field->version_id;
> >>>> +    void *tmp = g_malloc(size);
> >>>> +
> >>>> +    /* Writes the parent field which is at the start of the tmp */
> >>>> +    *(void **)tmp = pv;
> >>>> +    ret = vmstate_load_state(f, vmsd, tmp, version_id);
> >>>> +    g_free(tmp);
> >>>> +    return ret;
> >>>> +}
> >>>> +
> >>>> +static void put_tmp(QEMUFile *f, void *pv, size_t size, VMStateField *field,
> >>>> +                    QJSON *vmdesc)
> >>>> +{
> >>>> +    const VMStateDescription *vmsd = field->vmsd;
> >>>> +    void *tmp = g_malloc(size);
> >>>> +
> >>>> +    /* Writes the parent field which is at the start of the tmp */
> >>>> +    *(void **)tmp = pv;
> >>>> +    vmstate_save_state(f, vmsd, tmp, vmdesc);
> >>>> +    g_free(tmp);
> >>>> +}
> >>>> +
> >>>> +const VMStateInfo vmstate_info_tmp = {
> >>>> +    .name = "tmp",
> >>>> +    .get = get_tmp,
> >>>> +    .put = put_tmp,
> >>>> +};
> >>>> +
> >>>>  /* bitmaps (as defined by bitmap.h). Note that size here is the size
> >>>>   * of the bitmap in bits. The on-the-wire format of a bitmap is 64
> >>>>   * bit words with the bits in big endian order. The in-memory format
> >>>
> >>
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK