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 22:53, Dr. David Alan Gilbert wrote:
> > * 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)
> 
> Hmm.. Actually I use migrate_postcopy_ram() to distinct these thing in
> loadvm_postcopy_handle_advise.. And it seem like it is a mistake. Are
> migration capabilities available on target?.. On the other hand
> postcopy-test doesn't fail...

Libvirt does set it on the destination, and it's already useful for checking
the destination host has the appropriate kernel userfault support;
so I'm fine with requiring it.
However it's good where possible to fail nicely if someone doesn't set it.

Dave

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