| Message ID | 1343227834-5400-9-git-send-email-owasserm@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/25/2012 08:50 AM, Orit Wasserman wrote: > In the outgoing migration check to see if the page is cached and > changed than send compressed page by using save_xbrle_page function. s/changed than/changed, then/ > In the incoming migration check to see if RAM_SAVE_FLAG_XBZRLE 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> > +/* struct contains XBZRLE cache and a static page > + used by the compression */ > +static struct { > + /* buffer used for XBZRLE encoding */ > + uint8_t *encoded_buf; > + /* buffer for storing page content */ > + uint8_t *current_buf; > + /* buffer used for XBZRLE decoding */ > + uint8_t *decoded_buf; > + /* Cache for XBZRLE */ > + PageCache *cache; > +} XBZRLE = { > + .encoded_buf = NULL, > + .current_buf = NULL, > + .decoded_buf = NULL, > + .cache = NULL, > +}; Explicit zero-initialization of a static object is pointless; C already guarantees that this will be the case without an initializer, due to the static lifetime. > +#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 encoded_len = 0, bytes_sent = -1; > + XBZRLEHeader hdr = { > + .xh_len = 0, > + .xh_flags = 0, > + }; [In contrast, this explicit zero-initialization of an automatic variable is essential.] > + > + /* 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 */ > + memcpy(prev_cached_page, XBZRLE.current_buf, TARGET_PAGE_SIZE); Comment is out of date - a memcpy is not decoding onto the cache, but overwriting the entire cached page. > + > + 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); This looks like an off by one. sizeof(hdr) is 4 (2 bytes for xh_len, 1 byte for xh_flags, then padded out to 2-byte multiple due to uint16_t member); but you really only sent 3 bytes (1 for xh_flags, 2 for xh_len), not 4. > @@ -321,7 +423,18 @@ static int ram_save_setup(QEMUFile *f, void *opaque) > last_offset = 0; > sort_ram_list(); > > - /* Make sure all dirty bits are set */ Why are you losing this comment? It is still appropriate for the code in context after your insertion. > @@ -436,6 +549,49 @@ static int ram_save_complete(QEMUFile *f, void *opaque) > return 0; > } > > +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"); Shouldn't you also validate that no other bits in xh_flags are set, so as to allow future versions of the protocol to define additional bits if we come up with further compression methods and gracefully be detected when the receiving end does not understand those bits? That is, this should be: if (hdr.xh_flags != ENCODING_FLAG_XBZRLE) { > + 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); Technically, this fprintf is unreachable; xbzrle_decode_buffer should return -1 instead of a value larger than its incoming dlen. You could abort() here to indicate a programming error. > @@ -549,6 +705,19 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) > } > > qemu_get_buffer(f, host, TARGET_PAGE_SIZE); > + } else if (flags & RAM_SAVE_FLAG_XBZRLE) { > + if (!migrate_use_xbzrle()) { > + return -EINVAL; > + } > + void *host = host_from_stream_offset(f, addr, flags); > + if (!host) { > + return -EINVAL; > + } > + > + if (load_xbzrle(f, addr, host) < 0) { > + ret = -EINVAL; > + goto done; Is there any issue by returning early instead of using 'goto done' in all three error locations?
On 07/27/2012 01:20 AM, Eric Blake wrote: > On 07/25/2012 08:50 AM, Orit Wasserman wrote: >> In the outgoing migration check to see if the page is cached and >> changed than send compressed page by using save_xbrle_page function. > > s/changed than/changed, then/ > >> In the incoming migration check to see if RAM_SAVE_FLAG_XBZRLE 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> > >> +/* struct contains XBZRLE cache and a static page >> + used by the compression */ >> +static struct { >> + /* buffer used for XBZRLE encoding */ >> + uint8_t *encoded_buf; >> + /* buffer for storing page content */ >> + uint8_t *current_buf; >> + /* buffer used for XBZRLE decoding */ >> + uint8_t *decoded_buf; >> + /* Cache for XBZRLE */ >> + PageCache *cache; >> +} XBZRLE = { >> + .encoded_buf = NULL, >> + .current_buf = NULL, >> + .decoded_buf = NULL, >> + .cache = NULL, >> +}; > > Explicit zero-initialization of a static object is pointless; C already > guarantees that this will be the case without an initializer, due to the > static lifetime. You are right but as it harmless I will keep it. > >> +#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 encoded_len = 0, bytes_sent = -1; >> + XBZRLEHeader hdr = { >> + .xh_len = 0, >> + .xh_flags = 0, >> + }; > > [In contrast, this explicit zero-initialization of an automatic variable > is essential.] > >> + >> + /* 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 */ >> + memcpy(prev_cached_page, XBZRLE.current_buf, TARGET_PAGE_SIZE); > > Comment is out of date - a memcpy is not decoding onto the cache, but > overwriting the entire cached page. > will be updated. >> + >> + 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); > > This looks like an off by one. sizeof(hdr) is 4 (2 bytes for xh_len, 1 > byte for xh_flags, then padded out to 2-byte multiple due to uint16_t > member); but you really only sent 3 bytes (1 for xh_flags, 2 for > xh_len), not 4. correct, I was think of removing the header completely anyway. > >> @@ -321,7 +423,18 @@ static int ram_save_setup(QEMUFile *f, void *opaque) >> last_offset = 0; >> sort_ram_list(); >> >> - /* Make sure all dirty bits are set */ > > Why are you losing this comment? It is still appropriate for the code > in context after your insertion. looks like bad merge I will fix it. > >> @@ -436,6 +549,49 @@ static int ram_save_complete(QEMUFile *f, void *opaque) >> return 0; >> } >> >> +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"); > > Shouldn't you also validate that no other bits in xh_flags are set, so > as to allow future versions of the protocol to define additional bits if > we come up with further compression methods and gracefully be detected > when the receiving end does not understand those bits? That is, this > should be: > > if (hdr.xh_flags != ENCODING_FLAG_XBZRLE) { > ok >> + 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); > > Technically, this fprintf is unreachable; xbzrle_decode_buffer should > return -1 instead of a value larger than its incoming dlen. You could > abort() here to indicate a programming error. ok > >> @@ -549,6 +705,19 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) >> } >> >> qemu_get_buffer(f, host, TARGET_PAGE_SIZE); >> + } else if (flags & RAM_SAVE_FLAG_XBZRLE) { >> + if (!migrate_use_xbzrle()) { >> + return -EINVAL; >> + } >> + void *host = host_from_stream_offset(f, addr, flags); >> + if (!host) { >> + return -EINVAL; >> + } >> + >> + if (load_xbzrle(f, addr, host) < 0) { >> + ret = -EINVAL; >> + goto done; > > Is there any issue by returning early instead of using 'goto done' in > all three error locations? > I think that it is written in this way for tracing ... Thanks, Orit
diff --git a/arch_init.c b/arch_init.c index 78cdf50..3998edc 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,30 @@ 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 for storing page content */ + uint8_t *current_buf; + /* buffer used for XBZRLE decoding */ + uint8_t *decoded_buf; + /* Cache for XBZRLE */ + PageCache *cache; +} XBZRLE = { + .encoded_buf = NULL, + .current_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,6 +207,61 @@ 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 encoded_len = 0, bytes_sent = -1; + XBZRLEHeader hdr = { + .xh_len = 0, + .xh_flags = 0, + }; + uint8_t *prev_cached_page; + + if (!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); + + /* save current buffer into memory */ + memcpy(XBZRLE.current_buf, current_data, TARGET_PAGE_SIZE); + + /* XBZRLE encoding (if there is no overflow) */ + encoded_len = xbzrle_encode_buffer(prev_cached_page, XBZRLE.current_buf, + TARGET_PAGE_SIZE, XBZRLE.encoded_buf, + TARGET_PAGE_SIZE); + if (encoded_len == 0) { + DPRINTF("Skipping unmodified page\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 */ + memcpy(prev_cached_page, XBZRLE.current_buf, TARGET_PAGE_SIZE); + + 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; @@ -198,6 +279,7 @@ static int ram_save_block(QEMUFile *f) 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); @@ -218,13 +300,24 @@ 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()) { + current_addr = block->offset + offset; + bytes_sent = save_xbzrle_page(f, p, current_addr, block, + offset, cont); + p = get_cached_data(XBZRLE.cache, current_addr); + } + + /* either we didn't send yet (we may have had 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, continue to the next */ + if (bytes_sent != 0) { + break; + } } offset += TARGET_PAGE_SIZE; @@ -302,6 +395,15 @@ 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); + g_free(XBZRLE.current_buf); + g_free(XBZRLE.decoded_buf); + XBZRLE.cache = NULL; + } } static void ram_migration_cancel(void *opaque) @@ -321,7 +423,18 @@ static int ram_save_setup(QEMUFile *f, void *opaque) last_offset = 0; sort_ram_list(); - /* Make sure all dirty bits are set */ + 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); + XBZRLE.current_buf = g_malloc(TARGET_PAGE_SIZE); + } + QLIST_FOREACH(block, &ram_list.blocks, next) { for (addr = 0; addr < block->length; addr += TARGET_PAGE_SIZE) { if (!memory_region_get_dirty(block->mr, addr, TARGET_PAGE_SIZE, @@ -436,6 +549,49 @@ static int ram_save_complete(QEMUFile *f, void *opaque) return 0; } +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) @@ -549,6 +705,19 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) } qemu_get_buffer(f, host, TARGET_PAGE_SIZE); + } else if (flags & RAM_SAVE_FLAG_XBZRLE) { + if (!migrate_use_xbzrle()) { + return -EINVAL; + } + 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 e844290..167398a 100644 --- a/migration.c +++ b/migration.c @@ -43,6 +43,9 @@ enum { #define MAX_THROTTLE (32 << 20) /* Migration speed throttling */ +/* Migration XBZRLE default cache size */ +#define DEFAULT_MIGRATE_CACHE_SIZE (64 * 1024 * 1024) + static NotifierList migration_state_notifiers = NOTIFIER_LIST_INITIALIZER(migration_state_notifiers); @@ -55,6 +58,7 @@ 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; @@ -422,6 +426,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)); @@ -431,6 +436,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; s->total_time = qemu_get_clock_ms(rt_clock); @@ -529,3 +535,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 743c366..cdf6787 100644 --- a/migration.h +++ b/migration.h @@ -41,6 +41,7 @@ struct MigrationState MigrationParams params; int64_t total_time; bool enabled_capabilities[MIGRATION_CAPABILITY_MAX]; + int64_t xbzrle_cache_size; }; void process_incoming_migration(QEMUFile *f); @@ -104,4 +105,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