From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55180) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bAIE2-0000xi-GC for qemu-devel@nongnu.org; Tue, 07 Jun 2016 10:43:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bAIDy-0003Ve-C9 for qemu-devel@nongnu.org; Tue, 07 Jun 2016 10:43:25 -0400 Date: Tue, 7 Jun 2016 15:43:13 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20160607144313.GC2257@work-vm> References: <1464717764-9040-1-git-send-email-duanj@linux.vnet.ibm.com> <1464717764-9040-5-git-send-email-duanj@linux.vnet.ibm.com> <877fe7iqj3.fsf@oc4731375738.ibm.com> <57505AA6.6080201@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <57505AA6.6080201@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jianjun Duan Cc: Sascha Silbe , qemu-devel@nongnu.org, veroniabahaa@gmail.com, peter.maydell@linaro.org, quintela@redhat.com, mst@redhat.com, mark.cave-ayland@ilande.co.uk, mdroth@linux.vnet.ibm.com, mreitz@redhat.com, blauwirbel@gmail.com, dmitry@daynix.com, qemu-ppc@nongnu.org, kraxel@redhat.com, pbonzini@redhat.com, amit.shah@redhat.com, kwolf@redhat.com, david@gibson.dropbear.id.au, leon.alrae@imgtec.com, aurelien@aurel32.net, rth@twiddle.net * Jianjun Duan (duanj@linux.vnet.ibm.com) wrote: > Hi Sascha, > > On 06/02/2016 08:01 AM, Sascha Silbe wrote: > > Dear Jianjun, > > > > Jianjun Duan writes: > > > > [include/migration/vmstate.h] > >> @@ -185,6 +185,8 @@ 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*/ > >> + VMS_CSTM = 0x8000, > > > > Can you describe (in the comment) how this customised handling is > > performed, please? I.e. describe what exactly happens if the flag is > > set, from the point of view of an API consumer. > > I will add more comments. When this flag is set VMStateInfo.get/put will > be used in vmstate_load/save_state instead of recursive call. And the > user should implement VMStateInfo.get/put to handle the concerned data > structure. > > Also, why do you need this flag at all? The only change I can see is > > that you pass additional information to VMStateInfo.get() / .put(), > > using NULL if it's not set. Why don't you just always pass the > > additional information? If the additional information is not needed by > > get() / put() the parameter will be unused anyway. > You can do it without creating this flag. Instead just to check if info > is set in the field. However I think it is more readable and more robust > to check this flag in vmstate_load/get_state. Apologies for not getting around to this sooner; I was on holiday when you first sent it. But: a) Is there a reason to use the 'bool' between each element; can't you count the length of the queue, send the length and then send the contents? In that case it should look a lot more like an ARRAY case on the wire. b) I think you should really try and split it into two parts: 1) A VMSTATE_ARRAY_CUSTOM (?) - so it's an array of elements but we've got a special way of allocating/counting/reading the elements 2) A version of that which is for a QTAILQ. It's important that whatever ends up on the migration stream doesn't reflect that it happens to be implemented as a QTAILQ; so if you decide to change it later the migration compatibility doesn't break. c) Use new trace_ names for get_qtailq rather than misusing trace_vmstate_load_state* d) Add a trace_ for put_qtailq as well - that way when it goes horribly wrong we can turn the tracing on on both sides :-) e) Is there anyway to make QTAILQ_RAW_INSERT_TAIL any more readable? I don't think I understand why you can't use the standard QTAILQ macros. f) You might need to fix up Amit's scripts/vmstate-static-checker.py Dave > > Sascha > > > > Thanks, > Jianjun > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK