[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 03/17] migration: split common postcopy out of r
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-block] [PATCH 03/17] migration: split common postcopy out of ram postcopy |
Date: |
Mon, 6 Feb 2017 11:30:53 +0000 |
User-agent: |
Mutt/1.7.1 (2016-10-04) |
* Vladimir Sementsov-Ogievskiy (address@hidden) wrote:
> 01.02.2017 14:06, Vladimir Sementsov-Ogievskiy 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)
> >
> > We would like to stick current solution if it is not a problem. About
> > postcopy-foo: on the one hand, the format may be determined from
> > migration capabilities, on the other hand we can add for example 4bytes
> > format identifier starting from postcopy-foo and make all parameters
> > size to be at least 20 bytes. Or something like this.
> >
>
> Ok?
Yes OK; just please make sure it's all very commented and obvious.
Dave
> >
> >
> > >
> > > Dave
> > >
> > > > --
> > > > Best regards,
> > > > Vladimir
> > > >
> > > --
> > > Dr. David Alan Gilbert / address@hidden / Manchester, UK
> >
> >
>
>
> --
> Best regards,
> Vladimir
>
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK