From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60876) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bwPPv-0000wr-Jl for qemu-devel@nongnu.org; Tue, 18 Oct 2016 04:06:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bwPPr-0000a6-Ff for qemu-devel@nongnu.org; Tue, 18 Oct 2016 04:06:35 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33172) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bwPPr-0000Zo-7k for qemu-devel@nongnu.org; Tue, 18 Oct 2016 04:06:31 -0400 Date: Tue, 18 Oct 2016 09:06:24 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20161018080623.GA2190@work-vm> References: <20161011171833.20803-1-dgilbert@redhat.com> <20161011171833.20803-2-dgilbert@redhat.com> <20161017033134.GM25390@umbus.fritz.box> <61d82189-401b-7063-ffa5-4442c7d11d25@linux.vnet.ibm.com> <20161017185243.GE12934@work-vm> <5a464924-aae1-1219-c30b-034d14a143ea@linux.vnet.ibm.com> <20161017191643.GH12934@work-vm> <8084dac0-fc0c-3c68-8683-118e003f6f3f@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8084dac0-fc0c-3c68-8683-118e003f6f3f@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [very-WIP 1/7] migration: Add VMSTATE_WITH_TMP List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jianjun Duan Cc: David Gibson , qemu-devel@nongnu.org, amit.shah@redhat.com, quintela@redhat.com, pasic@linux.vnet.ibm.com * Jianjun Duan (duanj@linux.vnet.ibm.com) wrote: > > > On 10/17/2016 12:16 PM, Dr. David Alan Gilbert wrote: > > * 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" > >>>>>> > >>>>>> 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? > > > IIUC, from the put_tmp, I didn't see how tmp is filled with data. I > suppose it is to be filled by pre_save. So tmp pointer needs to find a > way from inside pre_save to put_tmp. How does it happen? The tmp.parent pointer is filled by the '*(void **)tmp = pv;' in the put_tmp and get_tmp. Only after that is the child vmsd run and it's passed the tmp pointer; it's the child's pre_save/pre_load that has the chance to do whatever it likes to fill the tmp. Dave > Thanks, > Jianjun > > > > Dave > > > >> > >> Thanks, > >> Jianjun > >>> Dave > >>> > >>>> > >>>> Thanks, > >>>> Jianjun > >>>> > >>>>>> Signed-off-by: Dr. David Alan Gilbert > >>>>> > >>>>> 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 > >>>>> > >>>>>> --- > >>>>>> 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 > > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK