[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] xbzrle: don't check the value in the vm ram rep
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH] xbzrle: don't check the value in the vm ram repeatedly |
Date: |
Sat, 29 Mar 2014 08:26:06 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.3.0 |
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