[Qemu-devel] [PATCH 0/4] QEMUFile improvements and simplifications

[Qemu-devel] [PATCH 0/4] QEMUFile improvements and simplifications

[Paolo Bonzini]

This fixes Kevin's reported regression with savevm, and simplifies the
QEMUFile code further.

Patch 2 could be made a bit smaller at the expense of fixing the
regression in the last patch only.  I prefer to fix the bug earlier.

Tested with Autotest.

Paolo Bonzini (4):
  migration: set f->is_write and flush in add_to_iovec
  migration: use a single I/O operation when writev_buffer is not defined
  migration: drop is_write complications
  migration: simplify writev vs. non-writev logic

 savevm.c | 104 ++++++++++++++++++++++++++-------------------------------------
 1 file changed, 42 insertions(+), 62 deletions(-)

-- 
1.8.2

[Qemu-devel] [PATCH 1/4] migration: set f->is_write and flush in add_to_iovec

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 savevm.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/savevm.c b/savevm.c
index b1d8988..c952c41 100644
--- a/savevm.c
+++ b/savevm.c
@@ -631,6 +631,11 @@ static void add_to_iovec(QEMUFile *f, const uint8_t *buf, int size)
         f->iov[f->iovcnt].iov_base = (uint8_t *)buf;
         f->iov[f->iovcnt++].iov_len = size;
     }
+
+    f->is_write = 1;
+    if (f->buf_index >= IO_BUF_SIZE || f->iovcnt >= MAX_IOV_SIZE) {
+        qemu_fflush(f);
+    }
 }
 
 void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, int size)
@@ -645,14 +650,8 @@ void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, int size)
         abort();
     }
 
-    add_to_iovec(f, buf, size);
-
-    f->is_write = 1;
     f->bytes_xfer += size;
-
-    if (f->buf_index >= IO_BUF_SIZE || f->iovcnt >= MAX_IOV_SIZE) {
-        qemu_fflush(f);
-    }
+    add_to_iovec(f, buf, size);
 }
 
 void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
@@ -674,7 +673,6 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
         if (l > size)
             l = size;
         memcpy(f->buf + f->buf_index, buf, l);
-        f->is_write = 1;
         f->buf_index += l;
         qemu_put_buffer_async(f, f->buf + (f->buf_index - l), l);
         if (qemu_file_get_error(f)) {
@@ -697,15 +695,10 @@ void qemu_put_byte(QEMUFile *f, int v)
         abort();
     }
 
-    f->buf[f->buf_index++] = v;
-    f->is_write = 1;
+    f->buf[f->buf_index] = v;
     f->bytes_xfer++;
-
-    add_to_iovec(f, f->buf + (f->buf_index - 1), 1);
-
-    if (f->buf_index >= IO_BUF_SIZE || f->iovcnt >= MAX_IOV_SIZE) {
-        qemu_fflush(f);
-    }
+    add_to_iovec(f, f->buf + f->buf_index, 1);
+    f->buf_index++;
 }
 
 static void qemu_file_skip(QEMUFile *f, int size)
-- 
1.8.2

[Qemu-devel] [PATCH 2/4] migration: use a single I/O operation when writev_buffer is not defined

[Paolo Bonzini]

The recent patches to use vectored I/O for RAM migration caused a
regression in savevm speed.  To restore previous performance,
add data to the buffer in qemu_put_buffer_async whenever writev_buffer
is not available in the QEMUFile.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 savevm.c | 49 ++++++++++++++++++++++++++++++++++---------------
 1 file changed, 34 insertions(+), 15 deletions(-)

diff --git a/savevm.c b/savevm.c
index c952c41..b32e0a7 100644
--- a/savevm.c
+++ b/savevm.c
@@ -525,27 +525,24 @@ static void qemu_file_set_error(QEMUFile *f, int ret)
 static void qemu_fflush(QEMUFile *f)
 {
     ssize_t ret = 0;
-    int i = 0;
 
     if (!f->ops->writev_buffer && !f->ops->put_buffer) {
         return;
     }
 
-    if (f->is_write && f->iovcnt > 0) {
+    if (f->is_write) {
         if (f->ops->writev_buffer) {
-            ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt);
-            if (ret >= 0) {
-                f->pos += ret;
+            if (f->iovcnt > 0) {
+                ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt);
             }
         } else {
-            for (i = 0; i < f->iovcnt && ret >= 0; i++) {
-                ret = f->ops->put_buffer(f->opaque, f->iov[i].iov_base, f->pos,
-                                         f->iov[i].iov_len);
-                if (ret >= 0) {
-                    f->pos += ret;
-                }
+            if (f->buf_index > 0) {
+                ret = f->ops->put_buffer(f->opaque, f->buf, f->pos, f->buf_index);
             }
         }
+        if (ret >= 0) {
+            f->pos += ret;
+        }
         f->buf_index = 0;
         f->iovcnt = 0;
     }
@@ -640,6 +637,11 @@ static void add_to_iovec(QEMUFile *f, const uint8_t *buf, int size)
 
 void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, int size)
 {
+    if (!f->ops->writev_buffer) {
+        qemu_put_buffer(f, buf, size);
+        return;
+    }
+
     if (f->last_error) {
         return;
     }
@@ -673,8 +675,17 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
         if (l > size)
             l = size;
         memcpy(f->buf + f->buf_index, buf, l);
-        f->buf_index += l;
-        qemu_put_buffer_async(f, f->buf + (f->buf_index - l), l);
+        f->bytes_xfer += size;
+        if (f->ops->writev_buffer) {
+            add_to_iovec(f, f->buf + f->buf_index, l);
+            f->buf_index += l;
+        } else {
+            f->is_write = 1;
+            f->buf_index += l;
+            if (f->buf_index == IO_BUF_SIZE) {
+                qemu_fflush(f);
+            }
+        }
         if (qemu_file_get_error(f)) {
             break;
         }
@@ -697,8 +708,16 @@ void qemu_put_byte(QEMUFile *f, int v)
 
     f->buf[f->buf_index] = v;
     f->bytes_xfer++;
-    add_to_iovec(f, f->buf + f->buf_index, 1);
-    f->buf_index++;
+    if (f->ops->writev_buffer) {
+        add_to_iovec(f, f->buf + f->buf_index, 1);
+        f->buf_index++;
+    } else {
+        f->is_write = 1;
+        f->buf_index++;
+        if (f->buf_index == IO_BUF_SIZE) {
+            qemu_fflush(f);
+        }
+    }
 }
 
 static void qemu_file_skip(QEMUFile *f, int size)
-- 
1.8.2

[Qemu-devel] [PATCH 3/4] migration: drop is_write complications

[Paolo Bonzini]

The same QEMUFile is never used for both read and write.  Simplify
the logic to simply look for presence or absence of the right ops.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 savevm.c | 68 +++++++++++++++++++---------------------------------------------
 1 file changed, 20 insertions(+), 48 deletions(-)

diff --git a/savevm.c b/savevm.c
index b32e0a7..a2f6bc0 100644
--- a/savevm.c
+++ b/savevm.c
@@ -119,7 +119,6 @@ void qemu_announce_self(void)
 struct QEMUFile {
     const QEMUFileOps *ops;
     void *opaque;
-    int is_write;
 
     int64_t bytes_xfer;
     int64_t xfer_limit;
@@ -500,7 +499,6 @@ QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops)
 
     f->opaque = opaque;
     f->ops = ops;
-    f->is_write = 0;
     return f;
 }
 
@@ -516,6 +514,11 @@ static void qemu_file_set_error(QEMUFile *f, int ret)
     }
 }
 
+static inline bool qemu_file_is_writable(QEMUFile *f)
+{
+    return f->ops->writev_buffer || f->ops->put_buffer;
+}
+
 /**
  * Flushes QEMUFile buffer
  *
@@ -526,26 +529,24 @@ static void qemu_fflush(QEMUFile *f)
 {
     ssize_t ret = 0;
 
-    if (!f->ops->writev_buffer && !f->ops->put_buffer) {
+    if (!qemu_file_is_writable(f)) {
         return;
     }
 
-    if (f->is_write) {
-        if (f->ops->writev_buffer) {
-            if (f->iovcnt > 0) {
-                ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt);
-            }
-        } else {
-            if (f->buf_index > 0) {
-                ret = f->ops->put_buffer(f->opaque, f->buf, f->pos, f->buf_index);
-            }
+    if (f->ops->writev_buffer) {
+        if (f->iovcnt > 0) {
+            ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt);
         }
-        if (ret >= 0) {
-            f->pos += ret;
+    } else {
+        if (f->buf_index > 0) {
+            ret = f->ops->put_buffer(f->opaque, f->buf, f->pos, f->buf_index);
         }
-        f->buf_index = 0;
-        f->iovcnt = 0;
     }
+    if (ret >= 0) {
+        f->pos += ret;
+    }
+    f->buf_index = 0;
+    f->iovcnt = 0;
     if (ret < 0) {
         qemu_file_set_error(f, ret);
     }
@@ -556,11 +557,7 @@ static void qemu_fill_buffer(QEMUFile *f)
     int len;
     int pending;
 
-    if (!f->ops->get_buffer)
-        return;
-
-    if (f->is_write)
-        abort();
+    assert(!qemu_file_is_writable(f));
 
     pending = f->buf_size - f->buf_index;
     if (pending > 0) {
@@ -629,7 +626,6 @@ static void add_to_iovec(QEMUFile *f, const uint8_t *buf, int size)
         f->iov[f->iovcnt++].iov_len = size;
     }
 
-    f->is_write = 1;
     if (f->buf_index >= IO_BUF_SIZE || f->iovcnt >= MAX_IOV_SIZE) {
         qemu_fflush(f);
     }
@@ -646,12 +642,6 @@ void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, int size)
         return;
     }
 
-    if (f->is_write == 0 && f->buf_index > 0) {
-        fprintf(stderr,
-                "Attempted to write to buffer while read buffer is not empty\n");
-        abort();
-    }
-
     f->bytes_xfer += size;
     add_to_iovec(f, buf, size);
 }
@@ -664,12 +654,6 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
         return;
     }
 
-    if (f->is_write == 0 && f->buf_index > 0) {
-        fprintf(stderr,
-                "Attempted to write to buffer while read buffer is not empty\n");
-        abort();
-    }
-
     while (size > 0) {
         l = IO_BUF_SIZE - f->buf_index;
         if (l > size)
@@ -680,7 +664,6 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
             add_to_iovec(f, f->buf + f->buf_index, l);
             f->buf_index += l;
         } else {
-            f->is_write = 1;
             f->buf_index += l;
             if (f->buf_index == IO_BUF_SIZE) {
                 qemu_fflush(f);
@@ -700,19 +683,12 @@ void qemu_put_byte(QEMUFile *f, int v)
         return;
     }
 
-    if (f->is_write == 0 && f->buf_index > 0) {
-        fprintf(stderr,
-                "Attempted to write to buffer while read buffer is not empty\n");
-        abort();
-    }
-
     f->buf[f->buf_index] = v;
     f->bytes_xfer++;
     if (f->ops->writev_buffer) {
         add_to_iovec(f, f->buf + f->buf_index, 1);
         f->buf_index++;
     } else {
-        f->is_write = 1;
         f->buf_index++;
         if (f->buf_index == IO_BUF_SIZE) {
             qemu_fflush(f);
@@ -732,9 +708,7 @@ static int qemu_peek_buffer(QEMUFile *f, uint8_t *buf, int size, size_t offset)
     int pending;
     int index;
 
-    if (f->is_write) {
-        abort();
-    }
+    assert(!qemu_file_is_writable(f));
 
     index = f->buf_index + offset;
     pending = f->buf_size - index;
@@ -779,9 +753,7 @@ static int qemu_peek_byte(QEMUFile *f, int offset)
 {
     int index = f->buf_index + offset;
 
-    if (f->is_write) {
-        abort();
-    }
+    assert(!qemu_file_is_writable(f));
 
     if (index >= f->buf_size) {
         qemu_fill_buffer(f);
-- 
1.8.2

[Qemu-devel] [PATCH 4/4] migration: simplify writev vs. non-writev logic

[Paolo Bonzini]

Check f->iovcnt in add_to_iovec, f->buf_index in qemu_put_buffer/byte.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 savevm.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/savevm.c b/savevm.c
index a2f6bc0..63f4c82 100644
--- a/savevm.c
+++ b/savevm.c
@@ -626,7 +626,7 @@ static void add_to_iovec(QEMUFile *f, const uint8_t *buf, int size)
         f->iov[f->iovcnt++].iov_len = size;
     }
 
-    if (f->buf_index >= IO_BUF_SIZE || f->iovcnt >= MAX_IOV_SIZE) {
+    if (f->iovcnt >= MAX_IOV_SIZE) {
         qemu_fflush(f);
     }
 }
@@ -662,12 +662,10 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
         f->bytes_xfer += size;
         if (f->ops->writev_buffer) {
             add_to_iovec(f, f->buf + f->buf_index, l);
-            f->buf_index += l;
-        } else {
-            f->buf_index += l;
-            if (f->buf_index == IO_BUF_SIZE) {
-                qemu_fflush(f);
-            }
+        }
+        f->buf_index += l;
+        if (f->buf_index == IO_BUF_SIZE) {
+            qemu_fflush(f);
         }
         if (qemu_file_get_error(f)) {
             break;
@@ -687,12 +685,10 @@ void qemu_put_byte(QEMUFile *f, int v)
     f->bytes_xfer++;
     if (f->ops->writev_buffer) {
         add_to_iovec(f, f->buf + f->buf_index, 1);
-        f->buf_index++;
-    } else {
-        f->buf_index++;
-        if (f->buf_index == IO_BUF_SIZE) {
-            qemu_fflush(f);
-        }
+    }
+    f->buf_index++;
+    if (f->buf_index == IO_BUF_SIZE) {
+        qemu_fflush(f);
     }
 }
 
-- 
1.8.2

Re: [Qemu-devel] [PATCH 1/4] migration: set f->is_write and flush in add_to_iovec

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  savevm.c | 25 +++++++++----------------
>  1 file changed, 9 insertions(+), 16 deletions(-)
>
> diff --git a/savevm.c b/savevm.c
> index b1d8988..c952c41 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -631,6 +631,11 @@ static void add_to_iovec(QEMUFile *f, const uint8_t *buf, int size)
>          f->iov[f->iovcnt].iov_base = (uint8_t *)buf;
>          f->iov[f->iovcnt++].iov_len = size;
>      }
> +
> +    f->is_write = 1;
> +    if (f->buf_index >= IO_BUF_SIZE || f->iovcnt >= MAX_IOV_SIZE) {
> +        qemu_fflush(f);
> +    }
>  }
>  
>  void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, int size)
> @@ -645,14 +650,8 @@ void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, int size)
>          abort();
>      }
>  
> -    add_to_iovec(f, buf, size);
> -
> -    f->is_write = 1;
>      f->bytes_xfer += size;
> -
> -    if (f->buf_index >= IO_BUF_SIZE || f->iovcnt >= MAX_IOV_SIZE) {
> -        qemu_fflush(f);
> -    }
> +    add_to_iovec(f, buf, size);
>  }
>  
>  void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
> @@ -674,7 +673,6 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
>          if (l > size)
>              l = size;
>          memcpy(f->buf + f->buf_index, buf, l);
> -        f->is_write = 1;
>          f->buf_index += l;

we increase buf_index

>          qemu_put_buffer_async(f, f->buf + (f->buf_index - l), l);

and we call add_to_iovec() here inside.  Notice the torture to get the
old buf_index value.

>          if (qemu_file_get_error(f)) {
> @@ -697,15 +695,10 @@ void qemu_put_byte(QEMUFile *f, int v)
>          abort();
>      }
>  
> -    f->buf[f->buf_index++] = v;
> -    f->is_write = 1;
> +    f->buf[f->buf_index] = v;
>      f->bytes_xfer++;
> -
> -    add_to_iovec(f, f->buf + (f->buf_index - 1), 1);
> -
> -    if (f->buf_index >= IO_BUF_SIZE || f->iovcnt >= MAX_IOV_SIZE) {
> -        qemu_fflush(f);
> -    }
> +    add_to_iovec(f, f->buf + f->buf_index, 1);
> +    f->buf_index++;

And here,  we call add_to_iovec() and then increase buf_index

Is there any good reason for not being consistent?

Once there,  I think that moving the handling of buf_index to inside
add_to_iovec() looks like a good idea?

Later,  Juan.

>  }
>  
>  static void qemu_file_skip(QEMUFile *f, int size)

Re: [Qemu-devel] [PATCH 2/4] migration: use a single I/O operation when writev_buffer is not defined

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> The recent patches to use vectored I/O for RAM migration caused a
> regression in savevm speed.  To restore previous performance,
> add data to the buffer in qemu_put_buffer_async whenever writev_buffer
> is not available in the QEMUFile.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  savevm.c | 49 ++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 34 insertions(+), 15 deletions(-)
>
> diff --git a/savevm.c b/savevm.c
> index c952c41..b32e0a7 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -525,27 +525,24 @@ static void qemu_file_set_error(QEMUFile *f, int ret)
>  static void qemu_fflush(QEMUFile *f)
>  {
>      ssize_t ret = 0;
> -    int i = 0;
>  
>      if (!f->ops->writev_buffer && !f->ops->put_buffer) {
>          return;
>      }
>  
> -    if (f->is_write && f->iovcnt > 0) {
> +    if (f->is_write) {
>          if (f->ops->writev_buffer) {
> -            ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt);
> -            if (ret >= 0) {
> -                f->pos += ret;

Code was there,  and it is a case of of style,  but adding 0 to one
ammount is a NOP.

> +            if (f->iovcnt > 0) {
> +                ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt);
>              }
>          } else {
> -            for (i = 0; i < f->iovcnt && ret >= 0; i++) {
> -                ret = f->ops->put_buffer(f->opaque, f->iov[i].iov_base, f->pos,
> -                                         f->iov[i].iov_len);
> -                if (ret >= 0) {
> -                    f->pos += ret;
> -                }
> +            if (f->buf_index > 0) {
> +                ret = f->ops->put_buffer(f->opaque, f->buf, f->pos, f->buf_index);
>              }
>          }
> +        if (ret >= 0) {
> +            f->pos += ret;
> +        }
>          f->buf_index = 0;
>          f->iovcnt = 0;
>      }
> @@ -640,6 +637,11 @@ static void add_to_iovec(QEMUFile *f, const uint8_t *buf, int size)
>  
>  void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, int size)
>  {
> +    if (!f->ops->writev_buffer) {
> +        qemu_put_buffer(f, buf, size);
> +        return;
> +    }
> +

This

>      if (f->last_error) {
>          return;
>      }
> @@ -673,8 +675,17 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
>          if (l > size)
>              l = size;
>          memcpy(f->buf + f->buf_index, buf, l);
> -        f->buf_index += l;
> -        qemu_put_buffer_async(f, f->buf + (f->buf_index - l), l);
> +        f->bytes_xfer += size;
> +        if (f->ops->writev_buffer) {
> +            add_to_iovec(f, f->buf + f->buf_index, l);
> +            f->buf_index += l;
> +        } else {
> +            f->is_write = 1;
> +            f->buf_index += l;
> +            if (f->buf_index == IO_BUF_SIZE) {
> +                qemu_fflush(f);
> +            }
> +        }

And this

>          if (qemu_file_get_error(f)) {
>              break;
>          }
> @@ -697,8 +708,16 @@ void qemu_put_byte(QEMUFile *f, int v)
>  
>      f->buf[f->buf_index] = v;
>      f->bytes_xfer++;
> -    add_to_iovec(f, f->buf + f->buf_index, 1);
> -    f->buf_index++;
> +    if (f->ops->writev_buffer) {
> +        add_to_iovec(f, f->buf + f->buf_index, 1);
> +        f->buf_index++;
> +    } else {
> +        f->is_write = 1;
> +        f->buf_index++;
> +        if (f->buf_index == IO_BUF_SIZE) {
> +            qemu_fflush(f);
> +        }
> +    }

And this



is not needed if we just put the code inside add_to_iovec() no?

Users of add_to_iovec() should not care if we are using vritev or
put_buffer() right.

and as a second note.  Having qemu_put_buffer_async() calling
qemu_put_buffer() and qemu_put_buffer() calling qemu_put_buffer_async()
is ..... weird?

>  }
>  
>  static void qemu_file_skip(QEMUFile *f, int size)

Re: [Qemu-devel] [PATCH 1/4] migration: set f->is_write and flush in add_to_iovec

[Paolo Bonzini]

Il 09/04/2013 13:32, Juan Quintela ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  savevm.c | 25 +++++++++----------------
>>  1 file changed, 9 insertions(+), 16 deletions(-)
>>
>> diff --git a/savevm.c b/savevm.c
>> index b1d8988..c952c41 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -631,6 +631,11 @@ static void add_to_iovec(QEMUFile *f, const uint8_t *buf, int size)
>>          f->iov[f->iovcnt].iov_base = (uint8_t *)buf;
>>          f->iov[f->iovcnt++].iov_len = size;
>>      }
>> +
>> +    f->is_write = 1;
>> +    if (f->buf_index >= IO_BUF_SIZE || f->iovcnt >= MAX_IOV_SIZE) {
>> +        qemu_fflush(f);
>> +    }
>>  }
>>  
>>  void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, int size)
>> @@ -645,14 +650,8 @@ void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, int size)
>>          abort();
>>      }
>>  
>> -    add_to_iovec(f, buf, size);
>> -
>> -    f->is_write = 1;
>>      f->bytes_xfer += size;
>> -
>> -    if (f->buf_index >= IO_BUF_SIZE || f->iovcnt >= MAX_IOV_SIZE) {
>> -        qemu_fflush(f);
>> -    }
>> +    add_to_iovec(f, buf, size);
>>  }
>>  
>>  void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
>> @@ -674,7 +673,6 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
>>          if (l > size)
>>              l = size;
>>          memcpy(f->buf + f->buf_index, buf, l);
>> -        f->is_write = 1;
>>          f->buf_index += l;
> 
> we increase buf_index
> 
>>          qemu_put_buffer_async(f, f->buf + (f->buf_index - l), l);
> 
> and we call add_to_iovec() here inside.  Notice the torture to get the
> old buf_index value.
> 
>>          if (qemu_file_get_error(f)) {
>> @@ -697,15 +695,10 @@ void qemu_put_byte(QEMUFile *f, int v)
>>          abort();
>>      }
>>  
>> -    f->buf[f->buf_index++] = v;
>> -    f->is_write = 1;
>> +    f->buf[f->buf_index] = v;
>>      f->bytes_xfer++;
>> -
>> -    add_to_iovec(f, f->buf + (f->buf_index - 1), 1);
>> -
>> -    if (f->buf_index >= IO_BUF_SIZE || f->iovcnt >= MAX_IOV_SIZE) {
>> -        qemu_fflush(f);
>> -    }
>> +    add_to_iovec(f, f->buf + f->buf_index, 1);
>> +    f->buf_index++;
> 
> And here,  we call add_to_iovec() and then increase buf_index
> 
> Is there any good reason for not being consistent?

The reason is that I didn't want to switch
qemu_put_buffer_async->add_to_iovec in this patch.  I do it in the next one.

> Once there,  I think that moving the handling of buf_index to inside
> add_to_iovec() looks like a good idea?

add_to_iovec() is not called always with something from f->buf.  But it
is a good idea to move this handling of buf_index:

    if (f->buf_index >= IO_BUF_SIZE || f->iovcnt >= MAX_IOV_SIZE) {

out of add_to_iovec.  I do that in patch 4.

Paolo

Re: [Qemu-devel] [PATCH 3/4] migration: drop is_write complications

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> The same QEMUFile is never used for both read and write.  Simplify
> the logic to simply look for presence or absence of the right ops.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Love this one.

> @@ -556,11 +557,7 @@ static void qemu_fill_buffer(QEMUFile *f)
>      int len;
>      int pending;
>  
> -    if (!f->ops->get_buffer)
> -        return;

Why are we removing this test?  this has nothing to do with the is_write
removal?
And yes,  having a better way of knowing that the operations are there
looks like a good idea,  but that is independent of this series.


Thanks,  Juan.

Re: [Qemu-devel] [PATCH 2/4] migration: use a single I/O operation when writev_buffer is not defined

[Paolo Bonzini]

Il 09/04/2013 13:38, Juan Quintela ha scritto:
> And this
> 
> 
> 
> is not needed if we just put the code inside add_to_iovec() no?
> 
> Users of add_to_iovec() should not care if we are using vritev or
> put_buffer() right.

I'm taking the opposite stance here.  add_to_iovec() should not be
called at all if using put_buffer.

> and as a second note.  Having qemu_put_buffer_async() calling
> qemu_put_buffer() and qemu_put_buffer() calling qemu_put_buffer_async()
> is ..... weird?

Yes, in fact after this patch qemu_put_buffer() is not calling
qemu_put_buffer_async() anymore.

Paolo

Re: [Qemu-devel] [PATCH 4/4] migration: simplify writev vs. non-writev logic

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> Check f->iovcnt in add_to_iovec, f->buf_index in qemu_put_buffer/byte.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  savevm.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/savevm.c b/savevm.c
> index a2f6bc0..63f4c82 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -626,7 +626,7 @@ static void add_to_iovec(QEMUFile *f, const uint8_t *buf, int size)
>          f->iov[f->iovcnt++].iov_len = size;
>      }
>  
> -    if (f->buf_index >= IO_BUF_SIZE || f->iovcnt >= MAX_IOV_SIZE) {
> +    if (f->iovcnt >= MAX_IOV_SIZE) {
>          qemu_fflush(f);
>      }
>  }
> @@ -662,12 +662,10 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
>          f->bytes_xfer += size;
>          if (f->ops->writev_buffer) {
>              add_to_iovec(f, f->buf + f->buf_index, l);
> -            f->buf_index += l;
> -        } else {
> -            f->buf_index += l;
> -            if (f->buf_index == IO_BUF_SIZE) {
> -                qemu_fflush(f);
> -            }
> +        }
> +        f->buf_index += l;
> +        if (f->buf_index == IO_BUF_SIZE) {
> +            qemu_fflush(f);
>          }
>          if (qemu_file_get_error(f)) {
>              break;
> @@ -687,12 +685,10 @@ void qemu_put_byte(QEMUFile *f, int v)
>      f->bytes_xfer++;
>      if (f->ops->writev_buffer) {
>          add_to_iovec(f, f->buf + f->buf_index, 1);
> -        f->buf_index++;
> -    } else {
> -        f->buf_index++;
> -        if (f->buf_index == IO_BUF_SIZE) {
> -            qemu_fflush(f);
> -        }
> +    }
> +    f->buf_index++;
> +    if (f->buf_index == IO_BUF_SIZE) {
> +        qemu_fflush(f);
>      }
>  }

If you follow my advice of moving the call to add_to_iovec() you get
this one simplified and only one place to do this.

Thanks,  Juan.

Re: [Qemu-devel] [PATCH 4/4] migration: simplify writev vs. non-writev logic

[Paolo Bonzini]

Il 09/04/2013 13:43, Juan Quintela ha scritto:
>> > @@ -687,12 +685,10 @@ void qemu_put_byte(QEMUFile *f, int v)
>> >      f->bytes_xfer++;
>> >      if (f->ops->writev_buffer) {
>> >          add_to_iovec(f, f->buf + f->buf_index, 1);
>> > -        f->buf_index++;
>> > -    } else {
>> > -        f->buf_index++;
>> > -        if (f->buf_index == IO_BUF_SIZE) {
>> > -            qemu_fflush(f);
>> > -        }
>> > +    }
>> > +    f->buf_index++;
>> > +    if (f->buf_index == IO_BUF_SIZE) {
>> > +        qemu_fflush(f);
>> >      }
>> >  }
> If you follow my advice of moving the call to add_to_iovec() you get
> this one simplified and only one place to do this.

Moving what call?  The apparent complication is because the old logic
was a bit more involute than necessary.  If you look at the code after
the patches, not the patches themselves, you'll see for yourself.

The logic now is:

   add byte
   if using iovs
       add byte to iov list
   if buffer full
       flush

add_to_iovec has no business checking the buffer.  Why should
qemu_put_buffer_async() check the buffer?

The duplication between qemu_put_byte and qemu_put_buffer is a different
topic.  I think it's acceptable in the name of performance, but perhaps
you can just call qemu_put_buffer(f, &c, 1).

Paolo

Re: [Qemu-devel] [PATCH 3/4] migration: drop is_write complications

[Paolo Bonzini]

Il 09/04/2013 13:42, Juan Quintela ha scritto:
>> > @@ -556,11 +557,7 @@ static void qemu_fill_buffer(QEMUFile *f)
>> >      int len;
>> >      int pending;
>> >  
>> > -    if (!f->ops->get_buffer)
>> > -        return;
> Why are we removing this test?  this has nothing to do with the is_write
> removal?

This test assumes that it makes sense to call qemu_get_byte on a
write-opened QEMUFile.  This is not true anymore after this patch.
After eliminating is_write, the right thing to do is abort.

Paolo

> And yes,  having a better way of knowing that the operations are there
> looks like a good idea,  but that is independent of this series.

Re: [Qemu-devel] [PATCH 4/4] migration: simplify writev vs. non-writev logic

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 09/04/2013 13:43, Juan Quintela ha scritto:
>>> > @@ -687,12 +685,10 @@ void qemu_put_byte(QEMUFile *f, int v)
>>> >      f->bytes_xfer++;
>>> >      if (f->ops->writev_buffer) {
>>> >          add_to_iovec(f, f->buf + f->buf_index, 1);
>>> > -        f->buf_index++;
>>> > -    } else {
>>> > -        f->buf_index++;
>>> > -        if (f->buf_index == IO_BUF_SIZE) {
>>> > -            qemu_fflush(f);
>>> > -        }
>>> > +    }
>>> > +    f->buf_index++;
>>> > +    if (f->buf_index == IO_BUF_SIZE) {
>>> > +        qemu_fflush(f);
>>> >      }
>>> >  }
>> If you follow my advice of moving the call to add_to_iovec() you get
>> this one simplified and only one place to do this.
>
> Moving what call?  The apparent complication is because the old logic
> was a bit more involute than necessary.  If you look at the code after
> the patches, not the patches themselves, you'll see for yourself.
>
> The logic now is:
>
>    add byte
>    if using iovs
>        add byte to iov list
>    if buffer full
>        flush
>
> add_to_iovec has no business checking the buffer.  Why should
> qemu_put_buffer_async() check the buffer?

We can rename the function.  I agree with that.

>
> The duplication between qemu_put_byte and qemu_put_buffer is a different
> topic.  I think it's acceptable in the name of performance, but perhaps
> you can just call qemu_put_buffer(f, &c, 1).

Right now,  it is important because all the integer types are sent as
callas to qemu_put_byte(),  but Orit had a patch to rewrote those using
qemu_put_buffer() dirrectly,  and then we don't use so many
qemu_put_byte()'s anymore.

Later,  Juan.

Re: [Qemu-devel] [PATCH 3/4] migration: drop is_write complications

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 09/04/2013 13:42, Juan Quintela ha scritto:
>>> > @@ -556,11 +557,7 @@ static void qemu_fill_buffer(QEMUFile *f)
>>> >      int len;
>>> >      int pending;
>>> >  
>>> > -    if (!f->ops->get_buffer)
>>> > -        return;
>> Why are we removing this test?  this has nothing to do with the is_write
>> removal?
>
> This test assumes that it makes sense to call qemu_get_byte on a
> write-opened QEMUFile.  This is not true anymore after this patch.
> After eliminating is_write, the right thing to do is abort.


But this would not abort,  it would do a segmenation fault!

I would not complain to a:

assert(!f->ops->get_buffer);

It would told us from where we got the "invalid" call,  but this removal
will change a "silent fail" (that I don't like either) to a segmentation
fault (that is even worse).

Later,  Juan.

Re: [Qemu-devel] [PATCH 4/4] migration: simplify writev vs. non-writev logic

[Orit Wasserman]

On 04/09/2013 02:53 PM, Paolo Bonzini wrote:
> Il 09/04/2013 13:43, Juan Quintela ha scritto:
>>>> @@ -687,12 +685,10 @@ void qemu_put_byte(QEMUFile *f, int v)
>>>>      f->bytes_xfer++;
>>>>      if (f->ops->writev_buffer) {
>>>>          add_to_iovec(f, f->buf + f->buf_index, 1);
>>>> -        f->buf_index++;
>>>> -    } else {
>>>> -        f->buf_index++;
>>>> -        if (f->buf_index == IO_BUF_SIZE) {
>>>> -            qemu_fflush(f);
>>>> -        }
>>>> +    }
>>>> +    f->buf_index++;
>>>> +    if (f->buf_index == IO_BUF_SIZE) {
>>>> +        qemu_fflush(f);
>>>>      }
>>>>  }
>> If you follow my advice of moving the call to add_to_iovec() you get
>> this one simplified and only one place to do this.
> 
> Moving what call?  The apparent complication is because the old logic
> was a bit more involute than necessary.  If you look at the code after
> the patches, not the patches themselves, you'll see for yourself.
> 
> The logic now is:
> 
>    add byte
>    if using iovs
>        add byte to iov list
>    if buffer full
>        flush
> 
> add_to_iovec has no business checking the buffer.  Why should
> qemu_put_buffer_async() check the buffer?
> 
> The duplication between qemu_put_byte and qemu_put_buffer is a different
> topic.  I think it's acceptable in the name of performance, but perhaps
> you can just call qemu_put_buffer(f, &c, 1).
I thought about it too, we can keep the optimization by checking the size

Orit
> 
> Paolo
> 

Re: [Qemu-devel] [PATCH 3/4] migration: drop is_write complications

[Paolo Bonzini]

Il 09/04/2013 14:17, Juan Quintela ha scritto:
>> > This test assumes that it makes sense to call qemu_get_byte on a
>> > write-opened QEMUFile.  This is not true anymore after this patch.
>> > After eliminating is_write, the right thing to do is abort.
> 
> But this would not abort,  it would do a segmenation fault!
> 
> I would not complain to a:
> 
> assert(!f->ops->get_buffer);
> 
> It would told us from where we got the "invalid" call,  but this removal
> will change a "silent fail" (that I don't like either) to a segmentation
> fault (that is even worse).

It would still assert if you open a file in the wrong mode:

-    if (!f->ops->get_buffer)
-        return;
-
-    if (f->is_write)
-        abort();
+    assert(!qemu_file_is_writable(f));

It would segfault if you declare the QEMUFileOps wrong (e.g. no *_buffer
operation), but that's a bug in the QEMUFile implementation rather than
the usage.  I think a segfault is acceptable for that.

Paolo

Re: [Qemu-devel] [PATCH 0/4] QEMUFile improvements and simplifications

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> This fixes Kevin's reported regression with savevm, and simplifies the
> QEMUFile code further.
>
> Patch 2 could be made a bit smaller at the expense of fixing the
> regression in the last patch only.  I prefer to fix the bug earlier.
>
> Tested with Autotest.

Reviewed-by: Juan Quintela <quintela@redhat.com>

There are code duplicated now,  but it was already there,  and the
patches simplify the rest of the code.

Re: [Qemu-devel] [PATCH 0/4] QEMUFile improvements and simplifications

[Liuji (Jeremy)]

Hi, Paolo

I tested your 4 patches in the latest version of qemu.git/master(commit:
93b48c201eb6c0404d15550a0eaa3c0f7937e35e,2013-04-09).
These patches resolve the "savevm hanging" problem, which is detailedly described
in my preceding mail:"After executing "savevm", the QEMU process is hanging".

But, I found two other problem:
1、My VM's OS is winxp. After the execution of "savevm" is completed, I exec "loadvm".
But the winxp change to "blue screen", and then restart. I tested 3 times, but the results are same.

2、The block migration is not OK. The qemu-system-x86_64 process of source host is core-dump.
In the latest version of qemu.git/master(commit:93b48c201eb6c0404d15550a0eaa3c0f7937e35e,2013-04-09), 
the block migration is OK.


The info of core-dump file:
#0  0x00007f8a44cec341 in migration_thread (opaque=0x7f8a45259bc0) at migration.c:545
545	            double bandwidth = transferred_bytes / time_spent;
(gdb) bt
#0  0x00007f8a44cec341 in migration_thread (opaque=0x7f8a45259bc0) at migration.c:545
#1  0x00007f8a42fb7d14 in ?? ()
#2  0x0000000000000000 in ?? ()



Best Regards,

Jeremy Liu


> This fixes Kevin's reported regression with savevm, and simplifies the
> QEMUFile code further.
> 
> Patch 2 could be made a bit smaller at the expense of fixing the
> regression in the last patch only.  I prefer to fix the bug earlier.
> 
> Tested with Autotest.
> 
> Paolo Bonzini (4):
>   migration: set f->is_write and flush in add_to_iovec
>   migration: use a single I/O operation when writev_buffer is not defined
>   migration: drop is_write complications
>   migration: simplify writev vs. non-writev logic
> 
>  savevm.c | 104 ++++++++++++++++++++++++++-------------------------------------
>  1 file changed, 42 insertions(+), 62 deletions(-)
> 
> --
> 1.8.2
> 

Re: [Qemu-devel] [PATCH 0/4] QEMUFile improvements and simplifications

[Paolo Bonzini]

Il 10/04/2013 14:48, Liuji (Jeremy) ha scritto:
> Hi, Paolo
> 
> I tested your 4 patches in the latest version of qemu.git/master(commit:
> 93b48c201eb6c0404d15550a0eaa3c0f7937e35e,2013-04-09).
> These patches resolve the "savevm hanging" problem, which is detailedly described
> in my preceding mail:"After executing "savevm", the QEMU process is hanging".
> 
> But, I found two other problem:
> 1、My VM's OS is winxp. After the execution of "savevm" is completed, I exec "loadvm".
> But the winxp change to "blue screen", and then restart. I tested 3 times, but the results are same.

Does it work with commit 5cc11c46cf187c7d5306b68e730ec0d372cd7ef0?

> 2、The block migration is not OK. The qemu-system-x86_64 process of source host is core-dump.
> In the latest version of qemu.git/master(commit:93b48c201eb6c0404d15550a0eaa3c0f7937e35e,2013-04-09), 
> the block migration is OK.

This is a simple division by zero.  Juan, can you look at it?

Paolo

> 
> The info of core-dump file:
> #0  0x00007f8a44cec341 in migration_thread (opaque=0x7f8a45259bc0) at migration.c:545
> 545	            double bandwidth = transferred_bytes / time_spent;
> (gdb) bt
> #0  0x00007f8a44cec341 in migration_thread (opaque=0x7f8a45259bc0) at migration.c:545
> #1  0x00007f8a42fb7d14 in ?? ()
> #2  0x0000000000000000 in ?? ()
> 
> 
> 
> Best Regards,
> 
> Jeremy Liu
> 
> 
>> This fixes Kevin's reported regression with savevm, and simplifies the
>> QEMUFile code further.
>>
>> Patch 2 could be made a bit smaller at the expense of fixing the
>> regression in the last patch only.  I prefer to fix the bug earlier.
>>
>> Tested with Autotest.
>>
>> Paolo Bonzini (4):
>>   migration: set f->is_write and flush in add_to_iovec
>>   migration: use a single I/O operation when writev_buffer is not defined
>>   migration: drop is_write complications
>>   migration: simplify writev vs. non-writev logic
>>
>>  savevm.c | 104 ++++++++++++++++++++++++++-------------------------------------
>>  1 file changed, 42 insertions(+), 62 deletions(-)
>>
>> --
>> 1.8.2
>>
> 

Re: [Qemu-devel] [PATCH 0/4] QEMUFile improvements and simplifications

[Juan Quintela]

"Liuji (Jeremy)" <jeremy.liu@huawei.com> wrote:
> Hi, Paolo
>
> I tested your 4 patches in the latest version of qemu.git/master(commit:
> 93b48c201eb6c0404d15550a0eaa3c0f7937e35e,2013-04-09).
> These patches resolve the "savevm hanging" problem, which is detailedly described
> in my preceding mail:"After executing "savevm", the QEMU process is hanging".
>
> But, I found two other problem:
> 1、My VM's OS is winxp. After the execution of "savevm" is completed,
> I exec "loadvm".
> But the winxp change to "blue screen", and then restart. I tested 3
> times, but the results are same.
>
> 2、The block migration is not OK. The qemu-system-x86_64 process of
> source host is core-dump.
> In the latest version of
> qemu.git/master(commit:93b48c201eb6c0404d15550a0eaa3c0f7937e35e,2013-04-09),
> the block migration is OK.
>
>
> The info of core-dump file:
> #0 0x00007f8a44cec341 in migration_thread (opaque=0x7f8a45259bc0) at
> migration.c:545
> 545	            double bandwidth = transferred_bytes / time_spent;
> (gdb) bt
> #0 0x00007f8a44cec341 in migration_thread (opaque=0x7f8a45259bc0) at
> migration.c:545
> #1  0x00007f8a42fb7d14 in ?? ()
> #2  0x0000000000000000 in ?? ()
>

Could you recompile with -g to see what is going on?
This really makes no sense :p  It looks like the source file and the
compiled version don't agree.

Paolo,  any clue?

/me re-reads: block-migration,  ok,  testing goes.

Later,  Juan.

Re: [Qemu-devel] [PATCH 0/4] QEMUFile improvements and simplifications

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 10/04/2013 14:48, Liuji (Jeremy) ha scritto:
>> Hi, Paolo
>> 
>> I tested your 4 patches in the latest version of qemu.git/master(commit:
>> 93b48c201eb6c0404d15550a0eaa3c0f7937e35e,2013-04-09).
>> These patches resolve the "savevm hanging" problem, which is
>> detailedly described
>> in my preceding mail:"After executing "savevm", the QEMU process is hanging".
>> 
>> But, I found two other problem:
>> 1、My VM's OS is winxp. After the execution of "savevm" is
>> completed, I exec "loadvm".
>> But the winxp change to "blue screen", and then restart. I tested 3
>> times, but the results are same.
>
> Does it work with commit 5cc11c46cf187c7d5306b68e730ec0d372cd7ef0?
>
>> 2、The block migration is not OK. The qemu-system-x86_64 process of
>> source host is core-dump.
>> In the latest version of
>> qemu.git/master(commit:93b48c201eb6c0404d15550a0eaa3c0f7937e35e,2013-04-09),
>> the block migration is OK.
>
> This is a simple division by zero.  Juan, can you look at it?


Oops,  will take a look.  Thanks for the tip.

Later,  Juan.

>
> Paolo
>
>> 
>> The info of core-dump file:
>> #0 0x00007f8a44cec341 in migration_thread (opaque=0x7f8a45259bc0) at
>> migration.c:545
>> 545	            double bandwidth = transferred_bytes / time_spent;
>> (gdb) bt
>> #0 0x00007f8a44cec341 in migration_thread (opaque=0x7f8a45259bc0) at
>> migration.c:545
>> #1  0x00007f8a42fb7d14 in ?? ()
>> #2  0x0000000000000000 in ?? ()
>> 
>> 
>> 
>> Best Regards,
>> 
>> Jeremy Liu
>> 
>> 
>>> This fixes Kevin's reported regression with savevm, and simplifies the
>>> QEMUFile code further.
>>>
>>> Patch 2 could be made a bit smaller at the expense of fixing the
>>> regression in the last patch only.  I prefer to fix the bug earlier.
>>>
>>> Tested with Autotest.
>>>
>>> Paolo Bonzini (4):
>>>   migration: set f->is_write and flush in add_to_iovec
>>>   migration: use a single I/O operation when writev_buffer is not defined
>>>   migration: drop is_write complications
>>>   migration: simplify writev vs. non-writev logic
>>>
>>>  savevm.c | 104 ++++++++++++++++++++++++++-------------------------------------
>>>  1 file changed, 42 insertions(+), 62 deletions(-)
>>>
>>> --
>>> 1.8.2
>>>
>> 

Re: [Qemu-devel] [PATCH 0/4] QEMUFile improvements and simplifications

[Liuji (Jeremy)]

> Il 10/04/2013 14:48, Liuji (Jeremy) ha scritto:
> > Hi, Paolo
> >
> > I tested your 4 patches in the latest version of qemu.git/master(commit:
> > 93b48c201eb6c0404d15550a0eaa3c0f7937e35e,2013-04-09).
> > These patches resolve the "savevm hanging" problem, which is detailedly
> described
> > in my preceding mail:"After executing "savevm", the QEMU process is
> hanging".
> >
> > But, I found two other problem:
> > 1、My VM's OS is winxp. After the execution of "savevm" is completed, I exec
> "loadvm".
> > But the winxp change to "blue screen", and then restart. I tested 3 times, but
> the results are same.
> 
> Does it work with commit 5cc11c46cf187c7d5306b68e730ec0d372cd7ef0?

Hi, Paolo

Thanks for your reply.

In the commit 5cc11c46cf187c7d5306b68e730ec0d372cd7ef0, no "savevm hanging" 
problem, but also have "blue screen" problem.

> 
> > 2、The block migration is not OK. The qemu-system-x86_64 process of source
> host is core-dump.
> > In the latest version of
> qemu.git/master(commit:93b48c201eb6c0404d15550a0eaa3c0f7937e35e,201
> 3-04-09),
> > the block migration is OK.
> 
> This is a simple division by zero.  Juan, can you look at it?
> 
> Paolo
> 
> >
> > The info of core-dump file:
> > #0  0x00007f8a44cec341 in migration_thread (opaque=0x7f8a45259bc0) at
> migration.c:545
> > 545	            double bandwidth = transferred_bytes / time_spent;
> > (gdb) bt
> > #0  0x00007f8a44cec341 in migration_thread (opaque=0x7f8a45259bc0) at
> migration.c:545
> > #1  0x00007f8a42fb7d14 in ?? ()
> > #2  0x0000000000000000 in ?? ()
> >
> >
> >
> > Best Regards,
> >
> > Jeremy Liu
> >
> >
> >> This fixes Kevin's reported regression with savevm, and simplifies the
> >> QEMUFile code further.
> >>
> >> Patch 2 could be made a bit smaller at the expense of fixing the
> >> regression in the last patch only.  I prefer to fix the bug earlier.
> >>
> >> Tested with Autotest.
> >>
> >> Paolo Bonzini (4):
> >>   migration: set f->is_write and flush in add_to_iovec
> >>   migration: use a single I/O operation when writev_buffer is not defined
> >>   migration: drop is_write complications
> >>   migration: simplify writev vs. non-writev logic
> >>
> >>  savevm.c | 104 ++++++++++++++++++++++++++-------------------------------------
> >>  1 file changed, 42 insertions(+), 62 deletions(-)
> >>
> >> --
> >> 1.8.2
> >>
> >

Re: [Qemu-devel] [PATCH 0/4] QEMUFile improvements and simplifications

[Liuji (Jeremy)]

Hi, Juan

Thanks for your reply.

Yesterday, my disk has no space. So, the core-dump file not saved completely.
The info of core-dump file is:
#0  0x00007f7a0dbff341 in migration_thread (opaque=0x7f7a0e16cbc0) at migration.c:545
545	            double bandwidth = transferred_bytes / time_spent;
(gdb) bt
#0  0x00007f7a0dbff341 in migration_thread (opaque=0x7f7a0e16cbc0) at migration.c:545
#1  0x00007f7a0becad14 in start_thread (arg=0x7f7957fff700) at pthread_create.c:309
#2  0x00007f7a07cf267d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:115
(gdb) l
540	        }
541	        current_time = qemu_get_clock_ms(rt_clock);
542	        if (current_time >= initial_time + BUFFER_DELAY) {
543	            uint64_t transferred_bytes = qemu_ftell(s->file) - initial_bytes;
544	            uint64_t time_spent = current_time - initial_time - sleep_time;
545	            double bandwidth = transferred_bytes / time_spent;
546	            max_size = bandwidth * migrate_max_downtime() / 1000000;
547	
548	            DPRINTF("transferred %" PRIu64 " time_spent %" PRIu64
549	                    " bandwidth %g max_size %" PRId64 "\n",
(gdb) p time_spent
$1 = 0
(gdb) p current_time
$2 = 23945934
(gdb) p initial_time
$3 = 23945833
(gdb) p sleep_time
$4 = 101
(gdb) p s->file->last_error
$5 = 0

I tested three times. And the value of sleep_time are: 101,100,101

I think that the transfer may be so fast(use a very little time, the bytes_xfer > xfer_limit), 
and the "g_usleep" function may not be very accurate. So the value of sleep_time may be 
100(BUFFER_DELAY) or just a bit more than 100(BUFFER_DELAY).
I don't know whether my understanding is correct?

Below is my simple patch for evade this problem. Is that correct?
But I don't know why using your patch may trigger the problem.


diff --git a/migration.c b/migration.c
index 3b4b467..58d69fb 100644
--- a/migration.c
+++ b/migration.c
@@ -503,6 +503,7 @@ static void *migration_thread(void *opaque)
     int64_t max_size = 0;
     int64_t start_time = initial_time;
     bool old_vm_running = false;
+    double bandwidth = 0;
 
     DPRINTF("beginning savevm\n");
     qemu_savevm_state_begin(s->file, &s->params);
@@ -542,7 +543,13 @@ static void *migration_thread(void *opaque)
         if (current_time >= initial_time + BUFFER_DELAY) {
             uint64_t transferred_bytes = qemu_ftell(s->file) - initial_bytes;
             uint64_t time_spent = current_time - initial_time - sleep_time;
-            double bandwidth = transferred_bytes / time_spent;
+            if (time_spent > 0) {
+                bandwidth = transferred_bytes / time_spent;
+            }
+            else {
+                //when time_spent <= 0, don't change the value of bandwidth.
+                DPRINTF("time_spent=%" PRIu64 " is too small.\n",time_spent);
+            }
             max_size = bandwidth * migrate_max_downtime() / 1000000;
 
             DPRINTF("transferred %" PRIu64 " time_spent %" PRIu64
@@ -550,7 +557,7 @@ static void *migration_thread(void *opaque)
                     transferred_bytes, time_spent, bandwidth, max_size);
             /* if we haven't sent anything, we don't want to recalculate
                10000 is a small enough number for our purposes */
-            if (s->dirty_bytes_rate && transferred_bytes > 10000) {
+            if (s->dirty_bytes_rate && transferred_bytes > 10000 && bandwidth > 0) {
                 s->expected_downtime = s->dirty_bytes_rate / bandwidth;
             }



>  Re: [PATCH 0/4] QEMUFile improvements and simplifications
> 
> "Liuji (Jeremy)" <jeremy.liu@huawei.com> wrote:
> > Hi, Paolo
> >
> > I tested your 4 patches in the latest version of qemu.git/master(commit:
> > 93b48c201eb6c0404d15550a0eaa3c0f7937e35e,2013-04-09).
> > These patches resolve the "savevm hanging" problem, which is detailedly
> described
> > in my preceding mail:"After executing "savevm", the QEMU process is
> hanging".
> >
> > But, I found two other problem:
> > 1、My VM's OS is winxp. After the execution of "savevm" is completed,
> > I exec "loadvm".
> > But the winxp change to "blue screen", and then restart. I tested 3
> > times, but the results are same.
> >
> > 2、The block migration is not OK. The qemu-system-x86_64 process of
> > source host is core-dump.
> > In the latest version of
> >
> qemu.git/master(commit:93b48c201eb6c0404d15550a0eaa3c0f7937e35e,201
> 3-04-09),
> > the block migration is OK.
> >
> >
> > The info of core-dump file:
> > #0 0x00007f8a44cec341 in migration_thread (opaque=0x7f8a45259bc0) at
> > migration.c:545
> > 545	            double bandwidth = transferred_bytes / time_spent;
> > (gdb) bt
> > #0 0x00007f8a44cec341 in migration_thread (opaque=0x7f8a45259bc0) at
> > migration.c:545
> > #1  0x00007f8a42fb7d14 in ?? ()
> > #2  0x0000000000000000 in ?? ()
> >
> 
> Could you recompile with -g to see what is going on?
> This really makes no sense :p  It looks like the source file and the
> compiled version don't agree.
> 
> Paolo,  any clue?
> 
> /me re-reads: block-migration,  ok,  testing goes.
> 
> Later,  Juan.