From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48851) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WUjMw-00036V-F2 for qemu-devel@nongnu.org; Mon, 31 Mar 2014 17:03:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WUjMr-0003du-JP for qemu-devel@nongnu.org; Mon, 31 Mar 2014 17:03:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:62646) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WUjMr-0003dN-Bd for qemu-devel@nongnu.org; Mon, 31 Mar 2014 17:03:41 -0400 Message-ID: <5339D826.6030305@redhat.com> Date: Mon, 31 Mar 2014 23:03:34 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1396266961-11044-1-git-send-email-arei.gonglei@huawei.com> <1396266961-11044-2-git-send-email-arei.gonglei@huawei.com> <20140331140028.GA11125@work-vm> In-Reply-To: <20140331140028.GA11125@work-vm> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 1/2] xbzrle: don't check the value in the vm ram repeatedly List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert" , arei.gonglei@huawei.com Cc: ChenLiang , weidong.huang@huawei.com, qemu-devel@nongnu.org, quintela@redhat.com Il 31/03/2014 16:00, Dr. David Alan Gilbert ha scritto: > * arei.gonglei@huawei.com (arei.gonglei@huawei.com) wrote: >> From: ChenLiang >> >> xbzrle_encode_buffer checks the value in the vm ram repeatedly. >> It is risk if runs xbzrle_encode_buffer on changing data. >> And it is not necessary. >> >> Reported-by: Dr. David Alan Gilbert >> Signed-off-by: ChenLiang >> Signed-off-by: Gonglei >> --- >> xbzrle.c | 20 +++++++++++++++----- >> 1 file changed, 15 insertions(+), 5 deletions(-) >> >> diff --git a/xbzrle.c b/xbzrle.c >> index fbcb35d..bf08c56 100644 >> --- a/xbzrle.c >> +++ b/xbzrle.c >> @@ -27,7 +27,7 @@ int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t *new_buf, int slen, >> uint8_t *dst, int dlen) >> { >> uint32_t zrun_len = 0, nzrun_len = 0; >> - int d = 0, i = 0; >> + int d = 0, i = 0, j; >> long res, xor; >> uint8_t *nzrun_start = NULL; >> >> @@ -82,6 +82,8 @@ int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t *new_buf, int slen, >> if (d + 2 > dlen) { >> return -1; >> } >> + i++; >> + nzrun_len++; >> /* not aligned to sizeof(long) */ >> res = (slen - i) % sizeof(long); >> while (res && old_buf[i] != new_buf[i]) { >> @@ -98,11 +100,17 @@ int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t *new_buf, int slen, >> xor = *(long *)(old_buf + i) ^ *(long *)(new_buf + i); >> if ((xor - mask) & ~xor & (mask << 7)) { >> /* found the end of an nzrun within the current long */ >> - while (old_buf[i] != new_buf[i]) { >> - nzrun_len++; >> - i++; >> + for (j = 0; j < sizeof(long); j++) { >> + if (old_buf[i] != new_buf[i]) { >> + nzrun_len++; >> + i++; >> + } else { >> + break; >> + } >> + } >> + if (j != sizeof(long)) { >> + break; > > I wonder if it would be easier just to use the value of 'xor' - since that > already contains the value that we read, and if we've got this far is guaranteed > to have a none-equal byte in it. That would be something like (untested): > > for (j = 0; j < sizeof(long); j++) { > if (get_byte(xor, j) != 0) { > break; > } > } > nzrun_len += j; > i += j; Or this: #ifdef HOST_WORDS_BIGENDIAN j = clzl(xor) >> 3; #else j = ctzl(xor) >> 3; #endif nzrun_len += j; i += j; Paolo