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
