* [Qemu-devel] [PATCH v2 0/2] xbzrle: fix one corruption issue @ 2014-03-31 11:55 arei.gonglei 2014-03-31 11:56 ` [Qemu-devel] [PATCH v2 1/2] xbzrle: don't check the value in the vm ram repeatedly arei.gonglei 2014-03-31 11:56 ` [Qemu-devel] [PATCH v2 2/2] xbzrle: check 8 bytes at a time after an concurrency scene arei.gonglei 0 siblings, 2 replies; 5+ messages in thread From: arei.gonglei @ 2014-03-31 11:55 UTC (permalink / raw) To: qemu-devel Cc: ChenLiang, weidong.huang, quintela, dgilbert, owasserm, pbonzini From: ChenLiang <chenliang88@huawei.com> It is risk if runs xbzrle_encode_buffer on changing data. Changes since v1: * avoid to stuck in loop * check 8 bytes at a time after an concurrency scene ChenLiang (2): xbzrle: don't check the value in the vm ram repeatedly xbzrle: check 8 bytes at a time after an concurrency scene xbzrle.c | 48 +++++++++++++++++++++++++++++++++--------------- 1 file changed, 33 insertions(+), 15 deletions(-) -- 1.7.12.4 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [Qemu-devel] [PATCH v2 1/2] xbzrle: don't check the value in the vm ram repeatedly 2014-03-31 11:55 [Qemu-devel] [PATCH v2 0/2] xbzrle: fix one corruption issue arei.gonglei @ 2014-03-31 11:56 ` arei.gonglei 2014-03-31 14:00 ` Dr. David Alan Gilbert 2014-03-31 11:56 ` [Qemu-devel] [PATCH v2 2/2] xbzrle: check 8 bytes at a time after an concurrency scene arei.gonglei 1 sibling, 1 reply; 5+ messages in thread From: arei.gonglei @ 2014-03-31 11:56 UTC (permalink / raw) To: qemu-devel Cc: ChenLiang, weidong.huang, quintela, dgilbert, owasserm, Gonglei, pbonzini From: ChenLiang <chenliang88@huawei.com> 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 <dgilbert@redhat.com> Signed-off-by: ChenLiang <chenliang88@huawei.com> Signed-off-by: Gonglei <arei.gonglei@huawei.com> --- 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; } - break; } else { i += sizeof(long); nzrun_len += sizeof(long); @@ -118,6 +126,8 @@ int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t *new_buf, int slen, memcpy(dst + d, nzrun_start, nzrun_len); d += nzrun_len; nzrun_len = 0; + i++; + zrun_len++; } return d; -- 1.7.12.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] xbzrle: don't check the value in the vm ram repeatedly 2014-03-31 11:56 ` [Qemu-devel] [PATCH v2 1/2] xbzrle: don't check the value in the vm ram repeatedly arei.gonglei @ 2014-03-31 14:00 ` Dr. David Alan Gilbert 2014-03-31 21:03 ` Paolo Bonzini 0 siblings, 1 reply; 5+ messages in thread From: Dr. David Alan Gilbert @ 2014-03-31 14:00 UTC (permalink / raw) To: arei.gonglei; +Cc: ChenLiang, pbonzini, weidong.huang, qemu-devel, quintela * arei.gonglei@huawei.com (arei.gonglei@huawei.com) wrote: > From: ChenLiang <chenliang88@huawei.com> > > 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 <dgilbert@redhat.com> > Signed-off-by: ChenLiang <chenliang88@huawei.com> > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > --- > 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] xbzrle: don't check the value in the vm ram repeatedly 2014-03-31 14:00 ` Dr. David Alan Gilbert @ 2014-03-31 21:03 ` Paolo Bonzini 0 siblings, 0 replies; 5+ messages in thread From: Paolo Bonzini @ 2014-03-31 21:03 UTC (permalink / raw) To: Dr. David Alan Gilbert, arei.gonglei Cc: ChenLiang, weidong.huang, qemu-devel, quintela Il 31/03/2014 16:00, Dr. David Alan Gilbert ha scritto: > * arei.gonglei@huawei.com (arei.gonglei@huawei.com) wrote: >> From: ChenLiang <chenliang88@huawei.com> >> >> 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 <dgilbert@redhat.com> >> Signed-off-by: ChenLiang <chenliang88@huawei.com> >> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >> --- >> 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [Qemu-devel] [PATCH v2 2/2] xbzrle: check 8 bytes at a time after an concurrency scene 2014-03-31 11:55 [Qemu-devel] [PATCH v2 0/2] xbzrle: fix one corruption issue arei.gonglei 2014-03-31 11:56 ` [Qemu-devel] [PATCH v2 1/2] xbzrle: don't check the value in the vm ram repeatedly arei.gonglei @ 2014-03-31 11:56 ` arei.gonglei 1 sibling, 0 replies; 5+ messages in thread From: arei.gonglei @ 2014-03-31 11:56 UTC (permalink / raw) To: qemu-devel Cc: ChenLiang, weidong.huang, quintela, dgilbert, owasserm, Gonglei, pbonzini From: ChenLiang <chenliang88@huawei.com> The logic of old code is correct. But Checking byte by byte will consume time after an concurrency scene. Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Signed-off-by: ChenLiang <chenliang88@huawei.com> Signed-off-by: Gonglei <arei.gonglei@huawei.com> --- xbzrle.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/xbzrle.c b/xbzrle.c index bf08c56..f2824db 100644 --- a/xbzrle.c +++ b/xbzrle.c @@ -50,16 +50,24 @@ int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t *new_buf, int slen, /* word at a time for speed */ if (!res) { - while (i < slen && - (*(long *)(old_buf + i)) == (*(long *)(new_buf + i))) { - i += sizeof(long); - zrun_len += sizeof(long); - } - - /* go over the rest */ - while (i < slen && old_buf[i] == new_buf[i]) { - zrun_len++; - i++; + while (i < slen) { + if ((*(long *)(old_buf + i)) == (*(long *)(new_buf + i))) { + i += sizeof(long); + zrun_len += sizeof(long); + } else { + /* go over the rest */ + for (j = 0; j < sizeof(long); j++) { + if (old_buf[i] == new_buf[i]) { + i++; + zrun_len++; + } else { + break; + } + } + if (j != sizeof(long)) { + break; + } + } } } -- 1.7.12.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-03-31 21:03 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-03-31 11:55 [Qemu-devel] [PATCH v2 0/2] xbzrle: fix one corruption issue arei.gonglei 2014-03-31 11:56 ` [Qemu-devel] [PATCH v2 1/2] xbzrle: don't check the value in the vm ram repeatedly arei.gonglei 2014-03-31 14:00 ` Dr. David Alan Gilbert 2014-03-31 21:03 ` Paolo Bonzini 2014-03-31 11:56 ` [Qemu-devel] [PATCH v2 2/2] xbzrle: check 8 bytes at a time after an concurrency scene arei.gonglei
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).