Re: [PATCH v8 --for 4.6 COLO 03/25] libxc/migration: Specification update for DIRTY_BITMAP records

[Yang Hongyang <yanghy@cn.fujitsu.com>]

On 07/16/2015 01:13 AM, Andrew Cooper wrote:
> On 15/07/15 10:18, Yang Hongyang wrote:
>> Used by secondary to send it's dirty bitmap to primary under COLO.
>>
>> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
>> ---
>>   docs/specs/libxc-migration-stream.pandoc | 24 +++++++++++++++++++++++-
>>   tools/libxc/xc_sr_common.c               |  1 +
>>   tools/libxc/xc_sr_stream_format.h        |  1 +
>>   3 files changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/specs/libxc-migration-stream.pandoc b/docs/specs/libxc-migration-stream.pandoc
>> index 68fa513..480d357 100644
>> --- a/docs/specs/libxc-migration-stream.pandoc
>> +++ b/docs/specs/libxc-migration-stream.pandoc
>> @@ -227,7 +227,9 @@ type         0x00000000: END
>>
>>                0x0000000E: CHECKPOINT
>>
>> -             0x0000000F - 0x7FFFFFFF: Reserved for future _mandatory_
>> +             0x0000000F: DIRTY_BITMAP
>> +
>> +             0x00000010 - 0x7FFFFFFF: Reserved for future _mandatory_
>>                records.
>>
>>                0x80000000 - 0xFFFFFFFF: Reserved for future _optional_
>> @@ -601,6 +603,26 @@ CHECKPOINT record or an END record.
>>
>>   \clearpage
>>
>> +DIRTY_BITMAP
>> +------------
>
> I would name this DIRTY_PFN_LIST or similar, as the content of data
> isn't actually a bitmap.

This sounds better, thanks!

>
>> +
>> +A dirty_bitmap record is used for secondary to send it's dirty bitmap
>> +to primary while doing a checkpoint under COLO. This record only exists
>> +in back channel.
>
> This section should purely be a description of the content.  i.e.
>
> "A DIRTY\_xxx record is used to convey information about dirty memory in
> the VM.  It is an unordered list of PFNs."

Ok.

>
>> +
>> +     0     1     2     3     4     5     6     7 octet
>> +    +-------------------------------------------------+
>> +    | pfn[0]                                          |
>> +    +-------------------------------------------------+
>> +    ...
>> +    +-------------------------------------------------+
>> +    | pfn[C-1]                                        |
>> +    +-------------------------------------------------+
>> +
>> +The count of the pfn is: record->length/sizeof(uint64_t).
>
> "The count of pfns is", although I would like to hope that this is
> obvious from the diagram.
>
> Down here, there should be more description of record circumstances,
> e.g. currently only applicable in the backchannel of a checkpointed stream.
>
> Also please put some validation logic for this in
> tools/python/xen/migration/libxc.py

Will do, thank you.

>
> ~Andrew
> .
>

-- 
Thanks,
Yang.