Patchwork [v9,09/10] Add set_cachesize command

login
register
mail settings
Submitter Orit Wasserman
Date April 11, 2012, 6:49 p.m.
Message ID <1334170153-9503-10-git-send-email-owasserm@redhat.com>
Download mbox | patch
Permalink /patch/151860/
State New
Headers show

Comments

Orit Wasserman - April 11, 2012, 6:49 p.m.
Change XBZRLE cache size in bytes (the size should be a power of 2).
If XBZRLE cache size is too small there will be many cache miss.

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
Signed-off-by: Benoit Hudzia <benoit.hudzia@sap.com>
Signed-off-by: Petter Svard <petters@cs.umu.se>
Signed-off-by: Aidan Shribman <aidan.shribman@sap.com>
---
 arch_init.c      |    5 +++++
 hmp-commands.hx  |   15 +++++++++++++++
 hmp.c            |   13 +++++++++++++
 hmp.h            |    1 +
 migration.c      |   31 ++++++++++++++++++++++++++++++-
 migration.h      |    2 ++
 qapi-schema.json |   13 +++++++++++++
 qmp-commands.hx  |   22 ++++++++++++++++++++++
 8 files changed, 101 insertions(+), 1 deletions(-)
Juan Quintela - April 18, 2012, 5:07 p.m.
Orit Wasserman <owasserm@redhat.com> wrote:
> Change XBZRLE cache size in bytes (the size should be a power of 2).
> If XBZRLE cache size is too small there will be many cache miss.

> +        .name       = "migrate_set_cachesize",
> +        .args_type  = "value:o",

This mean that we can assign values like 256M, right?  i.e. no way to
pass negative values?
> index 7578163..071a1b9 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -167,7 +167,7 @@ MigrationCapList *qmp_query_migration_caps(Error **errp)
>      MigrationCapList *caps_list = g_malloc0(sizeof(*caps_list));
>  
>      caps_list->value = g_malloc(sizeof(*caps_list->value));
> -    caps_list->value->name = g_strdup("uleb");
> +    caps_list->value->name = g_strdup("xbzrle");
>      caps_list->next = NULL;
>  

Shouldn't this go in the previous patche?

> +void qmp_migrate_set_cachesize(int64_t value, Error **errp)
> +{
> +    MigrationState *s;
> +
> +    /* Check for truncation */
> +    if (value != (size_t)value) {
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> +                  "exceeding address space");
> +        return;
> +    }
> +
> +    /* power of 2 */
> +    if (value != 1 && (value & (value - 1))) {
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> +                  "needs to be power of 2");
> +        return;
> +    }

Don't we have a function to calculate if a value is a power of 2 in
qemu?  I guess we need one _now_.

> +
> +    value = MIN(UINT64_MAX, value);
> +    if (value ==  migrate_cache_size) {
> +        return;
> +    }
> +
> +    migrate_cache_size =  value;

Can't we just remove the migrate_cache_size as a new field of
migration_state.  bandwidth_limit is the example on how to do it.

Thanks, Juan.
Anthony Liguori - April 18, 2012, 5:37 p.m.
On 04/11/2012 01:49 PM, Orit Wasserman wrote:
> Change XBZRLE cache size in bytes (the size should be a power of 2).
> If XBZRLE cache size is too small there will be many cache miss.
>
> Signed-off-by: Orit Wasserman<owasserm@redhat.com>
> Signed-off-by: Benoit Hudzia<benoit.hudzia@sap.com>
> Signed-off-by: Petter Svard<petters@cs.umu.se>
> Signed-off-by: Aidan Shribman<aidan.shribman@sap.com>
> ---
>   arch_init.c      |    5 +++++
>   hmp-commands.hx  |   15 +++++++++++++++
>   hmp.c            |   13 +++++++++++++
>   hmp.h            |    1 +
>   migration.c      |   31 ++++++++++++++++++++++++++++++-
>   migration.h      |    2 ++
>   qapi-schema.json |   13 +++++++++++++
>   qmp-commands.hx  |   22 ++++++++++++++++++++++
>   8 files changed, 101 insertions(+), 1 deletions(-)
>
> diff --git a/arch_init.c b/arch_init.c
> index 793f0be..af9fcc3 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -175,6 +175,11 @@ typedef struct __attribute__((packed)) XBZRLEHeader {
>
>   static uint8 *prev_cache_page;
>
> +void xbzrle_cache_resize(int64_t new_size)
> +{
> +    cache_resize(new_size);
> +}

If it needs to be a power of 2, then take an integer order instead of a byte. 
That makes the function impossible to misuse.

Regards,

Anthony Liguori
Orit Wasserman - April 19, 2012, 6:20 a.m.
On 04/18/2012 08:07 PM, Juan Quintela wrote:
> Orit Wasserman <owasserm@redhat.com> wrote:
>> Change XBZRLE cache size in bytes (the size should be a power of 2).
>> If XBZRLE cache size is too small there will be many cache miss.
> 
>> +        .name       = "migrate_set_cachesize",
>> +        .args_type  = "value:o",
> 
> This mean that we can assign values like 256M, right?  i.e. no way to
> pass negative values?
>> index 7578163..071a1b9 100644
>> --- a/migration.c
>> +++ b/migration.c
>> @@ -167,7 +167,7 @@ MigrationCapList *qmp_query_migration_caps(Error **errp)
>>      MigrationCapList *caps_list = g_malloc0(sizeof(*caps_list));
>>  
>>      caps_list->value = g_malloc(sizeof(*caps_list->value));
>> -    caps_list->value->name = g_strdup("uleb");
>> +    caps_list->value->name = g_strdup("xbzrle");
>>      caps_list->next = NULL;
>>  
> 
> Shouldn't this go in the previous patche?sure
> 
>> +void qmp_migrate_set_cachesize(int64_t value, Error **errp)
>> +{
>> +    MigrationState *s;
>> +
>> +    /* Check for truncation */
>> +    if (value != (size_t)value) {
>> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
>> +                  "exceeding address space");
>> +        return;
>> +    }
>> +
>> +    /* power of 2 */
>> +    if (value != 1 && (value & (value - 1))) {
>> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
>> +                  "needs to be power of 2");
>> +        return;
>> +    }
> 
> Don't we have a function to calculate if a value is a power of 2 in
> qemu?  I guess we need one _now_.
I will look into it. maybe there is a function like that ...
.
> 
>> +
>> +    value = MIN(UINT64_MAX, value);
>> +    if (value ==  migrate_cache_size) {
>> +        return;
>> +    }
>> +
>> +    migrate_cache_size =  value;
> 
> Can't we just remove the migrate_cache_size as a new field of
> migration_state.  bandwidth_limit is the example on how to do it.
Ok

Thanks,
Orit

> 
> Thanks, Juan.
Orit Wasserman - April 19, 2012, 7:11 a.m.
On 04/18/2012 08:37 PM, Anthony Liguori wrote:
> On 04/11/2012 01:49 PM, Orit Wasserman wrote:
>> Change XBZRLE cache size in bytes (the size should be a power of 2).
>> If XBZRLE cache size is too small there will be many cache miss.
>>
>> Signed-off-by: Orit Wasserman<owasserm@redhat.com>
>> Signed-off-by: Benoit Hudzia<benoit.hudzia@sap.com>
>> Signed-off-by: Petter Svard<petters@cs.umu.se>
>> Signed-off-by: Aidan Shribman<aidan.shribman@sap.com>
>> ---
>>   arch_init.c      |    5 +++++
>>   hmp-commands.hx  |   15 +++++++++++++++
>>   hmp.c            |   13 +++++++++++++
>>   hmp.h            |    1 +
>>   migration.c      |   31 ++++++++++++++++++++++++++++++-
>>   migration.h      |    2 ++
>>   qapi-schema.json |   13 +++++++++++++
>>   qmp-commands.hx  |   22 ++++++++++++++++++++++
>>   8 files changed, 101 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch_init.c b/arch_init.c
>> index 793f0be..af9fcc3 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -175,6 +175,11 @@ typedef struct __attribute__((packed)) XBZRLEHeader {
>>
>>   static uint8 *prev_cache_page;
>>
>> +void xbzrle_cache_resize(int64_t new_size)
>> +{
>> +    cache_resize(new_size);
>> +}
> 
> If it needs to be a power of 2, then take an integer order instead of a byte. That makes the function impossible to misuse.
That is a good idea.
> 
> Regards,
> 
> Anthony Liguori

Patch

diff --git a/arch_init.c b/arch_init.c
index 793f0be..af9fcc3 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -175,6 +175,11 @@  typedef struct __attribute__((packed)) XBZRLEHeader {
 
 static uint8 *prev_cache_page;
 
+void xbzrle_cache_resize(int64_t new_size)
+{
+    cache_resize(new_size);
+}
+
 /***********************************************************/
 /* XBRLE page cache implementation */
 static CacheItem *cache_item_get(unsigned long pos, int item)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index a73b538..6ef2b6b 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -833,6 +833,21 @@  STEXI
 @item migrate_cancel
 @findex migrate_cancel
 Cancel the current VM migration.
+
+ETEXI
+
+    {
+        .name       = "migrate_set_cachesize",
+        .args_type  = "value:o",
+        .params     = "value",
+        .help       = "set cache size (in bytes) for XBZRLE migrations. The cache size effects the number of cache misses. In case of a high cache miss ratio you need to increase the cache size",
+        .mhandler.cmd = hmp_migrate_set_cachesize,
+    },
+
+STEXI
+@item migrate_set_cachesize @var{value}
+@findex migrate_set_cache
+Set cache size to @var{value} (in bytes) for xbzrle migrations.
 ETEXI
 
     {
diff --git a/hmp.c b/hmp.c
index d681bfc..b569887 100644
--- a/hmp.c
+++ b/hmp.c
@@ -740,6 +740,19 @@  void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict)
     qmp_migrate_set_downtime(value, NULL);
 }
 
+void hmp_migrate_set_cachesize(Monitor *mon, const QDict *qdict)
+{
+    int64_t value = qdict_get_int(qdict, "value");
+    Error *err = NULL;
+
+    qmp_migrate_set_cachesize(value, &err);
+    if (err) {
+        monitor_printf(mon, "%s\n", error_get_pretty(err));
+        error_free(err);
+        return;
+    }
+}
+
 void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict)
 {
     int64_t value = qdict_get_int(qdict, "value");
diff --git a/hmp.h b/hmp.h
index 868bfb3..4cce377 100644
--- a/hmp.h
+++ b/hmp.h
@@ -52,6 +52,7 @@  void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict);
 void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict);
+void hmp_migrate_set_cachesize(Monitor *mon, const QDict *qdict);
 void hmp_set_password(Monitor *mon, const QDict *qdict);
 void hmp_expire_password(Monitor *mon, const QDict *qdict);
 void hmp_eject(Monitor *mon, const QDict *qdict);
diff --git a/migration.c b/migration.c
index 7578163..071a1b9 100644
--- a/migration.c
+++ b/migration.c
@@ -167,7 +167,7 @@  MigrationCapList *qmp_query_migration_caps(Error **errp)
     MigrationCapList *caps_list = g_malloc0(sizeof(*caps_list));
 
     caps_list->value = g_malloc(sizeof(*caps_list->value));
-    caps_list->value->name = g_strdup("uleb");
+    caps_list->value->name = g_strdup("xbzrle");
     caps_list->next = NULL;
 
     return caps_list;
@@ -465,6 +465,35 @@  void qmp_migrate_cancel(Error **errp)
     migrate_fd_cancel(migrate_get_current());
 }
 
+void qmp_migrate_set_cachesize(int64_t value, Error **errp)
+{
+    MigrationState *s;
+
+    /* Check for truncation */
+    if (value != (size_t)value) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
+                  "exceeding address space");
+        return;
+    }
+
+    /* power of 2 */
+    if (value != 1 && (value & (value - 1))) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
+                  "needs to be power of 2");
+        return;
+    }
+
+    value = MIN(UINT64_MAX, value);
+    if (value ==  migrate_cache_size) {
+        return;
+    }
+
+    migrate_cache_size =  value;
+    s = migrate_get_current();
+    s->params.xbzrle_cache_size = value;
+    xbzrle_cache_resize(value);
+}
+
 void qmp_migrate_set_speed(int64_t value, Error **errp)
 {
     MigrationState *s;
diff --git a/migration.h b/migration.h
index 06ce8ab..3426c94 100644
--- a/migration.h
+++ b/migration.h
@@ -109,4 +109,6 @@  int decode_page(uint8_t *src, int slen, uint8_t *dst, int dlen);
 int migrate_use_xbzrle(void);
 int64_t migrate_xbzrle_cache_size(void);
 
+void xbzrle_cache_resize(int64_t new_size);
+
 #endif
diff --git a/qapi-schema.json b/qapi-schema.json
index 775fe56..956caed 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1310,6 +1310,19 @@ 
 { 'command': 'migrate_set_speed', 'data': {'value': 'int'} }
 
 ##
+# @migrate_set_cachesize
+#
+# Set XBZRLE cache size
+#
+# @value: cache size in bytes
+#
+# Returns: nothing on success
+#
+# Since: 1.1
+##
+{ 'command': 'migrate_set_cachesize', 'data': {'value': 'int'} }
+
+##
 # @ObjectPropertyInfo:
 #
 # @name: the name of the property
diff --git a/qmp-commands.hx b/qmp-commands.hx
index b994951..3f2c9e2 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -522,6 +522,28 @@  Example:
 <- { "return": {} }
 
 EQMP
+{
+        .name       = "migrate_set_cachesize",
+        .args_type  = "value:o",
+        .mhandler.cmd_new = qmp_marshal_input_migrate_set_cachesize,
+    },
+
+SQMP
+migrate_set_cachesize
+---------------------
+
+Set cache size to be used by XBZRLE migration
+
+Arguments:
+
+- "value": cache size in bytes (json-int)
+
+Example:
+
+-> { "execute": "migrate_set_cachesize", "arguments": { "value": 512 } }
+<- { "return": {} }
+
+EQMP
 
     {
         .name       = "migrate_set_speed",