[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v15 6/9] Add xbzrle_encode_buffer and xbzrle_dec

From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH v15 6/9] Add xbzrle_encode_buffer and xbzrle_decode_buffer functions
Date: Thu, 05 Jul 2012 07:55:36 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120615 Thunderbird/13.0.1

On 07/05/2012 06:51 AM, Orit Wasserman wrote:

This commit message is a bit sparse.  I'd document at least the fact
that our nzrun detection code in xbzrle_encode_buffer borrows
long-word-at-a-time NUL-detection tricks from strcmp(), as it is not an
intuitive trick known by all developers.

> Signed-off-by: Benoit Hudzia <address@hidden>
> Signed-off-by: Petter Svard <address@hidden>
> Signed-off-by: Aidan Shribman <address@hidden>
> Signed-off-by: Orit Wasserman <address@hidden>

I think I touched this one heavily enough that it warrants adding:

Signed-off-by: Eric Blake <address@hidden>

Other than the commit message, I'm happy with this patch contents now.
Still some nits, though:

> +
> +         /* not aligned to sizeof(long) */
> +        res = (slen - i) % sizeof(long);

Comment indentation is off.

> +        while (res && old_buf[i] == new_buf[i]) {
> +            zrun_len++;
> +            i++;
> +            res--;
> +        }
> +
> +        if (!res) {

A comment here might help:

/* word at a time for speed */

> +            while (i < slen &&
> +                   (*(long *)(old_buf + i)) == (*(long *)(new_buf + i))) {

On re-reading this, I'm worried whether it violates strict C99 aliasing
rules; I'm hoping the compiler doesn't mis-optimize it based on a
loophole of us using questionable semantics.  In practice, I'm confident
that we are only doing aligned reads (otherwise I wouldn't have
suggested this line in the first place), which is why I'm personally
okay with it even if it technically violates C99.  However, I'm open to
suggestions from anyone else on the list on how to better express the
notion of intentionally accessing a long at a time from an aligned
offset into a byte array, if that will help us avoid even theoretical

> +            nzrun_len++;
> +            res--;
> +        }
> +
> +        if (!res) {
> +            /* truncation to 32-bit long okay */

Again, a longer comment might help:

/* word at a time for speed, use of 32-bit long okay */

> +            long mask = 0x0101010101010101ULL;
> +            while (i < slen) {
> +                xor = *(long *)(old_buf + i) ^ *(long *)(new_buf + i);

Same potential strict aliasing problems as for zrun detection.

Eric Blake   address@hidden    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]