Re: [Qemu-devel] [PATCH v7 30/42] Page request: Add MIG_RP_MSG_REQ_PAGES reverse command

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <address@hidden> wrote:
> From: "Dr. David Alan Gilbert" <address@hidden>
>
> Add MIG_RP_MSG_REQ_PAGES command on Return path for the postcopy
> destination to request a page from the source.
>
> Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> ---
>  include/migration/migration.h |  4 +++
>  migration/migration.c         | 70 
> +++++++++++++++++++++++++++++++++++++++++++
>  trace-events                  |  1 +
>  3 files changed, 75 insertions(+)
>
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 68a1731..8742d53 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -47,6 +47,8 @@ enum mig_rp_message_type {
>      MIG_RP_MSG_INVALID = 0,  /* Must be 0 */
>      MIG_RP_MSG_SHUT,         /* sibling will not send any more RP messages */
>      MIG_RP_MSG_PONG,         /* Response to a PING; data (seq: be32 ) */
> +
> +    MIG_RP_MSG_REQ_PAGES,    /* data (start: be64, len: be64) */

Not that I really care, buht I think that leng could be 32bits.  I am
not seing networking getting good at multigigabytes transfers soon O:-)


> +void migrate_send_rp_req_pages(MigrationIncomingState *mis, const char* 
> rbname,
> +                              ram_addr_t start, ram_addr_t len);

Shouldn't len be a size_t?
(yes, I know that migration code is not really consistent about that)

>  void ram_control_before_iterate(QEMUFile *f, uint64_t flags);
>  void ram_control_after_iterate(QEMUFile *f, uint64_t flags);
> diff --git a/migration/migration.c b/migration/migration.c
> index 3e5a7c8..0373b77 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -113,6 +113,36 @@ static void deferred_incoming_migration(Error **errp)
>      deferred_incoming = true;
>  }
>  
> +/* Request a range of pages from the source VM at the given
> + * start address.
> + *   rbname: Name of the RAMBlock to request the page in, if NULL it's the 
> same
> + *           as the last request (a name must have been given previously)
> + *   Start: Address offset within the RB
> + *   Len: Length in bytes required - must be a multiple of pagesize
> + */
> +void migrate_send_rp_req_pages(MigrationIncomingState *mis, const char 
> *rbname,
> +                               ram_addr_t start, ram_addr_t len)
> +{
> +    uint8_t bufc[16+1+255]; /* start (8 byte), len (8 byte), rbname upto 256 
> */
> +    uint64_t *buf64 = (uint64_t *)bufc;
> +    size_t msglen = 16; /* start + len */
> +
> +    assert(!(len & 1));

ohhhh, why can't we get a real flags field?

Scratch that.  Seeing the rest of the code, can't we have two commands:

MIG_RP_MSG_REQ_PAGES
MIG_RP_MSG_REQ_PAGES_WITH_ID

I am not really sure that it makes sense getting a command that can be
of two different lengths only for that?


I am not sure, but having a command with two different payloads look
strange.

Later, Juan.