From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v3 17/28] tools/libxl: Infrastructure for reading a libxl migration v2 stream Date: Mon, 13 Jul 2015 15:53:18 +0100 Message-ID: <55A3D0DE.3070905@citrix.com> References: <1436788907-1921-1-git-send-email-andrew.cooper3@citrix.com> <1436788907-1921-18-git-send-email-andrew.cooper3@citrix.com> <21923.52187.738764.317319@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <21923.52187.738764.317319@mariner.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: Ross Lagerwall , Wei Liu , Ian Campbell , Xen-devel List-Id: xen-devel@lists.xenproject.org On 13/07/15 15:31, Ian Jackson wrote: > Andrew Cooper writes ("[PATCH v3 17/28] tools/libxl: Infrastructure for reading a libxl migration v2 stream"): > ... >> + * PHASE_NORMAL: >> + * This phase is used for regular migration or resume from file. >> + * Records are read one at time and immediately processed. (The >> + * record queue will not contain more than a single record.) > I see that you added better comments about the record state later in > the series, thanks. > > I'm not entirely sure that it was right for that to be added later, > but I'm not going to quibble about its location in the series. I am > however going to comment on the content: > > * Internal states: > * running in_checkpoint phase > * Undefined - - - > * Idle false false - > * Active true false normal > * Checkpoint(buf) true true buffering > * Checkpoint(unbuf) true true unbuffering > * Active true false normal > * Finished false false - > > The `Active' line here appears twice. Surely some mistake. > > What is the difference between Finished and Idle ? I think none ? So > Finished doesn't belong here. > > Also you should probably explictly say that Checkpoint is a variant of > Active, from the outside. (You don't explicitly list the outside > states, but as I say I think they are the usual Undefined / Idle / > Active.) > > The use of `-' for `undefined value' is not proper, I think. You > probably cribbed this from the `-' in one cell in the `spawn > implementation' comment in libxl_exec.c (near line 242). But that `-' > means `no such object is allocated but the pointer has an undefined > value', which is rather special. > > Maybe you want this instead: > > * Internal states: > * phase running in_checkpoint > * Undefined undef undef undef > * Idle undef false false > * Active NORMAL true false > * Active BUFFERING true true > * Active UNBUFFERING true true > > I also asked for you to write down which state variables may take > which values in which state. running and in_checkpoint are only part > of this. > > For example, there is also incoming_record, and record_queue (which > may be undefined, empty, one entry, >=2 entries), and the datacopier. These cannot be expressed in the above terms. incoming_record (as already noted in libxl_internal.h) is only used while reading a record, which happens in both Active/NORMAL and Active/BUFFERING record_queue is strictly 0 or 1 entries during NORMAL, grows from 0 to >=1 during BUFFERING, and shrinks back to 0 during UNBUFFERING. How would you go about expressing this without a state transition diagram? (I really don't think an ascii art state transition diagram will help make this any clearer) > > Is the datacopier always active ? I think it is except > - when we are processing one of our own callbacks, or > - in BUFFERING/UNBUFFERING, when process_record is setting up > its own callbacks The datacopier is only active while reading a record from the stream, but that is only (logically) half of the work to do. It will never be active when processing records. There is a second datacopier for qemu use which is only active while processing an EMULATOR record. > > Maybe you want to say in a comment that there is always _either_ a > datacopier callback to come, _or_ a callback set up by process_record. ok. > > >> +static void stream_continue(libxl__egc *egc, >> + libxl__stream_read_state *stream) >> +{ > ... >> + switch (stream->phase) { >> + case SRS_PHASE_NORMAL: >> + /* >> + * Normal phase (regular migration or restore from file): >> + * >> + * logically: >> + * do { read_record(); process_record(); } while ( not END ); >> + * >> + * Alternate between reading a record from the stream, and >> + * processing the record. There should never be two records >> + * in the queue. >> + */ >> + if (LIBXL_STAILQ_EMPTY(&stream->record_queue)) >> + setup_read_record(egc, stream); >> + else { >> + if (process_record(egc, stream)) >> + setup_read_record(egc, stream); > Weren't you going to add an assert here, that the queue is empty ? > > > Last time I wrote: > > > + libxl__sr_record_buf *incoming_record; > > + LIBXL_STAILQ_HEAD(, libxl__sr_record_buf) record_queue; > > This comes from malloc, not from the gc. That needs a comment. (Is > the same true of incoming_record ? I think it is.) > > I don't see that comment. You have moved incoming_record away > record_queue so now it needs two comments. It is in the paragraph below PHASE_NORMAL, which you have snipped. ~Andrew