Re: [Qemu-devel] [PATCH 07/11] Add XBZRLE to ram_save_block and ram_save_live

[Orit Wasserman <owasserm@redhat.com>]

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