From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>,
Wei Liu <wei.liu2@citrix.com>,
Ian Campbell <Ian.Campbell@citrix.com>,
Xen-devel <xen-devel@lists.xen.org>
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 [thread overview]
Message-ID: <55A3D0DE.3070905@citrix.com> (raw)
In-Reply-To: <21923.52187.738764.317319@mariner.uk.xensource.com>
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
next prev parent reply other threads:[~2015-07-13 14:53 UTC|newest]
Thread overview: 62+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-13 12:01 [PATCH v3 00/27] Libxl migration v2 Andrew Cooper
2015-07-13 12:01 ` [PATCH v3 01/28] bsd-sys-queue-h-seddery: Massage `offsetof' Andrew Cooper
2015-07-13 12:01 ` [PATCH v3 02/28] tools/libxc: Always compile the compat qemu variables into xc_sr_context Andrew Cooper
2015-07-13 12:01 ` [PATCH v3 03/28] tools/libxl: Introduce ROUNDUP() Andrew Cooper
2015-07-13 12:01 ` [PATCH v3 04/28] tools/libxl: Introduce libxl__kill() Andrew Cooper
2015-07-13 12:01 ` [PATCH v3 05/28] tools/libxl: Stash all restore parameters in domain_create_state Andrew Cooper
2015-07-13 12:01 ` [PATCH v3 06/28] tools/libxl: Split libxl__domain_create_state.restore_fd in two Andrew Cooper
2015-07-13 12:01 ` [PATCH v3 07/28] tools/libxl: Extra management APIs for the save helper Andrew Cooper
2015-07-13 13:21 ` Ian Campbell
2015-07-13 15:01 ` Ian Jackson
2015-07-13 12:01 ` [PATCH v3 08/28] tools/libxl: Add save_helper_state pointers to libxl__xc_domain_{save, restore}() Andrew Cooper
2015-07-13 13:24 ` Ian Campbell
2015-07-13 12:01 ` [PATCH v3 09/28] tools/xl: Mandatory flag indicating the format of the migration stream Andrew Cooper
2015-07-13 12:01 ` [PATCH v3 10/28] docs: Libxl migration v2 stream specification Andrew Cooper
2015-07-13 12:01 ` [PATCH v3 11/28] tools/python: Libxc migration v2 infrastructure Andrew Cooper
2015-07-13 12:01 ` [PATCH v3 12/28] tools/python: Libxl " Andrew Cooper
2015-07-13 12:01 ` [PATCH v3 13/28] tools/python: Other migration infrastructure Andrew Cooper
2015-07-13 12:01 ` [PATCH v3 14/28] tools/python: Verification utility for v2 stream spec compliance Andrew Cooper
2015-07-13 12:01 ` [PATCH v3 15/28] tools/python: Conversion utility for legacy migration streams Andrew Cooper
2015-07-13 12:01 ` [PATCH v3 16/28] tools/libxl: Migration v2 stream format Andrew Cooper
2015-07-13 12:01 ` [PATCH v3 17/28] tools/libxl: Infrastructure for reading a libxl migration v2 stream Andrew Cooper
2015-07-13 13:42 ` Ian Campbell
2015-07-13 13:53 ` Andrew Cooper
2015-07-13 14:11 ` Ian Jackson
2015-07-13 14:03 ` Ian Jackson
2015-07-13 14:31 ` Ian Jackson
2015-07-13 14:53 ` Andrew Cooper [this message]
2015-07-13 15:33 ` Ian Jackson
2015-07-13 15:08 ` Ian Jackson
2015-07-13 15:13 ` Andrew Cooper
2015-07-13 15:35 ` Ian Jackson
2015-07-13 12:01 ` [PATCH v3 18/28] tools/libxl: Infrastructure to convert a legacy stream Andrew Cooper
2015-07-13 14:45 ` Ian Jackson
2015-07-13 15:08 ` Andrew Cooper
2015-07-13 15:44 ` Ian Jackson
2015-07-13 15:53 ` Andrew Cooper
2015-07-13 15:56 ` Ian Jackson
2015-07-13 16:04 ` Ian Campbell
2015-07-13 12:01 ` [PATCH v3 19/28] tools/libxl: Convert a legacy stream if needed Andrew Cooper
2015-07-13 14:51 ` Ian Jackson
2015-07-13 15:20 ` Andrew Cooper
2015-07-13 15:44 ` Ian Jackson
2015-07-13 12:01 ` [PATCH v3 20/28] tools/libxc+libxl+xl: Restore v2 streams Andrew Cooper
2015-07-13 15:02 ` Ian Jackson
2015-07-13 12:01 ` [PATCH v3 21/28] tools/libxl: Infrastructure for writing a v2 stream Andrew Cooper
2015-07-13 15:09 ` Ian Jackson
2015-07-13 15:21 ` Ian Jackson
2015-07-13 15:33 ` Andrew Cooper
2015-07-13 15:47 ` Ian Jackson
2015-07-13 15:56 ` Andrew Cooper
2015-07-13 12:01 ` [PATCH v3 22/28] tools/libxc+libxl+xl: Save v2 streams Andrew Cooper
2015-07-13 15:25 ` Ian Jackson
2015-07-13 12:01 ` [PATCH v3 23/28] docs/libxl: Introduce CHECKPOINT_END to support migration v2 remus streams Andrew Cooper
2015-07-13 12:01 ` [PATCH v3 24/28] tools/libxl: Write checkpoint records into the stream Andrew Cooper
2015-07-13 12:01 ` [PATCH v3 25/28] tools/libx{c, l}: Introduce restore_callbacks.checkpoint() Andrew Cooper
2015-07-13 12:01 ` [PATCH v3 26/28] tools/libxl: Handle checkpoint records in a libxl migration v2 stream Andrew Cooper
2015-07-13 13:53 ` Ian Campbell
2015-07-14 10:33 ` Yang Hongyang
2015-07-14 10:35 ` Andrew Cooper
2015-07-13 12:01 ` [PATCH v3 27/28] tools/libxc: Drop all XG_LIBXL_HVM_COMPAT code from libxc Andrew Cooper
2015-07-13 12:01 ` [PATCH v3 28/28] tools/libxl: Drop all knowledge of toolstack callbacks Andrew Cooper
2015-07-13 15:55 ` Ian Jackson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55A3D0DE.3070905@citrix.com \
--to=andrew.cooper3@citrix.com \
--cc=Ian.Campbell@citrix.com \
--cc=Ian.Jackson@eu.citrix.com \
--cc=ross.lagerwall@citrix.com \
--cc=wei.liu2@citrix.com \
--cc=xen-devel@lists.xen.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.