Patchwork [v3,27/35] postcopy/outgoing: implement forward/backword prefault

login
register
mail settings
Submitter Isaku Yamahata
Date Oct. 30, 2012, 8:33 a.m.
Message ID <ff9579ff48a2cba1ebf5fd97a6dde4246ea1175d.1351582535.git.yamahata@valinux.co.jp>
Download mbox | patch
Permalink /patch/195427/
State New
Headers show

Comments

Isaku Yamahata - Oct. 30, 2012, 8:33 a.m.
When page is requested, send surrounding pages are also sent.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hmp-commands.hx      |   15 ++++++++-----
 hmp.c                |    3 +++
 migration-postcopy.c |   57 +++++++++++++++++++++++++++++++++++++++++++++-----
 migration.c          |   20 ++++++++++++++++++
 migration.h          |    2 ++
 qapi-schema.json     |    3 ++-
 6 files changed, 89 insertions(+), 11 deletions(-)
Eric Blake - Nov. 1, 2012, 8:10 p.m.
On 10/30/2012 02:33 AM, Isaku Yamahata wrote:
> When page is requested, send surrounding pages are also sent.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hmp-commands.hx      |   15 ++++++++-----
>  hmp.c                |    3 +++
>  migration-postcopy.c |   57 +++++++++++++++++++++++++++++++++++++++++++++-----
>  migration.c          |   20 ++++++++++++++++++
>  migration.h          |    2 ++
>  qapi-schema.json     |    3 ++-
>  6 files changed, 89 insertions(+), 11 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index b054760..5e2c77c 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -826,26 +826,31 @@ ETEXI
>  
>      {
>          .name       = "migrate",
> -        .args_type  = "detach:-d,blk:-b,inc:-i,postcopy:-p,nobg:-n,uri:s",
> -        .params     = "[-d] [-b] [-i] [-p [-n]] uri",
> +        .args_type  = "detach:-d,blk:-b,inc:-i,postcopy:-p,nobg:-n,uri:s,"
> +	              "forward:i?,backward:i?",
> +        .params     = "[-d] [-b] [-i] [-p [-n] uri [forward] [backword]",

I don't care what we do to the 'migrate' HMP command, but for QMP...

> +++ b/qapi-schema.json
> @@ -2095,7 +2095,8 @@
>  ##
>  { 'command': 'migrate',
>    'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' ,
> -           '*postcopy': 'bool', '*nobg': 'bool'} }
> +           '*postcopy': 'bool', '*nobg': 'bool',
> +           '*forward': 'int', '*backward': 'int'} }

Do we really want to be adding new options to migrate (and if so,
where's the documentation), or do we need a new monitor command similar
to migrate-set-capabilities or migrate-set-cache-size?
Isaku Yamahata - Nov. 2, 2012, 5:24 a.m.
On Thu, Nov 01, 2012 at 02:10:45PM -0600, Eric Blake wrote:
> On 10/30/2012 02:33 AM, Isaku Yamahata wrote:
> > When page is requested, send surrounding pages are also sent.
> > 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > ---
> >  hmp-commands.hx      |   15 ++++++++-----
> >  hmp.c                |    3 +++
> >  migration-postcopy.c |   57 +++++++++++++++++++++++++++++++++++++++++++++-----
> >  migration.c          |   20 ++++++++++++++++++
> >  migration.h          |    2 ++
> >  qapi-schema.json     |    3 ++-
> >  6 files changed, 89 insertions(+), 11 deletions(-)
> > 
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index b054760..5e2c77c 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -826,26 +826,31 @@ ETEXI
> >  
> >      {
> >          .name       = "migrate",
> > -        .args_type  = "detach:-d,blk:-b,inc:-i,postcopy:-p,nobg:-n,uri:s",
> > -        .params     = "[-d] [-b] [-i] [-p [-n]] uri",
> > +        .args_type  = "detach:-d,blk:-b,inc:-i,postcopy:-p,nobg:-n,uri:s,"
> > +	              "forward:i?,backward:i?",
> > +        .params     = "[-d] [-b] [-i] [-p [-n] uri [forward] [backword]",
> 
> I don't care what we do to the 'migrate' HMP command, but for QMP...
> 
> > +++ b/qapi-schema.json
> > @@ -2095,7 +2095,8 @@
> >  ##
> >  { 'command': 'migrate',
> >    'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' ,
> > -           '*postcopy': 'bool', '*nobg': 'bool'} }
> > +           '*postcopy': 'bool', '*nobg': 'bool',
> > +           '*forward': 'int', '*backward': 'int'} }
> 
> Do we really want to be adding new options to migrate (and if so,
> where's the documentation), or do we need a new monitor command similar
> to migrate-set-capabilities or migrate-set-cache-size?

Okay, migrate-set-capabilities seems usable for boolean and scalable
for future extension.
On the other hand, migrate-set-cache-size takes only single integer
as arguments. So it doesn't seem usable without modification.
How about this?

{ 'type': 'MigrationParameters',
  'data': {'parameter': 'name': 'str', 'value': 'int' } }

{ 'command': 'migrate-set-parameters',
   'data': { 'parameters' ['MigrationParameters']}}


{ 'command': 'query-migrate-parameters',
  'returns': [['MigrationParameters']]}
Eric Blake - Nov. 2, 2012, 3:22 p.m.
On 11/01/2012 11:24 PM, Isaku Yamahata wrote:
>>> +++ b/qapi-schema.json
>>> @@ -2095,7 +2095,8 @@
>>>  ##
>>>  { 'command': 'migrate',
>>>    'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' ,
>>> -           '*postcopy': 'bool', '*nobg': 'bool'} }
>>> +           '*postcopy': 'bool', '*nobg': 'bool',
>>> +           '*forward': 'int', '*backward': 'int'} }
>>
>> Do we really want to be adding new options to migrate (and if so,
>> where's the documentation), or do we need a new monitor command similar
>> to migrate-set-capabilities or migrate-set-cache-size?
> 
> Okay, migrate-set-capabilities seems usable for boolean and scalable
> for future extension.
> On the other hand, migrate-set-cache-size takes only single integer
> as arguments. So it doesn't seem usable without modification.
> How about this?
> 
> { 'type': 'MigrationParameters',
>   'data': {'parameter': 'name': 'str', 'value': 'int' } }

More like:

{ 'enum': 'MigrationParameterName',
  'data': ['ParameterName'... ] }

{ 'type': 'MigrationParameter',
  'data': {'parameter': 'MigrationParameterName', 'value': 'int' } }

> 
> { 'command': 'migrate-set-parameters',
>    'data': { 'parameters' ['MigrationParameters']}}

Yes, this seems more extensible.

> 
> 
> { 'command': 'query-migrate-parameters',
>   'returns': [['MigrationParameters']]}

One layer too many of [], but yes, this also seems reasonable.

Patch

diff --git a/hmp-commands.hx b/hmp-commands.hx
index b054760..5e2c77c 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -826,26 +826,31 @@  ETEXI
 
     {
         .name       = "migrate",
-        .args_type  = "detach:-d,blk:-b,inc:-i,postcopy:-p,nobg:-n,uri:s",
-        .params     = "[-d] [-b] [-i] [-p [-n]] uri",
+        .args_type  = "detach:-d,blk:-b,inc:-i,postcopy:-p,nobg:-n,uri:s,"
+	              "forward:i?,backward:i?",
+        .params     = "[-d] [-b] [-i] [-p [-n] uri [forward] [backword]",
         .help       = "migrate to URI (using -d to not wait for completion)"
 		      "\n\t\t\t -b for migration without shared storage with"
 		      " full copy of disk\n\t\t\t -i for migration without "
 		      "shared storage with incremental copy of disk "
 		      "(base image shared between src and destination)"
 		      "\n\t\t\t-p for migration with postcopy mode enabled"
-		      "\n\t\t\t-n for no background transfer of postcopy mode",
+		      "\n\t\t\t-n for no background transfer of postcopy mode"
+		      "\n\t\t\tforward: the number of pages to "
+		      "forward-prefault when postcopy (default 0)"
+		      "\n\t\t\tbackward: the number of pages to "
+		      "backward-prefault when postcopy (default 0)",
         .mhandler.cmd = hmp_migrate,
     },
 
 
 STEXI
-@item migrate [-d] [-b] [-i] [-p [-n]] @var{uri}
+@item migrate [-d] [-b] [-i] [-p [-n]] @var{uri} @var{forward} @var{backward}
 @findex migrate
 Migrate to @var{uri} (using -d to not wait for completion).
 	-b for migration with full copy of disk
 	-i for migration with incremental copy of disk (base image is shared)
-	-p for migration with postcopy mode enabled
+	-p for migration with postcopy mode enabled (forward/backward is prefault size when postcopy)
 	-n for migration with postcopy mode enabled without background transfer
 ETEXI
 
diff --git a/hmp.c b/hmp.c
index 203b552..fb1275d 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1037,11 +1037,14 @@  void hmp_migrate(Monitor *mon, const QDict *qdict)
     int inc = qdict_get_try_bool(qdict, "inc", 0);
     int postcopy = qdict_get_try_bool(qdict, "postcopy", 0);
     int nobg = qdict_get_try_bool(qdict, "nobg", 0);
+    int forward = qdict_get_try_int(qdict, "forward", 0);
+    int backward = qdict_get_try_int(qdict, "backward", 0);
     const char *uri = qdict_get_str(qdict, "uri");
     Error *err = NULL;
 
     qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false,
                 !!postcopy, postcopy, !!nobg, nobg,
+                !!forward, forward, !!backward, backward,
                 &err);
     if (err) {
         monitor_printf(mon, "migrate: %s\n", error_get_pretty(err));
diff --git a/migration-postcopy.c b/migration-postcopy.c
index 5f98ae6..3d51898 100644
--- a/migration-postcopy.c
+++ b/migration-postcopy.c
@@ -344,6 +344,37 @@  int postcopy_outgoing_ram_save_complete(QEMUFile *f, void *opaque)
     return 0;
 }
 
+static void postcopy_outgoing_ram_save_page(PostcopyOutgoingState *s,
+                                            uint64_t pgoffset, bool *written,
+                                            bool forward,
+                                            int prefault_pgoffset)
+{
+    ram_addr_t offset;
+    int ret;
+
+    if (forward) {
+        pgoffset += prefault_pgoffset;
+    } else {
+        if (pgoffset < prefault_pgoffset) {
+            return;
+        }
+        pgoffset -= prefault_pgoffset;
+    }
+
+    offset = pgoffset << TARGET_PAGE_BITS;
+    if (offset >= s->last_block_read->length) {
+        assert(forward);
+        assert(prefault_pgoffset > 0);
+        return;
+    }
+
+    ret = ram_save_page(s->mig_buffered_write, s->last_block_read, offset,
+                        false);
+    if (ret > 0) {
+        *written = true;
+    }
+}
+
 /*
  * return value
  *   0: continue postcopy mode
@@ -355,6 +386,7 @@  static int postcopy_outgoing_handle_req(PostcopyOutgoingState *s,
                                         bool *written)
 {
     int i;
+    uint64_t j;
     RAMBlock *block;
 
     DPRINTF("cmd %d state %d\n", req->cmd, s->state);
@@ -387,11 +419,26 @@  static int postcopy_outgoing_handle_req(PostcopyOutgoingState *s,
             break;
         }
         for (i = 0; i < req->nr; i++) {
-            DPRINTF("offs[%d] 0x%"PRIx64"\n", i, req->pgoffs[i]);
-            int ret = ram_save_page(s->mig_buffered_write, s->last_block_read,
-                                    req->pgoffs[i] << TARGET_PAGE_BITS, false);
-            if (ret > 0) {
-                *written = true;
+            DPRINTF("pgoffs[%d] 0x%"PRIx64"\n", i, req->pgoffs[i]);
+            postcopy_outgoing_ram_save_page(s, req->pgoffs[i], written,
+                                            true, 0);
+        }
+        /* forward prefault */
+        for (j = 1; j <= s->ms->params.prefault_forward; j++) {
+            for (i = 0; i < req->nr; i++) {
+                DPRINTF("pgoffs[%d] + 0x%"PRIx64" 0x%"PRIx64"\n",
+                        i, j, req->pgoffs[i] + j);
+                postcopy_outgoing_ram_save_page(s, req->pgoffs[i], written,
+                                                true, j);
+            }
+        }
+        /* backward prefault */
+        for (j = 1; j <= s->ms->params.prefault_backward; j++) {
+            for (i = 0; i < req->nr; i++) {
+                DPRINTF("pgoffs[%d] - 0x%"PRIx64" 0x%"PRIx64"\n",
+                        i, j, req->pgoffs[i] - j);
+                postcopy_outgoing_ram_save_page(s, req->pgoffs[i], written,
+                                                false, j);
             }
         }
         break;
diff --git a/migration.c b/migration.c
index 279dda5..f29e3bb 100644
--- a/migration.c
+++ b/migration.c
@@ -511,6 +511,8 @@  void migrate_del_blocker(Error *reason)
 void qmp_migrate(const char *uri, bool has_blk, bool blk,
                  bool has_inc, bool inc, bool has_detach, bool detach,
                  bool has_postcopy, bool postcopy, bool has_nobg, bool nobg,
+                 bool has_forward, int64_t forward,
+                 bool has_backward, int64_t backward,
                  Error **errp)
 {
     MigrationState *s = migrate_get_current();
@@ -522,6 +524,24 @@  void qmp_migrate(const char *uri, bool has_blk, bool blk,
     params.shared = inc;
     params.postcopy = postcopy;
     params.nobg = nobg;
+    params.prefault_forward = 0;
+    if (has_forward) {
+        if (forward < 0) {
+            error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                      "forward", "forward >= 0");
+            return;
+        }
+        params.prefault_forward = forward;
+    }
+    params.prefault_backward = 0;
+    if (has_backward) {
+        if (backward < 0) {
+            error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                      "backward", "backward >= 0");
+            return;
+        }
+        params.prefault_backward = backward;
+    }
 
     if (s->state == MIG_STATE_ACTIVE) {
         error_set(errp, QERR_MIGRATION_ACTIVE);
diff --git a/migration.h b/migration.h
index 6724c19..8462251 100644
--- a/migration.h
+++ b/migration.h
@@ -26,6 +26,8 @@  struct MigrationParams {
     bool shared;
     bool postcopy;
     bool nobg;
+    int64_t prefault_forward;
+    int64_t prefault_backward;
 };
 
 typedef struct MigrationState MigrationState;
diff --git a/qapi-schema.json b/qapi-schema.json
index 70d0577..746bf21 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2095,7 +2095,8 @@ 
 ##
 { 'command': 'migrate',
   'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' ,
-           '*postcopy': 'bool', '*nobg': 'bool'} }
+           '*postcopy': 'bool', '*nobg': 'bool',
+           '*forward': 'int', '*backward': 'int'} }
 
 # @xen-save-devices-state:
 #