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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5786 bytes --]

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?

-- 
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 --]