| Message ID | 1337169582-28312-9-git-send-email-owasserm@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 05/16/2012 05:59 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> > > + > +void xbzrle_cache_resize(int64_t order) > +{ > + cache_resize(XBZRLE.cache, pow(2, order)); '1 << order' is much more efficient than a call to pow(). > +void qmp_migrate_set_cachesize(int64_t value, Error **errp) > + > + /* power of 2 */ > + if (value != 1 && !is_power_of_2(value)) { > + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size", > + "needs to be power of 2"); We already have QERR_PROPERTY_VALUE_NOT_POWER_OF_2, why aren't you using that here? > + return; > + } > + > + s->xbzrle_cache_size = value; > + xbzrle_cache_resize(log2(value)); log2() is rather expensive, ffs() from <strings.h> is more efficient at converting a single bit into the appropriate order. > ## > +# @migrate_set_cachesize > +# > +# Set XBZRLE cache size > +# > +# @value: cache size in bytes > +# > +# Returns: nothing on success Document the error for a non-power-of-2 or for overflow. Document whether this command is safe for an ongoing migration, or whether it must be called in advance of a migration. > +# > +# Since: 1.1 1.2. > +static inline bool is_power_of_2(int64_t value) > +{ > + return !(value & (value - 1)); > +} This says '0' is a power of 2, which is not true. Either fix the logic to exclude 0, or fix the function name to state that you are really checking that at most one bit is set. Also, if value is 0x8000000000000000, you are triggering unspecified behavior per C99. Is it worth using uint64_t for defined behavior, or do you need to take precautions regarding negative values? > +SQMP > +migrate_set_cachesize > +--------------------- > + > +Set cache size to be used by XBZRLE migration > + > +Arguments: > + > +- "value": cache size in bytes (json-int) Would it be any easier to take 'order' (log2 of the size) instead of the actual cache size? That is, instead of calling "value":1048576, I would rather type "value":20. > + > +Example: > + > +-> { "execute": "migrate_set_cachesize", "arguments": { "value": 512 } } Isn't 512 bytes rather small? And given my argument about taking order rather than bytes as being easier to use, don't you really mean 512 megabytes (order 29) rather than 512 bytes (order 9)?
On 05/16/2012 07:45 PM, Eric Blake wrote: > On 05/16/2012 05:59 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> > >> >> + >> +void xbzrle_cache_resize(int64_t order) >> +{ >> + cache_resize(XBZRLE.cache, pow(2, order)); > > '1 << order' is much more efficient than a call to pow(). ok > > >> +void qmp_migrate_set_cachesize(int64_t value, Error **errp) > >> + >> + /* power of 2 */ >> + if (value != 1 && !is_power_of_2(value)) { >> + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size", >> + "needs to be power of 2"); > > We already have QERR_PROPERTY_VALUE_NOT_POWER_OF_2, why aren't you using > that here? I will update it. > >> + return; >> + } >> + >> + s->xbzrle_cache_size = value; >> + xbzrle_cache_resize(log2(value)); > > log2() is rather expensive, ffs() from <strings.h> is more efficient at > converting a single bit into the appropriate order. ok > > >> ## >> +# @migrate_set_cachesize >> +# >> +# Set XBZRLE cache size >> +# >> +# @value: cache size in bytes >> +# >> +# Returns: nothing on success > > Document the error for a non-power-of-2 or for overflow. > > Document whether this command is safe for an ongoing migration, or > whether it must be called in advance of a migration. sure > >> +# >> +# Since: 1.1 > > 1.2. > > >> +static inline bool is_power_of_2(int64_t value) >> +{ >> + return !(value & (value - 1)); >> +} > > This says '0' is a power of 2, which is not true. Either fix the logic > to exclude 0, or fix the function name to state that you are really > checking that at most one bit is set. > > Also, if value is 0x8000000000000000, you are triggering unspecified > behavior per C99. Is it worth using uint64_t for defined behavior, or > do you need to take precautions regarding negative values? The input is int64 so I prefer to keep it this way. The calling function does the check for 0 , negative numbers and overflow but I can add those checks here too. > > >> +SQMP >> +migrate_set_cachesize >> +--------------------- >> + >> +Set cache size to be used by XBZRLE migration >> + >> +Arguments: >> + >> +- "value": cache size in bytes (json-int) > > Would it be any easier to take 'order' (log2 of the size) instead of the > actual cache size? That is, instead of calling "value":1048576, I would > rather type "value":20. Well the user is considering how much memory is going to be used and I though that it is simpler to use 1G than 30. But I guess the user is libvirt so it can be changed to order. > >> + >> +Example: >> + >> +-> { "execute": "migrate_set_cachesize", "arguments": { "value": 512 } } > > Isn't 512 bytes rather small? And given my argument about taking order > rather than bytes as being easier to use, don't you really mean 512 > megabytes (order 29) rather than 512 bytes (order 9)? > correct 512M not bytes ... Orit
On 05/16/2012 11:04 AM, Orit Wasserman wrote: >>> +- "value": cache size in bytes (json-int) >> >> Would it be any easier to take 'order' (log2 of the size) instead of the >> actual cache size? That is, instead of calling "value":1048576, I would >> rather type "value":20. > Well the user is considering how much memory is going to be used and I though that it > is simpler to use 1G than 30. Libvirt can cope with either style, so maybe it's worth waiting for anyone else to chime in on which style is easier.
On 05/16/2012 08:58 PM, Eric Blake wrote: > On 05/16/2012 11:04 AM, Orit Wasserman wrote: > > >>> +- "value": cache size in bytes (json-int) > >> > >> Would it be any easier to take 'order' (log2 of the size) instead of the > >> actual cache size? That is, instead of calling "value":1048576, I would > >> rather type "value":20. > > Well the user is considering how much memory is going to be used and I though that it > > is simpler to use 1G than 30. > > Libvirt can cope with either style, so maybe it's worth waiting for > anyone else to chime in on which style is easier. Let's be consistent. It's best to use bytes everywhere (not kilobytes, not megabytes, not pages, not order, or anything else we can come up with). If you really want to specify order (not that I can think of a reason why), we can use a suffix: 20ORD == 1M == 1048576. btw, maybe it's better to handle a non-power-of-two cache size by rounding down. Less errors, less puzzlement, and less memory used.
On 05/17/2012 12:46 PM, Avi Kivity wrote: > On 05/16/2012 08:58 PM, Eric Blake wrote: >> On 05/16/2012 11:04 AM, Orit Wasserman wrote: >> >>>>> +- "value": cache size in bytes (json-int) >>>> >>>> Would it be any easier to take 'order' (log2 of the size) instead of the >>>> actual cache size? That is, instead of calling "value":1048576, I would >>>> rather type "value":20. >>> Well the user is considering how much memory is going to be used and I though that it >>> is simpler to use 1G than 30. >> >> Libvirt can cope with either style, so maybe it's worth waiting for >> anyone else to chime in on which style is easier. > > Let's be consistent. It's best to use bytes everywhere (not kilobytes, > not megabytes, not pages, not order, or anything else we can come up with). > > If you really want to specify order (not that I can think of a reason > why), we can use a suffix: 20ORD == 1M == 1048576. That is what used at the moment. > > btw, maybe it's better to handle a non-power-of-two cache size by > rounding down. Less errors, less puzzlement, and less memory used. Sounds good to me. Orit >
diff --git a/arch_init.c b/arch_init.c index 7ebdb7a..851e45d 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> @@ -153,6 +154,12 @@ static struct { Cache *cache; } XBZRLE = {0}; + +void xbzrle_cache_resize(int64_t order) +{ + cache_resize(XBZRLE.cache, pow(2, order)); +} + 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 e14e7be..abc9403 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -829,6 +829,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 e73132b..0e4d63a 100644 --- a/hmp.c +++ b/hmp.c @@ -751,6 +751,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 5f9d842..9559559 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 ba11adb..4fb3b8a 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 = @@ -501,6 +502,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 = migrate_get_current(); + + /* Check for truncation */ + if (value != (size_t)value) { + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size", + "exceeding address space"); + return; + } + + value = MIN(UINT64_MAX, value); + + /* no change */ + if (value == s->xbzrle_cache_size) { + return; + } + + /* power of 2 */ + if (value != 1 && !is_power_of_2(value)) { + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size", + "needs to be power of 2"); + return; + } + + s->xbzrle_cache_size = value; + xbzrle_cache_resize(log2(value)); +} + void qmp_migrate_set_speed(int64_t value, Error **errp) { MigrationState *s; diff --git a/migration.h b/migration.h index 175c729..6a5bc0e 100644 --- a/migration.h +++ b/migration.h @@ -106,4 +106,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 2887c51..4d30552 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1357,6 +1357,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/qemu-common.h b/qemu-common.h index 3d0f66f..da44c17 100644 --- a/qemu-common.h +++ b/qemu-common.h @@ -415,4 +415,9 @@ static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c) int uleb128_encode_small(uint8_t *out, uint32_t n); int uleb128_decode_small(const uint8_t *in, uint32_t *n); +static inline bool is_power_of_2(int64_t value) +{ + return !(value & (value - 1)); +} + #endif diff --git a/qmp-commands.hx b/qmp-commands.hx index f276e08..3bdbaae 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -520,6 +520,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",