* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.