[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
problems.
> + 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
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v15 0/9] XBZRLE delta for live migration of large memory app, Orit Wasserman, 2012/07/05
- [Qemu-devel] [PATCH v15 1/9] Add migration capabilities, Orit Wasserman, 2012/07/05
- [Qemu-devel] [PATCH v15 4/9] Add uleb encoding/decoding functions, Orit Wasserman, 2012/07/05
- [Qemu-devel] [PATCH v15 5/9] Change ram_save_block to return -1 if there are no more changes, Orit Wasserman, 2012/07/05
- [Qemu-devel] [PATCH v15 6/9] Add xbzrle_encode_buffer and xbzrle_decode_buffer functions, Orit Wasserman, 2012/07/05
- Re: [Qemu-devel] [PATCH v15 6/9] Add xbzrle_encode_buffer and xbzrle_decode_buffer functions,
Eric Blake <=
- [Qemu-devel] [PATCH v15 3/9] Add cache handling functions, Orit Wasserman, 2012/07/05
- [Qemu-devel] [PATCH v15 2/9] Add XBZRLE documentation, Orit Wasserman, 2012/07/05
- [Qemu-devel] [PATCH v15 7/9] Add XBZRLE to ram_save_block and ram_save_live, Orit Wasserman, 2012/07/05
- [Qemu-devel] [PATCH v15 8/9] Add set_cachesize command, Orit Wasserman, 2012/07/05