Re: [Qemu-devel] [PATCHv4 7/9] migration: do not sent zero pages in bulk stage

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2675 bytes --]

On 03/22/2013 06:46 AM, Peter Lieven wrote:
> during bulk stage of ram migration if a page is a
> zero page do not send it at all.
> the memory at the destination reads as zero anyway.
> 
> even if there is an madvise with QEMU_MADV_DONTNEED
> at the target upon receipt of a zero page I have observed
> that the target starts swapping if the memory is overcommitted.
> it seems that the pages are dropped asynchronously.

Your commit message fails to mention that you are updating QMP to record
a new stat, although I agree with what you've done.  If you do respin,
make mention of this fact in the commit message.

> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  arch_init.c                   |   24 ++++++++++++++++++++----
>  hmp.c                         |    2 ++
>  include/migration/migration.h |    2 ++
>  migration.c                   |    3 ++-
>  qapi-schema.json              |    6 ++++--
>  qmp-commands.hx               |    3 ++-
>  6 files changed, 32 insertions(+), 8 deletions(-)
> 

> +++ b/qapi-schema.json
> @@ -496,7 +496,9 @@
>  #
>  # @total: total amount of bytes involved in the migration process
>  #
> -# @duplicate: number of duplicate pages (since 1.2)
> +# @duplicate: number of duplicate (zero) pages (since 1.2)
> +#
> +# @skipped: number of skipped zero pages (since 1.5)
>  #
>  # @normal : number of normal pages (since 1.2)
>  #
> @@ -510,7 +512,7 @@
>  { 'type': 'MigrationStats',
>    'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
>             'duplicate': 'int', 'normal': 'int', 'normal-bytes': 'int',
> -           'dirty-pages-rate' : 'int' } }
> +           'dirty-pages-rate' : 'int', 'skipped': 'int' } }

Your layout here doesn't match the order that you documented things in.
 But it is a dictionary of name-value pairs, so order is not significant
to the interface.  About the only thing the order might affect is
whether the rest of your code, which assigns fields in documentation
order, is slightly less efficient because it is jumping around the C
struct rather than hitting it in linear order, but that would be in the
noise on a benchmark.  So I won't insist on a respin.  However, since
you are touching QMP, it wouldn't hurt to have Luiz chime in.

I'm okay if this goes in as-is.  Or, if you do spin a v5 for other
reasons, then lay out MigrationStats in documentation order, and improve
the commit message.  If those are the only changes you make, then you
can keep:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]