Re: [Qemu-devel] [PATCH v2 1/2] xbzrle: don't check the value in the vm ram repeatedly

[Dr. David Alan Gilbert]

* address@hidden (address@hidden) wrote:
> From: ChenLiang <address@hidden>
> 
> 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 <address@hidden>
> Signed-off-by: ChenLiang <address@hidden>
> Signed-off-by: Gonglei <address@hidden>
> ---
>  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 / address@hidden / Manchester, UK