Re: [Qemu-devel] [PATCH v4 7/8] migration: optimize xbzrle by reducing data copy

[Gonglei (Arei)]

> 
> 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
Nice catch. I will send another patch to fix it. Thanks!

Best regards,
-Gonglei