Re: [Qemu-devel] [PATCH v11 7/9] Add XBZRLE to ram_save_block and ram_save_live

[Orit Wasserman]

On 06/01/2012 02:42 PM, Juan Quintela wrote:
> Orit Wasserman <address@hidden> 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.
>> In the incoming migration check to see if RAM_SAVE_FLAG_XBRLE is set
>> and decompress the page (by using load_xbrle function).
> 
> 
> This patch can be split to easy review.
Sure.
> 
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -43,6 +43,15 @@
>>  #include "hw/smbios.h"
>>  #include "exec-memory.h"
>>  #include "hw/pcspk.h"
>> +#include "qemu/cache.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
> 
> Independent of xbzrle.
> 
>>  
>>  #ifdef TARGET_SPARC
>>  int graphic_width = 1024;
>> @@ -94,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>
>> @@ -157,6 +167,22 @@ static int is_dup_page(uint8_t *page)
>>      return 1;
>>  }
>>  
>> +/* XBZRLE (Xor Based Zero Length Encoding */
>> +typedef struct XBZRLEHeader {
>> +    uint32_t xh_cksum;
> 
> We are still not using this value, and we are sending it anyway (with a
> value of zero).  What happens when we start using if for a checksum, and
> we migration to a new version that "expects" it to be valid?  I would
> preffer not to sent it, or sent the correct value.
I think I will remove it, checksum should be used for all migration not just 
XBZRLE.
I guess we can add it to the protocol in the future.
> 
>> +    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;
>> +    /* Cache for XBZRLE */
>> +    Cache *cache;
>> +} XBZRLE = {0};
> 
> Use c99 initializers, please.
> 
> { .encoded_buf = NULL,
>   .cache = NULL,
> }
> 
> More instances in other parts.
> 
>> +
>>  static void save_block_hdr(QEMUFile *f, RAMBlock *block, ram_addr_t offset,
>>          int cont, int flag)
>  >  {
>> @@ -169,19 +195,78 @@ 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, ret = -1;
>> +    XBZRLEHeader hdr = {0};
>> +    uint8_t *prev_cached_page;
>> +
>> +    /* check to see if page is cached , if not cache and return */
>> +    if (!cache_is_cached(XBZRLE.cache, current_addr)) {
>> +        cache_insert(XBZRLE.cache, current_addr, g_memdup(current_data,
>> +                                                          
>> TARGET_PAGE_SIZE));
>> +        goto done;
>> +    }
>> +
>> +    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) {
>> +        bytes_sent = 0;
>> +        DPRINTF("Unmodifed page or overflow skipping\n");
>> +        goto done;
>> +    } else if (encoded_len == -1) {
>> +        bytes_sent = -1;
>> +        DPRINTF("Overflow\n");
>> +        /* update data in the cache */
>> +        memcpy(prev_cached_page, current_data, TARGET_PAGE_SIZE);
>> +        goto done;
>> +    }
>> +
>> +    /* 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_be32(f, hdr.xh_cksum);
>> +    qemu_put_buffer(f, XBZRLE.encoded_buf, encoded_len);
>> +    bytes_sent = encoded_len + sizeof(hdr);
>> +
>> +done:
>> +    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;
>> +    int bytes_sent = -1;
>>      MemoryRegion *mr;
>> +    ram_addr_t current_addr;
>>  
>>      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,
>> @@ -198,7 +283,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()) {
>> +                /* in stage 1 none of the pages are cached so we just want 
>> to
>> +                   cache them for next stages, and send the cached copy */
>> +                if (stage == 1) {
>> +                    cache_insert(XBZRLE.cache, current_addr,
>> +                                 g_memdup(p, TARGET_PAGE_SIZE));
>> +                } else {
>> +                    bytes_sent = save_xbzrle_page(f, p, current_addr, block,
>> +                                                  offset, cont);
>> +                }
>> +                /* send the cached page copy for stage 1 and 2*/
>> +                if (stage != 3) {
>> +                    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;
> 
> 
> I think that code is not right when save_xbzrle_page() returns 0.  That
> means that page hasn't changed since last time we sent that page.  We
> shouldn't break in that case.  Just continue with next page, right?
> 
You are right I missed that , will be fixed

> On the other hand ... Why are we doing the stage == 1 test?  stage 1
> normally only sent part of the pages, so we could use the generic code
> there?  It would just return -1 as bytes_sent, and do the same code that
> we have now?

we need to add the pages to the cache in stage 1 (for the next stage),
and there is no need for checking if the page is cached.
and send the pages from the cache for consistency

> 
> The optimization for stage 3 is not done backwards?  We are inserting
> the page in the cache even if we are on stage 3.  In stage three we
> should:
> - look if page is on the cache: do usual xbrlze trick
> - if it is not, just sent the whole page without inserting it into the
> cache?  We are never going to reuse it, so putting it into the cache
> would not help us at all.  We are just making an extra copy?
right no need to insert the page into the cache in stage 3, I will remove it
> 
> 
>>  
>>      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());
>> +
> 
> This belongs to debugging patch.
> 
>> +    /* load data and decode */
>> +    xbzrle_buf = g_malloc0(TARGET_PAGE_SIZE);
> 
> can't we have a static buffer of that size, and avoid all the
> malloc/free business?  If space is tight, we can allways put it on the
> xbrle structure and assign it only for migration.
good idea
> 
>> @@ -481,16 +657,33 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
>>              void *host;
>>  
>>              host = host_from_stream_offset(f, addr, flags);
>> +            if (!host) {
>> +                return -EINVAL;
>> +            }
> 
> Why is this check only needed now?
I wish I knew, looks like it is missing in upstream.
Do you think I should fix it separately ?

Thanks,
Orit
> 
> Later, Juan.