Re: [Qemu-devel] [PATCH] xbzrle: don't check the value in the vm ram repeatedly

[Eric Blake]

On 03/29/2014 08:15 AM, 陈梁 wrote:

>>
>> Insufficient.  Observe what we did at line 52 when looking for the zero-run:
>>
>>>>        /* word at a time for speed */
>>>>        if (!res) {
>>>>            while (i < slen &&
>>>>                   (*(long *)(old_buf + i)) == (*(long *)(new_buf + i))) {
>>
>> This dereferences 8 bytes at new_buf[i] (on a 64-bit machine)...
>>
>>>>                i += sizeof(long);
>>>>                zrun_len += sizeof(long);
>>>>            }
>>>>
>>>>            /* go over the rest */
>>>>            while (i < slen && old_buf[i] == new_buf[i]) {
>>
>> ...and this also dereferences those same bytes.  But your argument is
>> that new_buf[i] is volatile.
>>
>> You MUST read new_buf[i] into a temporary variable, and if the first
>> read found a difference, then the "go over the rest" analysis must be on
>> that temporary, rather than going back to reading directly from new_buf.
>>
> It is ok, we just need to guarantee that the pages in cache are same to the 
> page in dest side. 
> Don‘t care about whether they are same to src side. Because the modified 
> pages during this
> time will be sent at next time.

No, it is not okay - if you are reading the same bytes from new_buf, and
new_buf is volatile, then you MUST read each byte of new_buf exactly
once.  Consider this sequence:

old_buf contains

00 00 00 00 00 00 00 00

new_buf initially contains

00 00 00 00 00 00 00 01

so the zrun detection spots that the two buffers are different on the
8-byte read.  But then another thread modifies new_buf to be all zeros.
 Now the "go over the rest" loop does 8 reads, expecting to find a
difference, but it can't find one.

You really need to do the "go over the rest" loop on an 8-byte temporary
variable.  Ever since your patch made new_buf be a volatile buffer,
rather than a static copy, you MUST visit each byte of new_buf exactly once.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature