* [Qemu-devel] [PATCH v14 00/13] XBZRLE delta for live migration of large memory app
@ 2012-07-03 13:52 Orit Wasserman
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 01/13] Add MigrationParams structure Orit Wasserman
` (12 more replies)
0 siblings, 13 replies; 33+ messages in thread
From: Orit Wasserman @ 2012-07-03 13:52 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, aliguori, quintela, Petter Svard, stefanha,
mdroth, Benoit Hudzia, blauwirbel, Orit Wasserman, chegu_vinod,
avi, Aidan Shribman, pbonzini, eblake
Changes from v13:
- Fix round to power of 2 of cache size
- Add more checks to the XBZRLE encoding.
- use comparison instead of XOR when calculating zrun_len
- use strcmp trick for calculating nzrun_len (algorithm from Eric Blake)
- Fix other comments by Eric Blake
- Fix comments from Blue Swirl
- Display migration statics after migration completes
Changes from v12:
- use bool for blk and shared params
- use long when decoding buffer
- fix QMP commands
- always display migration parameters in "info migrate"
- update current_addr inside the while loop in ram_save_block
- display statistics after migration completes
- fix other review comments from Eric Blake
Changes from v11:
- divide patch 7 to several smaller patches.
- Use an array for setting migration parameters QMP only (there
is not support for arrays in HMP commands). parameters can be enabled
or disabled.
- Do not use XBZRLE in stage 3 , it is a very sensitive stage and CPU
can be an issue.
- Fix review comments by Juan Quintela and Eric Blake
Changes from v10:
- Cache size will be in bytes, in case it is not a power of 2 it will be
reduced to the nearest power of 2.
- fix documentation
- use cache_init with number of pages not cache size.
Changes from v9:
- move cache implementation to separate files. Kept our own implementation because GCache or GHashTable have no size limit.
- Add migrate_set_parameter function
- removed XBZRLE option from migrate command
- add cache size information to query_migrate command
- add documantation file
- write/read the exact XBZRLE header format
- fix other review comments by Anthony and Juan
Changes from v8:
Implement more effiecent cache_resize method
fix set_cachesize command
Changes from v7:
Copy current page before encoding it, this will prevents page content
change during the encoding.
Allow changing the cache size during an active migration.
Fix comments by Avi.
Changes from v6:
1) add assert checks to ULEB encoding/decoding
2) no need to send last zero run
Changes from v5:
1) Add migration capabilities
2) Use ULEB to encode run length
3) Do not send unmodified (dirty) page
3) Fix other patch comments
Using GCache or GHashTable requires allocating new buffer on every content change and have no size limit ,
so I decided to keep the simple cache implementation.
Changes from v4:
1) Rebase
2) divide patch into 9 patches
3) move memory allocation into cache_insert
Future work :
Use SSE for encoding.
Page ranking acording to their dirty rate and automatic activation/deactivation of the feature - will be sent in a separate patch series.
By using XBZRLE (Xor Based Zero Run Length Encoding) we can reduce VM downtime
and total live-migration time of VMs running memory write intensive workloads
typical of large enterprise applications such as SAP ERP Systems, and generally
speaking for any application with a sparse memory update pattern.
The compression format uses the fact that we will have many zero (zero represents
an unchanged value).
We repesent the page data delta by zero and non zero runs.
We represent a zero run with it's length (in bytes).
We represent a non zero run with it's length (in bytes) and the data.
The run length is encoded using ULEB128 (http://en.wikipedia.org/wiki/LEB128)
page = zrun nzrun
| zrun nzrun page
zrun = length
nzrun = length byte...
length = uleb128 encoded integer
On the sender side XBZRLE is used as a compact delta encoding of page updates,
retrieving the old page content from an LRU cache (default size of 512 MB). The
receiving side uses the existing page content and XBZRLE to decode the new page
content.
This is a more compact way to store the delta than the previous version.
This work was originally based on research results published VEE 2011: Evaluation of
Delta Compression Techniques for Efficient Live Migration of Large Virtual
Machines by Benoit, Svard, Tordsson and Elmroth. Additionally the delta encoder
XBRLE was improved further using XBZRLE instead.
XBZRLE has a sustained bandwidth of 2-2.5 GB/s for typical workloads making it
ideal for in-line, real-time encoding such as is needed for live-migration.
A typical usage scenario:
{qemu} migrate_set_cachesize 256m
{qemu} migrate_set_parameter xbzrle
{qemu} migrate -d tcp:destination.host:4444
{qemu} info migrate
...
transferred ram: A kbytes
remaining ram: B kbytes
total ram: C kbytes
cache size: D bytes
xbzrle transferred: E kbytes
xbzrle pages: F pages
xbzrle cache miss: G
xbzrle overflow : H
Testing: live migration with XBZRLE completed in 110 seconds, without live
migration was not able to complete.
A simple synthetic memory r/w load generator:
.. include <stdlib.h>
.. include <stdio.h>
.. int main()
.. {
.. char *buf = (char *) calloc(4096, 4096);
.. while (1) {
.. int i;
.. for (i = 0; i < 4096 * 4; i++) {
.. buf[i * 4096 / 4]++;
.. }
.. printf(".");
.. }
.. }
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>
Orit Wasserman (13):
Add MigrationParams structure
Add migration capabilities
Add XBZRLE documentation
Add cache handling functions
Add uleb encoding/decoding functions
Add save_block_hdr function
Add debugging infrastructure
Change ram_save_block to return -1 if there are no more changes
Add migration_end function
Add xbzrle_encode_buffer and xbzrle_decode_buffer functions
Add XBZRLE to ram_save_block and ram_save_live
Add set_cachesize command
Add XBZRLE statistics
Makefile.objs | 1 +
arch_init.c | 336 +++++++++++++++++++++++++++++++++++++++++----
block-migration.c | 8 +-
cutils.c | 32 +++++
docs/xbzrle.txt | 133 ++++++++++++++++++
hmp-commands.hx | 35 +++++
hmp.c | 89 ++++++++++++
hmp.h | 3 +
include/qemu/page_cache.h | 79 +++++++++++
migration.c | 175 ++++++++++++++++++++++-
migration.h | 29 ++++-
monitor.c | 7 +
page_cache.c | 209 ++++++++++++++++++++++++++++
qapi-schema.json | 102 +++++++++++++-
qemu-common.h | 27 ++++
qmp-commands.hx | 111 +++++++++++++++-
savevm.c | 185 ++++++++++++++++++++++++-
sysemu.h | 3 +-
vmstate.h | 2 +-
19 files changed, 1511 insertions(+), 55 deletions(-)
create mode 100644 docs/xbzrle.txt
create mode 100644 include/qemu/page_cache.h
create mode 100644 page_cache.c
--
1.7.7.6
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v14 01/13] Add MigrationParams structure
2012-07-03 13:52 [Qemu-devel] [PATCH v14 00/13] XBZRLE delta for live migration of large memory app Orit Wasserman
@ 2012-07-03 13:52 ` Orit Wasserman
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 02/13] Add migration capabilities Orit Wasserman
` (11 subsequent siblings)
12 siblings, 0 replies; 33+ messages in thread
From: Orit Wasserman @ 2012-07-03 13:52 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, aliguori, quintela, stefanha, mdroth, blauwirbel,
Orit Wasserman, chegu_vinod, avi, pbonzini, eblake,
Isaku Yamahata
From: Isaku Yamahata <yamahata@valinux.co.jp>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
block-migration.c | 8 ++++----
migration.c | 13 ++++++++-----
migration.h | 8 ++++++--
qemu-common.h | 1 +
savevm.c | 13 +++++++++----
sysemu.h | 3 ++-
vmstate.h | 2 +-
7 files changed, 31 insertions(+), 17 deletions(-)
diff --git a/block-migration.c b/block-migration.c
index fd2ffff..b95b4e1 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -700,13 +700,13 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
return 0;
}
-static void block_set_params(int blk_enable, int shared_base, void *opaque)
+static void block_set_params(const MigrationParams *params, void *opaque)
{
- block_mig_state.blk_enable = blk_enable;
- block_mig_state.shared_base = shared_base;
+ block_mig_state.blk_enable = params->blk;
+ block_mig_state.shared_base = params->shared;
/* shared base means that blk_enable = 1 */
- block_mig_state.blk_enable |= shared_base;
+ block_mig_state.blk_enable |= params->shared;
}
void blk_mig_init(void)
diff --git a/migration.c b/migration.c
index 3f485d3..810727f 100644
--- a/migration.c
+++ b/migration.c
@@ -352,7 +352,7 @@ void migrate_fd_connect(MigrationState *s)
migrate_fd_close);
DPRINTF("beginning savevm\n");
- ret = qemu_savevm_state_begin(s->file, s->blk, s->shared);
+ ret = qemu_savevm_state_begin(s->file, &s->params);
if (ret < 0) {
DPRINTF("failed, %d\n", ret);
migrate_fd_error(s);
@@ -361,15 +361,14 @@ void migrate_fd_connect(MigrationState *s)
migrate_fd_put_ready(s);
}
-static MigrationState *migrate_init(int blk, int inc)
+static MigrationState *migrate_init(const MigrationParams *params)
{
MigrationState *s = migrate_get_current();
int64_t bandwidth_limit = s->bandwidth_limit;
memset(s, 0, sizeof(*s));
s->bandwidth_limit = bandwidth_limit;
- s->blk = blk;
- s->shared = inc;
+ s->params = *params;
s->bandwidth_limit = bandwidth_limit;
s->state = MIG_STATE_SETUP;
@@ -394,9 +393,13 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
Error **errp)
{
MigrationState *s = migrate_get_current();
+ MigrationParams params;
const char *p;
int ret;
+ params.blk = blk;
+ params.shared = inc;
+
if (s->state == MIG_STATE_ACTIVE) {
error_set(errp, QERR_MIGRATION_ACTIVE);
return;
@@ -411,7 +414,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
return;
}
- s = migrate_init(blk, inc);
+ s = migrate_init(¶ms);
if (strstart(uri, "tcp:", &p)) {
ret = tcp_start_outgoing_migration(s, p, errp);
diff --git a/migration.h b/migration.h
index 2e9ca2e..35207bd 100644
--- a/migration.h
+++ b/migration.h
@@ -19,6 +19,11 @@
#include "notify.h"
#include "error.h"
+struct MigrationParams {
+ bool blk;
+ bool shared;
+};
+
typedef struct MigrationState MigrationState;
struct MigrationState
@@ -31,8 +36,7 @@ struct MigrationState
int (*close)(MigrationState *s);
int (*write)(MigrationState *s, const void *buff, size_t size);
void *opaque;
- int blk;
- int shared;
+ MigrationParams params;
};
void process_incoming_migration(QEMUFile *f);
diff --git a/qemu-common.h b/qemu-common.h
index 9d9e603..c8c6b2a 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -17,6 +17,7 @@ typedef struct DeviceState DeviceState;
struct Monitor;
typedef struct Monitor Monitor;
+typedef struct MigrationParams MigrationParams;
/* we put basic includes here to avoid repeating them in device drivers */
#include <stdlib.h>
diff --git a/savevm.c b/savevm.c
index faa8145..d1d9020 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1561,7 +1561,8 @@ bool qemu_savevm_state_blocked(Error **errp)
return false;
}
-int qemu_savevm_state_begin(QEMUFile *f, int blk_enable, int shared)
+int qemu_savevm_state_begin(QEMUFile *f,
+ const MigrationParams *params)
{
SaveStateEntry *se;
int ret;
@@ -1569,8 +1570,8 @@ int qemu_savevm_state_begin(QEMUFile *f, int blk_enable, int shared)
QTAILQ_FOREACH(se, &savevm_handlers, entry) {
if(se->set_params == NULL) {
continue;
- }
- se->set_params(blk_enable, shared, se->opaque);
+ }
+ se->set_params(params, se->opaque);
}
qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
@@ -1708,13 +1709,17 @@ void qemu_savevm_state_cancel(QEMUFile *f)
static int qemu_savevm_state(QEMUFile *f)
{
int ret;
+ MigrationParams params = {
+ .blk = 0,
+ .shared = 0
+ };
if (qemu_savevm_state_blocked(NULL)) {
ret = -EINVAL;
goto out;
}
- ret = qemu_savevm_state_begin(f, 0, 0);
+ ret = qemu_savevm_state_begin(f, ¶ms);
if (ret < 0)
goto out;
diff --git a/sysemu.h b/sysemu.h
index bc2c788..6540c79 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -77,7 +77,8 @@ void do_info_snapshots(Monitor *mon);
void qemu_announce_self(void);
bool qemu_savevm_state_blocked(Error **errp);
-int qemu_savevm_state_begin(QEMUFile *f, int blk_enable, int shared);
+int qemu_savevm_state_begin(QEMUFile *f,
+ const MigrationParams *params);
int qemu_savevm_state_iterate(QEMUFile *f);
int qemu_savevm_state_complete(QEMUFile *f);
void qemu_savevm_state_cancel(QEMUFile *f);
diff --git a/vmstate.h b/vmstate.h
index 82d97ae..5af45e0 100644
--- a/vmstate.h
+++ b/vmstate.h
@@ -26,7 +26,7 @@
#ifndef QEMU_VMSTATE_H
#define QEMU_VMSTATE_H 1
-typedef void SaveSetParamsHandler(int blk_enable, int shared, void * opaque);
+typedef void SaveSetParamsHandler(const MigrationParams *params, void * opaque);
typedef void SaveStateHandler(QEMUFile *f, void *opaque);
typedef int SaveLiveStateHandler(QEMUFile *f, int stage, void *opaque);
typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id);
--
1.7.7.6
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v14 02/13] Add migration capabilities
2012-07-03 13:52 [Qemu-devel] [PATCH v14 00/13] XBZRLE delta for live migration of large memory app Orit Wasserman
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 01/13] Add MigrationParams structure Orit Wasserman
@ 2012-07-03 13:52 ` Orit Wasserman
2012-07-03 18:36 ` Eric Blake
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 03/13] Add XBZRLE documentation Orit Wasserman
` (10 subsequent siblings)
12 siblings, 1 reply; 33+ messages in thread
From: Orit Wasserman @ 2012-07-03 13:52 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, aliguori, quintela, stefanha, mdroth, blauwirbel,
Orit Wasserman, chegu_vinod, avi, pbonzini, eblake
Add migration capabilities that can be queried by the management.
The management can query the source QEMU and the destination QEMU in order to
verify both support some migration capability (currently only XBZRLE).
The management can enable a capability for the next migration by using
migrate_set_parameter command.
Signed-off-by: Orit Wasserman <owasserm@redhat.com>
---
hmp-commands.hx | 16 ++++++++++++
hmp.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++
hmp.h | 2 +
migration.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
migration.h | 2 +
monitor.c | 7 +++++
qapi-schema.json | 48 +++++++++++++++++++++++++++++++++++-
qmp-commands.hx | 50 +++++++++++++++++++++++++++++++++++++
8 files changed, 257 insertions(+), 3 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index f5d9d91..a0c8df2 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -861,6 +861,20 @@ Set maximum tolerated downtime (in seconds) for migration.
ETEXI
{
+ .name = "migrate_set_parameter",
+ .args_type = "capability:s,state:b",
+ .params = "",
+ .help = "Enable the usage of a capability for migration",
+ .mhandler.cmd = hmp_migrate_set_parameter,
+ },
+
+STEXI
+@item migrate_set_parameter @var{capability} @var{state}
+@findex migrate_set_parameter
+Enable/Disable the usage of a capability @var{capability} for migration.
+ETEXI
+
+ {
.name = "client_migrate_info",
.args_type = "protocol:s,hostname:s,port:i?,tls-port:i?,cert-subject:s?",
.params = "protocol hostname port tls-port cert-subject",
@@ -1419,6 +1433,8 @@ show CPU statistics
show user network stack connection states
@item info migrate
show migration status
+@item info migration_capabilities
+show migration capabilities
@item info balloon
show balloon information
@item info qtree
diff --git a/hmp.c b/hmp.c
index b9cec1d..69711fb 100644
--- a/hmp.c
+++ b/hmp.c
@@ -131,9 +131,19 @@ void hmp_info_mice(Monitor *mon)
void hmp_info_migrate(Monitor *mon)
{
MigrationInfo *info;
+ MigrationCapabilityInfoList *cap;
info = qmp_query_migrate(NULL);
+ if (info->has_capabilities && info->capabilities) {
+ monitor_printf(mon, "capabilities: ");
+ for (cap = info->capabilities; cap; cap = cap->next) {
+ monitor_printf(mon, "%s: %s ",
+ MigrationCapability_lookup[cap->value->capability],
+ cap->value->state ? "on" : "off");
+ }
+ monitor_printf(mon, "\n");
+ }
if (info->has_status) {
monitor_printf(mon, "Migration status: %s\n", info->status);
}
@@ -159,6 +169,24 @@ void hmp_info_migrate(Monitor *mon)
qapi_free_MigrationInfo(info);
}
+void hmp_info_migration_capabilities(Monitor *mon)
+{
+ MigrationCapabilityInfoList *caps_list, *cap;
+
+ caps_list = qmp_query_migration_capabilities(NULL);
+ if (!caps_list) {
+ monitor_printf(mon, "No migration capabilities found\n");
+ return;
+ }
+
+ for (cap = caps_list; cap; cap = cap->next) {
+ monitor_printf(mon, "%s ",
+ MigrationCapability_lookup[cap->value->capability]);
+ }
+
+ qapi_free_MigrationCapabilityInfoList(caps_list);
+}
+
void hmp_info_cpus(Monitor *mon)
{
CpuInfoList *cpu_list, *cpu;
@@ -733,6 +761,41 @@ void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict)
qmp_migrate_set_speed(value, NULL);
}
+void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
+{
+ const char *cap = qdict_get_str(qdict, "capability");
+ bool state = qdict_get_bool(qdict, "state");
+ Error *err = NULL;
+ MigrationCapabilityInfoList *params = NULL;
+ int i;
+
+ for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) {
+ if (strcmp(cap, MigrationCapability_lookup[i]) == 0) {
+ if (!params) {
+ params = g_malloc0(sizeof(*params));
+ }
+ params->value = g_malloc0(sizeof(*params->value));
+ params->value->capability = i;
+ params->value->state = state;
+ params->next = NULL;
+ qmp_migrate_set_parameters(params, &err);
+ break;
+ }
+ }
+
+ if (i == MIGRATION_CAPABILITY_MAX) {
+ error_set(&err, QERR_INVALID_PARAMETER, cap);
+ }
+
+ qapi_free_MigrationCapabilityInfoList(params);
+
+ if (err) {
+ monitor_printf(mon, "migrate_set_parameter: %s\n",
+ error_get_pretty(err));
+ error_free(err);
+ }
+}
+
void hmp_set_password(Monitor *mon, const QDict *qdict)
{
const char *protocol = qdict_get_str(qdict, "protocol");
diff --git a/hmp.h b/hmp.h
index 79d138d..09ba198 100644
--- a/hmp.h
+++ b/hmp.h
@@ -25,6 +25,7 @@ void hmp_info_uuid(Monitor *mon);
void hmp_info_chardev(Monitor *mon);
void hmp_info_mice(Monitor *mon);
void hmp_info_migrate(Monitor *mon);
+void hmp_info_migration_capabilities(Monitor *mon);
void hmp_info_cpus(Monitor *mon);
void hmp_info_block(Monitor *mon);
void hmp_info_blockstats(Monitor *mon);
@@ -51,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_parameter(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 810727f..4b68146 100644
--- a/migration.c
+++ b/migration.c
@@ -117,12 +117,36 @@ MigrationInfo *qmp_query_migrate(Error **errp)
{
MigrationInfo *info = g_malloc0(sizeof(*info));
MigrationState *s = migrate_get_current();
+ int i;
switch (s->state) {
case MIG_STATE_SETUP:
- /* no migration has happened ever */
+ /* no migration has happened ever show enabled capabilities */
+ for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) {
+ if (!info->has_capabilities) {
+ info->capabilities = g_malloc0(sizeof(*info->capabilities));
+ info->has_capabilities = true;
+ }
+ info->capabilities->value =
+ g_malloc(sizeof(*info->capabilities->value));
+ info->capabilities->value->capability = i;
+ info->capabilities->value->state = s->enabled_capabilities[i];
+ info->capabilities->next = NULL;
+ }
break;
case MIG_STATE_ACTIVE:
+ for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) {
+ if (!info->has_capabilities) {
+ info->capabilities = g_malloc0(sizeof(*info->capabilities));
+ info->has_capabilities = true;
+ }
+ info->capabilities->value =
+ g_malloc(sizeof(*info->capabilities->value));
+ info->capabilities->value->capability = i;
+ info->capabilities->value->state = s->enabled_capabilities[i];
+ info->capabilities->next = NULL;
+ }
+
info->has_status = true;
info->status = g_strdup("active");
@@ -141,6 +165,18 @@ MigrationInfo *qmp_query_migrate(Error **errp)
}
break;
case MIG_STATE_COMPLETED:
+ for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) {
+ if (!info->has_capabilities) {
+ info->capabilities = g_malloc0(sizeof(*info->capabilities));
+ info->has_capabilities = true;
+ }
+ info->capabilities->value =
+ g_malloc(sizeof(*info->capabilities->value));
+ info->capabilities->value->capability = i;
+ info->capabilities->value->state = s->enabled_capabilities[i];
+ info->capabilities->next = NULL;
+ }
+
info->has_status = true;
info->status = g_strdup("completed");
break;
@@ -157,6 +193,33 @@ MigrationInfo *qmp_query_migrate(Error **errp)
return info;
}
+MigrationCapabilityInfoList *qmp_query_migration_capabilities(Error **errp)
+{
+ MigrationCapabilityInfoList *caps_list = g_malloc0(sizeof(*caps_list));
+
+ caps_list->value = g_malloc(sizeof(*caps_list->value));
+ caps_list->value->capability = MIGRATION_CAPABILITY_XBZRLE;
+ caps_list->next = NULL;
+
+ return caps_list;
+}
+
+void qmp_migrate_set_parameters(MigrationCapabilityInfoList *params,
+ Error **errp)
+{
+ MigrationState *s = migrate_get_current();
+ MigrationCapabilityInfoList *cap;
+
+ if (s->state == MIG_STATE_ACTIVE) {
+ error_set(errp, QERR_MIGRATION_ACTIVE);
+ return;
+ }
+
+ for (cap = params; cap; cap = cap->next) {
+ s->enabled_capabilities[cap->value->capability] = cap->value->state;
+ }
+}
+
/* shared migration helpers */
static int migrate_fd_cleanup(MigrationState *s)
@@ -365,12 +428,17 @@ static MigrationState *migrate_init(const MigrationParams *params)
{
MigrationState *s = migrate_get_current();
int64_t bandwidth_limit = s->bandwidth_limit;
+ bool enabled_capabilities[MIGRATION_CAPABILITY_MAX];
+
+ memcpy(enabled_capabilities, s->enabled_capabilities,
+ sizeof(enabled_capabilities));
memset(s, 0, sizeof(*s));
s->bandwidth_limit = bandwidth_limit;
s->params = *params;
+ memcpy(s->enabled_capabilities, enabled_capabilities,
+ sizeof(enabled_capabilities));
- s->bandwidth_limit = bandwidth_limit;
s->state = MIG_STATE_SETUP;
return s;
diff --git a/migration.h b/migration.h
index 35207bd..1ae99f1 100644
--- a/migration.h
+++ b/migration.h
@@ -18,6 +18,7 @@
#include "qemu-common.h"
#include "notify.h"
#include "error.h"
+#include "qapi-types.h"
struct MigrationParams {
bool blk;
@@ -37,6 +38,7 @@ struct MigrationState
int (*write)(MigrationState *s, const void *buff, size_t size);
void *opaque;
MigrationParams params;
+ bool enabled_capabilities[MIGRATION_CAPABILITY_MAX];
};
void process_incoming_migration(QEMUFile *f);
diff --git a/monitor.c b/monitor.c
index f6107ba..e2be6cd 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2687,6 +2687,13 @@ static mon_cmd_t info_cmds[] = {
.mhandler.info = hmp_info_migrate,
},
{
+ .name = "migration_capabilities",
+ .args_type = "",
+ .params = "",
+ .help = "show migration capabilities",
+ .mhandler.info = hmp_info_migration_capabilities,
+ },
+ {
.name = "balloon",
.args_type = "",
.params = "",
diff --git a/qapi-schema.json b/qapi-schema.json
index 3b6e346..3eb3003 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -286,7 +286,8 @@
##
{ 'type': 'MigrationInfo',
'data': {'*status': 'str', '*ram': 'MigrationStats',
- '*disk': 'MigrationStats'} }
+ '*disk': 'MigrationStats',
+ '*capabilities': ['MigrationCapabilityInfo']} }
##
# @query-migrate
@@ -300,6 +301,51 @@
{ 'command': 'query-migrate', 'returns': 'MigrationInfo' }
##
+# @MigrationCapability
+#
+# Migration capabilities enumeration
+#
+# @xbzrle: current migration supports xbzrle
+#
+# Since: 1.2
+##
+{ 'enum': 'MigrationCapability',
+ 'data': ['xbzrle'] }
+
+##
+# @MigrationCapabilityInfo
+#
+# Migration capability information
+#
+# @capability: capability enum
+#
+# Since: 1.2
+##
+{ 'type': 'MigrationCapabilityInfo',
+ 'data': { 'capability' : 'MigrationCapability', 'state' : 'bool' } }
+
+##
+# @query-migration-capabilities
+#
+# Returns information about current migration process capabilties.
+#
+# Returns: @MigrationCapabilityInfo list
+#
+# Since: 1.2
+##
+{ 'command': 'query-migration-capabilities', 'returns': ['MigrationCapabilityInfo'] }
+
+##
+# @migrate_set_parameters
+#
+# Enable/Disable the following migration capabilities (like xbzrle)
+#
+# Since: 1.2
+##
+{ 'command': 'migrate-set-parameters',
+ 'data': { 'capabilities': ['MigrationCapabilityInfo'] } }
+
+##
# @MouseInfo:
#
# Information about a mouse device.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 2e1a38e..7e85e0d 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2135,6 +2135,56 @@ EQMP
},
SQMP
+query-migration-capabilities
+-------
+
+Query migration capabilities
+
+- "xbzrle": xbzrle support
+
+Arguments:
+
+Example:
+
+-> { "execute": "query-migration-capabilities"}
+<- { "return": [ { "capability": "xbzrle", "state": true },
+ { "capability": "foobar", "state": false } ] }
+
+EQMP
+
+ {
+ .name = "query-migration-capabilities",
+ .args_type = "",
+ .mhandler.cmd_new = qmp_marshal_input_query_migration_capabilities,
+ },
+
+SQMP
+migrate_set_parameters
+-------
+
+Enable/Disable migration capabilities
+
+- "xbzrle": xbzrle support
+
+Arguments:
+
+Example:
+
+-> { "execute": "migrate_set_parameters" , "arguments":
+ { "parameters": { "capability": "xbzrle", "state": true } ] } }
+
+EQMP
+
+ {
+ .name = "migrate_set_parameters",
+ .args_type = "parameters:O",
+ .params = "capability:s,state:b",
+ .mhandler.cmd_new = qmp_marshal_input_migrate_set_parameters,
+ },
+
+
+
+SQMP
query-balloon
-------------
--
1.7.7.6
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v14 03/13] Add XBZRLE documentation
2012-07-03 13:52 [Qemu-devel] [PATCH v14 00/13] XBZRLE delta for live migration of large memory app Orit Wasserman
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 01/13] Add MigrationParams structure Orit Wasserman
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 02/13] Add migration capabilities Orit Wasserman
@ 2012-07-03 13:52 ` Orit Wasserman
2012-07-03 19:45 ` Eric Blake
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 04/13] Add cache handling functions Orit Wasserman
` (9 subsequent siblings)
12 siblings, 1 reply; 33+ messages in thread
From: Orit Wasserman @ 2012-07-03 13:52 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, aliguori, quintela, stefanha, mdroth, blauwirbel,
Orit Wasserman, chegu_vinod, avi, pbonzini, eblake
Signed-off-by: Orit Wasserman <owasserm@redhat.com>
---
docs/xbzrle.txt | 133 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 133 insertions(+), 0 deletions(-)
create mode 100644 docs/xbzrle.txt
diff --git a/docs/xbzrle.txt b/docs/xbzrle.txt
new file mode 100644
index 0000000..f00d63f
--- /dev/null
+++ b/docs/xbzrle.txt
@@ -0,0 +1,133 @@
+XBZRLE (Xor Based Zero Run Length Encoding)
+===========================================
+
+Using XBZRLE (Xor Based Zero Run Length Encoding) allows for the reduction
+of VM downtime and the total live-migration time of Virtual machines.
+It is particularly useful for virtual machines running memory write intensive
+workloads that are typical of large enterprise applications such as SAP ERP
+Systems, and generally speaking for any application that uses a sparse memory
+update pattern.
+
+Instead of sending the changed guest memory page this solution will send a
+compressed version of the updates, thus reducing the amount of data sent during
+live migration.
+In order to be able to calculate the update, the previous memory pages need to
+be stored on the source. Those pages are stored in a dedicated cache
+(hash table) and are
+accessed by their address.
+The larger the cache size the better the chances are that the page has already
+been stored in the cache.
+A small cache size will result in high cache miss rate.
+Cache size can be changed before and during migration.
+
+Format
+=======
+
+The compression format performs a XOR between the previous and current content
+of the page, where zero represents an unchanged value.
+The page data delta is represented by zero and non zero runs.
+A zero run is represented by its length (in bytes).
+A non zero run is represented by its length (in bytes) and the new data.
+The run length is encoded using ULEB128 (http://en.wikipedia.org/wiki/LEB128)
+
+There can be more than one valid encoding, the sender may send a longer encoding
+for the benefit of reducing computation cost.
+
+page = zrun nzrun
+ | zrun nzrun page
+
+zrun = length
+
+nzrun = length byte...
+
+length = uleb128 encoded integer
+
+On the sender side XBZRLE is used as a compact delta encoding of page updates,
+retrieving the old page content from the cache (default size of 512 MB). The
+receiving side uses the existing page's content and XBZRLE to decode the new
+page's content.
+
+This work was originally based on research results published
+VEE 2011: Evaluation of Delta Compression Techniques for Efficient Live
+Migration of Large Virtual Machines by Benoit, Svard, Tordsson and Elmroth.
+Additionally the delta encoder XBRLE was improved further using the XBZRLE
+instead.
+
+XBZRLE has a sustained bandwidth of 2-2.5 GB/s for typical workloads making it
+ideal for in-line, real-time encoding such as is needed for live-migration.
+
+Example
+old buffer:
+1000 zeros
+05 06 07 08 09 0a 0b 0c 0d 0e 0f 10 11 12 13 00 00 6b 00 6d
+3072 zeros
+
+new buffer:
+1000 zeros
+01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 00 00 67 00 69
+3702 zeros
+
+encoded buffer:
+
+encoded length 29
+e8 07 18 00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 00 00 67 00 69 00 00
+00 80 18
+
+Migration Capabilities
+======================
+In order to use XBZRLE the destination QEMU version should be able to
+decode the new format.
+Adding a new migration capabilities command that will allow external management
+to query for it support.
+A typical use for the destination
+ {qemu} info migrate_capabilities
+ {qemu} xbzrle, ...
+
+In order to enable capabilities for future live migration,
+a new command migrate_set_parameter is introduced:
+ {qemu} migrate_set_parameter xbzrle
+
+Usage
+======
+
+1. Activate xbzrle
+2. Set the XBZRLE cache size - the cache size is in MBytes and should be a
+power of 2. The cache default value is 64MBytes.
+3. start outgoing migration
+
+A typical usage scenario:
+ {qemu} migrate_set_parameter xbzrle
+ {qemu} migrate_set_cachesize 256m
+ {qemu} migrate -d tcp:destination.host:4444
+ {qemu} info migrate
+ ...
+ transferred ram-duplicate: A kbytes
+ transferred ram-normal: B kbytes
+ transferred ram-xbrle: C kbytes
+ overflow ram-xbrle: D pages
+ cache-miss ram-xbrle: E pages
+
+cache-miss: the number of cache misses to date - high cache-miss rate
+indicates that the cache size is set too low.
+overflow: the number of overflows in the decoding which where the delta could
+not be compressed. This can happen if the changes in the pages are too large
+or there are many short changes; for example, changing every second byte (half a
+page).
+
+Testing: Testing indicated that live migration with XBZRLE was completed in 110
+seconds, whereas without it would not be able to complete.
+
+A simple synthetic memory r/w load generator:
+.. include <stdlib.h>
+.. include <stdio.h>
+.. int main()
+.. {
+.. char *buf = (char *) calloc(4096, 4096);
+.. while (1) {
+.. int i;
+.. for (i = 0; i < 4096 * 4; i++) {
+.. buf[i * 4096 / 4]++;
+.. }
+.. printf(".");
+.. }
+.. }
--
1.7.7.6
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v14 04/13] Add cache handling functions
2012-07-03 13:52 [Qemu-devel] [PATCH v14 00/13] XBZRLE delta for live migration of large memory app Orit Wasserman
` (2 preceding siblings ...)
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 03/13] Add XBZRLE documentation Orit Wasserman
@ 2012-07-03 13:52 ` Orit Wasserman
2012-07-03 19:23 ` Blue Swirl
2012-07-03 20:24 ` Eric Blake
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 05/13] Add uleb encoding/decoding functions Orit Wasserman
` (8 subsequent siblings)
12 siblings, 2 replies; 33+ messages in thread
From: Orit Wasserman @ 2012-07-03 13:52 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, aliguori, quintela, Petter Svard, stefanha,
mdroth, Benoit Hudzia, blauwirbel, Orit Wasserman, chegu_vinod,
avi, Aidan Shribman, pbonzini, eblake
Add LRU page cache mechanism.
The page are accessed by their address.
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>
---
Makefile.objs | 1 +
include/qemu/page_cache.h | 79 +++++++++++++++++
page_cache.c | 209 +++++++++++++++++++++++++++++++++++++++++++++
qemu-common.h | 18 ++++
4 files changed, 307 insertions(+), 0 deletions(-)
create mode 100644 include/qemu/page_cache.h
create mode 100644 page_cache.c
diff --git a/Makefile.objs b/Makefile.objs
index 625c4d5..d7a199e 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -77,6 +77,7 @@ common-obj-y += qemu-char.o #aio.o
common-obj-y += block-migration.o iohandler.o
common-obj-y += pflib.o
common-obj-y += bitmap.o bitops.o
+common-obj-y += page_cache.o
common-obj-$(CONFIG_POSIX) += migration-exec.o migration-unix.o migration-fd.o
common-obj-$(CONFIG_WIN32) += version.o
diff --git a/include/qemu/page_cache.h b/include/qemu/page_cache.h
new file mode 100644
index 0000000..3839ac7
--- /dev/null
+++ b/include/qemu/page_cache.h
@@ -0,0 +1,79 @@
+/*
+ * Page cache for QEMU
+ * The cache is base on a hash of the page address
+ *
+ * Copyright 2012 Red Hat, Inc. and/or its affiliates
+ *
+ * Authors:
+ * Orit Wasserman <owasserm@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#ifndef PAGE_CACHE_H
+#define PAGE_CACHE_H
+
+/* Page cache for storing guest pages */
+typedef struct PageCache PageCache;
+
+/**
+ * cache_init: Initialize the page cache
+ *
+ *
+ * Returns new allocated cache or NULL on error
+ *
+ * @cache pointer to the PageCache struct
+ * @num_pages: cache maximal number of cached pages
+ * @page_size: cache page size
+ */
+PageCache *cache_init(int64_t num_pages, unsigned int page_size);
+
+/**
+ * cache_fini: free all cache resources
+ * @cache pointer to the PageCache struct
+ */
+void cache_fini(PageCache *cache);
+
+/**
+ * cache_is_cached: Checks to see if the page is cached
+ *
+ * Returns %true if page is cached
+ *
+ * @cache pointer to the PageCache struct
+ * @addr: page addr
+ */
+bool cache_is_cached(const PageCache *cache, uint64_t addr);
+
+/**
+ * get_cached_data: Get the data cached for an addr
+ *
+ * Returns pointer to the data cached or NULL if not cached
+ *
+ * @cache pointer to the PageCache struct
+ * @addr: page addr
+ */
+uint8_t *get_cached_data(const PageCache *cache, uint64_t addr);
+
+/**
+ * cache_insert: insert the page into the cache. the previous value will be overwritten
+ *
+ * @cache pointer to the PageCache struct
+ * @addr: page address
+ * @pdata: pointer to the page
+ */
+void cache_insert(PageCache *cache, uint64_t addr, uint8_t *pdata);
+
+/**
+ * cache_resize: resize the page cache. In case of size reduction the extra
+ * pages will be freed
+ *
+ * Returns -1 on error new cache size on success
+ *
+ * @cache pointer to the PageCache struct
+ * @num_pages: new page cache size (in pages)
+ */
+int64_t cache_resize(PageCache *cache, int64_t num_pages);
+
+#endif
diff --git a/page_cache.c b/page_cache.c
new file mode 100644
index 0000000..7122756
--- /dev/null
+++ b/page_cache.c
@@ -0,0 +1,209 @@
+/*
+ * Page cache for QEMU
+ * The cache is base on a hash of the page address
+ *
+ * Copyright 2012 Red Hat, Inc. and/or its affiliates
+ *
+ * Authors:
+ * Orit Wasserman <owasserm@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <strings.h>
+#include <string.h>
+#include <sys/time.h>
+#include <sys/types.h>
+#include <stdbool.h>
+#include <glib.h>
+#include <strings.h>
+
+#include "qemu-common.h"
+#include "qemu/page_cache.h"
+
+#ifdef DEBUG_CACHE
+#define DPRINTF(fmt, ...) \
+ do { fprintf(stdout, "cache: " fmt, ## __VA_ARGS__); } while (0)
+#else
+#define DPRINTF(fmt, ...) \
+ do { } while (0)
+#endif
+
+typedef struct CacheItem CacheItem;
+
+struct CacheItem {
+ uint64_t it_addr;
+ unsigned long it_age;
+ uint8_t *it_data;
+};
+
+struct PageCache {
+ CacheItem *page_cache;
+ unsigned int page_size;
+ int64_t max_num_items;
+ uint64_t max_item_age;
+ int64_t num_items;
+};
+
+PageCache *cache_init(int64_t num_pages, unsigned int page_size)
+{
+ int i;
+
+ PageCache *cache = g_malloc(sizeof(PageCache));
+
+ if (num_pages <= 0) {
+ DPRINTF("invalid number pages\n");
+ return NULL;
+ }
+
+ /* round down to the nearest power of 2 */
+ if (!is_power_of_2(num_pages)) {
+ num_pages = round2pow2(num_pages);
+ DPRINTF("rounding down to %" PRId64 "\n", num_pages);
+ }
+ cache->page_size = page_size;
+ cache->num_items = 0;
+ cache->max_item_age = 0;
+ cache->max_num_items = num_pages;
+
+ DPRINTF("Setting cache buckets to %" PRId64 "\n", cache->max_num_items);
+
+ cache->page_cache = g_malloc((cache->max_num_items) *
+ sizeof(CacheItem));
+
+ for (i = 0; i < cache->max_num_items; i++) {
+ cache->page_cache[i].it_data = NULL;
+ cache->page_cache[i].it_age = 0;
+ cache->page_cache[i].it_addr = -1;
+ }
+
+ return cache;
+}
+
+void cache_fini(PageCache *cache)
+{
+ int i;
+
+ g_assert(cache);
+ g_assert(cache->page_cache);
+
+ for (i = 0; i < cache->max_num_items; i++) {
+ g_free(cache->page_cache[i].it_data);
+ cache->page_cache[i].it_data = 0;
+ }
+
+ g_free(cache->page_cache);
+ cache->page_cache = NULL;
+}
+
+static unsigned long cache_get_cache_pos(const PageCache *cache,
+ uint64_t address)
+{
+ unsigned long pos;
+
+ g_assert(cache->max_num_items);
+ pos = (address / cache->page_size) & (cache->max_num_items - 1);
+ return pos;
+}
+
+bool cache_is_cached(const PageCache *cache, uint64_t addr)
+{
+ unsigned long pos;
+
+ g_assert(cache);
+ g_assert(cache->page_cache);
+
+ pos = cache_get_cache_pos(cache, addr);
+
+ return (cache->page_cache[pos].it_addr == addr);
+}
+
+static CacheItem *cache_get_by_addr(const PageCache *cache, uint64_t addr)
+{
+ unsigned long pos;
+
+ g_assert(cache);
+ g_assert(cache->page_cache);
+
+ pos = cache_get_cache_pos(cache, addr);
+
+ return &cache->page_cache[pos];
+}
+
+uint8_t *get_cached_data(const PageCache *cache, uint64_t addr)
+{
+ return cache_get_by_addr(cache, addr)->it_data;
+}
+
+void cache_insert(PageCache *cache, unsigned long addr, uint8_t *pdata)
+{
+
+ CacheItem *it = NULL;
+
+ g_assert(cache);
+ g_assert(cache->page_cache);
+
+ /* actual update of entry */
+ it = cache_get_by_addr(cache, addr);
+
+ if (!it->it_data) {
+ cache->num_items++;
+ }
+
+ it->it_data = pdata;
+ it->it_age = ++cache->max_item_age;
+ it->it_addr = addr;
+}
+
+int64_t cache_resize(PageCache *cache, int64_t new_num_pages)
+{
+ PageCache *new_cache;
+ int i;
+
+ CacheItem *old_it, *new_it;
+
+ g_assert(cache);
+
+ /* cache was not inited */
+ if (cache->page_cache == NULL) {
+ return -1;
+ }
+
+ /* same size */
+ if (round2pow2(new_num_pages) == cache->max_num_items) {
+ return cache->max_num_items;
+ }
+
+ new_cache = cache_init(new_num_pages, cache->page_size);
+ if (!(new_cache)) {
+ DPRINTF("Error creating new cache\n");
+ return -1;
+ }
+
+ /* move all data from old cache */
+ for (i = 0; i < cache->max_num_items; i++) {
+ old_it = &cache->page_cache[i];
+ if (old_it->it_addr != -1) {
+ /* check for collision , if there is, keep the first value */
+ new_it = cache_get_by_addr(new_cache, old_it->it_addr);
+ if (new_it->it_data) {
+ g_free(old_it->it_data);
+ } else {
+ cache_insert(new_cache, old_it->it_addr, old_it->it_data);
+ }
+ }
+ }
+
+ cache->page_cache = new_cache->page_cache;
+ cache->max_num_items = new_cache->max_num_items;
+ cache->num_items = new_cache->num_items;
+
+ g_free(new_cache);
+
+ return cache->max_num_items;
+}
diff --git a/qemu-common.h b/qemu-common.h
index c8c6b2a..1ce7023 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -1,3 +1,4 @@
+
/* Common header file that is included by all of qemu. */
#ifndef QEMU_COMMON_H
#define QEMU_COMMON_H
@@ -417,6 +418,23 @@ static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
/* Round number up to multiple */
#define QEMU_ALIGN_UP(n, m) QEMU_ALIGN_DOWN((n) + (m) - 1, (m))
+static inline bool is_power_of_2(int64_t value)
+{
+ if (!value) {
+ return 0;
+ }
+
+ return !(value & (value - 1));
+}
+
+static inline int64_t round2pow2(int64_t value)
+{
+ while (!is_power_of_2(value)) {
+ value &= ~(1 << (ffs(value) - 1));
+ }
+ return value;
+}
+
#include "module.h"
#endif
--
1.7.7.6
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v14 05/13] Add uleb encoding/decoding functions
2012-07-03 13:52 [Qemu-devel] [PATCH v14 00/13] XBZRLE delta for live migration of large memory app Orit Wasserman
` (3 preceding siblings ...)
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 04/13] Add cache handling functions Orit Wasserman
@ 2012-07-03 13:52 ` Orit Wasserman
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 06/13] Add save_block_hdr function Orit Wasserman
` (7 subsequent siblings)
12 siblings, 0 replies; 33+ messages in thread
From: Orit Wasserman @ 2012-07-03 13:52 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, aliguori, quintela, stefanha, mdroth, blauwirbel,
Orit Wasserman, chegu_vinod, avi, pbonzini, eblake
Implement Unsigned Little Endian Base 128.
Signed-off-by: Orit Wasserman <owasserm@redhat.com>
---
cutils.c | 32 ++++++++++++++++++++++++++++++++
qemu-common.h | 8 ++++++++
2 files changed, 40 insertions(+), 0 deletions(-)
diff --git a/cutils.c b/cutils.c
index af308cd..3f81d53 100644
--- a/cutils.c
+++ b/cutils.c
@@ -549,3 +549,35 @@ int qemu_sendv(int sockfd, struct iovec *iov, int len, int iov_offset)
return do_sendv_recvv(sockfd, iov, len, iov_offset, 1);
}
+/*
+ * Implementation of ULEB128 (http://en.wikipedia.org/wiki/LEB128)
+ * Input is limited to 14-bit numbers
+ */
+int uleb128_encode_small(uint8_t *out, uint32_t n)
+{
+ g_assert(n <= 0x3fff);
+ if (n < 0x80) {
+ *out++ = n;
+ return 1;
+ } else {
+ *out++ = (n & 0x7f) | 0x80;
+ *out++ = n >> 7;
+ return 2;
+ }
+}
+
+int uleb128_decode_small(const uint8_t *in, uint32_t *n)
+{
+ if (!(*in & 0x80)) {
+ *n = *in++;
+ return 1;
+ } else {
+ *n = *in++ & 0x7f;
+ /* we exceed 14 bit number */
+ if (*in & 0x80) {
+ return -1;
+ }
+ *n |= *in++ << 7;
+ return 2;
+ }
+}
diff --git a/qemu-common.h b/qemu-common.h
index 1ce7023..42e4498 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -437,4 +437,12 @@ static inline int64_t round2pow2(int64_t value)
#include "module.h"
+/*
+ * Implementation of ULEB128 (http://en.wikipedia.org/wiki/LEB128)
+ * Input is limited to 14-bit numbers
+ */
+
+int uleb128_encode_small(uint8_t *out, uint32_t n);
+int uleb128_decode_small(const uint8_t *in, uint32_t *n);
+
#endif
--
1.7.7.6
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v14 06/13] Add save_block_hdr function
2012-07-03 13:52 [Qemu-devel] [PATCH v14 00/13] XBZRLE delta for live migration of large memory app Orit Wasserman
` (4 preceding siblings ...)
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 05/13] Add uleb encoding/decoding functions Orit Wasserman
@ 2012-07-03 13:52 ` Orit Wasserman
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 07/13] Add debugging infrastructure Orit Wasserman
` (6 subsequent siblings)
12 siblings, 0 replies; 33+ messages in thread
From: Orit Wasserman @ 2012-07-03 13:52 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, aliguori, quintela, Petter Svard, stefanha,
mdroth, Benoit Hudzia, blauwirbel, Orit Wasserman, chegu_vinod,
avi, Aidan Shribman, pbonzini, eblake
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 | 26 ++++++++++++++------------
1 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/arch_init.c b/arch_init.c
index a9e8b74..9dafb6e 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -161,6 +161,18 @@ static int is_dup_page(uint8_t *page)
return 1;
}
+static void save_block_hdr(QEMUFile *f, RAMBlock *block, ram_addr_t offset,
+ int cont, int flag)
+{
+ qemu_put_be64(f, offset | cont | flag);
+ if (!cont) {
+ qemu_put_byte(f, strlen(block->idstr));
+ qemu_put_buffer(f, (uint8_t *)block->idstr,
+ strlen(block->idstr));
+ }
+
+}
+
static RAMBlock *last_block;
static ram_addr_t last_offset;
@@ -187,21 +199,11 @@ static int ram_save_block(QEMUFile *f)
p = memory_region_get_ram_ptr(mr) + offset;
if (is_dup_page(p)) {
- qemu_put_be64(f, offset | cont | RAM_SAVE_FLAG_COMPRESS);
- if (!cont) {
- qemu_put_byte(f, strlen(block->idstr));
- qemu_put_buffer(f, (uint8_t *)block->idstr,
- strlen(block->idstr));
- }
+ save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_COMPRESS);
qemu_put_byte(f, *p);
bytes_sent = 1;
} else {
- qemu_put_be64(f, offset | cont | RAM_SAVE_FLAG_PAGE);
- if (!cont) {
- qemu_put_byte(f, strlen(block->idstr));
- qemu_put_buffer(f, (uint8_t *)block->idstr,
- strlen(block->idstr));
- }
+ save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_PAGE);
qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
bytes_sent = TARGET_PAGE_SIZE;
}
--
1.7.7.6
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v14 07/13] Add debugging infrastructure
2012-07-03 13:52 [Qemu-devel] [PATCH v14 00/13] XBZRLE delta for live migration of large memory app Orit Wasserman
` (5 preceding siblings ...)
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 06/13] Add save_block_hdr function Orit Wasserman
@ 2012-07-03 13:52 ` Orit Wasserman
2012-07-03 19:25 ` Blue Swirl
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 08/13] Change ram_save_block to return -1 if there are no more changes Orit Wasserman
` (5 subsequent siblings)
12 siblings, 1 reply; 33+ messages in thread
From: Orit Wasserman @ 2012-07-03 13:52 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, aliguori, quintela, stefanha, mdroth, blauwirbel,
Orit Wasserman, chegu_vinod, avi, pbonzini, eblake
Signed-off-by: Orit Wasserman <owasserm@redhat.com>
---
arch_init.c | 33 +++++++++++++++++++++++++++------
1 files changed, 27 insertions(+), 6 deletions(-)
diff --git a/arch_init.c b/arch_init.c
index 9dafb6e..ee20c33 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -44,6 +44,14 @@
#include "exec-memory.h"
#include "hw/pcspk.h"
+#ifdef DEBUG_ARCH_INIT
+#define DPRINTF(fmt, ...) \
+ do { fprintf(stdout, "arch_init: " fmt, ## __VA_ARGS__); } while (0)
+#else
+#define DPRINTF(fmt, ...) \
+ do { } while (0)
+#endif
+
#ifdef TARGET_SPARC
int graphic_width = 1024;
int graphic_height = 768;
@@ -380,6 +388,9 @@ int ram_save_live(QEMUFile *f, int stage, void *opaque)
expected_time = ram_save_remaining() * TARGET_PAGE_SIZE / bwidth;
+ DPRINTF("ram_save_live: expected(%ld) <= max(%ld)?\n", expected_time,
+ migrate_max_downtime());
+
return (stage == 2) && (expected_time <= migrate_max_downtime());
}
@@ -416,8 +427,11 @@ static inline void *host_from_stream_offset(QEMUFile *f,
int ram_load(QEMUFile *f, void *opaque, int version_id)
{
ram_addr_t addr;
- int flags;
+ int flags, ret = 0;
int error;
+ static uint64_t seq_iter;
+
+ seq_iter++;
if (version_id < 4 || version_id > 4) {
return -EINVAL;
@@ -447,8 +461,10 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
QLIST_FOREACH(block, &ram_list.blocks, next) {
if (!strncmp(id, block->idstr, sizeof(id))) {
- if (block->length != length)
- return -EINVAL;
+ if (block->length != length) {
+ ret = -EINVAL;
+ goto done;
+ }
break;
}
}
@@ -456,7 +472,8 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
if (!block) {
fprintf(stderr, "Unknown ramblock \"%s\", cannot "
"accept migration\n", id);
- return -EINVAL;
+ ret = -EINVAL;
+ goto done;
}
total_ram_bytes -= length;
@@ -490,11 +507,15 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
}
error = qemu_file_get_error(f);
if (error) {
- return error;
+ ret = error;
+ goto done;
}
} while (!(flags & RAM_SAVE_FLAG_EOS));
- return 0;
+done:
+ DPRINTF("Completed load of VM with exit code %d seq iteration %ld\n",
+ ret, seq_iter);
+ return ret;
}
#ifdef HAS_AUDIO
--
1.7.7.6
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v14 08/13] Change ram_save_block to return -1 if there are no more changes
2012-07-03 13:52 [Qemu-devel] [PATCH v14 00/13] XBZRLE delta for live migration of large memory app Orit Wasserman
` (6 preceding siblings ...)
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 07/13] Add debugging infrastructure Orit Wasserman
@ 2012-07-03 13:52 ` Orit Wasserman
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 09/13] Add migration_end function Orit Wasserman
` (4 subsequent siblings)
12 siblings, 0 replies; 33+ messages in thread
From: Orit Wasserman @ 2012-07-03 13:52 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, aliguori, quintela, stefanha, mdroth, blauwirbel,
Orit Wasserman, chegu_vinod, avi, pbonzini, eblake
It will return 0 if the page is unmodifed.
Signed-off-by: Orit Wasserman <owasserm@redhat.com>
---
arch_init.c | 11 +++++++----
1 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/arch_init.c b/arch_init.c
index ee20c33..e763909 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -188,7 +188,7 @@ static int ram_save_block(QEMUFile *f)
{
RAMBlock *block = last_block;
ram_addr_t offset = last_offset;
- int bytes_sent = 0;
+ int bytes_sent = -1;
MemoryRegion *mr;
if (!block)
@@ -354,8 +354,11 @@ int ram_save_live(QEMUFile *f, int stage, void *opaque)
int bytes_sent;
bytes_sent = ram_save_block(f);
- bytes_transferred += bytes_sent;
- if (bytes_sent == 0) { /* no more blocks */
+ /* bytes_sent 0 represent unchanged page,
+ bytes_sent -1 represent no more blocks*/
+ if (bytes_sent > 0) {
+ bytes_transferred += bytes_sent;
+ } else if (bytes_sent == -1) { /* no more blocks */
break;
}
}
@@ -378,7 +381,7 @@ int ram_save_live(QEMUFile *f, int stage, void *opaque)
int bytes_sent;
/* flush all remaining blocks regardless of rate limiting */
- while ((bytes_sent = ram_save_block(f)) != 0) {
+ while ((bytes_sent = ram_save_block(f)) != -1) {
bytes_transferred += bytes_sent;
}
memory_global_dirty_log_stop();
--
1.7.7.6
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v14 09/13] Add migration_end function
2012-07-03 13:52 [Qemu-devel] [PATCH v14 00/13] XBZRLE delta for live migration of large memory app Orit Wasserman
` (7 preceding siblings ...)
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 08/13] Change ram_save_block to return -1 if there are no more changes Orit Wasserman
@ 2012-07-03 13:52 ` Orit Wasserman
2012-07-03 20:38 ` Eric Blake
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 10/13] Add xbzrle_encode_buffer and xbzrle_decode_buffer functions Orit Wasserman
` (3 subsequent siblings)
12 siblings, 1 reply; 33+ messages in thread
From: Orit Wasserman @ 2012-07-03 13:52 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, aliguori, quintela, stefanha, mdroth, blauwirbel,
Orit Wasserman, chegu_vinod, avi, pbonzini, eblake
Signed-off-by: Orit Wasserman <owasserm@redhat.com>
---
arch_init.c | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/arch_init.c b/arch_init.c
index e763909..66df017 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -304,6 +304,11 @@ static void sort_ram_list(void)
g_free(blocks);
}
+static void migration_end(void)
+{
+ memory_global_dirty_log_stop();
+}
+
int ram_save_live(QEMUFile *f, int stage, void *opaque)
{
ram_addr_t addr;
@@ -313,7 +318,7 @@ int ram_save_live(QEMUFile *f, int stage, void *opaque)
int ret;
if (stage < 0) {
- memory_global_dirty_log_stop();
+ migration_end();
return 0;
}
--
1.7.7.6
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v14 10/13] Add xbzrle_encode_buffer and xbzrle_decode_buffer functions
2012-07-03 13:52 [Qemu-devel] [PATCH v14 00/13] XBZRLE delta for live migration of large memory app Orit Wasserman
` (8 preceding siblings ...)
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 09/13] Add migration_end function Orit Wasserman
@ 2012-07-03 13:52 ` Orit Wasserman
2012-07-03 21:32 ` Eric Blake
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 11/13] Add XBZRLE to ram_save_block and ram_save_live Orit Wasserman
` (2 subsequent siblings)
12 siblings, 1 reply; 33+ messages in thread
From: Orit Wasserman @ 2012-07-03 13:52 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, aliguori, quintela, Petter Svard, stefanha,
mdroth, Benoit Hudzia, blauwirbel, Orit Wasserman, chegu_vinod,
avi, Aidan Shribman, pbonzini, eblake
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>
---
migration.h | 4 ++
savevm.c | 172 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 176 insertions(+), 0 deletions(-)
diff --git a/migration.h b/migration.h
index 1ae99f1..7582ecb 100644
--- a/migration.h
+++ b/migration.h
@@ -99,4 +99,8 @@ void migrate_add_blocker(Error *reason);
*/
void migrate_del_blocker(Error *reason);
+int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t *new_buf, int slen,
+ uint8_t *dst, int dlen);
+int xbzrle_decode_buffer(uint8_t *src, int slen, uint8_t *dst, int dlen);
+
#endif
diff --git a/savevm.c b/savevm.c
index d1d9020..2ab0986 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2374,3 +2374,175 @@ void vmstate_register_ram_global(MemoryRegion *mr)
{
vmstate_register_ram(mr, NULL);
}
+
+/*
+ page = zrun nzrun
+ | zrun nzrun page
+
+ zrun = length
+
+ nzrun = length byte...
+
+ length = uleb128 encoded integer
+ */
+int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t *new_buf, int slen,
+ uint8_t *dst, int dlen)
+{
+ uint32_t zrun_len = 0, nzrun_len = 0;
+ int d = 0, i = 0, start;
+ long res, xor;
+ uint8_t *nzrun_start = NULL;
+
+ g_assert(!((uintptr_t)old_buf & (sizeof(long) - 1)) &&
+ !((uintptr_t)new_buf & (sizeof(long) - 1)) &&
+ !(slen & (sizeof(long) - 1)));
+
+ while (i < slen) {
+ /* overflow */
+ if (d + 2 > dlen) {
+ return -1;
+ }
+
+ /* not aligned to sizeof(long) */
+ res = (slen - i) % sizeof(long);
+ if (res) {
+ start = i;
+ while (!(old_buf[i] ^ new_buf[i]) && (i - start) <= res) {
+ zrun_len++;
+ i++;
+ }
+ }
+
+ if (zrun_len == res) {
+ while (i <= (slen - sizeof(long)) &&
+ (*(long *)(old_buf + i)) == (*(long *)(new_buf + i))) {
+ i += sizeof(long);
+ zrun_len += sizeof(long);
+ }
+
+ /* go over the rest */
+ while (old_buf[i] == new_buf[i] && ++i <= slen) {
+ zrun_len++;
+ }
+ }
+
+ /* buffer unchanged */
+ if (zrun_len == slen) {
+ return 0;
+ }
+
+ /* skip last zero run */
+ if (i == slen + 1) {
+ return d;
+ }
+
+ d += uleb128_encode_small(dst + d, zrun_len);
+
+ /* no nzrun */
+ if (i == slen) {
+ return d;
+ }
+
+ zrun_len = 0;
+ nzrun_start = new_buf + i;
+
+ /* not aligned to sizeof(long) */
+ res = (slen - i) % sizeof(long);
+ if (res) {
+ start = i;
+ while (old_buf[i] != new_buf[i] && i - start < res) {
+ i++;
+ nzrun_len++;
+ }
+ }
+
+ if (nzrun_len == res) {
+ /* truncation to 32-bit long okay */
+ long mask = 0x0101010101010101ULL;
+ xor = *(long *)(old_buf + i) ^ *(long *)(new_buf + i);
+ printf("cont\n");
+ while (i <= (slen - sizeof(long))) {
+ if ((xor - mask) & ~xor & (mask << 7)) {
+ /* found the end of an nzrun within the current long */
+ while (old_buf[i] != new_buf[i] && ++i <= slen) {
+ nzrun_len++;
+ }
+ break;
+ } else {
+ i += sizeof(long);
+ nzrun_len += sizeof(long);
+ xor = *(long *)(old_buf + i) ^ *(long *)(new_buf + i);
+ }
+ }
+
+ while (old_buf[i] != new_buf[i] && ++i <= slen) {
+ nzrun_len++;
+ }
+ }
+
+ /* overflow */
+ if (d + nzrun_len + 2 > dlen) {
+ return -1;
+ }
+
+ d += uleb128_encode_small(dst + d, nzrun_len);
+ memcpy(dst + d, nzrun_start, nzrun_len);
+ d += nzrun_len;
+ nzrun_len = 0;
+ }
+
+ return d;
+}
+
+int xbzrle_decode_buffer(uint8_t *src, int slen, uint8_t *dst, int dlen)
+{
+ int i = 0, d = 0;
+ int ret;
+ uint32_t count = 0;
+
+ while (i < slen) {
+
+ /* zrun */
+ if ((slen - i) < 2 && *(src + i) & 0x80) {
+ return -1;
+ }
+
+ ret = uleb128_decode_small(src + i, &count);
+ if (ret < 0) {
+ return -1;
+ }
+ i += ret;
+ d += count;
+
+ /* overflow */
+ if (d > dlen) {
+ return -1;
+ }
+
+ /* completed decoding */
+ if (i == slen - 1) {
+ return d;
+ }
+
+ /* nzrun */
+ if ((slen - i) < 2 && *(src + i) & 0x80) {
+ return -1;
+ }
+ ret = uleb128_decode_small(src + i, &count);
+ if (ret < 0) {
+ return -1;
+ }
+ i += ret;
+
+ /* overflow */
+ if (d + count > dlen) {
+ return -1;
+ }
+
+ memcpy(dst + d , src + i, count);
+ d += count;
+ i += count;
+ }
+
+ return d;
+}
--
1.7.7.6
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v14 11/13] Add XBZRLE to ram_save_block and ram_save_live
2012-07-03 13:52 [Qemu-devel] [PATCH v14 00/13] XBZRLE delta for live migration of large memory app Orit Wasserman
` (9 preceding siblings ...)
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 10/13] Add xbzrle_encode_buffer and xbzrle_decode_buffer functions Orit Wasserman
@ 2012-07-03 13:52 ` Orit Wasserman
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 12/13] Add set_cachesize command Orit Wasserman
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 13/13] Add XBZRLE statistics Orit Wasserman
12 siblings, 0 replies; 33+ messages in thread
From: Orit Wasserman @ 2012-07-03 13:52 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, aliguori, quintela, Petter Svard, stefanha,
mdroth, Benoit Hudzia, blauwirbel, Orit Wasserman, chegu_vinod,
avi, Aidan Shribman, pbonzini, eblake
In the outgoing migration check to see if the page is cached and
changed than send compressed page by using save_xbrle_page function.
In the incoming migration check to see if RAM_SAVE_FLAG_XBRLE is set
and decompress the page (by using load_xbrle function).
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 | 185 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
migration.c | 26 ++++++++-
migration.h | 4 +
3 files changed, 208 insertions(+), 7 deletions(-)
diff --git a/arch_init.c b/arch_init.c
index 66df017..d88cb82 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -43,6 +43,7 @@
#include "hw/smbios.h"
#include "exec-memory.h"
#include "hw/pcspk.h"
+#include "qemu/page_cache.h"
#ifdef DEBUG_ARCH_INIT
#define DPRINTF(fmt, ...) \
@@ -102,6 +103,7 @@ const uint32_t arch_type = QEMU_ARCH;
#define RAM_SAVE_FLAG_PAGE 0x08
#define RAM_SAVE_FLAG_EOS 0x10
#define RAM_SAVE_FLAG_CONTINUE 0x20
+#define RAM_SAVE_FLAG_XBZRLE 0x40
#ifdef __ALTIVEC__
#include <altivec.h>
@@ -169,6 +171,27 @@ static int is_dup_page(uint8_t *page)
return 1;
}
+/* XBZRLE (Xor Based Zero Length Encoding */
+typedef struct XBZRLEHeader {
+ uint16_t xh_len;
+ uint8_t xh_flags;
+} XBZRLEHeader;
+
+/* struct contains XBZRLE cache and a static page
+ used by the compression */
+static struct {
+ /* buffer used for XBZRLE encoding */
+ uint8_t *encoded_buf;
+ /* buffer used for XBZRLE decoding */
+ uint8_t *decoded_buf;
+ /* Cache for XBZRLE */
+ PageCache *cache;
+} XBZRLE = {
+ .encoded_buf = NULL,
+ .decoded_buf = NULL,
+ .cache = NULL,
+};
+
static void save_block_hdr(QEMUFile *f, RAMBlock *block, ram_addr_t offset,
int cont, int flag)
{
@@ -181,19 +204,80 @@ static void save_block_hdr(QEMUFile *f, RAMBlock *block, ram_addr_t offset,
}
+#define ENCODING_FLAG_XBZRLE 0x1
+
+static int save_xbzrle_page(QEMUFile *f, uint8_t *current_data,
+ ram_addr_t current_addr, RAMBlock *block,
+ ram_addr_t offset, int cont, int stage)
+{
+ int encoded_len = 0, bytes_sent = -1, ret = -1;
+ XBZRLEHeader hdr = {
+ .xh_len = 0,
+ .xh_flags = 0,
+ };
+ uint8_t *prev_cached_page;
+
+ /* Stage 1 cache the page and exit.
+ Stage 2 check to see if page is cached , if not cache the page.
+ Stage 3 check if the page is cached and if not exit.
+ */
+ if (stage == 1 || !cache_is_cached(XBZRLE.cache, current_addr)) {
+ cache_insert(XBZRLE.cache, current_addr,
+ g_memdup(current_data, TARGET_PAGE_SIZE));
+ return -1;
+ }
+
+ prev_cached_page = get_cached_data(XBZRLE.cache, current_addr);
+
+ /* XBZRLE encoding (if there is no overflow) */
+ encoded_len = xbzrle_encode_buffer(prev_cached_page, current_data,
+ TARGET_PAGE_SIZE, XBZRLE.encoded_buf,
+ TARGET_PAGE_SIZE);
+ if (encoded_len == 0) {
+ DPRINTF("Unmodifed page skipping\n");
+ return 0;
+ } else if (encoded_len == -1) {
+ DPRINTF("Overflow\n");
+ /* update data in the cache */
+ memcpy(prev_cached_page, current_data, TARGET_PAGE_SIZE);
+ return -1;
+ }
+
+ /* we need to update the data in the cache, in order to get the same data
+ we cached we decode the encoded page on the cached data */
+ ret = xbzrle_decode_buffer(XBZRLE.encoded_buf, encoded_len,
+ prev_cached_page, TARGET_PAGE_SIZE);
+ g_assert(ret != -1);
+
+ hdr.xh_len = encoded_len;
+ hdr.xh_flags |= ENCODING_FLAG_XBZRLE;
+
+ /* Send XBZRLE based compressed page */
+ save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_XBZRLE);
+ qemu_put_byte(f, hdr.xh_flags);
+ qemu_put_be16(f, hdr.xh_len);
+ qemu_put_buffer(f, XBZRLE.encoded_buf, encoded_len);
+ bytes_sent = encoded_len + sizeof(hdr);
+
+ return bytes_sent;
+}
+
static RAMBlock *last_block;
static ram_addr_t last_offset;
-static int ram_save_block(QEMUFile *f)
+static int ram_save_block(QEMUFile *f, int stage)
{
RAMBlock *block = last_block;
ram_addr_t offset = last_offset;
int bytes_sent = -1;
MemoryRegion *mr;
+ ram_addr_t current_addr;
if (!block)
block = QLIST_FIRST(&ram_list.blocks);
+
+
do {
mr = block->mr;
if (memory_region_get_dirty(mr, offset, TARGET_PAGE_SIZE,
@@ -210,13 +294,31 @@ static int ram_save_block(QEMUFile *f)
save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_COMPRESS);
qemu_put_byte(f, *p);
bytes_sent = 1;
- } else {
+ } else if (migrate_use_xbzrle() && stage != 3) {
+ current_addr = block->offset + offset;
+ /* In stage 1 we only cache the pages before sending them
+ from the cache (uncompressed).
+ We doen't use compression for stage 3.
+ */
+ bytes_sent = save_xbzrle_page(f, p, current_addr, block,
+ offset, cont, stage);
+
+ /* send the cached page copy for consistency
+ In stage 3 we send the host page */
+ p = get_cached_data(XBZRLE.cache, current_addr);
+ }
+
+ /* either we didn't send yet (we may got XBZRLE overflow) */
+ if (bytes_sent == -1) {
save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_PAGE);
qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
bytes_sent = TARGET_PAGE_SIZE;
}
- break;
+ /* if page is unmodified lets continue to the next */
+ if (bytes_sent != 0) {
+ break;
+ }
}
offset += TARGET_PAGE_SIZE;
@@ -307,6 +409,13 @@ static void sort_ram_list(void)
static void migration_end(void)
{
memory_global_dirty_log_stop();
+
+ if (migrate_use_xbzrle()) {
+ cache_fini(XBZRLE.cache);
+ g_free(XBZRLE.cache);
+ g_free(XBZRLE.encoded_buf);
+ XBZRLE.cache = NULL;
+ }
}
int ram_save_live(QEMUFile *f, int stage, void *opaque)
@@ -331,6 +440,17 @@ int ram_save_live(QEMUFile *f, int stage, void *opaque)
last_offset = 0;
sort_ram_list();
+ if (migrate_use_xbzrle()) {
+ XBZRLE.cache = cache_init(migrate_xbzrle_cache_size() /
+ TARGET_PAGE_SIZE,
+ TARGET_PAGE_SIZE);
+ if (!XBZRLE.cache) {
+ DPRINTF("Error creating cache\n");
+ return -1;
+ }
+ XBZRLE.encoded_buf = g_malloc0(TARGET_PAGE_SIZE);
+ }
+
/* Make sure all dirty bits are set */
QLIST_FOREACH(block, &ram_list.blocks, next) {
for (addr = 0; addr < block->length; addr += TARGET_PAGE_SIZE) {
@@ -358,7 +478,7 @@ int ram_save_live(QEMUFile *f, int stage, void *opaque)
while ((ret = qemu_file_rate_limit(f)) == 0) {
int bytes_sent;
- bytes_sent = ram_save_block(f);
+ bytes_sent = ram_save_block(f, stage);
/* bytes_sent 0 represent unchanged page,
bytes_sent -1 represent no more blocks*/
if (bytes_sent > 0) {
@@ -386,10 +506,10 @@ int ram_save_live(QEMUFile *f, int stage, void *opaque)
int bytes_sent;
/* flush all remaining blocks regardless of rate limiting */
- while ((bytes_sent = ram_save_block(f)) != -1) {
+ while ((bytes_sent = ram_save_block(f, stage)) != -1) {
bytes_transferred += bytes_sent;
}
- memory_global_dirty_log_stop();
+ migration_end();
}
qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
@@ -402,6 +522,49 @@ int ram_save_live(QEMUFile *f, int stage, void *opaque)
return (stage == 2) && (expected_time <= migrate_max_downtime());
}
+static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host)
+{
+ int ret, rc = 0;
+ XBZRLEHeader hdr = {
+ .xh_len = 0,
+ .xh_flags = 0,
+ };
+
+ if (!XBZRLE.decoded_buf) {
+ XBZRLE.decoded_buf = g_malloc(TARGET_PAGE_SIZE);
+ }
+
+ /* extract RLE header */
+ hdr.xh_flags = qemu_get_byte(f);
+ hdr.xh_len = qemu_get_be16(f);
+
+ if (!(hdr.xh_flags & ENCODING_FLAG_XBZRLE)) {
+ fprintf(stderr, "Failed to load XBZRLE page - wrong compression!\n");
+ return -1;
+ }
+
+ if (hdr.xh_len > TARGET_PAGE_SIZE) {
+ fprintf(stderr, "Failed to load XBZRLE page - len overflow!\n");
+ return -1;
+ }
+ /* load data and decode */
+ qemu_get_buffer(f, XBZRLE.decoded_buf, hdr.xh_len);
+
+ /* decode RLE */
+ ret = xbzrle_decode_buffer(XBZRLE.decoded_buf, hdr.xh_len, host,
+ TARGET_PAGE_SIZE);
+ if (ret == -1) {
+ fprintf(stderr, "Failed to load XBZRLE page - decode error!\n");
+ rc = -1;
+ } else if (ret > TARGET_PAGE_SIZE) {
+ fprintf(stderr, "Failed to load XBZRLE page - size %d exceeds %d!\n",
+ ret, TARGET_PAGE_SIZE);
+ rc = -1;
+ }
+
+ return rc;
+}
+
static inline void *host_from_stream_offset(QEMUFile *f,
ram_addr_t offset,
int flags)
@@ -512,6 +675,16 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
host = host_from_stream_offset(f, addr, flags);
qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
+ } else if (flags & RAM_SAVE_FLAG_XBZRLE) {
+ void *host = host_from_stream_offset(f, addr, flags);
+ if (!host) {
+ return -EINVAL;
+ }
+
+ if (load_xbzrle(f, addr, host) < 0) {
+ ret = -EINVAL;
+ goto done;
+ }
}
error = qemu_file_get_error(f);
if (error) {
diff --git a/migration.c b/migration.c
index 4b68146..fae3cd7 100644
--- a/migration.c
+++ b/migration.c
@@ -43,6 +43,9 @@ enum {
#define MAX_THROTTLE (32 << 20) /* Migration speed throttling */
+/* Migration XBZRLE cache size */
+#define DEFAULT_MIGRATE_CACHE_SIZE (64 * 1024 * 1024)
+
static NotifierList migration_state_notifiers =
NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
@@ -55,7 +58,8 @@ static MigrationState *migrate_get_current(void)
static MigrationState current_migration = {
.state = MIG_STATE_SETUP,
.bandwidth_limit = MAX_THROTTLE,
- };
+ .xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE,
+};
return ¤t_migration;
}
@@ -429,6 +433,7 @@ static MigrationState *migrate_init(const MigrationParams *params)
MigrationState *s = migrate_get_current();
int64_t bandwidth_limit = s->bandwidth_limit;
bool enabled_capabilities[MIGRATION_CAPABILITY_MAX];
+ int64_t xbzrle_cache_size = s->xbzrle_cache_size;
memcpy(enabled_capabilities, s->enabled_capabilities,
sizeof(enabled_capabilities));
@@ -438,6 +443,7 @@ static MigrationState *migrate_init(const MigrationParams *params)
s->params = *params;
memcpy(s->enabled_capabilities, enabled_capabilities,
sizeof(enabled_capabilities));
+ s->xbzrle_cache_size = xbzrle_cache_size;
s->state = MIG_STATE_SETUP;
@@ -535,3 +541,21 @@ void qmp_migrate_set_downtime(double value, Error **errp)
value = MAX(0, MIN(UINT64_MAX, value));
max_downtime = (uint64_t)value;
}
+
+int migrate_use_xbzrle(void)
+{
+ MigrationState *s;
+
+ s = migrate_get_current();
+
+ return s->enabled_capabilities[MIGRATION_CAPABILITY_XBZRLE];
+}
+
+int64_t migrate_xbzrle_cache_size(void)
+{
+ MigrationState *s;
+
+ s = migrate_get_current();
+
+ return s->xbzrle_cache_size;
+}
diff --git a/migration.h b/migration.h
index 7582ecb..4fe8122 100644
--- a/migration.h
+++ b/migration.h
@@ -39,6 +39,7 @@ struct MigrationState
void *opaque;
MigrationParams params;
bool enabled_capabilities[MIGRATION_CAPABILITY_MAX];
+ int64_t xbzrle_cache_size;
};
void process_incoming_migration(QEMUFile *f);
@@ -103,4 +104,7 @@ int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t *new_buf, int slen,
uint8_t *dst, int dlen);
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);
+
#endif
--
1.7.7.6
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v14 12/13] Add set_cachesize command
2012-07-03 13:52 [Qemu-devel] [PATCH v14 00/13] XBZRLE delta for live migration of large memory app Orit Wasserman
` (10 preceding siblings ...)
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 11/13] Add XBZRLE to ram_save_block and ram_save_live Orit Wasserman
@ 2012-07-03 13:52 ` Orit Wasserman
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 13/13] Add XBZRLE statistics Orit Wasserman
12 siblings, 0 replies; 33+ messages in thread
From: Orit Wasserman @ 2012-07-03 13:52 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, aliguori, quintela, Petter Svard, stefanha,
mdroth, Benoit Hudzia, blauwirbel, Orit Wasserman, chegu_vinod,
avi, Aidan Shribman, pbonzini, eblake
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 | 10 ++++++++++
hmp-commands.hx | 19 +++++++++++++++++++
hmp.c | 13 +++++++++++++
hmp.h | 1 +
migration.c | 17 ++++++++++++++++-
migration.h | 2 ++
qapi-schema.json | 16 ++++++++++++++++
qmp-commands.hx | 23 +++++++++++++++++++++++
8 files changed, 100 insertions(+), 1 deletions(-)
diff --git a/arch_init.c b/arch_init.c
index d88cb82..3179ed2 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -192,6 +192,16 @@ static struct {
.cache = NULL,
};
+
+int64_t xbzrle_cache_resize(int64_t new_size)
+{
+ if (XBZRLE.cache != NULL) {
+ return cache_resize(XBZRLE.cache, new_size / TARGET_PAGE_SIZE) *
+ TARGET_PAGE_SIZE;
+ }
+ return round2pow2(new_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..c5b7308 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -829,6 +829,25 @@ 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 rounded 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 69711fb..7711bf6 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 fae3cd7..f9e85ef 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 =
@@ -522,6 +523,20 @@ 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;
+ }
+
+ s->xbzrle_cache_size = 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..23bde24 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);
+int64_t xbzrle_cache_resize(int64_t new_size);
+
#endif
diff --git a/qapi-schema.json b/qapi-schema.json
index 3eb3003..f478db3 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1381,6 +1381,22 @@
{ 'command': 'migrate_set_speed', 'data': {'value': 'int'} }
##
+# @migrate_set_cachesize
+#
+# Set XBZRLE cache size
+#
+# @value: cache size in bytes
+#
+# The size will be rounded 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 7e85e0d..1bf3bfe 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": 536870912 } }
+<- { "return": {} }
+
+EQMP
{
.name = "migrate_set_speed",
--
1.7.7.6
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v14 13/13] Add XBZRLE statistics
2012-07-03 13:52 [Qemu-devel] [PATCH v14 00/13] XBZRLE delta for live migration of large memory app Orit Wasserman
` (11 preceding siblings ...)
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 12/13] Add set_cachesize command Orit Wasserman
@ 2012-07-03 13:52 ` Orit Wasserman
2012-07-04 1:35 ` Eric Blake
12 siblings, 1 reply; 33+ messages in thread
From: Orit Wasserman @ 2012-07-03 13:52 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, aliguori, quintela, Petter Svard, stefanha,
mdroth, Benoit Hudzia, blauwirbel, Orit Wasserman, chegu_vinod,
avi, Aidan Shribman, pbonzini, eblake
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 | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
hmp.c | 13 ++++++++++
migration.c | 49 ++++++++++++++++++++++++++++++++++++++
migration.h | 9 +++++++
qapi-schema.json | 40 +++++++++++++++++++++++++++----
qmp-commands.hx | 38 ++++++++++++++++++++++++++++-
6 files changed, 209 insertions(+), 8 deletions(-)
diff --git a/arch_init.c b/arch_init.c
index 3179ed2..da3491e 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -202,8 +202,66 @@ int64_t xbzrle_cache_resize(int64_t new_size)
return round2pow2(new_size);
}
+/* accounting */
+typedef struct AccountingInfo {
+ uint64_t dup_pages;
+ uint64_t norm_pages;
+ uint64_t xbzrle_bytes;
+ uint64_t xbzrle_pages;
+ uint64_t xbzrle_cache_miss;
+ uint64_t iterations;
+ uint64_t xbzrle_overflows;
+} AccountingInfo;
+
+static AccountingInfo acct_info;
+
+static void acct_clear(void)
+{
+ memset(&acct_info, 0, sizeof(acct_info));
+}
+
+uint64_t dup_mig_bytes_transferred(void)
+{
+ return acct_info.dup_pages * TARGET_PAGE_SIZE;
+}
+
+uint64_t dup_mig_pages_transferred(void)
+{
+ return acct_info.dup_pages;
+}
+
+uint64_t norm_mig_bytes_transferred(void)
+{
+ return acct_info.norm_pages * TARGET_PAGE_SIZE;
+}
+
+uint64_t norm_mig_pages_transferred(void)
+{
+ return acct_info.norm_pages;
+}
+
+uint64_t xbzrle_mig_bytes_transferred(void)
+{
+ return acct_info.xbzrle_bytes;
+}
+
+uint64_t xbzrle_mig_pages_transferred(void)
+{
+ return acct_info.xbzrle_pages;
+}
+
+uint64_t xbzrle_mig_pages_cache_miss(void)
+{
+ return acct_info.xbzrle_cache_miss;
+}
+
+uint64_t xbzrle_mig_pages_overflow(void)
+{
+ return acct_info.xbzrle_overflows;
+}
+
static void save_block_hdr(QEMUFile *f, RAMBlock *block, ram_addr_t offset,
- int cont, int flag)
+ int cont, int flag)
{
qemu_put_be64(f, offset | cont | flag);
if (!cont) {
@@ -234,6 +292,7 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t *current_data,
if (stage == 1 || !cache_is_cached(XBZRLE.cache, current_addr)) {
cache_insert(XBZRLE.cache, current_addr,
g_memdup(current_data, TARGET_PAGE_SIZE));
+ acct_info.xbzrle_cache_miss++;
return -1;
}
@@ -248,6 +307,7 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t *current_data,
return 0;
} else if (encoded_len == -1) {
DPRINTF("Overflow\n");
+ acct_info.xbzrle_overflows++;
/* update data in the cache */
memcpy(prev_cached_page, current_data, TARGET_PAGE_SIZE);
return -1;
@@ -267,7 +327,9 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t *current_data,
qemu_put_byte(f, hdr.xh_flags);
qemu_put_be16(f, hdr.xh_len);
qemu_put_buffer(f, XBZRLE.encoded_buf, encoded_len);
+ acct_info.xbzrle_pages++;
bytes_sent = encoded_len + sizeof(hdr);
+ acct_info.xbzrle_bytes += bytes_sent;
return bytes_sent;
}
@@ -301,6 +363,7 @@ static int ram_save_block(QEMUFile *f, int stage)
p = memory_region_get_ram_ptr(mr) + offset;
if (is_dup_page(p)) {
+ acct_info.dup_pages++;
save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_COMPRESS);
qemu_put_byte(f, *p);
bytes_sent = 1;
@@ -323,6 +386,7 @@ static int ram_save_block(QEMUFile *f, int stage)
save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_PAGE);
qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
bytes_sent = TARGET_PAGE_SIZE;
+ acct_info.norm_pages++;
}
/* if page is unmodified lets continue to the next */
@@ -459,6 +523,7 @@ int ram_save_live(QEMUFile *f, int stage, void *opaque)
return -1;
}
XBZRLE.encoded_buf = g_malloc0(TARGET_PAGE_SIZE);
+ acct_clear();
}
/* Make sure all dirty bits are set */
@@ -493,6 +558,7 @@ int ram_save_live(QEMUFile *f, int stage, void *opaque)
bytes_sent -1 represent no more blocks*/
if (bytes_sent > 0) {
bytes_transferred += bytes_sent;
+ acct_info.iterations++;
} else if (bytes_sent == -1) { /* no more blocks */
break;
}
diff --git a/hmp.c b/hmp.c
index 7711bf6..7d18284 100644
--- a/hmp.c
+++ b/hmp.c
@@ -166,6 +166,19 @@ void hmp_info_migrate(Monitor *mon)
info->disk->total >> 10);
}
+ if (info->has_cache) {
+ monitor_printf(mon, "cache size: %" PRIu64 " bytes\n",
+ info->cache->cache_size);
+ monitor_printf(mon, "xbzrle transferred: %" PRIu64 " kbytes\n",
+ info->cache->xbzrle_bytes >> 10);
+ monitor_printf(mon, "xbzrle pages: %" PRIu64 " pages\n",
+ info->cache->xbzrle_pages);
+ monitor_printf(mon, "xbzrle cache miss: %" PRIu64 "\n",
+ info->cache->xbzrle_cache_miss);
+ monitor_printf(mon, "xbzrle overflow : %" PRIu64 "\n",
+ info->cache->xbzrle_overflow);
+ }
+
qapi_free_MigrationInfo(info);
}
diff --git a/migration.c b/migration.c
index f9e85ef..a49ed70 100644
--- a/migration.c
+++ b/migration.c
@@ -138,6 +138,17 @@ MigrationInfo *qmp_query_migrate(Error **errp)
info->capabilities->value->state = s->enabled_capabilities[i];
info->capabilities->next = NULL;
}
+
+ /* display xbzrle cache size */
+ if (migrate_use_xbzrle()) {
+ info->has_cache = true;
+ info->cache = g_malloc0(sizeof(*info->cache));
+ info->cache->cache_size = migrate_xbzrle_cache_size();
+ info->cache->xbzrle_bytes = xbzrle_mig_bytes_transferred();
+ info->cache->xbzrle_pages = xbzrle_mig_pages_transferred();
+ info->cache->xbzrle_cache_miss = xbzrle_mig_pages_cache_miss();
+ info->cache->xbzrle_overflow = xbzrle_mig_pages_overflow();
+ }
break;
case MIG_STATE_ACTIVE:
for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) {
@@ -160,6 +171,8 @@ MigrationInfo *qmp_query_migrate(Error **errp)
info->ram->transferred = ram_bytes_transferred();
info->ram->remaining = ram_bytes_remaining();
info->ram->total = ram_bytes_total();
+ info->ram->duplicate = dup_mig_pages_transferred();
+ info->ram->normal = norm_mig_pages_transferred();
if (blk_mig_active()) {
info->has_disk = true;
@@ -168,6 +181,16 @@ MigrationInfo *qmp_query_migrate(Error **errp)
info->disk->remaining = blk_mig_bytes_remaining();
info->disk->total = blk_mig_bytes_total();
}
+
+ if (migrate_use_xbzrle()) {
+ info->has_cache = true;
+ info->cache = g_malloc0(sizeof(*info->cache));
+ info->cache->cache_size = migrate_xbzrle_cache_size();
+ info->cache->xbzrle_bytes = xbzrle_mig_bytes_transferred();
+ info->cache->xbzrle_pages = xbzrle_mig_pages_transferred();
+ info->cache->xbzrle_cache_miss = xbzrle_mig_pages_cache_miss();
+ info->cache->xbzrle_overflow = xbzrle_mig_pages_overflow();
+ }
break;
case MIG_STATE_COMPLETED:
for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) {
@@ -182,6 +205,32 @@ MigrationInfo *qmp_query_migrate(Error **errp)
info->capabilities->next = NULL;
}
+ info->has_ram = true;
+ info->ram = g_malloc0(sizeof(*info->ram));
+ info->ram->transferred = ram_bytes_transferred();
+ info->ram->remaining = ram_bytes_remaining();
+ info->ram->total = ram_bytes_total();
+ info->ram->duplicate = dup_mig_pages_transferred();
+ info->ram->normal = norm_mig_pages_transferred();
+
+ if (blk_mig_active()) {
+ info->has_disk = true;
+ info->disk = g_malloc0(sizeof(*info->disk));
+ info->disk->transferred = blk_mig_bytes_transferred();
+ info->disk->remaining = blk_mig_bytes_remaining();
+ info->disk->total = blk_mig_bytes_total();
+ }
+
+ if (migrate_use_xbzrle()) {
+ info->has_cache = true;
+ info->cache = g_malloc0(sizeof(*info->cache));
+ info->cache->cache_size = migrate_xbzrle_cache_size();
+ info->cache->xbzrle_bytes = xbzrle_mig_bytes_transferred();
+ info->cache->xbzrle_pages = xbzrle_mig_pages_transferred();
+ info->cache->xbzrle_cache_miss = xbzrle_mig_pages_cache_miss();
+ info->cache->xbzrle_overflow = xbzrle_mig_pages_overflow();
+ }
+
info->has_status = true;
info->status = g_strdup("completed");
break;
diff --git a/migration.h b/migration.h
index 23bde24..8ddf52d 100644
--- a/migration.h
+++ b/migration.h
@@ -83,6 +83,15 @@ uint64_t ram_bytes_remaining(void);
uint64_t ram_bytes_transferred(void);
uint64_t ram_bytes_total(void);
+uint64_t dup_mig_bytes_transferred(void);
+uint64_t dup_mig_pages_transferred(void);
+uint64_t norm_mig_bytes_transferred(void);
+uint64_t norm_mig_pages_transferred(void);
+uint64_t xbzrle_mig_bytes_transferred(void);
+uint64_t xbzrle_mig_pages_transferred(void);
+uint64_t xbzrle_mig_pages_overflow(void);
+uint64_t xbzrle_mig_pages_cache_miss(void);
+
int ram_save_live(QEMUFile *f, int stage, void *opaque);
int ram_load(QEMUFile *f, void *opaque, int version_id);
diff --git a/qapi-schema.json b/qapi-schema.json
index f478db3..5925b8b 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -260,10 +260,35 @@
#
# @total: total amount of bytes involved in the migration process
#
-# Since: 0.14.0.
+# @duplicate: #optional, number of duplicate pages
+#
+# @normal : #optional, number of normal pages
+#
+# Since: 0.14.0, 'duplicate' and 'normal' since 1.2
##
{ 'type': 'MigrationStats',
- 'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' } }
+ 'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int', '*duplicate': 'int', '*normal': 'int' } }
+
+##
+# @CacheStats
+#
+# Detailed XBZRLE migration cache statistics
+#
+# @cache_size: XBZRLE cache size
+#
+# @xbzrle_bytes: amount of bytes already transferred to the target VM
+#
+# @xbzrle_pages: amount of pages transferred to the target VM
+#
+# @xbzrle_cache_miss: number of cache miss
+#
+# @xbzrle_overflow: number of overflows
+#
+# Since: 1.2
+##
+{ 'type': 'CacheStats',
+ 'data': {'cache-size': 'int', 'xbzrle-bytes': 'int', 'xbzrle-pages': 'int',
+ 'xbzrle-cache-miss': 'int', 'xbzrle-overflow': 'int' } }
##
# @MigrationInfo
@@ -282,13 +307,18 @@
# status, only returned if status is 'active' and it is a block
# migration
#
-# Since: 0.14.0
+# @params: #optional a list describing all the migration capabilities state
+#
+# @cache: #optional @MigrationStats containing detailed XBZRLE migration
+# statistics
+#
+# Since: 0.14.0, 'params' and 'cache' since 1.2
##
{ 'type': 'MigrationInfo',
'data': {'*status': 'str', '*ram': 'MigrationStats',
'*disk': 'MigrationStats',
- '*capabilities': ['MigrationCapabilityInfo']} }
-
+ '*capabilities': ['MigrationCapabilityInfo'],
+ '*cache': 'CacheStats'} }
##
# @query-migrate
#
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 1bf3bfe..c9c8919 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2093,18 +2093,28 @@ The main json-object contains the following:
- "transferred": amount transferred (json-int)
- "remaining": amount remaining (json-int)
- "total": total (json-int)
+ - "duplicate": number of duplicated pages (json-int)
+ - "normal" : number of normal pages transferred (json-int)
- "disk": only present if "status" is "active" and it is a block migration,
it is a json-object with the following disk information (in bytes):
- "transferred": amount transferred (json-int)
- "remaining": amount remaining (json-int)
- "total": total (json-int)
+- "params": migration capabilites state
+ - "xbzrle" : on/off (json-bool)
+- "cache": only present if "status" and XBZRLE is active.
+ It is a json-object with the following XBZRLE information:
+ - "cache-size": XBZRLE cache size
+ - "xbzrle-bytes": total XBZRLE bytes transferred
+ - "xbzrle-pages": number of XBZRLE compressed pages
+ - "cache-miss": number of cache misses
+ - "overflow": number of XBZRLE overflows
Examples:
1. Before the first migration
-
-> { "execute": "query-migrate" }
-<- { "return": {} }
+<- { "return": { "params" : { "xbzrle" : "off" } } }
2. Migration is done and has succeeded
@@ -2122,6 +2132,7 @@ Examples:
<- {
"return":{
"status":"active",
+ "params" : { "xbzrle" : "off" },
"ram":{
"transferred":123,
"remaining":123,
@@ -2136,6 +2147,7 @@ Examples:
<- {
"return":{
"status":"active",
+ "params" : { "xbzrle" : "off" },
"ram":{
"total":1057024,
"remaining":1053304,
@@ -2149,6 +2161,28 @@ Examples:
}
}
+5. Migration is being performed and XBZRLE is active:
+
+-> { "execute": "query-migrate" }
+<- {
+ "return":{
+ "status":"active",
+ "params" : { "xbzrle" : "on" },
+ "ram":{
+ "total":1057024,
+ "remaining":1053304,
+ "transferred":3720
+ },
+ "cache":{
+ "cache-size": 1024
+ "xbzrle-transferred":20971520,
+ "xbzrle-pages":2444343,
+ "xbzrle-cache-miss":2244,
+ "xbzrle-overflow":34434
+ }
+ }
+ }
+
EQMP
{
--
1.7.7.6
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v14 02/13] Add migration capabilities
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 02/13] Add migration capabilities Orit Wasserman
@ 2012-07-03 18:36 ` Eric Blake
2012-07-05 10:09 ` Orit Wasserman
0 siblings, 1 reply; 33+ messages in thread
From: Eric Blake @ 2012-07-03 18:36 UTC (permalink / raw)
To: Orit Wasserman
Cc: peter.maydell, aliguori, quintela, stefanha, qemu-devel, mdroth,
blauwirbel, avi, pbonzini, chegu_vinod
[-- Attachment #1: Type: text/plain, Size: 2486 bytes --]
On 07/03/2012 07:52 AM, Orit Wasserman wrote:
> Add migration capabilities that can be queried by the management.
> The management can query the source QEMU and the destination QEMU in order to
> verify both support some migration capability (currently only XBZRLE).
> The management can enable a capability for the next migration by using
> migrate_set_parameter command.
Couple of comment nits below. Also, if you don't mind a bit of
bike-shedding...
[roughly translated - feel free to ignore the bulk of this email; the
patch as-is works, if you don't think my alternate design makes sense to
implement]
> +++ b/hmp-commands.hx
> @@ -861,6 +861,20 @@ Set maximum tolerated downtime (in seconds) for migration.
> ETEXI
>
> {
> + .name = "migrate_set_parameter",
> + .args_type = "capability:s,state:b",
> + .params = "",
> + .help = "Enable the usage of a capability for migration",
> + .mhandler.cmd = hmp_migrate_set_parameter,
This requires the 'state' parameter to be passed. But wouldn't it be
easier to default things to state=true if no parameter was present,
since the most common usage of this command will be to enable, rather
than disable, a migration parameter?
> - /* no migration has happened ever */
> + /* no migration has happened ever show enabled capabilities */
Missing some punctuation, so this was hard to read. Maybe:
/* no migration has ever happened; show enabled capabilities */
> +++ b/qapi-schema.json
> +##
> +# @MigrationCapabilityInfo
> +#
> +# Migration capability information
> +#
> +# @capability: capability enum
> +#
> +# Since: 1.2
> +##
> +{ 'type': 'MigrationCapabilityInfo',
> + 'data': { 'capability' : 'MigrationCapability', 'state' : 'bool' } }
Back to the bike-shedding - if you would mark this '*state':'bool', then
the enabled parameter becomes optional on input (it should still always
be present on query-migration-capabilities output, though).
> +migrate_set_parameters
> +-------
> +
> +Enable/Disable migration capabilities
> +
> +- "xbzrle": xbzrle support
> +
> +Arguments:
> +
> +Example:
> +
> +-> { "execute": "migrate_set_parameters" , "arguments":
> + { "parameters": { "capability": "xbzrle", "state": true } ] } }
Missing a '[' after "parameters":
--
Eric Blake eblake@redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v14 04/13] Add cache handling functions
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 04/13] Add cache handling functions Orit Wasserman
@ 2012-07-03 19:23 ` Blue Swirl
2012-07-03 19:49 ` Eric Blake
2012-07-03 20:24 ` Eric Blake
1 sibling, 1 reply; 33+ messages in thread
From: Blue Swirl @ 2012-07-03 19:23 UTC (permalink / raw)
To: Orit Wasserman
Cc: peter.maydell, aliguori, quintela, stefanha, qemu-devel,
Benoit Hudzia, mdroth, Petter Svard, chegu_vinod, avi,
Aidan Shribman, pbonzini, eblake
On Tue, Jul 3, 2012 at 1:52 PM, Orit Wasserman <owasserm@redhat.com> wrote:
> Add LRU page cache mechanism.
> The page are accessed by their address.
>
> 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>
> ---
> Makefile.objs | 1 +
> include/qemu/page_cache.h | 79 +++++++++++++++++
> page_cache.c | 209 +++++++++++++++++++++++++++++++++++++++++++++
> qemu-common.h | 18 ++++
> 4 files changed, 307 insertions(+), 0 deletions(-)
> create mode 100644 include/qemu/page_cache.h
> create mode 100644 page_cache.c
>
> diff --git a/Makefile.objs b/Makefile.objs
> index 625c4d5..d7a199e 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -77,6 +77,7 @@ common-obj-y += qemu-char.o #aio.o
> common-obj-y += block-migration.o iohandler.o
> common-obj-y += pflib.o
> common-obj-y += bitmap.o bitops.o
> +common-obj-y += page_cache.o
>
> common-obj-$(CONFIG_POSIX) += migration-exec.o migration-unix.o migration-fd.o
> common-obj-$(CONFIG_WIN32) += version.o
> diff --git a/include/qemu/page_cache.h b/include/qemu/page_cache.h
> new file mode 100644
> index 0000000..3839ac7
> --- /dev/null
> +++ b/include/qemu/page_cache.h
> @@ -0,0 +1,79 @@
> +/*
> + * Page cache for QEMU
> + * The cache is base on a hash of the page address
> + *
> + * Copyright 2012 Red Hat, Inc. and/or its affiliates
> + *
> + * Authors:
> + * Orit Wasserman <owasserm@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#ifndef PAGE_CACHE_H
> +#define PAGE_CACHE_H
> +
> +/* Page cache for storing guest pages */
> +typedef struct PageCache PageCache;
> +
> +/**
> + * cache_init: Initialize the page cache
> + *
> + *
> + * Returns new allocated cache or NULL on error
> + *
> + * @cache pointer to the PageCache struct
> + * @num_pages: cache maximal number of cached pages
> + * @page_size: cache page size
> + */
> +PageCache *cache_init(int64_t num_pages, unsigned int page_size);
> +
> +/**
> + * cache_fini: free all cache resources
> + * @cache pointer to the PageCache struct
> + */
> +void cache_fini(PageCache *cache);
> +
> +/**
> + * cache_is_cached: Checks to see if the page is cached
> + *
> + * Returns %true if page is cached
> + *
> + * @cache pointer to the PageCache struct
> + * @addr: page addr
> + */
> +bool cache_is_cached(const PageCache *cache, uint64_t addr);
> +
> +/**
> + * get_cached_data: Get the data cached for an addr
> + *
> + * Returns pointer to the data cached or NULL if not cached
> + *
> + * @cache pointer to the PageCache struct
> + * @addr: page addr
> + */
> +uint8_t *get_cached_data(const PageCache *cache, uint64_t addr);
> +
> +/**
> + * cache_insert: insert the page into the cache. the previous value will be overwritten
> + *
> + * @cache pointer to the PageCache struct
> + * @addr: page address
> + * @pdata: pointer to the page
> + */
> +void cache_insert(PageCache *cache, uint64_t addr, uint8_t *pdata);
> +
> +/**
> + * cache_resize: resize the page cache. In case of size reduction the extra
> + * pages will be freed
> + *
> + * Returns -1 on error new cache size on success
> + *
> + * @cache pointer to the PageCache struct
> + * @num_pages: new page cache size (in pages)
> + */
> +int64_t cache_resize(PageCache *cache, int64_t num_pages);
> +
> +#endif
> diff --git a/page_cache.c b/page_cache.c
> new file mode 100644
> index 0000000..7122756
> --- /dev/null
> +++ b/page_cache.c
> @@ -0,0 +1,209 @@
> +/*
> + * Page cache for QEMU
> + * The cache is base on a hash of the page address
> + *
> + * Copyright 2012 Red Hat, Inc. and/or its affiliates
> + *
> + * Authors:
> + * Orit Wasserman <owasserm@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <strings.h>
> +#include <string.h>
> +#include <sys/time.h>
> +#include <sys/types.h>
> +#include <stdbool.h>
> +#include <glib.h>
> +#include <strings.h>
> +
> +#include "qemu-common.h"
> +#include "qemu/page_cache.h"
> +
> +#ifdef DEBUG_CACHE
> +#define DPRINTF(fmt, ...) \
> + do { fprintf(stdout, "cache: " fmt, ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) \
> + do { } while (0)
> +#endif
> +
> +typedef struct CacheItem CacheItem;
> +
> +struct CacheItem {
> + uint64_t it_addr;
> + unsigned long it_age;
> + uint8_t *it_data;
> +};
> +
> +struct PageCache {
> + CacheItem *page_cache;
> + unsigned int page_size;
> + int64_t max_num_items;
> + uint64_t max_item_age;
> + int64_t num_items;
> +};
> +
> +PageCache *cache_init(int64_t num_pages, unsigned int page_size)
> +{
> + int i;
> +
> + PageCache *cache = g_malloc(sizeof(PageCache));
> +
> + if (num_pages <= 0) {
> + DPRINTF("invalid number pages\n");
> + return NULL;
> + }
> +
> + /* round down to the nearest power of 2 */
> + if (!is_power_of_2(num_pages)) {
> + num_pages = round2pow2(num_pages);
> + DPRINTF("rounding down to %" PRId64 "\n", num_pages);
> + }
> + cache->page_size = page_size;
> + cache->num_items = 0;
> + cache->max_item_age = 0;
> + cache->max_num_items = num_pages;
> +
> + DPRINTF("Setting cache buckets to %" PRId64 "\n", cache->max_num_items);
> +
> + cache->page_cache = g_malloc((cache->max_num_items) *
> + sizeof(CacheItem));
> +
> + for (i = 0; i < cache->max_num_items; i++) {
> + cache->page_cache[i].it_data = NULL;
> + cache->page_cache[i].it_age = 0;
> + cache->page_cache[i].it_addr = -1;
> + }
> +
> + return cache;
> +}
> +
> +void cache_fini(PageCache *cache)
> +{
> + int i;
> +
> + g_assert(cache);
> + g_assert(cache->page_cache);
> +
> + for (i = 0; i < cache->max_num_items; i++) {
> + g_free(cache->page_cache[i].it_data);
> + cache->page_cache[i].it_data = 0;
> + }
> +
> + g_free(cache->page_cache);
> + cache->page_cache = NULL;
> +}
> +
> +static unsigned long cache_get_cache_pos(const PageCache *cache,
> + uint64_t address)
> +{
> + unsigned long pos;
> +
> + g_assert(cache->max_num_items);
> + pos = (address / cache->page_size) & (cache->max_num_items - 1);
> + return pos;
> +}
> +
> +bool cache_is_cached(const PageCache *cache, uint64_t addr)
> +{
> + unsigned long pos;
> +
> + g_assert(cache);
> + g_assert(cache->page_cache);
> +
> + pos = cache_get_cache_pos(cache, addr);
> +
> + return (cache->page_cache[pos].it_addr == addr);
> +}
> +
> +static CacheItem *cache_get_by_addr(const PageCache *cache, uint64_t addr)
> +{
> + unsigned long pos;
> +
> + g_assert(cache);
> + g_assert(cache->page_cache);
> +
> + pos = cache_get_cache_pos(cache, addr);
> +
> + return &cache->page_cache[pos];
> +}
> +
> +uint8_t *get_cached_data(const PageCache *cache, uint64_t addr)
> +{
> + return cache_get_by_addr(cache, addr)->it_data;
> +}
> +
> +void cache_insert(PageCache *cache, unsigned long addr, uint8_t *pdata)
> +{
> +
> + CacheItem *it = NULL;
> +
> + g_assert(cache);
> + g_assert(cache->page_cache);
> +
> + /* actual update of entry */
> + it = cache_get_by_addr(cache, addr);
> +
> + if (!it->it_data) {
> + cache->num_items++;
> + }
> +
> + it->it_data = pdata;
> + it->it_age = ++cache->max_item_age;
> + it->it_addr = addr;
> +}
> +
> +int64_t cache_resize(PageCache *cache, int64_t new_num_pages)
> +{
> + PageCache *new_cache;
> + int i;
> +
> + CacheItem *old_it, *new_it;
> +
> + g_assert(cache);
> +
> + /* cache was not inited */
> + if (cache->page_cache == NULL) {
> + return -1;
> + }
> +
> + /* same size */
> + if (round2pow2(new_num_pages) == cache->max_num_items) {
> + return cache->max_num_items;
> + }
> +
> + new_cache = cache_init(new_num_pages, cache->page_size);
> + if (!(new_cache)) {
> + DPRINTF("Error creating new cache\n");
> + return -1;
> + }
> +
> + /* move all data from old cache */
> + for (i = 0; i < cache->max_num_items; i++) {
> + old_it = &cache->page_cache[i];
> + if (old_it->it_addr != -1) {
> + /* check for collision , if there is, keep the first value */
> + new_it = cache_get_by_addr(new_cache, old_it->it_addr);
> + if (new_it->it_data) {
> + g_free(old_it->it_data);
> + } else {
> + cache_insert(new_cache, old_it->it_addr, old_it->it_data);
> + }
> + }
> + }
> +
> + cache->page_cache = new_cache->page_cache;
> + cache->max_num_items = new_cache->max_num_items;
> + cache->num_items = new_cache->num_items;
> +
> + g_free(new_cache);
> +
> + return cache->max_num_items;
> +}
> diff --git a/qemu-common.h b/qemu-common.h
> index c8c6b2a..1ce7023 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -1,3 +1,4 @@
> +
> /* Common header file that is included by all of qemu. */
> #ifndef QEMU_COMMON_H
> #define QEMU_COMMON_H
> @@ -417,6 +418,23 @@ static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
> /* Round number up to multiple */
> #define QEMU_ALIGN_UP(n, m) QEMU_ALIGN_DOWN((n) + (m) - 1, (m))
>
> +static inline bool is_power_of_2(int64_t value)
> +{
> + if (!value) {
> + return 0;
> + }
> +
> + return !(value & (value - 1));
> +}
> +
> +static inline int64_t round2pow2(int64_t value)
> +{
> + while (!is_power_of_2(value)) {
> + value &= ~(1 << (ffs(value) - 1));
ffs() only uses 'int', not int64_t. ffsl() is not universally available.
> + }
> + return value;
> +}
> +
> #include "module.h"
>
> #endif
> --
> 1.7.7.6
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v14 07/13] Add debugging infrastructure
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 07/13] Add debugging infrastructure Orit Wasserman
@ 2012-07-03 19:25 ` Blue Swirl
2012-07-04 7:19 ` Orit Wasserman
0 siblings, 1 reply; 33+ messages in thread
From: Blue Swirl @ 2012-07-03 19:25 UTC (permalink / raw)
To: Orit Wasserman
Cc: peter.maydell, aliguori, quintela, stefanha, qemu-devel, mdroth,
chegu_vinod, avi, pbonzini, eblake
On Tue, Jul 3, 2012 at 1:52 PM, Orit Wasserman <owasserm@redhat.com> wrote:
> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> ---
> arch_init.c | 33 +++++++++++++++++++++++++++------
> 1 files changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/arch_init.c b/arch_init.c
> index 9dafb6e..ee20c33 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -44,6 +44,14 @@
> #include "exec-memory.h"
> #include "hw/pcspk.h"
>
> +#ifdef DEBUG_ARCH_INIT
> +#define DPRINTF(fmt, ...) \
> + do { fprintf(stdout, "arch_init: " fmt, ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) \
> + do { } while (0)
> +#endif
I think you missed my comments to the version that Juan sent, about
using trace points. Also in Juan's version the %lds were changed to
PRId64s which now are reverted, why?
> +
> #ifdef TARGET_SPARC
> int graphic_width = 1024;
> int graphic_height = 768;
> @@ -380,6 +388,9 @@ int ram_save_live(QEMUFile *f, int stage, void *opaque)
>
> expected_time = ram_save_remaining() * TARGET_PAGE_SIZE / bwidth;
>
> + DPRINTF("ram_save_live: expected(%ld) <= max(%ld)?\n", expected_time,
> + migrate_max_downtime());
> +
> return (stage == 2) && (expected_time <= migrate_max_downtime());
> }
>
> @@ -416,8 +427,11 @@ static inline void *host_from_stream_offset(QEMUFile *f,
> int ram_load(QEMUFile *f, void *opaque, int version_id)
> {
> ram_addr_t addr;
> - int flags;
> + int flags, ret = 0;
> int error;
> + static uint64_t seq_iter;
> +
> + seq_iter++;
>
> if (version_id < 4 || version_id > 4) {
> return -EINVAL;
> @@ -447,8 +461,10 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
>
> QLIST_FOREACH(block, &ram_list.blocks, next) {
> if (!strncmp(id, block->idstr, sizeof(id))) {
> - if (block->length != length)
> - return -EINVAL;
> + if (block->length != length) {
> + ret = -EINVAL;
> + goto done;
> + }
> break;
> }
> }
> @@ -456,7 +472,8 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
> if (!block) {
> fprintf(stderr, "Unknown ramblock \"%s\", cannot "
> "accept migration\n", id);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto done;
> }
>
> total_ram_bytes -= length;
> @@ -490,11 +507,15 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
> }
> error = qemu_file_get_error(f);
> if (error) {
> - return error;
> + ret = error;
> + goto done;
> }
> } while (!(flags & RAM_SAVE_FLAG_EOS));
>
> - return 0;
> +done:
> + DPRINTF("Completed load of VM with exit code %d seq iteration %ld\n",
> + ret, seq_iter);
> + return ret;
> }
>
> #ifdef HAS_AUDIO
> --
> 1.7.7.6
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v14 03/13] Add XBZRLE documentation
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 03/13] Add XBZRLE documentation Orit Wasserman
@ 2012-07-03 19:45 ` Eric Blake
2012-07-04 8:29 ` Orit Wasserman
0 siblings, 1 reply; 33+ messages in thread
From: Eric Blake @ 2012-07-03 19:45 UTC (permalink / raw)
To: Orit Wasserman
Cc: peter.maydell, aliguori, quintela, stefanha, qemu-devel, mdroth,
blauwirbel, avi, pbonzini, chegu_vinod
[-- Attachment #1: Type: text/plain, Size: 2856 bytes --]
On 07/03/2012 07:52 AM, Orit Wasserman wrote:
> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> ---
> docs/xbzrle.txt | 133 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 133 insertions(+), 0 deletions(-)
> create mode 100644 docs/xbzrle.txt
> +
> +Example
> +old buffer:
> +1000 zeros
> +05 06 07 08 09 0a 0b 0c 0d 0e 0f 10 11 12 13 00 00 6b 00 6d
> +3072 zeros
That's only 4092 bytes, but a page is typically 4096. I think you meant
3076 trailing zeros.
> +
> +new buffer:
> +1000 zeros
> +01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 00 00 67 00 69
> +3702 zeros
That's 4722 bytes; looks like a transposition error, and given the above
comment, I still think you want 3076 trailing zeros.
> +
> +encoded buffer:
> +
> +encoded length 29
> +e8 07 18 00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 00 00 67 00 69 00 00
> +00 80 18
Looks better, but still not quite accurate. Per the spec, you always
end on a nzrun, which means you should not have transmitted the last two
bytes (80 18 => 3072, which is the length of your trailing zrun). I
also find it weak that your example never shows an unchanged byte on
anything other than 00; having at least one non-zero unchanged byte may
make it more obvious in the encoded example that it is the xor
difference that determines a zrun vs. nzrun, and not the old or new
buffer contents.
Also, reading that encoding says you have 1000 zrun, then 24 bytes of
nzrun starting with a leading 00, but based on your old and new buffer,
the only way to have a leading 00 in your nzrun is if you count a zrun
of 999 bytes. Are you sure you didn't test with a buffer of 1001
leading zeros, then 20 bytes of interest, then 3073 trailing zeros?
Given your old and new buffer as-is, and my assumption that you are
compressing a page of exactly 4096 bytes (3076 trailing zeros after your
20-byte window of interesting data), I see at least the following four
valid encodings, listed in increasing difficulty of computation:
8-byte long at a time; all chunks that differ in at least one byte are
treated as at least 8 bytes of nzrun, transmit 27 bytes
e8 07 18 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 00 00 67 00 69 00
00 00 00
4 bytes at a time; all chunks that differ in at least one byte are
treated as at least 4 bytes of nzrun, transmit 23 bytes
e8 07 14 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 00 00 67 00 69
list every single zrun (except trailing), even if only one byte long,
transmit 24 bytes
e8 07 0f 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 02 01 67 01 01 69
avoid a 1-byte zrun, transmit 23 bytes
e8 07 0f 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 02 03 67 00 69
--
Eric Blake eblake@redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v14 04/13] Add cache handling functions
2012-07-03 19:23 ` Blue Swirl
@ 2012-07-03 19:49 ` Eric Blake
2012-07-04 7:04 ` Orit Wasserman
0 siblings, 1 reply; 33+ messages in thread
From: Eric Blake @ 2012-07-03 19:49 UTC (permalink / raw)
To: Blue Swirl
Cc: peter.maydell, aliguori, quintela, Petter Svard, stefanha,
qemu-devel, mdroth, Orit Wasserman, Benoit Hudzia, avi,
Aidan Shribman, pbonzini, chegu_vinod
[-- Attachment #1: Type: text/plain, Size: 825 bytes --]
On 07/03/2012 01:23 PM, Blue Swirl wrote:
>> +
>> +static inline int64_t round2pow2(int64_t value)
round up or down?
>> +{
>> + while (!is_power_of_2(value)) {
>> + value &= ~(1 << (ffs(value) - 1));
>
> ffs() only uses 'int', not int64_t. ffsl() is not universally available.
>
>> + }
>> + return value;
>> +}
Not to mention that iterating one bit at a time is inefficient. We
already gave you several more efficient solutions; my favorite being:
static inline int64_t pow2floor(int64_t value)
{
if (!is_power_of_2(value)) {
value = 0x8000000000000000ULL >> clz64(value);
}
return value;
}
since clz64() is already part of qemu sources.
--
Eric Blake eblake@redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v14 04/13] Add cache handling functions
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 04/13] Add cache handling functions Orit Wasserman
2012-07-03 19:23 ` Blue Swirl
@ 2012-07-03 20:24 ` Eric Blake
1 sibling, 0 replies; 33+ messages in thread
From: Eric Blake @ 2012-07-03 20:24 UTC (permalink / raw)
To: Orit Wasserman
Cc: peter.maydell, aliguori, quintela, stefanha, qemu-devel, mdroth,
blauwirbel, Petter Svard, Benoit Hudzia, avi, Aidan Shribman,
pbonzini, chegu_vinod
[-- Attachment #1: Type: text/plain, Size: 3122 bytes --]
On 07/03/2012 07:52 AM, Orit Wasserman wrote:
> Add LRU page cache mechanism.
> The page are accessed by their address.
>
> 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>
> +PageCache *cache_init(int64_t num_pages, unsigned int page_size)
> +{
> + int i;
Type mismatch. Either 'i' needs to be int64_t, or you don't need
num_pages to be 64-bit. Although I think it will be unlikely to
encounter someone with a desire to use 2**32 pages of 4096 bytes each as
their cache, I can't rule it out, so I think 'i' needs to be bigger
rather than changing your API to be smaller.
> +
> + PageCache *cache = g_malloc(sizeof(PageCache));
Personally, I like the style g_malloc(sizeof(*cache)), as it is immune
to type changes on cache. If you later change the type of 'cache', your
style has to make two edits to this line, while my style only makes one
edit.
> +
> + if (num_pages <= 0) {
> + DPRINTF("invalid number pages\n");
s/number pages/number of pages/
> + cache->page_cache = g_malloc((cache->max_num_items) *
> + sizeof(CacheItem));
Here my style is even more useful. With your style, I have to search
elsewhere in the code to validate that cache->page_cache really does
have the type CacheItem; but my style means I can see the result at once
without having to care about typing:
cache->page_cache = g_malloc(cache->max_num_items *
sizeof(*cache->page_cache));
> +void cache_fini(PageCache *cache)
> +{
> + int i;
Type mismatch again; max_num_items can be larger than INT_MAX, so you
need a bigger type for 'i'.
> +
> + g_assert(cache);
> + g_assert(cache->page_cache);
> +
> + for (i = 0; i < cache->max_num_items; i++) {
> + g_free(cache->page_cache[i].it_data);
> + cache->page_cache[i].it_data = 0;
Wasted assignment, since you are just about to free cache->page_cache
anyways.
> +uint8_t *get_cached_data(const PageCache *cache, uint64_t addr)
> +{
> + return cache_get_by_addr(cache, addr)->it_data;
> +}
> +
> +void cache_insert(PageCache *cache, unsigned long addr, uint8_t *pdata)
Why are you using 'uint64_t addr' in some places, and 'unsigned long
addr' in others?
> + /* move all data from old cache */
> + for (i = 0; i < cache->max_num_items; i++) {
> + old_it = &cache->page_cache[i];
> + if (old_it->it_addr != -1) {
> + /* check for collision , if there is, keep the first value */
s/ ,/,/ (no space before comma in English)
Shouldn't we instead prefer to keep the page with larger age, regardless
of whether we found it first or second? That is, a cache is more likely
to be useful on most-recently-used pages, rather than on which address
happened to map to the collision first.
--
Eric Blake eblake@redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v14 09/13] Add migration_end function
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 09/13] Add migration_end function Orit Wasserman
@ 2012-07-03 20:38 ` Eric Blake
2012-07-04 7:19 ` Orit Wasserman
0 siblings, 1 reply; 33+ messages in thread
From: Eric Blake @ 2012-07-03 20:38 UTC (permalink / raw)
To: Orit Wasserman
Cc: peter.maydell, aliguori, quintela, stefanha, qemu-devel, mdroth,
blauwirbel, avi, pbonzini, chegu_vinod
[-- Attachment #1: Type: text/plain, Size: 556 bytes --]
On 07/03/2012 07:52 AM, Orit Wasserman wrote:
> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> ---
> arch_init.c | 7 ++++++-
> 1 files changed, 6 insertions(+), 1 deletions(-)
You failed to account for my v13 review that you missed a hunk in this
patch (you mistakenly delayed it to 11/13). Furthermore, hasn't this
patch (and several others) were already picked up by Juan; you should
rebase on top of his pull request.
--
Eric Blake eblake@redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v14 10/13] Add xbzrle_encode_buffer and xbzrle_decode_buffer functions
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 10/13] Add xbzrle_encode_buffer and xbzrle_decode_buffer functions Orit Wasserman
@ 2012-07-03 21:32 ` Eric Blake
2012-07-03 21:39 ` Eric Blake
2012-07-04 7:24 ` Orit Wasserman
0 siblings, 2 replies; 33+ messages in thread
From: Eric Blake @ 2012-07-03 21:32 UTC (permalink / raw)
To: Orit Wasserman
Cc: peter.maydell, aliguori, quintela, stefanha, qemu-devel, mdroth,
blauwirbel, Petter Svard, Benoit Hudzia, avi, Aidan Shribman,
pbonzini, chegu_vinod
[-- Attachment #1: Type: text/plain, Size: 8585 bytes --]
On 07/03/2012 07:52 AM, Orit Wasserman wrote:
> 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>
> +int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t *new_buf, int slen,
> + uint8_t *dst, int dlen)
> +{
> + uint32_t zrun_len = 0, nzrun_len = 0;
> + int d = 0, i = 0, start;
> + long res, xor;
> + uint8_t *nzrun_start = NULL;
> +
> + g_assert(!((uintptr_t)old_buf & (sizeof(long) - 1)) &&
> + !((uintptr_t)new_buf & (sizeof(long) - 1)) &&
> + !(slen & (sizeof(long) - 1)));
I know I suggested this in v13, so now I'm bike-shedding my own review,
but I think it might be a bit more legible as:
g_assert(!(((uintptr_t)old_buf) | ((uintptr_t)new_buf) | slen) &
(sizeof(long) - 1)));
> +
> + while (i < slen) {
> + /* overflow */
> + if (d + 2 > dlen) {
> + return -1;
> + }
> +
> + /* not aligned to sizeof(long) */
> + res = (slen - i) % sizeof(long);
> + if (res) {
> + start = i;
> + while (!(old_buf[i] ^ new_buf[i]) && (i - start) <= res) {
> + zrun_len++;
> + i++;
> + }
> + }
I'm not sure why you need 'start'; this same block will do the same
amount of computation with less typing and register pressure:
res = (slen - i) % sizeof(long);
while (res && old_buf[i] == new_buf[i]) {
zrun_len++;
i++;
res--;
}
> +
> + if (zrun_len == res) {
If you use my above rewrite, then this condition changes to:
if (!res) {
> + while (i <= (slen - sizeof(long)) &&
I'd shorten this to:
while (i < slen &&
> + (*(long *)(old_buf + i)) == (*(long *)(new_buf + i))) {
> + i += sizeof(long);
> + zrun_len += sizeof(long);
> + }
> +
> + /* go over the rest */
> + while (old_buf[i] == new_buf[i] && ++i <= slen) {
Buffer overrun: if the page ends on a zrun, then at this point, i ==
slen, and old_buf[i] is now pointing one past the end of the array. You
need to swap the conditions, and only increment 'i' inside the {}, as in:
while (i < slen && old_buf[i] == new_buf[i]) {
i++;
> + zrun_len++;
> + }
> + }
> +
> + /* buffer unchanged */
> + if (zrun_len == slen) {
> + return 0;
> + }
> +
> + /* skip last zero run */
> + if (i == slen + 1) {
Buffer overrun. The loop should terminate at 'i == slen', not at 'i ==
slen + 1', since old_buf[slen - 1] is the last addressable byte.
> + return d;
> + }
> +
> + d += uleb128_encode_small(dst + d, zrun_len);
> +
> + /* no nzrun */
> + if (i == slen) {
> + return d;
> + }
This is the correct code but repeats the intent of what you were trying
just before the uleb128_encode_small().
> +
> + zrun_len = 0;
> + nzrun_start = new_buf + i;
Missing an overflow check here. If there are fewer than 2 bytes before
we hit dlen, then there is no way that we can fit in an nzrun, so we
might as well quit here.
> +
> + /* not aligned to sizeof(long) */
> + res = (slen - i) % sizeof(long);
> + if (res) {
> + start = i;
> + while (old_buf[i] != new_buf[i] && i - start < res) {
> + i++;
> + nzrun_len++;
> + }
> + }
Again, you can exploit the fact that slen is aligned, and shorten to:
res = (slen - i) % sizeof(long);
while (res && old_bf[i] != new_buf[i]) {
i++;
nzrun_len++;
res--;
}
> +
> + if (nzrun_len == res) {
and this would change to:
if (!res) {
> + /* truncation to 32-bit long okay */
> + long mask = 0x0101010101010101ULL;
> + xor = *(long *)(old_buf + i) ^ *(long *)(new_buf + i);
> + printf("cont\n");
Stray debugging comment.
> + while (i <= (slen - sizeof(long))) {
shorter to write as:
while (i < slen) {
> + if ((xor - mask) & ~xor & (mask << 7)) {
> + /* found the end of an nzrun within the current long */
> + while (old_buf[i] != new_buf[i] && ++i <= slen) {
No need to compare against slen here, since the outer while guarantees
that. You can simplify to:
while (old_buf[i] != new_buf[i]) {
i++;
> + nzrun_len++;
> + }
> + break;
> + } else {
> + i += sizeof(long);
> + nzrun_len += sizeof(long);
> + xor = *(long *)(old_buf + i) ^ *(long *)(new_buf + i);
> + }
> + }
> +
At this point, you are guaranteed to have found the end of the nzrun
(either you found two equal bytes, or you hit slen), which means...
> + while (old_buf[i] != new_buf[i] && ++i <= slen) {
> + nzrun_len++;
> + }
this while loop is dead code. Furthermore, this while loop is a
potential out-of-bounds-read (remember, old_buf[slen] is not valid
memory), so nuking it is the way to go.
> + }
> +
> + /* overflow */
> + if (d + nzrun_len + 2 > dlen) {
> + return -1;
There is a corner case where this reports overflow even when there was
none - that is when the old_buf ends in an nzrun of 0x7f bytes or less,
so the encoding occupies only 1 byte, and the encoded version would
still fit in dlen. I think you want to defer your overflow checking
until after...
> + }
> +
> + d += uleb128_encode_small(dst + d, nzrun_len);
...you know the encoded size. But that only works if you made sure
earlier that there was room to encode nzrun_len (up to 2 bytes) in the
first place.
> + memcpy(dst + d, nzrun_start, nzrun_len);
> + d += nzrun_len;
> + nzrun_len = 0;
> + }
> +
> + return d;
> +}
> +
Do you have any unit tests that pass in various inputs and compare
against expected outputs?
> +int xbzrle_decode_buffer(uint8_t *src, int slen, uint8_t *dst, int dlen)
> +{
> + int i = 0, d = 0;
> + int ret;
> + uint32_t count = 0;
> +
> + while (i < slen) {
> +
> + /* zrun */
> + if ((slen - i) < 2 && *(src + i) & 0x80) {
> + return -1;
> + }
You can simplify this - the protocol demands an nzrun after every zrun,
which means at this point in the decode, there must be at least 3 bytes
to be valid. It's simpler to write:
if (slen - i < 2) {
return -1;
}
> +
> + ret = uleb128_decode_small(src + i, &count);
> + if (ret < 0) {
> + return -1;
> + }
I'd also add a check that we only ever get a zero-length zrun at the
start of a buffer (anywhere else, a zero-length zrun is invalid); as in:
if (!count && i) {
return -1;
}
> + i += ret;
> + d += count;
> +
> + /* overflow */
> + if (d > dlen) {
> + return -1;
> + }
> +
> + /* completed decoding */
No, if we got here, then we were handed an invalid stream. Remember,
the protocol says that every zrun must be followed by an nzrun.
> + if (i == slen - 1) {
> + return d;
> + }
> +
> + /* nzrun */
> + if ((slen - i) < 2 && *(src + i) & 0x80) {
> + return -1;
> + }
Again, a valid nzrun is at least 2 bytes, so no need to worry about the
contents of *src, it is sufficient to check:
if (slen - i < 2) {
return -1;
}
> + ret = uleb128_decode_small(src + i, &count);
> + if (ret < 0) {
An nzrun should be a non-zero value; I'd write this as (ret <= 0) to
rule out an attempt to pass a zero-length nzrun.
> + return -1;
> + }
> + i += ret;
> +
> + /* overflow */
> + if (d + count > dlen) {
> + return -1;
> + }
> +
> + memcpy(dst + d , src + i, count);
> + d += count;
> + i += count;
> + }
> +
> + return d;
> +}
>
--
Eric Blake eblake@redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v14 10/13] Add xbzrle_encode_buffer and xbzrle_decode_buffer functions
2012-07-03 21:32 ` Eric Blake
@ 2012-07-03 21:39 ` Eric Blake
2012-07-04 0:20 ` Eric Blake
2012-07-04 7:24 ` Orit Wasserman
1 sibling, 1 reply; 33+ messages in thread
From: Eric Blake @ 2012-07-03 21:39 UTC (permalink / raw)
Cc: peter.maydell, aliguori, quintela, Petter Svard, stefanha,
qemu-devel, mdroth, blauwirbel, Orit Wasserman, chegu_vinod,
Benoit Hudzia, avi, pbonzini, Aidan Shribman
[-- Attachment #1: Type: text/plain, Size: 540 bytes --]
On 07/03/2012 03:32 PM, Eric Blake wrote:
>> + ret = uleb128_decode_small(src + i, &count);
>> + if (ret < 0) {
>
> An nzrun should be a non-zero value; I'd write this as (ret <= 0) to
> rule out an attempt to pass a zero-length nzrun.
Correcting myself,
if (ret < 0 || !count) {
At this point, I think I will just bite the bullet and post a version of
this code that incorporates my review.
--
Eric Blake eblake@redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v14 10/13] Add xbzrle_encode_buffer and xbzrle_decode_buffer functions
2012-07-03 21:39 ` Eric Blake
@ 2012-07-04 0:20 ` Eric Blake
2012-07-04 12:51 ` Orit Wasserman
0 siblings, 1 reply; 33+ messages in thread
From: Eric Blake @ 2012-07-04 0:20 UTC (permalink / raw)
Cc: peter.maydell, aliguori, quintela, stefanha, Orit Wasserman,
qemu-devel, mdroth, blauwirbel, Petter Svard, Benoit Hudzia, avi,
Aidan Shribman, pbonzini, chegu_vinod
[-- Attachment #1: Type: text/plain, Size: 4365 bytes --]
On 07/03/2012 03:39 PM, Eric Blake wrote:
> On 07/03/2012 03:32 PM, Eric Blake wrote:
>
>>> + ret = uleb128_decode_small(src + i, &count);
>>> + if (ret < 0) {
>>
>> An nzrun should be a non-zero value; I'd write this as (ret <= 0) to
>> rule out an attempt to pass a zero-length nzrun.
>
> Correcting myself,
>
> if (ret < 0 || !count) {
>
> At this point, I think I will just bite the bullet and post a version of
> this code that incorporates my review.
Something like this (lightly tested):
/*
page = zrun nzrun
| zrun nzrun page
zrun = length
nzrun = length byte...
length = uleb128 encoded integer
*/
int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t *new_buf, int slen,
uint8_t *dst, int dlen)
{
uint32_t zrun_len = 0, nzrun_len = 0;
int d = 0, i = 0;
long res, xor;
uint8_t *nzrun_start = NULL;
g_assert(!(((uintptr_t)old_buf | (uintptr_t)new_buf | slen) %
sizeof(long)));
while (i < slen) {
/* overflow */
if (d + 2 > dlen) {
return -1;
}
/* not aligned to sizeof(long) */
res = (slen - i) % sizeof(long);
while (res && old_buf[i] == new_buf[i]) {
zrun_len++;
i++;
res--;
}
if (!res) {
while (i < slen &&
(*(long *)(old_buf + i)) == (*(long *)(new_buf + i))) {
i += sizeof(long);
zrun_len += sizeof(long);
}
/* go over the rest */
while (i < slen && old_buf[i] == new_buf[i]) {
zrun_len++;
i++;
}
}
/* buffer unchanged */
if (zrun_len == slen) {
return 0;
}
/* skip last zero run */
if (i == slen) {
return d;
}
d += uleb128_encode_small(dst + d, zrun_len);
zrun_len = 0;
nzrun_start = new_buf + i;
/* overflow */
if (d + 2 > dlen) {
return -1;
}
/* not aligned to sizeof(long) */
res = (slen - i) % sizeof(long);
while (res && old_buf[i] != new_buf[i]) {
nzrun_len++;
i++;
res--;
}
if (!res) {
/* truncation to 32-bit long okay */
long mask = 0x0101010101010101ULL;
while (i < slen) {
xor = *(long *)(old_buf + i) ^ *(long *)(new_buf + i);
if ((xor - mask) & ~xor & (mask << 7)) {
/* found the end of an nzrun within the current long */
while (old_buf[i] != new_buf[i]) {
nzrun_len++;
i++;
}
break;
} else {
i += sizeof(long);
nzrun_len += sizeof(long);
}
}
}
d += uleb128_encode_small(dst + d, nzrun_len);
/* overflow */
if (d + nzrun_len > dlen) {
return -1;
}
memcpy(dst + d, nzrun_start, nzrun_len);
d += nzrun_len;
nzrun_len = 0;
}
return d;
}
int xbzrle_decode_buffer(uint8_t *src, int slen, uint8_t *dst, int dlen)
{
int i = 0, d = 0;
int ret;
uint32_t count = 0;
while (i < slen) {
/* zrun */
if (slen - i < 2) {
return -1;
}
ret = uleb128_decode_small(src + i, &count);
if (ret < 0 || (i && !count)) {
return -1;
}
i += ret;
d += count;
/* overflow */
if (d > dlen) {
return -1;
}
/* nzrun */
if (slen - i < 2) {
return -1;
}
ret = uleb128_decode_small(src + i, &count);
if (ret < 0 || !count) {
return -1;
}
i += ret;
/* overflow */
if (d + count > dlen || i + count > slen) {
return -1;
}
memcpy(dst + d , src + i, count);
d += count;
i += count;
}
return d;
}
--
Eric Blake eblake@redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v14 13/13] Add XBZRLE statistics
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 13/13] Add XBZRLE statistics Orit Wasserman
@ 2012-07-04 1:35 ` Eric Blake
0 siblings, 0 replies; 33+ messages in thread
From: Eric Blake @ 2012-07-04 1:35 UTC (permalink / raw)
To: Orit Wasserman
Cc: peter.maydell, aliguori, quintela, stefanha, qemu-devel, mdroth,
blauwirbel, Petter Svard, Benoit Hudzia, avi, Aidan Shribman,
pbonzini, chegu_vinod
[-- Attachment #1: Type: text/plain, Size: 2886 bytes --]
On 07/03/2012 07:52 AM, Orit Wasserman wrote:
> 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>
>
> ##
> # @MigrationInfo
> @@ -282,13 +307,18 @@
> # status, only returned if status is 'active' and it is a block
> # migration
> #
> -# Since: 0.14.0
> +# @params: #optional a list describing all the migration capabilities state
s/@params/@capabilities/ to match the schema. also, since
'*capabilities' was added in an earlier patch, this portion of the
documentation should be rebased into that same patch.
> +++ b/qmp-commands.hx
> @@ -2093,18 +2093,28 @@ The main json-object contains the following:
> - "transferred": amount transferred (json-int)
> - "remaining": amount remaining (json-int)
> - "total": total (json-int)
> + - "duplicate": number of duplicated pages (json-int)
> + - "normal" : number of normal pages transferred (json-int)
> - "disk": only present if "status" is "active" and it is a block migration,
> it is a json-object with the following disk information (in bytes):
> - "transferred": amount transferred (json-int)
> - "remaining": amount remaining (json-int)
> - "total": total (json-int)
> +- "params": migration capabilites state
s/params/capabilities/, s/capabilites/capabilities/
> + - "xbzrle" : on/off (json-bool)
on/off is not a json-bool; furthermore, MigrationCapabilityInfo (patch
2/13) is documented as being { "capability":"xbzrle", "state":true }
rather than { "xbzrle":"on" }
> 1. Before the first migration
> -
> -> { "execute": "query-migrate" }
> -<- { "return": {} }
> +<- { "return": { "params" : { "xbzrle" : "off" } } }
which means this example does not match your code
>
> 2. Migration is done and has succeeded
>
> @@ -2122,6 +2132,7 @@ Examples:
> <- {
> "return":{
> "status":"active",
> + "params" : { "xbzrle" : "off" },
neither does this. Should be:
"capabilities": [ { "capability":"xbzrle", "state":false } ],
> @@ -2136,6 +2147,7 @@ Examples:
> <- {
> "return":{
> "status":"active",
> + "params" : { "xbzrle" : "off" },
and again
> "ram":{
> "total":1057024,
> "remaining":1053304,
> @@ -2149,6 +2161,28 @@ Examples:
> }
> }
>
> +5. Migration is being performed and XBZRLE is active:
> +
> +-> { "execute": "query-migrate" }
> +<- {
> + "return":{
> + "status":"active",
> + "params" : { "xbzrle" : "on" },
and again
--
Eric Blake eblake@redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v14 04/13] Add cache handling functions
2012-07-03 19:49 ` Eric Blake
@ 2012-07-04 7:04 ` Orit Wasserman
0 siblings, 0 replies; 33+ messages in thread
From: Orit Wasserman @ 2012-07-04 7:04 UTC (permalink / raw)
To: Eric Blake
Cc: peter.maydell, aliguori, quintela, stefanha, qemu-devel, mdroth,
Blue Swirl, Petter Svard, Benoit Hudzia, avi, Aidan Shribman,
pbonzini, chegu_vinod
On 07/03/2012 10:49 PM, Eric Blake wrote:
> On 07/03/2012 01:23 PM, Blue Swirl wrote:
>
>>> +
>>> +static inline int64_t round2pow2(int64_t value)
>
> round up or down?
>
>>> +{
>>> + while (!is_power_of_2(value)) {
>>> + value &= ~(1 << (ffs(value) - 1));
>>
>> ffs() only uses 'int', not int64_t. ffsl() is not universally available.
>>
>>> + }
>>> + return value;
>>> +}
>
> Not to mention that iterating one bit at a time is inefficient. We
> already gave you several more efficient solutions; my favorite being:
>
> static inline int64_t pow2floor(int64_t value)
> {
> if (!is_power_of_2(value)) {
> value = 0x8000000000000000ULL >> clz64(value);
> }
> return value;
> }
>
> since clz64() is already part of qemu sources.
>
I will fix it
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v14 07/13] Add debugging infrastructure
2012-07-03 19:25 ` Blue Swirl
@ 2012-07-04 7:19 ` Orit Wasserman
0 siblings, 0 replies; 33+ messages in thread
From: Orit Wasserman @ 2012-07-04 7:19 UTC (permalink / raw)
To: Blue Swirl
Cc: peter.maydell, aliguori, quintela, stefanha, qemu-devel, mdroth,
chegu_vinod, avi, pbonzini, eblake
On 07/03/2012 10:25 PM, Blue Swirl wrote:
> On Tue, Jul 3, 2012 at 1:52 PM, Orit Wasserman <owasserm@redhat.com> wrote:
>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>> ---
>> arch_init.c | 33 +++++++++++++++++++++++++++------
>> 1 files changed, 27 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch_init.c b/arch_init.c
>> index 9dafb6e..ee20c33 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -44,6 +44,14 @@
>> #include "exec-memory.h"
>> #include "hw/pcspk.h"
>>
>> +#ifdef DEBUG_ARCH_INIT
>> +#define DPRINTF(fmt, ...) \
>> + do { fprintf(stdout, "arch_init: " fmt, ## __VA_ARGS__); } while (0)
>> +#else
>> +#define DPRINTF(fmt, ...) \
>> + do { } while (0)
>> +#endif
>
> I think you missed my comments to the version that Juan sent, about
> using trace points. Also in Juan's version the %lds were changed to
> PRId64s which now are reverted, why?
I will base the next version on top of Juan's. This patch would be removed
Orit
>
>> +
>> #ifdef TARGET_SPARC
>> int graphic_width = 1024;
>> int graphic_height = 768;
>> @@ -380,6 +388,9 @@ int ram_save_live(QEMUFile *f, int stage, void *opaque)
>>
>> expected_time = ram_save_remaining() * TARGET_PAGE_SIZE / bwidth;
>>
>> + DPRINTF("ram_save_live: expected(%ld) <= max(%ld)?\n", expected_time,
>> + migrate_max_downtime());
>> +
>> return (stage == 2) && (expected_time <= migrate_max_downtime());
>> }
>>
>> @@ -416,8 +427,11 @@ static inline void *host_from_stream_offset(QEMUFile *f,
>> int ram_load(QEMUFile *f, void *opaque, int version_id)
>> {
>> ram_addr_t addr;
>> - int flags;
>> + int flags, ret = 0;
>> int error;
>> + static uint64_t seq_iter;
>> +
>> + seq_iter++;
>>
>> if (version_id < 4 || version_id > 4) {
>> return -EINVAL;
>> @@ -447,8 +461,10 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
>>
>> QLIST_FOREACH(block, &ram_list.blocks, next) {
>> if (!strncmp(id, block->idstr, sizeof(id))) {
>> - if (block->length != length)
>> - return -EINVAL;
>> + if (block->length != length) {
>> + ret = -EINVAL;
>> + goto done;
>> + }
>> break;
>> }
>> }
>> @@ -456,7 +472,8 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
>> if (!block) {
>> fprintf(stderr, "Unknown ramblock \"%s\", cannot "
>> "accept migration\n", id);
>> - return -EINVAL;
>> + ret = -EINVAL;
>> + goto done;
>> }
>>
>> total_ram_bytes -= length;
>> @@ -490,11 +507,15 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
>> }
>> error = qemu_file_get_error(f);
>> if (error) {
>> - return error;
>> + ret = error;
>> + goto done;
>> }
>> } while (!(flags & RAM_SAVE_FLAG_EOS));
>>
>> - return 0;
>> +done:
>> + DPRINTF("Completed load of VM with exit code %d seq iteration %ld\n",
>> + ret, seq_iter);
>> + return ret;
>> }
>>
>> #ifdef HAS_AUDIO
>> --
>> 1.7.7.6
>>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v14 09/13] Add migration_end function
2012-07-03 20:38 ` Eric Blake
@ 2012-07-04 7:19 ` Orit Wasserman
0 siblings, 0 replies; 33+ messages in thread
From: Orit Wasserman @ 2012-07-04 7:19 UTC (permalink / raw)
To: Eric Blake
Cc: peter.maydell, aliguori, quintela, stefanha, qemu-devel, mdroth,
blauwirbel, avi, pbonzini, chegu_vinod
On 07/03/2012 11:38 PM, Eric Blake wrote:
> On 07/03/2012 07:52 AM, Orit Wasserman wrote:
>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>> ---
>> arch_init.c | 7 ++++++-
>> 1 files changed, 6 insertions(+), 1 deletions(-)
>
> You failed to account for my v13 review that you missed a hunk in this
> patch (you mistakenly delayed it to 11/13). Furthermore, hasn't this
> patch (and several others) were already picked up by Juan; you should
> rebase on top of his pull request.
>
Agreed.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v14 10/13] Add xbzrle_encode_buffer and xbzrle_decode_buffer functions
2012-07-03 21:32 ` Eric Blake
2012-07-03 21:39 ` Eric Blake
@ 2012-07-04 7:24 ` Orit Wasserman
2012-07-04 11:36 ` Eric Blake
1 sibling, 1 reply; 33+ messages in thread
From: Orit Wasserman @ 2012-07-04 7:24 UTC (permalink / raw)
To: Eric Blake
Cc: peter.maydell, aliguori, quintela, stefanha, qemu-devel, mdroth,
blauwirbel, Petter Svard, Benoit Hudzia, avi, Aidan Shribman,
pbonzini, chegu_vinod
On 07/04/2012 12:32 AM, Eric Blake wrote:
> On 07/03/2012 07:52 AM, Orit Wasserman wrote:
>> 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>
>
>> +int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t *new_buf, int slen,
>> + uint8_t *dst, int dlen)
>> +{
>> + uint32_t zrun_len = 0, nzrun_len = 0;
>> + int d = 0, i = 0, start;
>> + long res, xor;
>> + uint8_t *nzrun_start = NULL;
>> +
>> + g_assert(!((uintptr_t)old_buf & (sizeof(long) - 1)) &&
>> + !((uintptr_t)new_buf & (sizeof(long) - 1)) &&
>> + !(slen & (sizeof(long) - 1)));
>
> I know I suggested this in v13, so now I'm bike-shedding my own review,
> but I think it might be a bit more legible as:
>
> g_assert(!(((uintptr_t)old_buf) | ((uintptr_t)new_buf) | slen) &
> (sizeof(long) - 1)));
>
>> +
>> + while (i < slen) {
>> + /* overflow */
>> + if (d + 2 > dlen) {
>> + return -1;
>> + }
>> +
>> + /* not aligned to sizeof(long) */
>> + res = (slen - i) % sizeof(long);
>> + if (res) {
>> + start = i;
>> + while (!(old_buf[i] ^ new_buf[i]) && (i - start) <= res) {
>> + zrun_len++;
>> + i++;
>> + }
>> + }
>
> I'm not sure why you need 'start'; this same block will do the same
> amount of computation with less typing and register pressure:
>
> res = (slen - i) % sizeof(long);
> while (res && old_buf[i] == new_buf[i]) {
> zrun_len++;
> i++;
> res--;
> }
>
>> +
>> + if (zrun_len == res) {
>
> If you use my above rewrite, then this condition changes to:
>
> if (!res) {
>
>> + while (i <= (slen - sizeof(long)) &&
>
> I'd shorten this to:
>
> while (i < slen &&
>
>> + (*(long *)(old_buf + i)) == (*(long *)(new_buf + i))) {
>> + i += sizeof(long);
>> + zrun_len += sizeof(long);
>> + }
>> +
>> + /* go over the rest */
>> + while (old_buf[i] == new_buf[i] && ++i <= slen) {
>
> Buffer overrun: if the page ends on a zrun, then at this point, i ==
> slen, and old_buf[i] is now pointing one past the end of the array. You
> need to swap the conditions, and only increment 'i' inside the {}, as in:
>
> while (i < slen && old_buf[i] == new_buf[i]) {
> i++;
>
>> + zrun_len++;
>> + }
>> + }
>> +
>> + /* buffer unchanged */
>> + if (zrun_len == slen) {
>> + return 0;
>> + }
>> +
>> + /* skip last zero run */
>> + if (i == slen + 1) {
>
> Buffer overrun. The loop should terminate at 'i == slen', not at 'i ==
> slen + 1', since old_buf[slen - 1] is the last addressable byte.
>
>> + return d;
>> + }
>> +
>> + d += uleb128_encode_small(dst + d, zrun_len);
>> +
>> + /* no nzrun */
>> + if (i == slen) {
>> + return d;
>> + }
>
> This is the correct code but repeats the intent of what you were trying
> just before the uleb128_encode_small().
>
>> +
>> + zrun_len = 0;
>> + nzrun_start = new_buf + i;
>
> Missing an overflow check here. If there are fewer than 2 bytes before
> we hit dlen, then there is no way that we can fit in an nzrun, so we
> might as well quit here.
>
>> +
>> + /* not aligned to sizeof(long) */
>> + res = (slen - i) % sizeof(long);
>> + if (res) {
>> + start = i;
>> + while (old_buf[i] != new_buf[i] && i - start < res) {
>> + i++;
>> + nzrun_len++;
>> + }
>> + }
>
> Again, you can exploit the fact that slen is aligned, and shorten to:
>
> res = (slen - i) % sizeof(long);
> while (res && old_bf[i] != new_buf[i]) {
> i++;
> nzrun_len++;
> res--;
> }
>
>> +
>> + if (nzrun_len == res) {
>
> and this would change to:
>
> if (!res) {
>
>> + /* truncation to 32-bit long okay */
>> + long mask = 0x0101010101010101ULL;
>> + xor = *(long *)(old_buf + i) ^ *(long *)(new_buf + i);
>> + printf("cont\n");
>
> Stray debugging comment.
>
>> + while (i <= (slen - sizeof(long))) {
>
> shorter to write as:
>
> while (i < slen) {
>
>> + if ((xor - mask) & ~xor & (mask << 7)) {
>> + /* found the end of an nzrun within the current long */
>> + while (old_buf[i] != new_buf[i] && ++i <= slen) {
>
> No need to compare against slen here, since the outer while guarantees
> that. You can simplify to:
>
> while (old_buf[i] != new_buf[i]) {
> i++;
>
>> + nzrun_len++;
>> + }
>> + break;
>> + } else {
>> + i += sizeof(long);
>> + nzrun_len += sizeof(long);
>> + xor = *(long *)(old_buf + i) ^ *(long *)(new_buf + i);
>> + }
>> + }
>> +
>
> At this point, you are guaranteed to have found the end of the nzrun
> (either you found two equal bytes, or you hit slen), which means...
>
>> + while (old_buf[i] != new_buf[i] && ++i <= slen) {
>> + nzrun_len++;
>> + }
>
> this while loop is dead code. Furthermore, this while loop is a
> potential out-of-bounds-read (remember, old_buf[slen] is not valid
> memory), so nuking it is the way to go.
>
>> + }
>> +
>> + /* overflow */
>> + if (d + nzrun_len + 2 > dlen) {
>> + return -1;
>
> There is a corner case where this reports overflow even when there was
> none - that is when the old_buf ends in an nzrun of 0x7f bytes or less,
> so the encoding occupies only 1 byte, and the encoded version would
> still fit in dlen. I think you want to defer your overflow checking
> until after...
>
>> + }
>> +
>> + d += uleb128_encode_small(dst + d, nzrun_len);
>
> ...you know the encoded size. But that only works if you made sure
> earlier that there was room to encode nzrun_len (up to 2 bytes) in the
> first place.
>
>> + memcpy(dst + d, nzrun_start, nzrun_len);
>> + d += nzrun_len;
>> + nzrun_len = 0;
>> + }
>> +
>> + return d;
>> +}
>> +
>
> Do you have any unit tests that pass in various inputs and compare
> against expected outputs?
>
>> +int xbzrle_decode_buffer(uint8_t *src, int slen, uint8_t *dst, int dlen)
>> +{
>> + int i = 0, d = 0;
>> + int ret;
>> + uint32_t count = 0;
>> +
>> + while (i < slen) {
>> +
>> + /* zrun */
>> + if ((slen - i) < 2 && *(src + i) & 0x80) {
>> + return -1;
>> + }
>
> You can simplify this - the protocol demands an nzrun after every zrun,
> which means at this point in the decode, there must be at least 3 bytes
> to be valid. It's simpler to write:
>
> if (slen - i < 2) {
> return -1;
> }
>
>> +
>> + ret = uleb128_decode_small(src + i, &count);
>> + if (ret < 0) {
>> + return -1;
>> + }
>
> I'd also add a check that we only ever get a zero-length zrun at the
> start of a buffer (anywhere else, a zero-length zrun is invalid); as in:
>
> if (!count && i) {
> return -1;
> }
>
>> + i += ret;
>> + d += count;
>> +
>> + /* overflow */
>> + if (d > dlen) {
>> + return -1;
>> + }
>> +
>> + /* completed decoding */
>
> No, if we got here, then we were handed an invalid stream. Remember,
> the protocol says that every zrun must be followed by an nzrun.
>
>> + if (i == slen - 1) {
>> + return d;
>> + }
>> +
>> + /* nzrun */
>> + if ((slen - i) < 2 && *(src + i) & 0x80) {
>> + return -1;
>> + }
>
> Again, a valid nzrun is at least 2 bytes, so no need to worry about the
> contents of *src, it is sufficient to check:
>
> if (slen - i < 2) {
> return -1;
> }
>
>> + ret = uleb128_decode_small(src + i, &count);
>> + if (ret < 0) {
>
> An nzrun should be a non-zero value; I'd write this as (ret <= 0) to
> rule out an attempt to pass a zero-length nzrun.
decode can only return -1 (invalid) or the decoded len 1 or 2
so maybe you meant I should check that count is bigger than zero?
Orit
>
>> + return -1;
>> + }
>> + i += ret;
>> +
>> + /* overflow */
>> + if (d + count > dlen) {
>> + return -1;
>> + }
>> +
>> + memcpy(dst + d , src + i, count);
>> + d += count;
>> + i += count;
>> + }
>> +
>> + return d;
>> +}
>>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v14 03/13] Add XBZRLE documentation
2012-07-03 19:45 ` Eric Blake
@ 2012-07-04 8:29 ` Orit Wasserman
0 siblings, 0 replies; 33+ messages in thread
From: Orit Wasserman @ 2012-07-04 8:29 UTC (permalink / raw)
To: Eric Blake
Cc: peter.maydell, aliguori, quintela, stefanha, qemu-devel, mdroth,
blauwirbel, avi, pbonzini, chegu_vinod
On 07/03/2012 10:45 PM, Eric Blake wrote:
> On 07/03/2012 07:52 AM, Orit Wasserman wrote:
>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>> ---
>> docs/xbzrle.txt | 133 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 133 insertions(+), 0 deletions(-)
>> create mode 100644 docs/xbzrle.txt
>
>> +
>> +Example
>> +old buffer:
>> +1000 zeros
>> +05 06 07 08 09 0a 0b 0c 0d 0e 0f 10 11 12 13 00 00 6b 00 6d
>> +3072 zeros
>
> That's only 4092 bytes, but a page is typically 4096. I think you meant
> 3076 trailing zeros.
>
>> +
>> +new buffer:
>> +1000 zeros
>> +01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 00 00 67 00 69
>> +3702 zeros
>
> That's 4722 bytes; looks like a transposition error, and given the above
> comment, I still think you want 3076 trailing zeros.
>
>> +
>> +encoded buffer:
>> +
>> +encoded length 29
>> +e8 07 18 00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 00 00 67 00 69 00 00
>> +00 80 18
>
> Looks better, but still not quite accurate. Per the spec, you always
> end on a nzrun, which means you should not have transmitted the last two
> bytes (80 18 => 3072, which is the length of your trailing zrun). I
> also find it weak that your example never shows an unchanged byte on
> anything other than 00; having at least one non-zero unchanged byte may
> make it more obvious in the encoded example that it is the xor
> difference that determines a zrun vs. nzrun, and not the old or new
> buffer contents.
>
> Also, reading that encoding says you have 1000 zrun, then 24 bytes of
> nzrun starting with a leading 00, but based on your old and new buffer,
> the only way to have a leading 00 in your nzrun is if you count a zrun
> of 999 bytes. Are you sure you didn't test with a buffer of 1001
> leading zeros, then 20 bytes of interest, then 3073 trailing zeros?
yes you are right 1001 zeros ...
I fix it .
>
> Given your old and new buffer as-is, and my assumption that you are
> compressing a page of exactly 4096 bytes (3076 trailing zeros after your
> 20-byte window of interesting data), I see at least the following four
> valid encodings, listed in increasing difficulty of computation:
>
> 8-byte long at a time; all chunks that differ in at least one byte are
> treated as at least 8 bytes of nzrun, transmit 27 bytes
> e8 07 18 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 00 00 67 00 69 00
> 00 00 00
>
> 4 bytes at a time; all chunks that differ in at least one byte are
> treated as at least 4 bytes of nzrun, transmit 23 bytes
> e8 07 14 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 00 00 67 00 69
>
> list every single zrun (except trailing), even if only one byte long,
> transmit 24 bytes
> e8 07 0f 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 02 01 67 01 01 69
>
> avoid a 1-byte zrun, transmit 23 bytes
> e8 07 0f 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 02 03 67 00 69
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v14 10/13] Add xbzrle_encode_buffer and xbzrle_decode_buffer functions
2012-07-04 7:24 ` Orit Wasserman
@ 2012-07-04 11:36 ` Eric Blake
0 siblings, 0 replies; 33+ messages in thread
From: Eric Blake @ 2012-07-04 11:36 UTC (permalink / raw)
To: Orit Wasserman
Cc: peter.maydell, aliguori, quintela, stefanha, qemu-devel, mdroth,
blauwirbel, Petter Svard, Benoit Hudzia, avi, Aidan Shribman,
pbonzini, chegu_vinod
[-- Attachment #1: Type: text/plain, Size: 799 bytes --]
On 07/04/2012 01:24 AM, Orit Wasserman wrote:
>>
>>> + ret = uleb128_decode_small(src + i, &count);
>>> + if (ret < 0) {
>>
>> An nzrun should be a non-zero value; I'd write this as (ret <= 0) to
>> rule out an attempt to pass a zero-length nzrun.
> decode can only return -1 (invalid) or the decoded len 1 or 2
> so maybe you meant I should check that count is bigger than zero?
Yes, and I even corrected myself:
https://lists.gnu.org/archive/html/qemu-devel/2012-07/msg00368.html
as well as incorporated my comments (which probably picked up a couple
other fixes in the process):
https://lists.gnu.org/archive/html/qemu-devel/2012-07/msg00369.html
--
Eric Blake eblake@redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v14 10/13] Add xbzrle_encode_buffer and xbzrle_decode_buffer functions
2012-07-04 0:20 ` Eric Blake
@ 2012-07-04 12:51 ` Orit Wasserman
0 siblings, 0 replies; 33+ messages in thread
From: Orit Wasserman @ 2012-07-04 12:51 UTC (permalink / raw)
To: Eric Blake
Cc: peter.maydell, aliguori, quintela, stefanha, qemu-devel, mdroth,
blauwirbel, Petter Svard, Benoit Hudzia, avi, Aidan Shribman,
pbonzini, chegu_vinod
On 07/04/2012 03:20 AM, Eric Blake wrote:
> On 07/03/2012 03:39 PM, Eric Blake wrote:
>> On 07/03/2012 03:32 PM, Eric Blake wrote:
>>
>>>> + ret = uleb128_decode_small(src + i, &count);
>>>> + if (ret < 0) {
>>>
>>> An nzrun should be a non-zero value; I'd write this as (ret <= 0) to
>>> rule out an attempt to pass a zero-length nzrun.
>>
>> Correcting myself,
>>
>> if (ret < 0 || !count) {
>>
>> At this point, I think I will just bite the bullet and post a version of
>> this code that incorporates my review.
>
> Something like this (lightly tested):
>
> /*
> page = zrun nzrun
> | zrun nzrun page
>
> zrun = length
>
> nzrun = length byte...
>
> length = uleb128 encoded integer
> */
> int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t *new_buf, int slen,
> uint8_t *dst, int dlen)
> {
> uint32_t zrun_len = 0, nzrun_len = 0;
> int d = 0, i = 0;
> long res, xor;
> uint8_t *nzrun_start = NULL;
>
> g_assert(!(((uintptr_t)old_buf | (uintptr_t)new_buf | slen) %
> sizeof(long)));
>
> while (i < slen) {
> /* overflow */
> if (d + 2 > dlen) {
> return -1;
> }
>
> /* not aligned to sizeof(long) */
> res = (slen - i) % sizeof(long);
> while (res && old_buf[i] == new_buf[i]) {
> zrun_len++;
> i++;
> res--;
> }
>
> if (!res) {
> while (i < slen &&
> (*(long *)(old_buf + i)) == (*(long *)(new_buf + i))) {
> i += sizeof(long);
> zrun_len += sizeof(long);
> }
>
> /* go over the rest */
> while (i < slen && old_buf[i] == new_buf[i]) {
> zrun_len++;
> i++;
> }
> }
>
> /* buffer unchanged */
> if (zrun_len == slen) {
> return 0;
> }
>
> /* skip last zero run */
> if (i == slen) {
> return d;
> }
>
> d += uleb128_encode_small(dst + d, zrun_len);
>
> zrun_len = 0;
> nzrun_start = new_buf + i;
>
> /* overflow */
> if (d + 2 > dlen) {
> return -1;
> }
>
> /* not aligned to sizeof(long) */
> res = (slen - i) % sizeof(long);
> while (res && old_buf[i] != new_buf[i]) {
> nzrun_len++;
> i++;
> res--;
> }
>
> if (!res) {
> /* truncation to 32-bit long okay */
> long mask = 0x0101010101010101ULL;
> while (i < slen) {
> xor = *(long *)(old_buf + i) ^ *(long *)(new_buf + i);
> if ((xor - mask) & ~xor & (mask << 7)) {
> /* found the end of an nzrun within the current long */
> while (old_buf[i] != new_buf[i]) {
> nzrun_len++;
> i++;
> }
> break;
> } else {
> i += sizeof(long);
> nzrun_len += sizeof(long);
> }
> }
> }
>
> d += uleb128_encode_small(dst + d, nzrun_len);
>
> /* overflow */
> if (d + nzrun_len > dlen) {
> return -1;
> }
>
> memcpy(dst + d, nzrun_start, nzrun_len);
> d += nzrun_len;
> nzrun_len = 0;
> }
>
> return d;
> }
>
> int xbzrle_decode_buffer(uint8_t *src, int slen, uint8_t *dst, int dlen)
> {
> int i = 0, d = 0;
> int ret;
> uint32_t count = 0;
>
> while (i < slen) {
>
> /* zrun */
> if (slen - i < 2) {
> return -1;
> }
>
> ret = uleb128_decode_small(src + i, &count);
> if (ret < 0 || (i && !count)) {
> return -1;
> }
> i += ret;
> d += count;
>
> /* overflow */
> if (d > dlen) {
> return -1;
> }
>
> /* nzrun */
> if (slen - i < 2) {
> return -1;
> }
> ret = uleb128_decode_small(src + i, &count);
> if (ret < 0 || !count) {
> return -1;
> }
> i += ret;
>
> /* overflow */
> if (d + count > dlen || i + count > slen) {
> return -1;
> }
>
> memcpy(dst + d , src + i, count);
> d += count;
> i += count;
> }
>
> return d;
> }
>
thanks
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v14 02/13] Add migration capabilities
2012-07-03 18:36 ` Eric Blake
@ 2012-07-05 10:09 ` Orit Wasserman
0 siblings, 0 replies; 33+ messages in thread
From: Orit Wasserman @ 2012-07-05 10:09 UTC (permalink / raw)
To: Eric Blake
Cc: peter.maydell, aliguori, quintela, stefanha, mdroth, qemu-devel,
blauwirbel, avi, pbonzini, chegu_vinod
On 07/03/2012 09:36 PM, Eric Blake wrote:
> On 07/03/2012 07:52 AM, Orit Wasserman wrote:
>> Add migration capabilities that can be queried by the management.
>> The management can query the source QEMU and the destination QEMU in order to
>> verify both support some migration capability (currently only XBZRLE).
>> The management can enable a capability for the next migration by using
>> migrate_set_parameter command.
>
> Couple of comment nits below. Also, if you don't mind a bit of
> bike-shedding...
>
> [roughly translated - feel free to ignore the bulk of this email; the
> patch as-is works, if you don't think my alternate design makes sense to
> implement]
>
>> +++ b/hmp-commands.hx
>> @@ -861,6 +861,20 @@ Set maximum tolerated downtime (in seconds) for migration.
>> ETEXI
>>
>> {
>> + .name = "migrate_set_parameter",
>> + .args_type = "capability:s,state:b",
>> + .params = "",
>> + .help = "Enable the usage of a capability for migration",
>> + .mhandler.cmd = hmp_migrate_set_parameter,
>
> This requires the 'state' parameter to be passed. But wouldn't it be
> easier to default things to state=true if no parameter was present,
> since the most common usage of this command will be to enable, rather
> than disable, a migration parameter?
After thinking about it , I prefer to leave to command as it is.
For me it feels strange that the enable will be different from the disable.
Orit
>
>> - /* no migration has happened ever */
>> + /* no migration has happened ever show enabled capabilities */
>
> Missing some punctuation, so this was hard to read. Maybe:
>
> /* no migration has ever happened; show enabled capabilities */
>
>> +++ b/qapi-schema.json
>
>> +##
>> +# @MigrationCapabilityInfo
>> +#
>> +# Migration capability information
>> +#
>> +# @capability: capability enum
>> +#
>> +# Since: 1.2
>> +##
>> +{ 'type': 'MigrationCapabilityInfo',
>> + 'data': { 'capability' : 'MigrationCapability', 'state' : 'bool' } }
>
> Back to the bike-shedding - if you would mark this '*state':'bool', then
> the enabled parameter becomes optional on input (it should still always
> be present on query-migration-capabilities output, though).
>
>> +migrate_set_parameters
>> +-------
>> +
>> +Enable/Disable migration capabilities
>> +
>> +- "xbzrle": xbzrle support
>> +
>> +Arguments:
>> +
>> +Example:
>> +
>> +-> { "execute": "migrate_set_parameters" , "arguments":
>> + { "parameters": { "capability": "xbzrle", "state": true } ] } }
>
> Missing a '[' after "parameters":
>
^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2012-07-05 10:09 UTC | newest]
Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-03 13:52 [Qemu-devel] [PATCH v14 00/13] XBZRLE delta for live migration of large memory app Orit Wasserman
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 01/13] Add MigrationParams structure Orit Wasserman
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 02/13] Add migration capabilities Orit Wasserman
2012-07-03 18:36 ` Eric Blake
2012-07-05 10:09 ` Orit Wasserman
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 03/13] Add XBZRLE documentation Orit Wasserman
2012-07-03 19:45 ` Eric Blake
2012-07-04 8:29 ` Orit Wasserman
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 04/13] Add cache handling functions Orit Wasserman
2012-07-03 19:23 ` Blue Swirl
2012-07-03 19:49 ` Eric Blake
2012-07-04 7:04 ` Orit Wasserman
2012-07-03 20:24 ` Eric Blake
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 05/13] Add uleb encoding/decoding functions Orit Wasserman
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 06/13] Add save_block_hdr function Orit Wasserman
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 07/13] Add debugging infrastructure Orit Wasserman
2012-07-03 19:25 ` Blue Swirl
2012-07-04 7:19 ` Orit Wasserman
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 08/13] Change ram_save_block to return -1 if there are no more changes Orit Wasserman
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 09/13] Add migration_end function Orit Wasserman
2012-07-03 20:38 ` Eric Blake
2012-07-04 7:19 ` Orit Wasserman
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 10/13] Add xbzrle_encode_buffer and xbzrle_decode_buffer functions Orit Wasserman
2012-07-03 21:32 ` Eric Blake
2012-07-03 21:39 ` Eric Blake
2012-07-04 0:20 ` Eric Blake
2012-07-04 12:51 ` Orit Wasserman
2012-07-04 7:24 ` Orit Wasserman
2012-07-04 11:36 ` Eric Blake
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 11/13] Add XBZRLE to ram_save_block and ram_save_live Orit Wasserman
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 12/13] Add set_cachesize command Orit Wasserman
2012-07-03 13:52 ` [Qemu-devel] [PATCH v14 13/13] Add XBZRLE statistics Orit Wasserman
2012-07-04 1:35 ` Eric Blake
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.