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

[Dr. David Alan Gilbert]

* Juan Quintela (address@hidden) wrote:
> "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:-)

Done.

> > +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)

Done (fun combination with the change above, but still)

> >  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.

Done (I made it _ID rather than WITH_ID - it was getting a bit long).

Dave

> 
> Later, Juan.
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK