Re: [Qemu-block] [PATCH 03/17] migration: split common postcopy out of ram postcopy

[Dr. David Alan Gilbert]

* Vladimir Sementsov-Ogievskiy (address@hidden) wrote:
> 24.01.2017 12:24, Juan Quintela wrote:
> > Vladimir Sementsov-Ogievskiy <address@hidden> wrote:
> > > Split common postcopy staff from ram postcopy staff.
> > > 
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > > ---
> > >   include/migration/migration.h |  1 +
> > >   migration/migration.c         | 39 
> > > +++++++++++++++++++++++++++------------
> > >   migration/postcopy-ram.c      |  4 +++-
> > >   migration/savevm.c            | 31 ++++++++++++++++++++++---------
> > >   4 files changed, 53 insertions(+), 22 deletions(-)
> > > 
> > Hi
> > 
> > {
> > >       MigrationState *s;
> > > @@ -1587,9 +1592,11 @@ static int postcopy_start(MigrationState *ms, bool 
> > > *old_vm_running)
> > >        * need to tell the destination to throw any pages it's already 
> > > received
> > >        * that are dirty
> > >        */
> > > -    if (ram_postcopy_send_discard_bitmap(ms)) {
> > > -        error_report("postcopy send discard bitmap failed");
> > > -        goto fail;
> > > +    if (migrate_postcopy_ram()) {
> > > +        if (ram_postcopy_send_discard_bitmap(ms)) {
> > > +            error_report("postcopy send discard bitmap failed");
> > > +            goto fail;
> > > +        }
> > I will have preffered that for the ram commands, to embed the
> > migrate_postocpy_ram() check inside them, but that is taste, and
> > everyone has its own O:-)
> > 
> > > diff --git a/migration/savevm.c b/migration/savevm.c
> > > index e436cb2..c8a71c8 100644
> > > --- a/migration/savevm.c
> > > +++ b/migration/savevm.c
> > > @@ -73,7 +73,7 @@ static struct mig_cmd_args {
> > >       [MIG_CMD_INVALID]          = { .len = -1, .name = "INVALID" },
> > >       [MIG_CMD_OPEN_RETURN_PATH] = { .len =  0, .name = 
> > > "OPEN_RETURN_PATH" },
> > >       [MIG_CMD_PING]             = { .len = sizeof(uint32_t), .name = 
> > > "PING" },
> > > -    [MIG_CMD_POSTCOPY_ADVISE]  = { .len = 16, .name = "POSTCOPY_ADVISE" 
> > > },
> > > +    [MIG_CMD_POSTCOPY_ADVISE]  = { .len = -1, .name = "POSTCOPY_ADVISE" 
> > > },
> > >       [MIG_CMD_POSTCOPY_LISTEN]  = { .len =  0, .name = "POSTCOPY_LISTEN" 
> > > },
> > >       [MIG_CMD_POSTCOPY_RUN]     = { .len =  0, .name = "POSTCOPY_RUN" },
> > >       [MIG_CMD_POSTCOPY_RAM_DISCARD] = {
> > > @@ -827,12 +827,17 @@ int qemu_savevm_send_packaged(QEMUFile *f, const 
> > > uint8_t *buf, size_t len)
> > >   /* Send prior to any postcopy transfer */
> > >   void qemu_savevm_send_postcopy_advise(QEMUFile *f)
> > >   {
> > > -    uint64_t tmp[2];
> > > -    tmp[0] = cpu_to_be64(getpagesize());
> > > -    tmp[1] = cpu_to_be64(1ul << qemu_target_page_bits());
> > > +    if (migrate_postcopy_ram()) {
> > > +        uint64_t tmp[2];
> > > +        tmp[0] = cpu_to_be64(getpagesize());
> > > +        tmp[1] = cpu_to_be64(1ul << qemu_target_page_bits());
> > > -    trace_qemu_savevm_send_postcopy_advise();
> > > -    qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, 16, (uint8_t 
> > > *)tmp);
> > > +        trace_qemu_savevm_send_postcopy_advise();
> > > +        qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE,
> > > +                                 16, (uint8_t *)tmp);
> > > +    } else {
> > > +        qemu_savevm_command_send(f, MIG_CMD_POSTCOPY_ADVISE, 0, NULL);
> > > +    }
> > >   }
> > >   /* Sent prior to starting the destination running in postcopy, discard 
> > > pages
> > I haven't yet figured out why you are reusing this command with a
> > different number of parameters.
> > For this to pass, I need that Dave comment on this.
> > 
> > So,
> > 
> > Reviewed-by: Juan Quintela <address@hidden>
> > 
> > conditioned that Dave agrees with this.
> 
> These parameters are unrelated if ram postcopy is disabled. So, I should be
> better to have a possibility of skipping them, then to send unneeded numbers
> and write separate code to read them from the stream (rewriting
> loadvm_postcopy_handle_advise to just read these two numbers and do nothing
> about ram postcopy for this case).

I think I'd prefer either a new command or keeping these fields (probably all 0 
?)
my worry is what happens in the case if we add a 3rd postcopy subfeature;
In your case we have three possibilities:

    a) Postcopy RAM only - 16 bytes
    b) Postcopy persistent-dirty-bitmap only - 0 bytes
    c) Both - 16 bytes

Lets say we added postcopy-foo in the future and it wanted to add
another 16 bytes, what would it send if it was foo+persistent-dirty-bitmap and 
no RAM?
We'd end up with 16 bytes sent but you'd have to be very careful
never to get that confused with case (a) above.

(I don't feel too strongly about it though)

Dave

> -- 
> Best regards,
> Vladimir
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK