Patchwork [v5,8/9] QMP commands changes

login
register
mail settings
Submitter Orit Wasserman
Date Jan. 3, 2012, 3:34 p.m.
Message ID <1325604879-15862-9-git-send-email-owasserm@redhat.com>
Download mbox | patch
Permalink /patch/134026/
State New
Headers show

Comments

Orit Wasserman - Jan. 3, 2012, 3:34 p.m.
Signed-off-by: Orit Wasserman <owasserm@redhat.com>
---
 hmp-commands.hx |   34 ++++++++++++++++++++++++++--------
 qmp-commands.hx |   44 +++++++++++++++++++++++++++++++++++++-------
 2 files changed, 63 insertions(+), 15 deletions(-)
Stefan Hajnoczi - Jan. 3, 2012, 3:47 p.m.
On Tue, Jan 3, 2012 at 3:34 PM, Orit Wasserman <owasserm@redhat.com> wrote:
> +        .args_type  = "detach:-d,blk:-b,inc:-i,xbrle:-x,uri:s",
> +        .params     = "[-d] [-b] [-i] [-x] uri",
> +        .help       = "migrate to URI"
> +                      "\n\t -d to not wait for completion"
> +                      "\n\t -b for migration without shared storage with"
> +                      " full copy of disk"
> +                      "\n\t -i for migration without"
> +                      " shared storage with incremental copy of disk"
> +                      " (base image shared between source and destination)"
> +                      "\n\t -x to use XBRLE page delta compression",

It's too bad that this algorithm is user-visible and needs to be
expicitly enabled/disabled.  I think it would be useful in a much
wider range of cases if the trade-offs were understood well enough so
that QEMU can include a threshold or heuristic which chooses the
migration algorithm behind the scenes.

I'm afraid that locking XBRLE behind an explicit option means we're
merging dead code.  What do you think?

Stefan
Orit Wasserman - Jan. 3, 2012, 3:57 p.m.
On 01/03/2012 05:47 PM, Stefan Hajnoczi wrote:
> On Tue, Jan 3, 2012 at 3:34 PM, Orit Wasserman <owasserm@redhat.com> wrote:
>> +        .args_type  = "detach:-d,blk:-b,inc:-i,xbrle:-x,uri:s",
>> +        .params     = "[-d] [-b] [-i] [-x] uri",
>> +        .help       = "migrate to URI"
>> +                      "\n\t -d to not wait for completion"
>> +                      "\n\t -b for migration without shared storage with"
>> +                      " full copy of disk"
>> +                      "\n\t -i for migration without"
>> +                      " shared storage with incremental copy of disk"
>> +                      " (base image shared between source and destination)"
>> +                      "\n\t -x to use XBRLE page delta compression",
> 
> It's too bad that this algorithm is user-visible and needs to be
> expicitly enabled/disabled.  I think it would be useful in a much
> wider range of cases if the trade-offs were understood well enough so
> that QEMU can include a threshold or heuristic which chooses the
> migration algorithm behind the scenes.

I agree that an automatic switching on/off XBRLE is very desirable. 
At the first phase we merge it as a user option, and the in the next phase will be adding
the part that switch it on/off.
> 
> I'm afraid that locking XBRLE behind an explicit option means we're
> merging dead code.  What do you think?

Even as a user option this option will be useful , in many cases users are aware of 
their workload and can set this option on.

Orit
> 
> Stefan
Stefan Hajnoczi - Jan. 3, 2012, 4:20 p.m.
On Tue, Jan 3, 2012 at 3:57 PM, Orit Wasserman <owasserm@redhat.com> wrote:
> On 01/03/2012 05:47 PM, Stefan Hajnoczi wrote:
>> On Tue, Jan 3, 2012 at 3:34 PM, Orit Wasserman <owasserm@redhat.com> wrote:
>>> +        .args_type  = "detach:-d,blk:-b,inc:-i,xbrle:-x,uri:s",
>>> +        .params     = "[-d] [-b] [-i] [-x] uri",
>>> +        .help       = "migrate to URI"
>>> +                      "\n\t -d to not wait for completion"
>>> +                      "\n\t -b for migration without shared storage with"
>>> +                      " full copy of disk"
>>> +                      "\n\t -i for migration without"
>>> +                      " shared storage with incremental copy of disk"
>>> +                      " (base image shared between source and destination)"
>>> +                      "\n\t -x to use XBRLE page delta compression",
>>
>> It's too bad that this algorithm is user-visible and needs to be
>> expicitly enabled/disabled.  I think it would be useful in a much
>> wider range of cases if the trade-offs were understood well enough so
>> that QEMU can include a threshold or heuristic which chooses the
>> migration algorithm behind the scenes.
>
> I agree that an automatic switching on/off XBRLE is very desirable.
> At the first phase we merge it as a user option, and the in the next phase will be adding
> the part that switch it on/off.

Okay, if there's an intent to do that next phase then great.  I'll
take a look at this series and review it soon.

Stefan

Patch

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 14838b7..952d5cd 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -746,24 +746,29 @@  ETEXI
 
     {
         .name       = "migrate",
-        .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
-        .params     = "[-d] [-b] [-i] uri",
-        .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)",
+        .args_type  = "detach:-d,blk:-b,inc:-i,xbrle:-x,uri:s",
+        .params     = "[-d] [-b] [-i] [-x] uri",
+        .help       = "migrate to URI"
+                      "\n\t -d to not wait for completion"
+                      "\n\t -b for migration without shared storage with"
+                      " full copy of disk"
+                      "\n\t -i for migration without"
+                      " shared storage with incremental copy of disk"
+                      " (base image shared between source and destination)"
+                      "\n\t -x to use XBRLE page delta compression",
         .user_print = monitor_user_noop,	
 	.mhandler.cmd_new = do_migrate,
     },
 
 
 STEXI
-@item migrate [-d] [-b] [-i] @var{uri}
+@item migrate [-d] [-b] [-i] [-x] @var{uri}
 @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)
+	-x to use XBRLE page delta compression
+
 ETEXI
 
     {
@@ -781,6 +786,19 @@  Cancel the current VM migration.
 ETEXI
 
     {
+        .name       = "migrate_set_cachesize",
+        .args_type  = "value:s",
+        .params     = "value",
+        .help       = "set cache size (in MB) for XBRLE migrations",
+        .mhandler.cmd = do_migrate_set_cachesize,
+    },
+
+STEXI
+@item migrate_set_cachesize @var{value}
+Set cache size (in MB) for xbrle migrations.
+ETEXI
+
+    {
         .name       = "migrate_set_speed",
         .args_type  = "value:o",
         .params     = "value",
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 7e3f4b9..34bdfe9 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -430,13 +430,16 @@  EQMP
 
     {
         .name       = "migrate",
-        .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
-        .params     = "[-d] [-b] [-i] uri",
-        .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)",
+        .args_type  = "detach:-d,blk:-b,inc:-i,xbrle:-x,uri:s",
+        .params     = "[-d] [-b] [-i] [-x] uri",
+        .help       = "migrate to URI"
+                      "\n\t -d to not wait for completion"
+                      "\n\t -b for migration without shared storage with"
+                      " full copy of disk"
+                      "\n\t -i for migration without"
+                      " shared storage with incremental copy of disk"
+                      " (base image shared between source and destination)"
+                      "\n\t -x to use XBRLE page delta compression",
         .user_print = monitor_user_noop,	
 	.mhandler.cmd_new = do_migrate,
     },
@@ -452,6 +455,7 @@  Arguments:
 - "blk": block migration, full disk copy (json-bool, optional)
 - "inc": incremental disk copy (json-bool, optional)
 - "uri": Destination URI (json-string)
+- "xbrle": to use XBRLE page delta compression
 
 Example:
 
@@ -490,6 +494,32 @@  Example:
 EQMP
 
     {
+        .name       = "migrate_set_cachesize",
+        .args_type  = "value:s",
+        .params     = "value",
+        .help       = "set cache size (in MB) for xbrle migrations",
+        .mhandler.cmd = do_migrate_set_cachesize,
+    },
+
+SQMP
+migrate_set_cachesize
+---------------------
+
+Set cache size to be used by XBRLE migration
+
+Arguments:
+
+- "value": cache size in bytes (json-number)
+
+Example:
+
+-> { "execute": "migrate_set_cachesize", "arguments": { "value": 500M } }
+<- { "return": {} }
+
+
+EQMP
+
+    {
         .name       = "migrate_set_speed",
         .args_type  = "value:o",
         .mhandler.cmd_new = qmp_marshal_input_migrate_set_speed,