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

[Vladimir Sementsov-Ogievskiy]

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

--
Best regards,
Vladimir