From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42102) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WUexU-0005M4-F4 for qemu-devel@nongnu.org; Mon, 31 Mar 2014 12:21:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WUexP-0001tP-BC for qemu-devel@nongnu.org; Mon, 31 Mar 2014 12:21:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:14790) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WUexP-0001tF-1s for qemu-devel@nongnu.org; Mon, 31 Mar 2014 12:21:07 -0400 Date: Mon, 31 Mar 2014 15:00:28 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20140331140028.GA11125@work-vm> References: <1396266961-11044-1-git-send-email-arei.gonglei@huawei.com> <1396266961-11044-2-git-send-email-arei.gonglei@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1396266961-11044-2-git-send-email-arei.gonglei@huawei.com> 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: arei.gonglei@huawei.com Cc: ChenLiang , pbonzini@redhat.com, weidong.huang@huawei.com, qemu-devel@nongnu.org, quintela@redhat.com * 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; with get_byte being defined as: uint8_t get_byte(long l, unsigned int index) { #ifdef HOST_WORDS_BIGENDIAN index = (sizeof(long) - 1) - index; #endif return (l >> (index * 8)) & 0xff; } That way we really aren't reading the data again. Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK