Re: [Xen-devel] [PATCH v3 17/28] tools/libxl: Infrastructure for reading a libxl migration v2 stream

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel