diff mbox

[v13,12/13] Add set_cachesize command

Message ID 1340793261-11400-13-git-send-email-owasserm@redhat.com
State New
Headers show

Commit Message

Orit Wasserman June 27, 2012, 10:34 a.m. UTC 
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: Benoit Hudzia <benoit.hudzia@sap.com>
Signed-off-by: Petter Svard <petters@cs.umu.se>
Signed-off-by: Aidan Shribman <aidan.shribman@sap.com>
Signed-off-by: Orit Wasserman <owasserm@redhat.com>
---
 arch_init.c      |    9 +++++++++
 hmp-commands.hx  |   18 ++++++++++++++++++
 hmp.c            |   13 +++++++++++++
 hmp.h            |    1 +
 migration.c      |   23 ++++++++++++++++++++++-
 migration.h      |    2 ++
 qapi-schema.json |   16 ++++++++++++++++
 qmp-commands.hx  |   23 +++++++++++++++++++++++
 8 files changed, 104 insertions(+), 1 deletions(-)

Comments

Blue Swirl June 27, 2012, 5:56 p.m. UTC | #1 
On Wed, Jun 27, 2012 at 10:34 AM, 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.
>
> 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>
> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> ---
>  arch_init.c      |    9 +++++++++
>  hmp-commands.hx  |   18 ++++++++++++++++++
>  hmp.c            |   13 +++++++++++++
>  hmp.h            |    1 +
>  migration.c      |   23 ++++++++++++++++++++++-
>  migration.h      |    2 ++
>  qapi-schema.json |   16 ++++++++++++++++
>  qmp-commands.hx  |   23 +++++++++++++++++++++++
>  8 files changed, 104 insertions(+), 1 deletions(-)
>
> diff --git a/arch_init.c b/arch_init.c
> index bfc9f27..ae7c8b1 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -24,6 +24,7 @@
>  #include <stdint.h>
>  #include <stdarg.h>
>  #include <stdlib.h>
> +#include <math.h>
>  #ifndef _WIN32
>  #include <sys/types.h>
>  #include <sys/mman.h>
> @@ -192,6 +193,14 @@ static struct {
>     .cache = NULL,
>  };
>
> +
> +void xbzrle_cache_resize(int64_t new_size)
> +{
> +    if (XBZRLE.cache != NULL) {
> +        cache_resize(XBZRLE.cache, new_size/TARGET_PAGE_SIZE);

Please add spaces around '/'. Using checkpatch.pl would catch these.

> +    }
> +}
> +
>  static void save_block_hdr(QEMUFile *f, RAMBlock *block, ram_addr_t offset,
>         int cont, int flag)
>  {
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index a0c8df2..ef60dc0 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -829,6 +829,24 @@ 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 will be round down to the nearest power of 2.\n"
> +                     "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 c275fab..0a27d2d 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -755,6 +755,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 09ba198..7c5117d 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -53,6 +53,7 @@ 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_parameter(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 6063ed8..a892ae1 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -22,6 +22,7 @@
>  #include "qemu_socket.h"
>  #include "block-migration.h"
>  #include "qmp-commands.h"
> +#include <math.h>
>
>  //#define DEBUG_MIGRATION
>
> @@ -43,7 +44,7 @@ enum {
>
>  #define MAX_THROTTLE  (32 << 20)      /* Migration speed throttling */
>
> -/* Migration XBZRLE cache size */
> +/* Migration XBZRLE default cache size */
>  #define DEFAULT_MIGRATE_CACHE_SIZE (64 * 1024 * 1024)
>
>  static NotifierList migration_state_notifiers =
> @@ -519,6 +520,26 @@ void qmp_migrate_cancel(Error **errp)
>     migrate_fd_cancel(migrate_get_current());
>  }
>
> +void qmp_migrate_set_cachesize(int64_t value, Error **errp)
> +{
> +    MigrationState *s = migrate_get_current();
> +
> +    /* Check for truncation */
> +    if (value != (size_t)value) {
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> +                  "exceeding address space");
> +        return;
> +    }
> +
> +    /* no change */
> +    if (value == s->xbzrle_cache_size) {
> +        return;
> +    }
> +
> +    s->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 4fe8122..83dba4a 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -107,4 +107,6 @@ int xbzrle_decode_buffer(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 ccc5eb3..982c93a 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1379,6 +1379,22 @@
>  { 'command': 'migrate_set_speed', 'data': {'value': 'int'} }
>
>  ##
> +# @migrate_set_cachesize
> +#
> +# Set XBZRLE cache size
> +#
> +# @value: cache size in bytes
> +#
> +# The size will be round down to the nearest power of 2.
> +# The cache size can be modified before and during ongoing migration
> +#
> +# Returns: nothing on success
> +#
> +# Since: 1.2
> +##
> +{ '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 85b5920..0ca5cbb 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -520,6 +520,29 @@ 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, the cache size will be round down
> +to the nearset power of 2
> +
> +Arguments:
> +
> +- "value": cache size in bytes (json-int)
> +
> +Example:
> +
> +-> { "execute": "migrate_set_cachesize", "arguments": { "value": 512M } }
> +<- { "return": {} }
> +
> +EQMP
>
>     {
>         .name       = "migrate_set_speed",
> --
> 1.7.7.6
>
Orit Wasserman June 27, 2012, 6 p.m. UTC | #2 
On 06/27/2012 08:56 PM, Blue Swirl wrote:
> On Wed, Jun 27, 2012 at 10:34 AM, 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.
>>
>> 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>
>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>> ---
>>  arch_init.c      |    9 +++++++++
>>  hmp-commands.hx  |   18 ++++++++++++++++++
>>  hmp.c            |   13 +++++++++++++
>>  hmp.h            |    1 +
>>  migration.c      |   23 ++++++++++++++++++++++-
>>  migration.h      |    2 ++
>>  qapi-schema.json |   16 ++++++++++++++++
>>  qmp-commands.hx  |   23 +++++++++++++++++++++++
>>  8 files changed, 104 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch_init.c b/arch_init.c
>> index bfc9f27..ae7c8b1 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -24,6 +24,7 @@
>>  #include <stdint.h>
>>  #include <stdarg.h>
>>  #include <stdlib.h>
>> +#include <math.h>
>>  #ifndef _WIN32
>>  #include <sys/types.h>
>>  #include <sys/mman.h>
>> @@ -192,6 +193,14 @@ static struct {
>>     .cache = NULL,
>>  };
>>
>> +
>> +void xbzrle_cache_resize(int64_t new_size)
>> +{
>> +    if (XBZRLE.cache != NULL) {
>> +        cache_resize(XBZRLE.cache, new_size/TARGET_PAGE_SIZE);
> 
> Please add spaces around '/'. Using checkpatch.pl would catch these.
It actually does not catch it ...
> 
>> +    }
>> +}
>> +
>>  static void save_block_hdr(QEMUFile *f, RAMBlock *block, ram_addr_t offset,
>>         int cont, int flag)
>>  {
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index a0c8df2..ef60dc0 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -829,6 +829,24 @@ 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 will be round down to the nearest power of 2.\n"
>> +                     "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 c275fab..0a27d2d 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -755,6 +755,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 09ba198..7c5117d 100644
>> --- a/hmp.h
>> +++ b/hmp.h
>> @@ -53,6 +53,7 @@ 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_parameter(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 6063ed8..a892ae1 100644
>> --- a/migration.c
>> +++ b/migration.c
>> @@ -22,6 +22,7 @@
>>  #include "qemu_socket.h"
>>  #include "block-migration.h"
>>  #include "qmp-commands.h"
>> +#include <math.h>
>>
>>  //#define DEBUG_MIGRATION
>>
>> @@ -43,7 +44,7 @@ enum {
>>
>>  #define MAX_THROTTLE  (32 << 20)      /* Migration speed throttling */
>>
>> -/* Migration XBZRLE cache size */
>> +/* Migration XBZRLE default cache size */
>>  #define DEFAULT_MIGRATE_CACHE_SIZE (64 * 1024 * 1024)
>>
>>  static NotifierList migration_state_notifiers =
>> @@ -519,6 +520,26 @@ void qmp_migrate_cancel(Error **errp)
>>     migrate_fd_cancel(migrate_get_current());
>>  }
>>
>> +void qmp_migrate_set_cachesize(int64_t value, Error **errp)
>> +{
>> +    MigrationState *s = migrate_get_current();
>> +
>> +    /* Check for truncation */
>> +    if (value != (size_t)value) {
>> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
>> +                  "exceeding address space");
>> +        return;
>> +    }
>> +
>> +    /* no change */
>> +    if (value == s->xbzrle_cache_size) {
>> +        return;
>> +    }
>> +
>> +    s->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 4fe8122..83dba4a 100644
>> --- a/migration.h
>> +++ b/migration.h
>> @@ -107,4 +107,6 @@ int xbzrle_decode_buffer(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 ccc5eb3..982c93a 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -1379,6 +1379,22 @@
>>  { 'command': 'migrate_set_speed', 'data': {'value': 'int'} }
>>
>>  ##
>> +# @migrate_set_cachesize
>> +#
>> +# Set XBZRLE cache size
>> +#
>> +# @value: cache size in bytes
>> +#
>> +# The size will be round down to the nearest power of 2.
>> +# The cache size can be modified before and during ongoing migration
>> +#
>> +# Returns: nothing on success
>> +#
>> +# Since: 1.2
>> +##
>> +{ '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 85b5920..0ca5cbb 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -520,6 +520,29 @@ 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, the cache size will be round down
>> +to the nearset power of 2
>> +
>> +Arguments:
>> +
>> +- "value": cache size in bytes (json-int)
>> +
>> +Example:
>> +
>> +-> { "execute": "migrate_set_cachesize", "arguments": { "value": 512M } }
>> +<- { "return": {} }
>> +
>> +EQMP
>>
>>     {
>>         .name       = "migrate_set_speed",
>> --
>> 1.7.7.6
>>
Eric Blake June 27, 2012, 8:04 p.m. UTC | #3 
On 06/27/2012 04:34 AM, 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: Benoit Hudzia <benoit.hudzia@sap.com>
> Signed-off-by: Petter Svard <petters@cs.umu.se>
> Signed-off-by: Aidan Shribman <aidan.shribman@sap.com>
> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> ---
>  arch_init.c      |    9 +++++++++
>  hmp-commands.hx  |   18 ++++++++++++++++++
>  hmp.c            |   13 +++++++++++++
>  hmp.h            |    1 +
>  migration.c      |   23 ++++++++++++++++++++++-
>  migration.h      |    2 ++
>  qapi-schema.json |   16 ++++++++++++++++
>  qmp-commands.hx  |   23 +++++++++++++++++++++++
>  8 files changed, 104 insertions(+), 1 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index bfc9f27..ae7c8b1 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -24,6 +24,7 @@
>  #include <stdint.h>
>  #include <stdarg.h>
>  #include <stdlib.h>
> +#include <math.h>

Why?

Furthermore, I think I asked the same question about v12; it's rather
depressing to review a patch when not all the review comments from the
previous revision were addressed.

> +        .help       = "set cache size (in bytes) for XBZRLE migrations,"
> +		      "the cache size will be round down to the nearest power of 2.\n"

s/round/rounded/


> +void qmp_migrate_set_cachesize(int64_t value, Error **errp)
> +{
> +    MigrationState *s = migrate_get_current();
> +
> +    /* Check for truncation */
> +    if (value != (size_t)value) {
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> +                  "exceeding address space");
> +        return;
> +    }
> +
> +    /* no change */
> +    if (value == s->xbzrle_cache_size) {
> +        return;

Another place where factoring the rounding-down will make it more
efficient.  On the other hand, why do you even worry about it, since...

> +    }
> +
> +    s->xbzrle_cache_size = value;
> +    xbzrle_cache_resize(value);

...xbzrle_cache_resize should be a relatively fast no-op if the
rounded-down size is equal?  Also, aren't you better off storing the
rounded value and not the user's value (in which case, should
xbzrle_cache_resize return a value)?  For that matter, what if the user
requests a resize to a larger value that exceeds available host memory?
 Normally, out-of-memory makes qemu exit, but in this particular case,
we are better off leaving qemu running but just failing the monitor command.


> +# @migrate_set_cachesize
> +#
> +# Set XBZRLE cache size
> +#
> +# @value: cache size in bytes
> +#
> +# The size will be round down to the nearest power of 2.

s/round/rounded/

> +Example:
> +
> +-> { "execute": "migrate_set_cachesize", "arguments": { "value": 512M } }

512M is not a json-int.  I really don't think you even saw my v12
comments on this patch.
Orit Wasserman June 28, 2012, 7:18 a.m. UTC | #4 
On 06/27/2012 11:04 PM, Eric Blake wrote:
> On 06/27/2012 04:34 AM, 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: Benoit Hudzia <benoit.hudzia@sap.com>
>> Signed-off-by: Petter Svard <petters@cs.umu.se>
>> Signed-off-by: Aidan Shribman <aidan.shribman@sap.com>
>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>> ---
>>  arch_init.c      |    9 +++++++++
>>  hmp-commands.hx  |   18 ++++++++++++++++++
>>  hmp.c            |   13 +++++++++++++
>>  hmp.h            |    1 +
>>  migration.c      |   23 ++++++++++++++++++++++-
>>  migration.h      |    2 ++
>>  qapi-schema.json |   16 ++++++++++++++++
>>  qmp-commands.hx  |   23 +++++++++++++++++++++++
>>  8 files changed, 104 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch_init.c b/arch_init.c
>> index bfc9f27..ae7c8b1 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -24,6 +24,7 @@
>>  #include <stdint.h>
>>  #include <stdarg.h>
>>  #include <stdlib.h>
>> +#include <math.h>
> 
> Why?
> 
> Furthermore, I think I asked the same question about v12; it's rather
> depressing to review a patch when not all the review comments from the
> previous revision were addressed.
> 
I'm sorry , I'm somehow lost all the fixes I did to this patch.
I will be more careful next time.
Thanks a lot for reviewing it.
 
>> +        .help       = "set cache size (in bytes) for XBZRLE migrations,"
>> +		      "the cache size will be round down to the nearest power of 2.\n"
> 
> s/round/rounded/
> 
> 
>> +void qmp_migrate_set_cachesize(int64_t value, Error **errp)
>> +{
>> +    MigrationState *s = migrate_get_current();
>> +
>> +    /* Check for truncation */
>> +    if (value != (size_t)value) {
>> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
>> +                  "exceeding address space");
>> +        return;
>> +    }
>> +
>> +    /* no change */
>> +    if (value == s->xbzrle_cache_size) {
>> +        return;
> 
> Another place where factoring the rounding-down will make it more
> efficient.  On the other hand, why do you even worry about it, since...
> 
>> +    }
>> +
>> +    s->xbzrle_cache_size = value;
>> +    xbzrle_cache_resize(value);
> 
> ...xbzrle_cache_resize should be a relatively fast no-op if the
> rounded-down size is equal?  Also, aren't you better off storing the
> rounded value and not the user's value (in which case, should
> xbzrle_cache_resize return a value)?  For that matter, what if the user
> requests a resize to a larger value that exceeds available host memory?
>  Normally, out-of-memory makes qemu exit, but in this particular case,
> we are better off leaving qemu running but just failing the monitor command.
> 
Correct I should store the rounded size not user size.I will change
resize to return the new size.

I'm not sure it is possible to know what is the available host memory from QEMU,
it can also reduce with time.
We don't allocate a large size of memory here (only meta data) , the allocation is done
when caching a new page. The cache doesn't have to be full.
So checking available memory here doesn't guaranty that we will actually have memory later.
 
I agree aborting here is not the best behavior but it can happen with regular migration too
sadly. In regular migration the incoming QEMU can abort on out of memory when starting the guest.
 
I can switch back to malloc and check the return value, would that be ok ?

Orit
> 
>> +# @migrate_set_cachesize
>> +#
>> +# Set XBZRLE cache size
>> +#
>> +# @value: cache size in bytes
>> +#
>> +# The size will be round down to the nearest power of 2.
> 
> s/round/rounded/
> 
>> +Example:
>> +
>> +-> { "execute": "migrate_set_cachesize", "arguments": { "value": 512M } }
> 
> 512M is not a json-int.  I really don't think you even saw my v12
> comments on this patch.
>
diff mbox

Patch

diff --git a/arch_init.c b/arch_init.c
index bfc9f27..ae7c8b1 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -24,6 +24,7 @@ 
 #include <stdint.h>
 #include <stdarg.h>
 #include <stdlib.h>
+#include <math.h>
 #ifndef _WIN32
 #include <sys/types.h>
 #include <sys/mman.h>
@@ -192,6 +193,14 @@  static struct {
     .cache = NULL,
 };
 
+
+void xbzrle_cache_resize(int64_t new_size)
+{
+    if (XBZRLE.cache != NULL) {
+        cache_resize(XBZRLE.cache, new_size/TARGET_PAGE_SIZE);
+    }
+}
+
 static void save_block_hdr(QEMUFile *f, RAMBlock *block, ram_addr_t offset,
         int cont, int flag)
 {
diff --git a/hmp-commands.hx b/hmp-commands.hx
index a0c8df2..ef60dc0 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -829,6 +829,24 @@  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 will be round down to the nearest power of 2.\n"
+		      "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 c275fab..0a27d2d 100644
--- a/hmp.c
+++ b/hmp.c
@@ -755,6 +755,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 09ba198..7c5117d 100644
--- a/hmp.h
+++ b/hmp.h
@@ -53,6 +53,7 @@  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_parameter(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 6063ed8..a892ae1 100644
--- a/migration.c
+++ b/migration.c
@@ -22,6 +22,7 @@ 
 #include "qemu_socket.h"
 #include "block-migration.h"
 #include "qmp-commands.h"
+#include <math.h>
 
 //#define DEBUG_MIGRATION
 
@@ -43,7 +44,7 @@  enum {
 
 #define MAX_THROTTLE  (32 << 20)      /* Migration speed throttling */
 
-/* Migration XBZRLE cache size */
+/* Migration XBZRLE default cache size */
 #define DEFAULT_MIGRATE_CACHE_SIZE (64 * 1024 * 1024)
 
 static NotifierList migration_state_notifiers =
@@ -519,6 +520,26 @@  void qmp_migrate_cancel(Error **errp)
     migrate_fd_cancel(migrate_get_current());
 }
 
+void qmp_migrate_set_cachesize(int64_t value, Error **errp)
+{
+    MigrationState *s = migrate_get_current();
+
+    /* Check for truncation */
+    if (value != (size_t)value) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
+                  "exceeding address space");
+        return;
+    }
+
+    /* no change */
+    if (value == s->xbzrle_cache_size) {
+        return;
+    }
+
+    s->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 4fe8122..83dba4a 100644
--- a/migration.h
+++ b/migration.h
@@ -107,4 +107,6 @@  int xbzrle_decode_buffer(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 ccc5eb3..982c93a 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1379,6 +1379,22 @@ 
 { 'command': 'migrate_set_speed', 'data': {'value': 'int'} }
 
 ##
+# @migrate_set_cachesize
+#
+# Set XBZRLE cache size
+#
+# @value: cache size in bytes
+#
+# The size will be round down to the nearest power of 2.
+# The cache size can be modified before and during ongoing migration
+#
+# Returns: nothing on success
+#
+# Since: 1.2
+##
+{ '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 85b5920..0ca5cbb 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -520,6 +520,29 @@  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, the cache size will be round down
+to the nearset power of 2
+
+Arguments:
+
+- "value": cache size in bytes (json-int)
+
+Example:
+
+-> { "execute": "migrate_set_cachesize", "arguments": { "value": 512M } }
+<- { "return": {} }
+
+EQMP
 
     {
         .name       = "migrate_set_speed",