* arei.gong...@huawei.com (arei.gong...@huawei.com) wrote:
> From: ChenLiang <chenlian...@huawei.com>
>
> Reducing data copy can reduce cpu overhead.
>
> Signed-off-by: ChenLiang <chenlian...@huawei.com>
> Signed-off-by: Gonglei <arei.gong...@huawei.com>
> Reviewed-by: Juan Quintela <quint...@redhat.com>
> ---
> arch_init.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/arch_init.c b/arch_init.c
> index 84a4bd3..94b62e2 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -373,11 +373,8 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t
> **current_data,
>
> prev_cached_page = get_cached_data(XBZRLE.cache, current_addr);
>
> - /* save current buffer into memory */
> - memcpy(XBZRLE.current_buf, *current_data, TARGET_PAGE_SIZE);
> -
> /* XBZRLE encoding (if there is no overflow) */
> - encoded_len = xbzrle_encode_buffer(prev_cached_page, XBZRLE.current_buf,
> + encoded_len = xbzrle_encode_buffer(prev_cached_page, *current_data,
> TARGET_PAGE_SIZE, XBZRLE.encoded_buf,
> TARGET_PAGE_SIZE);
Hi Gonglei,
I've got a world which has this patch series on, and it's producing some XBZRLE
errors,
and I suspect that it's down to my original worries of running
xbzrle_encode_buffer
on changing data.
My setup is a pair of machines, with a guest with 4GB RAM running SPECjbb, 1GbE
between them and a 2GB xbzrle cache, and I can reliably trigger:
Failed to load XBZRLE page - decode error!
so I added some debug and saw:
qemu-system-x86_64: xbzrle_decode_buffer: Failed decode (uleb128 a) ret=1 i=45
count=0
xbzrle data:: 0000: 38 04 c0 d4 a4 fc 5c 04 d9 d4 a4 fc 5c 04 f2 d4
xbzrle data:: 0010: a4 fc 88 02 03 c1 dd 79 01 01 03 17 10 87 b2 a3
xbzrle data:: 0020: e8 8e b2 a3 e8 95 b2 a3 e8 9c b2 a3 e8 00*0c a3 <--- *
corresponds to i=45
xbzrle data:: 0030: b2 a3 e8 aa b2 a3 e8 b1 b2 a3 e8 00 10 b8 b2 a3
xbzrle data:: 0040: e8 bf b2 a3 e8 c6 b2 a3 e8 d2 ca a3 e8 00 0c d9
xbzrle data:: 0050: ca a3 e8 e0 ca a3 e8 e7 ca a3 e8 00 08 ee ca a3
xbzrle data:: 0060: e8 f5 ca a3 e8 00 08 fc ca a3 e8 03 cb a3 e8 00
xbzrle data:: 0070: 0c 0a cb a3 e8 11 cb a3 e8 18 cb a3 e8 00 08 1f
xbzrle data:: 0080: cb a3 e8 26 cb a3 e8 40 03 29 de 79 e9 02 01 82
xbzrle data:: 0090: 03 04 00 00 00 00 14 01 00 03 01 52 07 04 00 00
xbzrle data:: 00a0: 00 00 28 01 00 03 01 52 07 08 00 00 00 00 00 00
xbzrle data:: 00b0: 00 00 24 01 00 03 01 52 07 08 00 00 00 00 00 00
xbzrle data:: 00c0: 00 00 24 01 00 03 01 52 07 08 00 00 00 00 00 00
xbzrle data:: 00d0: 00 00 24 01 00 03 01 52 07 08 00 00 00 00 00 00
xbzrle data:: 00e0: 00 00 24 01 00 03 01 52 07 08 00 00 00 00 00 00
xbzrle data:: 00f0: 00 00 24 01 00 03 01 52 07 08 00 00 00 00 00 00
xbzrle data:: 0100: 00 00 24 01 00 03 01 52 07 08 00 00 00 00 00 00
xbzrle data:: 0110: 00 00 24 01 00 03 01 52 07 08 00 00 00 00 00 00
xbzrle data:: 0120: 00 00 24 01 00 03 01 52 07 08 00 00 00 00 00 00
xbzrle data:: 0130: 00 00 e0 08 01 5a 03 01 b1 01 01 b1
If I understand this correctly the zero-run was found to be '0' length, and
that should never happen - xbzrle should always output non-0 lengths
for both it's zero and nz run lengths according to my reading of the code
and the error check on the decode.
So I added:
@@ -73,6 +74,11 @@ int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t *new_buf,
int slen,
return d;
}
+ if (zrun_len == 0) {
+ error_report("xbzrle_encode_buffer: zrun_len=0! i=%d\n",i);
+ return -1;
+ }
+
d += uleb128_encode_small(dst + d, zrun_len);
on the encode side, and yes that's triggering (I also added it for the
nzrun version)
The code in xbzrle.c is basically like:
a loop {
b while *ptr == 0 increment
c save count of 0's
d while *ptr != 0 increment
e save count of none=0's
f }
With your patch the data can be changing during this loop since
the code now runs directly on current_data, so that a byte that might
have read as none-0 by loop (b) above, gets changed by the guest
to 0 just after (b) exits. When it enters (d) it reads the byte
find it's 0 and outputs a '0' length count which is invalid format.
Dave
> if (encoded_len == 0) {
> @@ -396,7 +393,8 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t
> **current_data,
>
> /* we need to update the data in the cache, in order to get the same
> data */
> if (!last_stage) {
> - memcpy(prev_cached_page, XBZRLE.current_buf, TARGET_PAGE_SIZE);
> + xbzrle_decode_buffer(XBZRLE.encoded_buf, encoded_len,
> prev_cached_page,
> + TARGET_PAGE_SIZE);
> }
>
> /* Send XBZRLE based compressed page */
> --
> 1.7.12.4
>
>
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
