Orit Wasserman <owass...@redhat.com> wrote:
> @@ -104,6 +104,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
missing space for alignment?
>
> #ifdef __ALTIVEC__
> #include <altivec.h>
> @@ -165,6 +166,15 @@ static unsigned long cache_get_cache_pos(ram_addr_t
> address);
> static CacheItem *cache_item_get(unsigned long pos, int item);
> static void cache_resize(int64_t new_size);
>
> +/* XBZRLE (Xor Based Zero Length Encoding */
> +typedef struct __attribute__((packed)) XBZRLEHeader {
> + uint8_t xh_flags;
> + uint16_t xh_len;
> + uint32_t xh_cksum;
> +} XBZRLEHeader;
We shouldn't use packed here. Explanation later.
And if we are worried about space, it is much better ordering to do it
by size:
typedef struct {
uint32_t xh_cksum;
uint16_t xh_len;
uint8_t xh_flags;
} XBZRLEHeader;
Once here, are we using the xh_cksum at all?
> +static uint8 *prev_cache_page;
Move this also to the bucket struct?
> @@ -359,19 +374,74 @@ 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 cache_location = -1, slot = -1, encoded_len = 0, bytes_sent = 0;
> + XBZRLEHeader hdr = {0};
> + CacheItem *it;
> + uint8_t *encoded_buf = NULL;
> +
> + /* get location */
> + slot = cache_is_cached(current_addr);
> + if (slot == -1) {
> + cache_insert(current_addr, current_data, 0);
> + goto done;
> + }
> + cache_location = cache_get_cache_pos(current_addr);
> +
> + /* abort if page changed too much */
> + it = cache_item_get(cache_location, slot);
Comment and code don't match?
> +
> + /* we need to copy the current data before encoding so it won't change
> + during encoding. cache_insert copies the data.
> + */
> + memcpy(it->it_data, prev_cache_page, TARGET_PAGE_SIZE);
> + cache_insert(current_addr, current_data, 0);
> +
> + /* XBZRLE encoding (if there is no overflow) */
> + encoded_buf = (uint8_t *) g_malloc(TARGET_PAGE_SIZE);
Cast not needed. Can we have a pagge allocated for this in Bucket
struct, XBRLE struct or whatever we want to call it? Having to do a
malloc for each page thaht we sent looks excesive?
> + encoded_len = encode_page(prev_cache_page, it->it_data, TARGET_PAGE_SIZE,
> + encoded_buf, TARGET_PAGE_SIZE);
> + if (encoded_len < 0) {
> + DPRINTF("Unmodifed page - skipping\n");
> + goto done;
> + }
> +
> + 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_buffer(f, (uint8_t *) &hdr, sizeof(hdr));
This is wrong (same for load part), we are breking inter-arch migration.
qemu_put_byte(f, hdr->xh_flags);
qemu_put_be16(f, hdr->xh_len);
qemu_put_be32(f, hdr->xh_cksum);
And the inverse on the load side.
> + qemu_put_buffer(f, encoded_buf, encoded_len);
> + bytes_sent = encoded_len + sizeof(hdr);
> +
> +done:
> + g_free(encoded_buf);
> + 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 = 0;
> MemoryRegion *mr;
> + ram_addr_t current_addr;
> + int use_xbzrle = migrate_use_xbzrle();
>
> if (!block)
> block = QLIST_FIRST(&ram_list.blocks);
>
> + current_addr = block->offset + offset;
> +
> do {
> mr = block->mr;
> if (memory_region_get_dirty(mr, offset, TARGET_PAGE_SIZE,
> @@ -388,7 +458,14 @@ 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 (stage == 2 && use_xbzrle) {
here
> + bytes_sent = save_xbzrle_page(f, p, current_addr, block,
> + offset, cont);
> + } else if (use_xbzrle && stage == 1) {
and here, can we used the check in the same order?
> + cache_insert(current_addr, p, 0);
> + }
Or even better, change the code to something like:
else if (use_xbzrle) {
if (stage == 1) {
cache_insert(current_addr, p, 0);
} else if (cache == 2) {
bytes_sent = save_xbzrle_page(...);
}
And I don't understand why this function care at all about the stages.
Normal code sent a page on stage == 1, here it is only saved on the
cache and sent as a normal page (without compression). And on stage==3,
it is also always sent without compression. I guess there is a method
on this, but a comment will help?
> +
> + if (!bytes_sent) {
> save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_PAGE);
> qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
> bytes_sent = TARGET_PAGE_SIZE;
> @@ -492,6 +569,9 @@ int ram_save_live(QEMUFile *f, int stage, void *opaque)
>
> if (stage < 0) {
> memory_global_dirty_log_stop();
> + if (migrate_use_xbzrle()) {
> + cache_fini();
> + }
> return 0;
> }
>
> @@ -504,6 +584,10 @@ int ram_save_live(QEMUFile *f, int stage, void *opaque)
> last_offset = 0;
> sort_ram_list();
>
> + if (migrate_use_xbzrle()) {
> + cache_init(migrate_xbzrle_cache_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) {
> @@ -531,9 +615,11 @@ 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_transferred += bytes_sent;
> - if (bytes_sent == 0) { /* no more blocks */
> + bytes_sent = ram_save_block(f, stage);
> + /* bytes_sent -1 represent unchanged page */
Interesting represtantion. Yes, I know that zero is already used (would
be the obvious here). Perhaps switching it with the old zero value, and
-1 pass to mean no more blocks?
> + if (bytes_sent > 0) {
> + bytes_transferred += bytes_sent;
> + } else if (bytes_sent == 0) { /* no more blocks */
> break;
> }
> }
> @@ -556,19 +642,67 @@ 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, stage)) != 0) {
> bytes_transferred += bytes_sent;
> }
> memory_global_dirty_log_stop();
> + if (migrate_use_xbzrle()) {
> + cache_fini();
> + }
> }
Not that this problem is introduced on this series, but could we get
this into a function migration_end/fini() that it just called in the two
places where a migration can end.
> qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
>
> 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());
> }
>
> +static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host)
> +{
> + int ret, rc = -1;
> + uint8_t *xbzrle_buf = NULL;
> + XBZRLEHeader hdr = {0};
> +
> + /* extract RLE header */
> + qemu_get_buffer(f, (uint8_t *) &hdr, sizeof(hdr));
This needs to be done by hand as told.
> + if (!(hdr.xh_flags & ENCODING_FLAG_XBZRLE)) {
> + fprintf(stderr, "Failed to load XBZRLE page - wrong compression!\n");
> + goto done;
> + }
> +
> + if (hdr.xh_len > TARGET_PAGE_SIZE) {
> + fprintf(stderr, "Failed to load XBZRLE page - len overflow!\n");
> + goto done;
> + }
> +
> + /* load data and decode */
> + xbzrle_buf = (uint8_t *) g_malloc0(TARGET_PAGE_SIZE);
cast not needed. Again, this can be done only _once_ for the whole
migration.
> + qemu_get_buffer(f, xbzrle_buf, hdr.xh_len);
> +
> + /* decode RLE */
> + ret = decode_page(xbzrle_buf, hdr.xh_len, host, TARGET_PAGE_SIZE);
> + if (ret == -1) {
> + fprintf(stderr, "Failed to load XBZRLE page - decode error!\n");
> + goto done;
> + }
> +
> + if (ret > TARGET_PAGE_SIZE) {
> + fprintf(stderr, "Failed to load XBZRLE page - size %d exceeds %d!\n",
> + ret, TARGET_PAGE_SIZE);
> + goto done;
> + }
> +
> + rc = 0;
> +
> +done:
> + g_free(xbzrle_buf);
> + return rc;
> +}
> +
> static inline void *host_from_stream_offset(QEMUFile *f,
> ram_addr_t offset,
> int flags)
> @@ -614,14 +748,18 @@ static inline void
> *host_from_stream_offset_versioned(int version_id,
> 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;
> }
>
> do {
> + void *host;
> addr = qemu_get_be64(f);
>
> flags = addr & ~TARGET_PAGE_MASK;
> @@ -645,8 +783,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;
> }
> }
> @@ -654,7 +794,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;
> @@ -693,14 +834,25 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
> return -EINVAL;
> }
> qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
> + } else if (flags & RAM_SAVE_FLAG_XBZRLE) {
> + host = host_from_stream_offset_versioned(version_id,
> + f, addr, flags);
> + if (load_xbzrle(f, addr, host) < 0) {
> + ret = -EINVAL;
> + goto done;
> + }
Why we don't needto check that host is NULL in this case?
[ I assume that encode/decode functions work ok, head hurts]
Can we change the name to xbzrle_page_encode()/decode()? name sounds
really too general. As they are not related to pages at all, we could
also call it: xbrzle_buffer_encode()/decode?
Thanks, Juan.
