[Qemu-devel] [PULL 0/5] migration pull

[Qemu-devel] [PULL 0/5] migration pull

[Amit Shah]

The following changes since commit 8eb779e4223a18db9838a49ece1bc72cfdfb7761:

  Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging (2016-02-22 16:55:41 +0000)

are available in the git repository at:

  https://git.kernel.org/pub/scm/virt/qemu/amit/migration.git tags/migration-for-2.6-3

for you to fetch changes up to 612f0af57aa1e8d4e09d7f1a1c442e1d943cbf0c:

  cutils: add avx2 instruction optimization (2016-02-23 12:53:03 +0530)

----------------------------------------------------------------
Migration:
 - enable avx2 instructions when available
 - fix a qcow2 assert
 - minor code rearrangement

----------------------------------------------------------------


Denis V. Lunev (2):
  migration: move bdrv_invalidate_cache_all of of coroutine context
  migration: move bdrv_invalidate_cache_all of of coroutine context

Liang Li (2):
  configure: detect ifunc and avx2 attribute
  cutils: add avx2 instruction optimization

Wei Yang (1):
  migration: reorder code to make it symmetric

 configure             |  21 +++++++++
 include/qemu-common.h |   8 +---
 migration/migration.c |  89 ++++++++++++++++++++-----------------
 migration/savevm.c    |  32 ++++++++------
 util/cutils.c         | 118 ++++++++++++++++++++++++++++++++++++++++++++++++--
 5 files changed, 204 insertions(+), 64 deletions(-)

-- 
2.5.0

[Qemu-devel] [PULL 1/5] migration: move bdrv_invalidate_cache_all of of coroutine context

[Amit Shah]

From: "Denis V. Lunev" <den@openvz.org>

There is a possibility to hit an assert in qcow2_get_specific_info that
s->qcow_version is undefined. This happens when VM in starting from
suspended state, i.e. it processes incoming migration, and in the same
time 'info block' is called.

The problem is that qcow2_invalidate_cache() closes the image and
memset()s BDRVQcowState in the middle.

The patch moves processing of bdrv_invalidate_cache_all out of
coroutine context for standard migration to avoid that.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Amit Shah <amit.shah@redhat.com>
Message-Id: <1455259174-3384-2-git-send-email-den@openvz.org>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 migration/migration.c | 89 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 49 insertions(+), 40 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index a64cfcd..1f8535e 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -323,13 +323,59 @@ void qemu_start_incoming_migration(const char *uri, Error **errp)
     }
 }
 
+static void process_incoming_migration_bh(void *opaque)
+{
+    Error *local_err = NULL;
+    MigrationIncomingState *mis = opaque;
+
+    /* Make sure all file formats flush their mutable metadata */
+    bdrv_invalidate_cache_all(&local_err);
+    if (local_err) {
+        migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
+                          MIGRATION_STATUS_FAILED);
+        error_report_err(local_err);
+        migrate_decompress_threads_join();
+        exit(EXIT_FAILURE);
+    }
+
+    /*
+     * This must happen after all error conditions are dealt with and
+     * we're sure the VM is going to be running on this host.
+     */
+    qemu_announce_self();
+
+    /* If global state section was not received or we are in running
+       state, we need to obey autostart. Any other state is set with
+       runstate_set. */
+
+    if (!global_state_received() ||
+        global_state_get_runstate() == RUN_STATE_RUNNING) {
+        if (autostart) {
+            vm_start();
+        } else {
+            runstate_set(RUN_STATE_PAUSED);
+        }
+    } else {
+        runstate_set(global_state_get_runstate());
+    }
+    migrate_decompress_threads_join();
+    /*
+     * This must happen after any state changes since as soon as an external
+     * observer sees this event they might start to prod at the VM assuming
+     * it's ready to use.
+     */
+    migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
+                      MIGRATION_STATUS_COMPLETED);
+    migration_incoming_state_destroy();
+}
+
 static void process_incoming_migration_co(void *opaque)
 {
     QEMUFile *f = opaque;
-    Error *local_err = NULL;
     MigrationIncomingState *mis;
     PostcopyState ps;
     int ret;
+    QEMUBH *bh;
 
     mis = migration_incoming_state_new(f);
     postcopy_state_set(POSTCOPY_INCOMING_NONE);
@@ -369,45 +415,8 @@ static void process_incoming_migration_co(void *opaque)
         exit(EXIT_FAILURE);
     }
 
-    /* Make sure all file formats flush their mutable metadata */
-    bdrv_invalidate_cache_all(&local_err);
-    if (local_err) {
-        migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
-                          MIGRATION_STATUS_FAILED);
-        error_report_err(local_err);
-        migrate_decompress_threads_join();
-        exit(EXIT_FAILURE);
-    }
-
-    /*
-     * This must happen after all error conditions are dealt with and
-     * we're sure the VM is going to be running on this host.
-     */
-    qemu_announce_self();
-
-    /* If global state section was not received or we are in running
-       state, we need to obey autostart. Any other state is set with
-       runstate_set. */
-
-    if (!global_state_received() ||
-        global_state_get_runstate() == RUN_STATE_RUNNING) {
-        if (autostart) {
-            vm_start();
-        } else {
-            runstate_set(RUN_STATE_PAUSED);
-        }
-    } else {
-        runstate_set(global_state_get_runstate());
-    }
-    migrate_decompress_threads_join();
-    /*
-     * This must happen after any state changes since as soon as an external
-     * observer sees this event they might start to prod at the VM assuming
-     * it's ready to use.
-     */
-    migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
-                      MIGRATION_STATUS_COMPLETED);
-    migration_incoming_state_destroy();
+    bh = qemu_bh_new(process_incoming_migration_bh, mis);
+    qemu_bh_schedule(bh);
 }
 
 void process_incoming_migration(QEMUFile *f)
-- 
2.5.0

[Qemu-devel] [PULL 2/5] migration: move bdrv_invalidate_cache_all of of coroutine context

[Amit Shah]

From: "Denis V. Lunev" <den@openvz.org>

There is a possibility to hit an assert in qcow2_get_specific_info that
s->qcow_version is undefined. This happens when VM in starting from
suspended state, i.e. it processes incoming migration, and in the same
time 'info block' is called.

The problem is that qcow2_invalidate_cache() closes the image and
memset()s BDRVQcowState in the middle.

The patch moves processing of bdrv_invalidate_cache_all out of
coroutine context for postcopy migration to avoid that. This function
is called with the following stack:
  process_incoming_migration_co
  qemu_loadvm_state
  qemu_loadvm_state_main
  loadvm_process_command
  loadvm_postcopy_handle_run

Signed-off-by: Denis V. Lunev <den@openvz.org>
Tested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Amit Shah <amit.shah@redhat.com>
Message-Id: <1455259174-3384-3-git-send-email-den@openvz.org>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 migration/savevm.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 94f2894..8415fd9 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1496,18 +1496,10 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
     return 0;
 }
 
-/* After all discards we can start running and asking for pages */
-static int loadvm_postcopy_handle_run(MigrationIncomingState *mis)
+static void loadvm_postcopy_handle_run_bh(void *opaque)
 {
-    PostcopyState ps = postcopy_state_set(POSTCOPY_INCOMING_RUNNING);
     Error *local_err = NULL;
 
-    trace_loadvm_postcopy_handle_run();
-    if (ps != POSTCOPY_INCOMING_LISTENING) {
-        error_report("CMD_POSTCOPY_RUN in wrong postcopy state (%d)", ps);
-        return -1;
-    }
-
     /* TODO we should move all of this lot into postcopy_ram.c or a shared code
      * in migration.c
      */
@@ -1519,7 +1511,6 @@ static int loadvm_postcopy_handle_run(MigrationIncomingState *mis)
     bdrv_invalidate_cache_all(&local_err);
     if (local_err) {
         error_report_err(local_err);
-        return -1;
     }
 
     trace_loadvm_postcopy_handle_run_cpu_sync();
@@ -1534,6 +1525,22 @@ static int loadvm_postcopy_handle_run(MigrationIncomingState *mis)
         /* leave it paused and let management decide when to start the CPU */
         runstate_set(RUN_STATE_PAUSED);
     }
+}
+
+/* After all discards we can start running and asking for pages */
+static int loadvm_postcopy_handle_run(MigrationIncomingState *mis)
+{
+    PostcopyState ps = postcopy_state_set(POSTCOPY_INCOMING_RUNNING);
+    QEMUBH *bh;
+
+    trace_loadvm_postcopy_handle_run();
+    if (ps != POSTCOPY_INCOMING_LISTENING) {
+        error_report("CMD_POSTCOPY_RUN in wrong postcopy state (%d)", ps);
+        return -1;
+    }
+
+    bh = qemu_bh_new(loadvm_postcopy_handle_run_bh, NULL);
+    qemu_bh_schedule(bh);
 
     /* We need to finish reading the stream from the package
      * and also stop reading anything more from the stream that loaded the
-- 
2.5.0

[Qemu-devel] [PULL 3/5] migration: reorder code to make it symmetric

[Amit Shah]

From: Wei Yang <richard.weiyang@gmail.com>

In qemu_savevm_state_complete_precopy(), it iterates on each device to add
a json object and transfer related status to destination, while the order
of the last two steps could be refined.

Current order:

    json_start_object()
    	save_section_header()
    	vmstate_save()
    json_end_object()
    	save_section_footer()

After the change:

    json_start_object()
    	save_section_header()
    	vmstate_save()
    	save_section_footer()
    json_end_object()

This patch reorder the code to to make it symmetric. No functional change.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
Reviewed-by: Amit Shah <amit.shah@redhat.com>
Message-Id: <1454626230-16334-1-git-send-email-richard.weiyang@gmail.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 migration/savevm.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 8415fd9..60ab119 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1088,12 +1088,11 @@ void qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only)
         json_prop_int(vmdesc, "instance_id", se->instance_id);
 
         save_section_header(f, se, QEMU_VM_SECTION_FULL);
-
         vmstate_save(f, se, vmdesc);
-
-        json_end_object(vmdesc);
         trace_savevm_section_end(se->idstr, se->section_id, 0);
         save_section_footer(f, se);
+
+        json_end_object(vmdesc);
     }
 
     if (!in_postcopy) {
-- 
2.5.0

[Qemu-devel] [PULL 4/5] configure: detect ifunc and avx2 attribute

[Amit Shah]

From: Liang Li <liang.z.li@intel.com>

Detect if the compiler can support the ifun and avx2, if so, set
CONFIG_AVX2_OPT which will be used to turn on the avx2 instruction
optimization.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Liang Li <liang.z.li@intel.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Message-Id: <1453880034-25076-2-git-send-email-liang.z.li@intel.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 configure | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/configure b/configure
index 0aa249b..d508f10 100755
--- a/configure
+++ b/configure
@@ -279,6 +279,7 @@ smartcard=""
 libusb=""
 usb_redir=""
 opengl=""
+avx2_opt="no"
 zlib="yes"
 lzo=""
 snappy=""
@@ -1772,6 +1773,21 @@ EOF
 fi
 
 ##########################################
+# avx2 optimization requirement check
+
+cat > $TMPC << EOF
+static void bar(void) {}
+static void *bar_ifunc(void) {return (void*) bar;}
+static void foo(void) __attribute__((ifunc("bar_ifunc")));
+int main(void) { foo(); return 0; }
+EOF
+if compile_prog "-mavx2" "" ; then
+    if readelf --syms $TMPE |grep "IFUNC.*foo" >/dev/null 2>&1; then
+        avx2_opt="yes"
+    fi
+fi
+
+#########################################
 # zlib check
 
 if test "$zlib" != "no" ; then
@@ -4776,6 +4792,7 @@ echo "bzip2 support     $bzip2"
 echo "NUMA host support $numa"
 echo "tcmalloc support  $tcmalloc"
 echo "jemalloc support  $jemalloc"
+echo "avx2 optimization $avx2_opt"
 
 if test "$sdl_too_old" = "yes"; then
 echo "-> Your SDL version is too old - please upgrade to have SDL support"
@@ -5160,6 +5177,10 @@ if test "$opengl" = "yes" ; then
   echo "OPENGL_LIBS=$opengl_libs" >> $config_host_mak
 fi
 
+if test "$avx2_opt" = "yes" ; then
+  echo "CONFIG_AVX2_OPT=y" >> $config_host_mak
+fi
+
 if test "$lzo" = "yes" ; then
   echo "CONFIG_LZO=y" >> $config_host_mak
 fi
-- 
2.5.0

[Qemu-devel] [PULL 5/5] cutils: add avx2 instruction optimization

[Amit Shah]

From: Liang Li <liang.z.li@intel.com>

buffer_find_nonzero_offset() is a hot function during live migration.
Now it use SSE2 instructions for optimization. For platform supports
AVX2 instructions, use AVX2 instructions for optimization can help
to improve the performance of buffer_find_nonzero_offset() about 30%
comparing to SSE2.

Live migration can be faster with this optimization, the test result
shows that for an 8GiB RAM idle guest just boots, this patch can help
to shorten the total live migration time about 6%.

This patch use the ifunc mechanism to select the proper function when
running, for platform supports AVX2, execute the AVX2 instructions,
else, execute the original instructions.

Signed-off-by: Liang Li <liang.z.li@intel.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Suggested-by: Richard Henderson <rth@twiddle.net>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Message-Id: <1453880034-25076-3-git-send-email-liang.z.li@intel.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 include/qemu-common.h |   8 +---
 util/cutils.c         | 118 ++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 115 insertions(+), 11 deletions(-)

diff --git a/include/qemu-common.h b/include/qemu-common.h
index f557be7..d26b2f1 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -477,13 +477,7 @@ void qemu_hexdump(const char *buf, FILE *fp, const char *prefix, size_t size);
 #endif
 
 #define BUFFER_FIND_NONZERO_OFFSET_UNROLL_FACTOR 8
-static inline bool
-can_use_buffer_find_nonzero_offset(const void *buf, size_t len)
-{
-    return (len % (BUFFER_FIND_NONZERO_OFFSET_UNROLL_FACTOR
-                   * sizeof(VECTYPE)) == 0
-            && ((uintptr_t) buf) % sizeof(VECTYPE) == 0);
-}
+bool can_use_buffer_find_nonzero_offset(const void *buf, size_t len);
 size_t buffer_find_nonzero_offset(const void *buf, size_t len);
 
 /*
diff --git a/util/cutils.c b/util/cutils.c
index 59e1f70..4522ded 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -160,6 +160,14 @@ int qemu_fdatasync(int fd)
 #endif
 }
 
+static bool
+can_use_buffer_find_nonzero_offset_inner(const void *buf, size_t len)
+{
+    return (len % (BUFFER_FIND_NONZERO_OFFSET_UNROLL_FACTOR
+                   * sizeof(VECTYPE)) == 0
+            && ((uintptr_t) buf) % sizeof(VECTYPE) == 0);
+}
+
 /*
  * Searches for an area with non-zero content in a buffer
  *
@@ -168,8 +176,8 @@ int qemu_fdatasync(int fd)
  * and addr must be a multiple of sizeof(VECTYPE) due to
  * restriction of optimizations in this function.
  *
- * can_use_buffer_find_nonzero_offset() can be used to check
- * these requirements.
+ * can_use_buffer_find_nonzero_offset_inner() can be used to
+ * check these requirements.
  *
  * The return value is the offset of the non-zero area rounded
  * down to a multiple of sizeof(VECTYPE) for the first
@@ -180,13 +188,13 @@ int qemu_fdatasync(int fd)
  * If the buffer is all zero the return value is equal to len.
  */
 
-size_t buffer_find_nonzero_offset(const void *buf, size_t len)
+static size_t buffer_find_nonzero_offset_inner(const void *buf, size_t len)
 {
     const VECTYPE *p = buf;
     const VECTYPE zero = (VECTYPE){0};
     size_t i;
 
-    assert(can_use_buffer_find_nonzero_offset(buf, len));
+    assert(can_use_buffer_find_nonzero_offset_inner(buf, len));
 
     if (!len) {
         return 0;
@@ -215,6 +223,108 @@ size_t buffer_find_nonzero_offset(const void *buf, size_t len)
     return i * sizeof(VECTYPE);
 }
 
+#ifdef CONFIG_AVX2_OPT
+#pragma GCC push_options
+#pragma GCC target("avx2")
+#include <cpuid.h>
+#include <immintrin.h>
+
+#define AVX2_VECTYPE        __m256i
+#define AVX2_SPLAT(p)       _mm256_set1_epi8(*(p))
+#define AVX2_ALL_EQ(v1, v2) \
+    (_mm256_movemask_epi8(_mm256_cmpeq_epi8(v1, v2)) == 0xFFFFFFFF)
+#define AVX2_VEC_OR(v1, v2) (_mm256_or_si256(v1, v2))
+
+static bool
+can_use_buffer_find_nonzero_offset_avx2(const void *buf, size_t len)
+{
+    return (len % (BUFFER_FIND_NONZERO_OFFSET_UNROLL_FACTOR
+                   * sizeof(AVX2_VECTYPE)) == 0
+            && ((uintptr_t) buf) % sizeof(AVX2_VECTYPE) == 0);
+}
+
+static size_t buffer_find_nonzero_offset_avx2(const void *buf, size_t len)
+{
+    const AVX2_VECTYPE *p = buf;
+    const AVX2_VECTYPE zero = (AVX2_VECTYPE){0};
+    size_t i;
+
+    assert(can_use_buffer_find_nonzero_offset_avx2(buf, len));
+
+    if (!len) {
+        return 0;
+    }
+
+    for (i = 0; i < BUFFER_FIND_NONZERO_OFFSET_UNROLL_FACTOR; i++) {
+        if (!AVX2_ALL_EQ(p[i], zero)) {
+            return i * sizeof(AVX2_VECTYPE);
+        }
+    }
+
+    for (i = BUFFER_FIND_NONZERO_OFFSET_UNROLL_FACTOR;
+         i < len / sizeof(AVX2_VECTYPE);
+         i += BUFFER_FIND_NONZERO_OFFSET_UNROLL_FACTOR) {
+        AVX2_VECTYPE tmp0 = AVX2_VEC_OR(p[i + 0], p[i + 1]);
+        AVX2_VECTYPE tmp1 = AVX2_VEC_OR(p[i + 2], p[i + 3]);
+        AVX2_VECTYPE tmp2 = AVX2_VEC_OR(p[i + 4], p[i + 5]);
+        AVX2_VECTYPE tmp3 = AVX2_VEC_OR(p[i + 6], p[i + 7]);
+        AVX2_VECTYPE tmp01 = AVX2_VEC_OR(tmp0, tmp1);
+        AVX2_VECTYPE tmp23 = AVX2_VEC_OR(tmp2, tmp3);
+        if (!AVX2_ALL_EQ(AVX2_VEC_OR(tmp01, tmp23), zero)) {
+            break;
+        }
+    }
+
+    return i * sizeof(AVX2_VECTYPE);
+}
+
+static bool avx2_support(void)
+{
+    int a, b, c, d;
+
+    if (__get_cpuid_max(0, NULL) < 7) {
+        return false;
+    }
+
+    __cpuid_count(7, 0, a, b, c, d);
+
+    return b & bit_AVX2;
+}
+
+bool can_use_buffer_find_nonzero_offset(const void *buf, size_t len) \
+         __attribute__ ((ifunc("can_use_buffer_find_nonzero_offset_ifunc")));
+size_t buffer_find_nonzero_offset(const void *buf, size_t len) \
+         __attribute__ ((ifunc("buffer_find_nonzero_offset_ifunc")));
+
+static void *buffer_find_nonzero_offset_ifunc(void)
+{
+    typeof(buffer_find_nonzero_offset) *func = (avx2_support()) ?
+        buffer_find_nonzero_offset_avx2 : buffer_find_nonzero_offset_inner;
+
+    return func;
+}
+
+static void *can_use_buffer_find_nonzero_offset_ifunc(void)
+{
+    typeof(can_use_buffer_find_nonzero_offset) *func = (avx2_support()) ?
+        can_use_buffer_find_nonzero_offset_avx2 :
+        can_use_buffer_find_nonzero_offset_inner;
+
+    return func;
+}
+#pragma GCC pop_options
+#else
+bool can_use_buffer_find_nonzero_offset(const void *buf, size_t len)
+{
+    return can_use_buffer_find_nonzero_offset_inner(buf, len);
+}
+
+size_t buffer_find_nonzero_offset(const void *buf, size_t len)
+{
+    return buffer_find_nonzero_offset_inner(buf, len);
+}
+#endif
+
 /*
  * Checks if a buffer is all zeroes
  *
-- 
2.5.0

Re: [Qemu-devel] [PULL 0/5] migration pull

[Peter Maydell]

On 23 February 2016 at 07:30, Amit Shah <amit.shah@redhat.com> wrote:
> The following changes since commit 8eb779e4223a18db9838a49ece1bc72cfdfb7761:
>
>   Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging (2016-02-22 16:55:41 +0000)
>
> are available in the git repository at:
>
>   https://git.kernel.org/pub/scm/virt/qemu/amit/migration.git tags/migration-for-2.6-3
>
> for you to fetch changes up to 612f0af57aa1e8d4e09d7f1a1c442e1d943cbf0c:
>
>   cutils: add avx2 instruction optimization (2016-02-23 12:53:03 +0530)
>
> ----------------------------------------------------------------
> Migration:
>  - enable avx2 instructions when available
>  - fix a qcow2 assert
>  - minor code rearrangement

Hi. I'm afraid this doesn't compile for x86-64 Linux:

/home/petmay01/linaro/qemu-for-merges/util/cutils.c: In function
‘can_use_buffer_find_nonzero_offset_avx2’:
/home/petmay01/linaro/qemu-for-merges/util/cutils.c:242:29: error:
‘__m256i’ undeclared (first use in this function)
                    * sizeof(AVX2_VECTYPE)) == 0
                             ^
/home/petmay01/linaro/qemu-for-merges/util/cutils.c:242:29: note: each
undeclared identifier is reported only once for each function it
appears in
/home/petmay01/linaro/qemu-for-merges/util/cutils.c: In function
‘buffer_find_nonzero_offset_avx2’:
/home/petmay01/linaro/qemu-for-merges/util/cutils.c:248:5: error:
unknown type name ‘__m256i’
     const AVX2_VECTYPE *p = buf;
     ^
/home/petmay01/linaro/qemu-for-merges/util/cutils.c:249:5: error:
unknown type name ‘__m256i’
     const AVX2_VECTYPE zero = (AVX2_VECTYPE){0};
     ^
/home/petmay01/linaro/qemu-for-merges/util/cutils.c:249:27: error:
‘__m256i’ undeclared (first use in this function)
     const AVX2_VECTYPE zero = (AVX2_VECTYPE){0};
                           ^
/home/petmay01/linaro/qemu-for-merges/util/cutils.c:249:35: error:
expected ‘,’ or ‘;’ before ‘{’ token
     const AVX2_VECTYPE zero = (AVX2_VECTYPE){0};
                                   ^
/home/petmay01/linaro/qemu-for-merges/util/cutils.c:259:9: error:
implicit declaration of function ‘_mm256_movemask_epi8’
[-Werror=implicit-function-declaration]
         if (!AVX2_ALL_EQ(p[i], zero)) {
         ^
/home/petmay01/linaro/qemu-for-merges/util/cutils.c:259:9: error:
nested extern declaration of ‘_mm256_movemask_epi8’
[-Werror=nested-externs]
/home/petmay01/linaro/qemu-for-merges/util/cutils.c:259:9: error:
implicit declaration of function ‘_mm256_cmpeq_epi8’
[-Werror=implicit-function-declaration]
/home/petmay01/linaro/qemu-for-merges/util/cutils.c:259:9: error:
nested extern declaration of ‘_mm256_cmpeq_epi8’
[-Werror=nested-externs]
/home/petmay01/linaro/qemu-for-merges/util/cutils.c:267:17: error:
expected ‘;’ before ‘tmp0’
         AVX2_VECTYPE tmp0 = AVX2_VEC_OR(p[i + 0], p[i + 1]);
                 ^
/home/petmay01/linaro/qemu-for-merges/util/cutils.c:268:17: error:
expected ‘;’ before ‘tmp1’
         AVX2_VECTYPE tmp1 = AVX2_VEC_OR(p[i + 2], p[i + 3]);
                 ^
/home/petmay01/linaro/qemu-for-merges/util/cutils.c:269:17: error:
expected ‘;’ before ‘tmp2’
         AVX2_VECTYPE tmp2 = AVX2_VEC_OR(p[i + 4], p[i + 5]);
                 ^
/home/petmay01/linaro/qemu-for-merges/util/cutils.c:270:17: error:
expected ‘;’ before ‘tmp3’
         AVX2_VECTYPE tmp3 = AVX2_VEC_OR(p[i + 6], p[i + 7]);
                 ^
/home/petmay01/linaro/qemu-for-merges/util/cutils.c:271:17: error:
expected ‘;’ before ‘tmp01’
         AVX2_VECTYPE tmp01 = AVX2_VEC_OR(tmp0, tmp1);
                 ^
/home/petmay01/linaro/qemu-for-merges/util/cutils.c:272:17: error:
expected ‘;’ before ‘tmp23’
         AVX2_VECTYPE tmp23 = AVX2_VEC_OR(tmp2, tmp3);
                 ^
/home/petmay01/linaro/qemu-for-merges/util/cutils.c:273:9: error:
implicit declaration of function ‘_mm256_or_si256’
[-Werror=implicit-function-declaration]
         if (!AVX2_ALL_EQ(AVX2_VEC_OR(tmp01, tmp23), zero)) {
         ^
/home/petmay01/linaro/qemu-for-merges/util/cutils.c:273:9: error:
nested extern declaration of ‘_mm256_or_si256’
[-Werror=nested-externs]
/home/petmay01/linaro/qemu-for-merges/util/cutils.c:273:71: error:
‘tmp01’ undeclared (first use in this function)
         if (!AVX2_ALL_EQ(AVX2_VEC_OR(tmp01, tmp23), zero)) {
                                                                       ^
/home/petmay01/linaro/qemu-for-merges/util/cutils.c:273:78: error:
‘tmp23’ undeclared (first use in this function)
         if (!AVX2_ALL_EQ(AVX2_VEC_OR(tmp01, tmp23), zero)) {
                                                                              ^
/home/petmay01/linaro/qemu-for-merges/util/cutils.c: In function
‘can_use_buffer_find_nonzero_offset_avx2’:
/home/petmay01/linaro/qemu-for-merges/util/cutils.c:244:1: error:
control reaches end of non-void function [-Werror=return-type]
 }
 ^
/home/petmay01/linaro/qemu-for-merges/util/cutils.c: In function
‘buffer_find_nonzero_offset_avx2’:
/home/petmay01/linaro/qemu-for-merges/util/cutils.c:279:1: error:
control reaches end of non-void function [-Werror=return-type]
 }
 ^
cc1: all warnings being treated as errors

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/5] migration pull

[Amit Shah]

On (Tue) 23 Feb 2016 [09:09:46], Peter Maydell wrote:
> On 23 February 2016 at 07:30, Amit Shah <amit.shah@redhat.com> wrote:
> > The following changes since commit 8eb779e4223a18db9838a49ece1bc72cfdfb7761:
> >
> >   Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging (2016-02-22 16:55:41 +0000)
> >
> > are available in the git repository at:
> >
> >   https://git.kernel.org/pub/scm/virt/qemu/amit/migration.git tags/migration-for-2.6-3
> >
> > for you to fetch changes up to 612f0af57aa1e8d4e09d7f1a1c442e1d943cbf0c:
> >
> >   cutils: add avx2 instruction optimization (2016-02-23 12:53:03 +0530)
> >
> > ----------------------------------------------------------------
> > Migration:
> >  - enable avx2 instructions when available
> >  - fix a qcow2 assert
> >  - minor code rearrangement
> 
> Hi. I'm afraid this doesn't compile for x86-64 Linux:

Compiles for me, but adding Liang Li so he can respond.

> 
> /home/petmay01/linaro/qemu-for-merges/util/cutils.c: In function
> ‘can_use_buffer_find_nonzero_offset_avx2’:
> /home/petmay01/linaro/qemu-for-merges/util/cutils.c:242:29: error:
> ‘__m256i’ undeclared (first use in this function)
>                     * sizeof(AVX2_VECTYPE)) == 0
>                              ^
> /home/petmay01/linaro/qemu-for-merges/util/cutils.c:242:29: note: each
> undeclared identifier is reported only once for each function it
> appears in
> /home/petmay01/linaro/qemu-for-merges/util/cutils.c: In function
> ‘buffer_find_nonzero_offset_avx2’:
> /home/petmay01/linaro/qemu-for-merges/util/cutils.c:248:5: error:
> unknown type name ‘__m256i’
>      const AVX2_VECTYPE *p = buf;
>      ^
> /home/petmay01/linaro/qemu-for-merges/util/cutils.c:249:5: error:
> unknown type name ‘__m256i’
>      const AVX2_VECTYPE zero = (AVX2_VECTYPE){0};
>      ^
> /home/petmay01/linaro/qemu-for-merges/util/cutils.c:249:27: error:
> ‘__m256i’ undeclared (first use in this function)
>      const AVX2_VECTYPE zero = (AVX2_VECTYPE){0};
>                            ^
> /home/petmay01/linaro/qemu-for-merges/util/cutils.c:249:35: error:
> expected ‘,’ or ‘;’ before ‘{’ token
>      const AVX2_VECTYPE zero = (AVX2_VECTYPE){0};
>                                    ^
> /home/petmay01/linaro/qemu-for-merges/util/cutils.c:259:9: error:
> implicit declaration of function ‘_mm256_movemask_epi8’
> [-Werror=implicit-function-declaration]
>          if (!AVX2_ALL_EQ(p[i], zero)) {
>          ^
> /home/petmay01/linaro/qemu-for-merges/util/cutils.c:259:9: error:
> nested extern declaration of ‘_mm256_movemask_epi8’
> [-Werror=nested-externs]
> /home/petmay01/linaro/qemu-for-merges/util/cutils.c:259:9: error:
> implicit declaration of function ‘_mm256_cmpeq_epi8’
> [-Werror=implicit-function-declaration]
> /home/petmay01/linaro/qemu-for-merges/util/cutils.c:259:9: error:
> nested extern declaration of ‘_mm256_cmpeq_epi8’
> [-Werror=nested-externs]
> /home/petmay01/linaro/qemu-for-merges/util/cutils.c:267:17: error:
> expected ‘;’ before ‘tmp0’
>          AVX2_VECTYPE tmp0 = AVX2_VEC_OR(p[i + 0], p[i + 1]);
>                  ^
> /home/petmay01/linaro/qemu-for-merges/util/cutils.c:268:17: error:
> expected ‘;’ before ‘tmp1’
>          AVX2_VECTYPE tmp1 = AVX2_VEC_OR(p[i + 2], p[i + 3]);
>                  ^
> /home/petmay01/linaro/qemu-for-merges/util/cutils.c:269:17: error:
> expected ‘;’ before ‘tmp2’
>          AVX2_VECTYPE tmp2 = AVX2_VEC_OR(p[i + 4], p[i + 5]);
>                  ^
> /home/petmay01/linaro/qemu-for-merges/util/cutils.c:270:17: error:
> expected ‘;’ before ‘tmp3’
>          AVX2_VECTYPE tmp3 = AVX2_VEC_OR(p[i + 6], p[i + 7]);
>                  ^
> /home/petmay01/linaro/qemu-for-merges/util/cutils.c:271:17: error:
> expected ‘;’ before ‘tmp01’
>          AVX2_VECTYPE tmp01 = AVX2_VEC_OR(tmp0, tmp1);
>                  ^
> /home/petmay01/linaro/qemu-for-merges/util/cutils.c:272:17: error:
> expected ‘;’ before ‘tmp23’
>          AVX2_VECTYPE tmp23 = AVX2_VEC_OR(tmp2, tmp3);
>                  ^
> /home/petmay01/linaro/qemu-for-merges/util/cutils.c:273:9: error:
> implicit declaration of function ‘_mm256_or_si256’
> [-Werror=implicit-function-declaration]
>          if (!AVX2_ALL_EQ(AVX2_VEC_OR(tmp01, tmp23), zero)) {
>          ^
> /home/petmay01/linaro/qemu-for-merges/util/cutils.c:273:9: error:
> nested extern declaration of ‘_mm256_or_si256’
> [-Werror=nested-externs]
> /home/petmay01/linaro/qemu-for-merges/util/cutils.c:273:71: error:
> ‘tmp01’ undeclared (first use in this function)
>          if (!AVX2_ALL_EQ(AVX2_VEC_OR(tmp01, tmp23), zero)) {
>                                                                        ^
> /home/petmay01/linaro/qemu-for-merges/util/cutils.c:273:78: error:
> ‘tmp23’ undeclared (first use in this function)
>          if (!AVX2_ALL_EQ(AVX2_VEC_OR(tmp01, tmp23), zero)) {
>                                                                               ^
> /home/petmay01/linaro/qemu-for-merges/util/cutils.c: In function
> ‘can_use_buffer_find_nonzero_offset_avx2’:
> /home/petmay01/linaro/qemu-for-merges/util/cutils.c:244:1: error:
> control reaches end of non-void function [-Werror=return-type]
>  }
>  ^
> /home/petmay01/linaro/qemu-for-merges/util/cutils.c: In function
> ‘buffer_find_nonzero_offset_avx2’:
> /home/petmay01/linaro/qemu-for-merges/util/cutils.c:279:1: error:
> control reaches end of non-void function [-Werror=return-type]
>  }
>  ^
> cc1: all warnings being treated as errors
> 
> thanks
> -- PMM

		Amit

Re: [Qemu-devel] [PULL 0/5] migration pull

[Paolo Bonzini]

On 23/02/2016 10:09, Peter Maydell wrote:
> Hi. I'm afraid this doesn't compile for x86-64 Linux:

What compiler is this, and does the following compile with no particular
extra options?

#pragma GCC target("avx2")
#include <immintrin.h>
__m256i foo;

Paolo

Re: [Qemu-devel] [PULL 0/5] migration pull

[Li, Liang Z]

> Cc: qemu list; Dr. David Alan Gilbert; Juan Quintela
> Subject: Re: [Qemu-devel] [PULL 0/5] migration pull
> 
> On 23 February 2016 at 07:30, Amit Shah <amit.shah@redhat.com> wrote:
> > The following changes since commit
> 8eb779e4223a18db9838a49ece1bc72cfdfb7761:
> >
> >   Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into
> > staging (2016-02-22 16:55:41 +0000)
> >
> > are available in the git repository at:
> >
> >   https://git.kernel.org/pub/scm/virt/qemu/amit/migration.git
> > tags/migration-for-2.6-3
> >
> > for you to fetch changes up to
> 612f0af57aa1e8d4e09d7f1a1c442e1d943cbf0c:
> >
> >   cutils: add avx2 instruction optimization (2016-02-23 12:53:03
> > +0530)
> >
> > ----------------------------------------------------------------
> > Migration:
> >  - enable avx2 instructions when available
> >  - fix a qcow2 assert
> >  - minor code rearrangement
> 
> Hi. I'm afraid this doesn't compile for x86-64 Linux:
> 

Hi PMM,

Sorry for that, could you tell me which version of GCC are you using? It's strange.

Liang

Re: [Qemu-devel] [PULL 0/5] migration pull

[Peter Maydell]

On 23 February 2016 at 09:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 23/02/2016 10:09, Peter Maydell wrote:
>> Hi. I'm afraid this doesn't compile for x86-64 Linux:
>
> What compiler is this, and does the following compile with no particular
> extra options?
>
> #pragma GCC target("avx2")
> #include <immintrin.h>
> __m256i foo;

This is stock gcc from Ubuntu trusty:
$ gcc --version
gcc (Ubuntu 4.8.4-2ubuntu1~14.04.1) 4.8.4

That code fragment you suggest compiles fine normally, but not if I
add -save-temps:

$ cat /tmp/zz9.c
#pragma GCC target("avx2")
#include <immintrin.h>
__m256i foo;
$ gcc -g -Wall -o /tmp/zz9.o -c /tmp/zz9.c
$ echo $?
0
$ gcc -g -Wall -o /tmp/zz9.o -c /tmp/zz9.c -save-temps
/tmp/zz9.c:4:1: error: unknown type name ‘__m256i’
 __m256i foo;
 ^
/tmp/zz9.c: In function ‘bar’:
/tmp/zz9.c:7:19: error: ‘__m256i’ undeclared (first use in this function)
     return sizeof(__m256i);
                   ^
/tmp/zz9.c:7:19: note: each undeclared identifier is reported only
once for each function it appears in
/tmp/zz9.c:8:1: warning: control reaches end of non-void function
[-Wreturn-type]
 }
 ^

This seems to be because -save-temps causes the #pragma not to
actually #define __AVX__.

This feels all pretty fragile to me and I think we should
probably avoid messing with the target pragma if we can.

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/5] migration pull

[Li, Liang Z]

> On 23 February 2016 at 09:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > On 23/02/2016 10:09, Peter Maydell wrote:
> >> Hi. I'm afraid this doesn't compile for x86-64 Linux:
> >
> > What compiler is this, and does the following compile with no
> > particular extra options?
> >
> > #pragma GCC target("avx2")
> > #include <immintrin.h>
> > __m256i foo;
> 
> This is stock gcc from Ubuntu trusty:
> $ gcc --version
> gcc (Ubuntu 4.8.4-2ubuntu1~14.04.1) 4.8.4
> 

The same version as I used for test, it compiles for me. What's lead to the different result?
The command I used for building:
# ./configure --target-list=x86_64-softmmu
# make -j4
# ./configure --target-list=x86_64-linux-user
#make -j4

Both success.

Liang

Re: [Qemu-devel] [PULL 0/5] migration pull

[Peter Maydell]

On 23 February 2016 at 10:43, Peter Maydell <peter.maydell@linaro.org> wrote:
> That code fragment you suggest compiles fine normally, but not if I
> add -save-temps:
>
> $ cat /tmp/zz9.c
> #pragma GCC target("avx2")
> #include <immintrin.h>
> __m256i foo;
> $ gcc -g -Wall -o /tmp/zz9.o -c /tmp/zz9.c
> $ echo $?
> 0
> $ gcc -g -Wall -o /tmp/zz9.o -c /tmp/zz9.c -save-temps
> /tmp/zz9.c:4:1: error: unknown type name ‘__m256i’
>  __m256i foo;
>  ^
> /tmp/zz9.c: In function ‘bar’:
> /tmp/zz9.c:7:19: error: ‘__m256i’ undeclared (first use in this function)
>      return sizeof(__m256i);
>                    ^
> /tmp/zz9.c:7:19: note: each undeclared identifier is reported only
> once for each function it appears in
> /tmp/zz9.c:8:1: warning: control reaches end of non-void function
> [-Wreturn-type]
>  }
>  ^
>
> This seems to be because -save-temps causes the #pragma not to
> actually #define __AVX__.

This is because -save-temps causes gcc to invoke the
preprocessor and the compiler as separate passes, and the
standalone preprocessor doesn't know that the target pragma
should result in a new #define, so the result is that the
immintrin.h doesn't pull in what it should.

This is also the reason why my build failed -- I use ccache,
which is another tool that results in the preprocessor being
done as a standalone pass rather than in the same pass as
compilation proper.

Arguably it's a gcc bug that the target pragma doesn't cause
the standalone preprocessor to define the same #defines that
you get if it's all in one pass, but regardless I don't think
we can break ccache builds, so you'll need to find a different
way to do this, I'm afraid.

(Also gcc's docs don't say anything about target pragmas
adding #defines so either the docs or the implementation are
wrong.)

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/5] migration pull

[Paolo Bonzini]

On 23/02/2016 12:25, Peter Maydell wrote:
> On 23 February 2016 at 10:43, Peter Maydell <peter.maydell@linaro.org> wrote:
>> That code fragment you suggest compiles fine normally, but not if I
>> add -save-temps:
>>
>> $ cat /tmp/zz9.c
>> #pragma GCC target("avx2")
>> #include <immintrin.h>
>> __m256i foo;
>> $ gcc -g -Wall -o /tmp/zz9.o -c /tmp/zz9.c
>> $ echo $?
>> 0
>> $ gcc -g -Wall -o /tmp/zz9.o -c /tmp/zz9.c -save-temps
>> /tmp/zz9.c:4:1: error: unknown type name ‘__m256i’
>>  __m256i foo;
>>  ^
>> /tmp/zz9.c: In function ‘bar’:
>> /tmp/zz9.c:7:19: error: ‘__m256i’ undeclared (first use in this function)
>>      return sizeof(__m256i);
>>                    ^
>> /tmp/zz9.c:7:19: note: each undeclared identifier is reported only
>> once for each function it appears in
>> /tmp/zz9.c:8:1: warning: control reaches end of non-void function
>> [-Wreturn-type]
>>  }
>>  ^
>>
>> This seems to be because -save-temps causes the #pragma not to
>> actually #define __AVX__.
> 
> This is because -save-temps causes gcc to invoke the
> preprocessor and the compiler as separate passes, and the
> standalone preprocessor doesn't know that the target pragma
> should result in a new #define, so the result is that the
> immintrin.h doesn't pull in what it should.
> 
> This is also the reason why my build failed -- I use ccache,
> which is another tool that results in the preprocessor being
> done as a standalone pass rather than in the same pass as
> compilation proper.
> 
> Arguably it's a gcc bug that the target pragma doesn't cause
> the standalone preprocessor to define the same #defines that
> you get if it's all in one pass, but regardless I don't think
> we can break ccache builds, so you'll need to find a different
> way to do this, I'm afraid.

It's a bug in the header file, it was fixed in 4.9.

https://gcc.gnu.org/ml/gcc-patches/2013-06/txtvBBiTsFs8g.txt

Amit or Liang, can you restrict the new optimization to GCC 4.9+?

Paolo

Re: [Qemu-devel] [PULL 0/5] migration pull

[Li, Liang Z]

> 
> 
> On 23/02/2016 12:25, Peter Maydell wrote:
> > On 23 February 2016 at 10:43, Peter Maydell <peter.maydell@linaro.org>
> wrote:
> >> That code fragment you suggest compiles fine normally, but not if I
> >> add -save-temps:
> >>
> >> $ cat /tmp/zz9.c
> >> #pragma GCC target("avx2")
> >> #include <immintrin.h>
> >> __m256i foo;
> >> $ gcc -g -Wall -o /tmp/zz9.o -c /tmp/zz9.c $ echo $?
> >> 0
> >> $ gcc -g -Wall -o /tmp/zz9.o -c /tmp/zz9.c -save-temps
> >> /tmp/zz9.c:4:1: error: unknown type name ‘__m256i’
> >>  __m256i foo;
> >>  ^
> >> /tmp/zz9.c: In function ‘bar’:
> >> /tmp/zz9.c:7:19: error: ‘__m256i’ undeclared (first use in this function)
> >>      return sizeof(__m256i);
> >>                    ^
> >> /tmp/zz9.c:7:19: note: each undeclared identifier is reported only
> >> once for each function it appears in
> >> /tmp/zz9.c:8:1: warning: control reaches end of non-void function
> >> [-Wreturn-type]  }  ^
> >>
> >> This seems to be because -save-temps causes the #pragma not to
> >> actually #define __AVX__.
> >
> > This is because -save-temps causes gcc to invoke the preprocessor and
> > the compiler as separate passes, and the standalone preprocessor
> > doesn't know that the target pragma should result in a new #define, so
> > the result is that the immintrin.h doesn't pull in what it should.
> >
> > This is also the reason why my build failed -- I use ccache, which is
> > another tool that results in the preprocessor being done as a
> > standalone pass rather than in the same pass as compilation proper.
> >
> > Arguably it's a gcc bug that the target pragma doesn't cause the
> > standalone preprocessor to define the same #defines that you get if
> > it's all in one pass, but regardless I don't think we can break ccache
> > builds, so you'll need to find a different way to do this, I'm afraid.
> 
> It's a bug in the header file, it was fixed in 4.9.
> 
> https://gcc.gnu.org/ml/gcc-patches/2013-06/txtvBBiTsFs8g.txt
> 
> Amit or Liang, can you restrict the new optimization to GCC 4.9+?
> 
> Paolo

Of course, how about using '#if QEMU_GNUC_PREREQ(4, 9) && defined CONFIG_AVX2_OPT' instead of '#ifdef CONFIG_AVX2_OPT'  in cutils.c?

Liang

Re: [Qemu-devel] [PULL 2/5] migration: move bdrv_invalidate_cache_all of of coroutine context

[Dr. David Alan Gilbert]

* Amit Shah (amit.shah@redhat.com) wrote:
> From: "Denis V. Lunev" <den@openvz.org>
> 
> There is a possibility to hit an assert in qcow2_get_specific_info that
> s->qcow_version is undefined. This happens when VM in starting from
> suspended state, i.e. it processes incoming migration, and in the same
> time 'info block' is called.
> 
> The problem is that qcow2_invalidate_cache() closes the image and
> memset()s BDRVQcowState in the middle.
> 
> The patch moves processing of bdrv_invalidate_cache_all out of
> coroutine context for postcopy migration to avoid that. This function
> is called with the following stack:
>   process_incoming_migration_co
>   qemu_loadvm_state
>   qemu_loadvm_state_main
>   loadvm_process_command
>   loadvm_postcopy_handle_run
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Tested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

hmm; actually - this segs in a variety of different ways;
there are two problems:

   a) +    bh = qemu_bh_new(loadvm_postcopy_handle_run_bh, NULL);
     That's the easy one; that NULL should be 'mis', because
     the bh is expecting to use it as a MigrationIncomingState
     so it segs fairly reliably in the qemu_bh_delete(mis->bh)

   b) The harder problem is that there's a race where qemu_bh_delete
      segs, and I'm not 100% sure why yet - it only does it sometime
      (i.e. run virt-test and leave it and it occasionally does it).
      From the core it looks like qemu->bh is corrupt (0x10101010...)
      so maybe mis has been freed at that point?
      I'm suspecting this is the postcopy_ram_listen_thread freeing
      mis at the end of it, but I don't know yet.

Dave

> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Amit Shah <amit.shah@redhat.com>
> Message-Id: <1455259174-3384-3-git-send-email-den@openvz.org>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  migration/savevm.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 94f2894..8415fd9 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1496,18 +1496,10 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
>      return 0;
>  }
>  
> -/* After all discards we can start running and asking for pages */
> -static int loadvm_postcopy_handle_run(MigrationIncomingState *mis)
> +static void loadvm_postcopy_handle_run_bh(void *opaque)
>  {
> -    PostcopyState ps = postcopy_state_set(POSTCOPY_INCOMING_RUNNING);
>      Error *local_err = NULL;
>  
> -    trace_loadvm_postcopy_handle_run();
> -    if (ps != POSTCOPY_INCOMING_LISTENING) {
> -        error_report("CMD_POSTCOPY_RUN in wrong postcopy state (%d)", ps);
> -        return -1;
> -    }
> -
>      /* TODO we should move all of this lot into postcopy_ram.c or a shared code
>       * in migration.c
>       */
> @@ -1519,7 +1511,6 @@ static int loadvm_postcopy_handle_run(MigrationIncomingState *mis)
>      bdrv_invalidate_cache_all(&local_err);
>      if (local_err) {
>          error_report_err(local_err);
> -        return -1;
>      }
>  
>      trace_loadvm_postcopy_handle_run_cpu_sync();
> @@ -1534,6 +1525,22 @@ static int loadvm_postcopy_handle_run(MigrationIncomingState *mis)
>          /* leave it paused and let management decide when to start the CPU */
>          runstate_set(RUN_STATE_PAUSED);
>      }
> +}
> +
> +/* After all discards we can start running and asking for pages */
> +static int loadvm_postcopy_handle_run(MigrationIncomingState *mis)
> +{
> +    PostcopyState ps = postcopy_state_set(POSTCOPY_INCOMING_RUNNING);
> +    QEMUBH *bh;
> +
> +    trace_loadvm_postcopy_handle_run();
> +    if (ps != POSTCOPY_INCOMING_LISTENING) {
> +        error_report("CMD_POSTCOPY_RUN in wrong postcopy state (%d)", ps);
> +        return -1;
> +    }
> +
> +    bh = qemu_bh_new(loadvm_postcopy_handle_run_bh, NULL);
> +    qemu_bh_schedule(bh);
>  
>      /* We need to finish reading the stream from the package
>       * and also stop reading anything more from the stream that loaded the
> -- 
> 2.5.0
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PULL 2/5] migration: move bdrv_invalidate_cache_all of of coroutine context

[Paolo Bonzini]

On 07/03/2016 13:49, Dr. David Alan Gilbert wrote:
>    b) The harder problem is that there's a race where qemu_bh_delete
>       segs, and I'm not 100% sure why yet - it only does it sometime
>       (i.e. run virt-test and leave it and it occasionally does it).
>       From the core it looks like qemu->bh is corrupt (0x10101010...)
>       so maybe mis has been freed at that point?
>       I'm suspecting this is the postcopy_ram_listen_thread freeing
>       mis at the end of it, but I don't know yet.

That should be it.  Maybe the patch can simply be reverted, because
loadvm_postcopy_handle_run runs from a thread and not a coroutine.  Is
this correct?

However I have a bug or two for you to fix, too:

1) as far as I can see, postcopy_ram_listen_thread is not holding the
mutex during the call to qemu_loadvm_state_main.  Is that a bug?

2) no one is currently joining mis->listen_thread, I suspect it actually
should be QEMU_THREAD_DETACHED.

:)

Paolo

Re: [Qemu-devel] [PULL 2/5] migration: move bdrv_invalidate_cache_all of of coroutine context

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> 
> 
> On 07/03/2016 13:49, Dr. David Alan Gilbert wrote:
> >    b) The harder problem is that there's a race where qemu_bh_delete
> >       segs, and I'm not 100% sure why yet - it only does it sometime
> >       (i.e. run virt-test and leave it and it occasionally does it).
> >       From the core it looks like qemu->bh is corrupt (0x10101010...)
> >       so maybe mis has been freed at that point?
> >       I'm suspecting this is the postcopy_ram_listen_thread freeing
> >       mis at the end of it, but I don't know yet.
> 
> That should be it.  Maybe the patch can simply be reverted, because
> loadvm_postcopy_handle_run runs from a thread and not a coroutine.  Is
> this correct?

That's still in the main thread, the 'run' comes from the packaged postcopy
state, but is after the 'listener' thread has been started.

I need to understand this anyway; the way it's supposed to work is that
if postcopy is being used then not much cleanup happens in process_incoming_migration_co
instead it exits and lets postcopy_ram_listen_thread do the cleanup
at the end; I've not quite figured out what's going on here
but it almost looks like both of them are cleaning up - that shouldn't
happen.

> However I have a bug or two for you to fix, too:
> 
> 1) as far as I can see, postcopy_ram_listen_thread is not holding the
> mutex during the call to qemu_loadvm_state_main.  Is that a bug?

No; the guest is running, the only thing that gets loaded by that
listen thread is data that's postcopied - i.e. currently just ram pages
that are loaded atomically.

> 2) no one is currently joining mis->listen_thread, I suspect it actually
> should be QEMU_THREAD_DETACHED.

OK, that looks like the easier one.

Dave

> 
> :)
> 
> Paolo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PULL 2/5] migration: move bdrv_invalidate_cache_all of of coroutine context

[Denis V. Lunev]

[-- Attachment #1: Type: text/plain, Size: 1914 bytes --]

On 03/07/2016 03:49 PM, Dr. David Alan Gilbert wrote:
> * Amit Shah (amit.shah@redhat.com) wrote:
>> From: "Denis V. Lunev" <den@openvz.org>
>>
>> There is a possibility to hit an assert in qcow2_get_specific_info that
>> s->qcow_version is undefined. This happens when VM in starting from
>> suspended state, i.e. it processes incoming migration, and in the same
>> time 'info block' is called.
>>
>> The problem is that qcow2_invalidate_cache() closes the image and
>> memset()s BDRVQcowState in the middle.
>>
>> The patch moves processing of bdrv_invalidate_cache_all out of
>> coroutine context for postcopy migration to avoid that. This function
>> is called with the following stack:
>>    process_incoming_migration_co
>>    qemu_loadvm_state
>>    qemu_loadvm_state_main
>>    loadvm_process_command
>>    loadvm_postcopy_handle_run
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> Tested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> hmm; actually - this segs in a variety of different ways;
> there are two problems:
>
>     a) +    bh = qemu_bh_new(loadvm_postcopy_handle_run_bh, NULL);
>       That's the easy one; that NULL should be 'mis', because
>       the bh is expecting to use it as a MigrationIncomingState
>       so it segs fairly reliably in the qemu_bh_delete(mis->bh)
>
>     b) The harder problem is that there's a race where qemu_bh_delete
>        segs, and I'm not 100% sure why yet - it only does it sometime
>        (i.e. run virt-test and leave it and it occasionally does it).
>        From the core it looks like qemu->bh is corrupt (0x10101010...)
>        so maybe mis has been freed at that point?
>        I'm suspecting this is the postcopy_ram_listen_thread freeing
>        mis at the end of it, but I don't know yet.
>
> Dave

Yes. this is exactly use-after-free. I have looked into the code
and this seems correct.

Could you try this simple patch?

Den



[-- Attachment #2: 1.diff --]
[-- Type: text/x-patch, Size: 1165 bytes --]

diff --git a/migration/savevm.c b/migration/savevm.c
index 96e7db5..9a020ef 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1446,15 +1446,6 @@ static void *postcopy_ram_listen_thread(void *opaque)
 
     migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
                                    MIGRATION_STATUS_COMPLETED);
-    /*
-     * If everything has worked fine, then the main thread has waited
-     * for us to start, and we're the last use of the mis.
-     * (If something broke then qemu will have to exit anyway since it's
-     * got a bad migration state).
-     */
-    migration_incoming_state_destroy();
-
-
     return NULL;
 }
 
@@ -1533,6 +1524,14 @@ static void loadvm_postcopy_handle_run_bh(void *opaque)
     }
 
     qemu_bh_delete(mis->bh);
+
+    /*
+     * If everything has worked fine, then the main thread has waited
+     * for us to start, and we're the last use of the mis.
+     * (If something broke then qemu will have to exit anyway since it's
+     * got a bad migration state).
+     */
+    migration_incoming_state_destroy();
 }
 
 /* After all discards we can start running and asking for pages */

Re: [Qemu-devel] [PULL 0/5] migration pull

[Amit Shah]

On (Wed) 24 Feb 2016 [09:27:50], Li, Liang Z wrote:
> > It's a bug in the header file, it was fixed in 4.9.
> > 
> > https://gcc.gnu.org/ml/gcc-patches/2013-06/txtvBBiTsFs8g.txt
> > 
> > Amit or Liang, can you restrict the new optimization to GCC 4.9+?
> > 
> > Paolo
> 
> Of course, how about using '#if QEMU_GNUC_PREREQ(4, 9) && defined CONFIG_AVX2_OPT' instead of '#ifdef CONFIG_AVX2_OPT'  in cutils.c?

Hi Liang, are you going to submit a new series with this?

Thanks,

		Amit

Re: [Qemu-devel] [PULL 0/5] migration pull

[Li, Liang Z]

> > > Amit or Liang, can you restrict the new optimization to GCC 4.9+?
> > >
> > > Paolo
> >
> > Of course, how about using '#if QEMU_GNUC_PREREQ(4, 9) && defined
> CONFIG_AVX2_OPT' instead of '#ifdef CONFIG_AVX2_OPT'  in cutils.c?
> 
> Hi Liang, are you going to submit a new series with this?

Yes,  just wait for a few minutes.

> 
> Thanks,
> 
> 		Amit

Re: [Qemu-devel] [PULL 2/5] migration: move bdrv_invalidate_cache_all of of coroutine context

[Dr. David Alan Gilbert]

* Denis V. Lunev (den@openvz.org) wrote:
> On 03/07/2016 03:49 PM, Dr. David Alan Gilbert wrote:
> >* Amit Shah (amit.shah@redhat.com) wrote:
> >>From: "Denis V. Lunev" <den@openvz.org>
> >>
> >>There is a possibility to hit an assert in qcow2_get_specific_info that
> >>s->qcow_version is undefined. This happens when VM in starting from
> >>suspended state, i.e. it processes incoming migration, and in the same
> >>time 'info block' is called.
> >>
> >>The problem is that qcow2_invalidate_cache() closes the image and
> >>memset()s BDRVQcowState in the middle.
> >>
> >>The patch moves processing of bdrv_invalidate_cache_all out of
> >>coroutine context for postcopy migration to avoid that. This function
> >>is called with the following stack:
> >>   process_incoming_migration_co
> >>   qemu_loadvm_state
> >>   qemu_loadvm_state_main
> >>   loadvm_process_command
> >>   loadvm_postcopy_handle_run
> >>
> >>Signed-off-by: Denis V. Lunev <den@openvz.org>
> >>Tested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >hmm; actually - this segs in a variety of different ways;
> >there are two problems:
> >
> >    a) +    bh = qemu_bh_new(loadvm_postcopy_handle_run_bh, NULL);
> >      That's the easy one; that NULL should be 'mis', because
> >      the bh is expecting to use it as a MigrationIncomingState
> >      so it segs fairly reliably in the qemu_bh_delete(mis->bh)
> >
> >    b) The harder problem is that there's a race where qemu_bh_delete
> >       segs, and I'm not 100% sure why yet - it only does it sometime
> >       (i.e. run virt-test and leave it and it occasionally does it).
> >       From the core it looks like qemu->bh is corrupt (0x10101010...)
> >       so maybe mis has been freed at that point?
> >       I'm suspecting this is the postcopy_ram_listen_thread freeing
> >       mis at the end of it, but I don't know yet.
> >
> >Dave
> 
> Yes. this is exactly use-after-free. I have looked into the code
> and this seems correct.
> 
> Could you try this simple patch?

Hmm no, that's not right.
The order for postcopy is that we are running the listen thread and then
receive the 'run', and the listening thread is still running - so you
can't destroy the incoming state during the run.
It can't get destroyed until both the main thread has finished loading
the migration AND the listen thread has finished.

Hmm - that does give me an idea about the other seg I saw; I need to check it;
but I think the problem is probably the case of a very short postcopy
where the listen thread exits before the handle_run_bh is triggered;
(and since I've only seen it in my virt-test setup, and I know it can do
very short postcopies)
I think the fix here is to pass loadvm_postcopy_handle_run_bh a pointer to it's
own bh structure rather than store it in mis->bh; that way it doesn't use mis
at all.

Dave

> Den
> 
> 

> diff --git a/migration/savevm.c b/migration/savevm.c
> index 96e7db5..9a020ef 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1446,15 +1446,6 @@ static void *postcopy_ram_listen_thread(void *opaque)
>  
>      migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
>                                     MIGRATION_STATUS_COMPLETED);
> -    /*
> -     * If everything has worked fine, then the main thread has waited
> -     * for us to start, and we're the last use of the mis.
> -     * (If something broke then qemu will have to exit anyway since it's
> -     * got a bad migration state).
> -     */
> -    migration_incoming_state_destroy();
> -
> -
>      return NULL;
>  }
>  
> @@ -1533,6 +1524,14 @@ static void loadvm_postcopy_handle_run_bh(void *opaque)
>      }
>  
>      qemu_bh_delete(mis->bh);
> +
> +    /*
> +     * If everything has worked fine, then the main thread has waited
> +     * for us to start, and we're the last use of the mis.
> +     * (If something broke then qemu will have to exit anyway since it's
> +     * got a bad migration state).
> +     */
> +    migration_incoming_state_destroy();
>  }
>  
>  /* After all discards we can start running and asking for pages */

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PULL 2/5] migration: move bdrv_invalidate_cache_all of of coroutine context

[Denis V. Lunev]

On 03/08/2016 01:45 PM, Dr. David Alan Gilbert wrote:
> * Denis V. Lunev (den@openvz.org) wrote:
>> On 03/07/2016 03:49 PM, Dr. David Alan Gilbert wrote:
>>> * Amit Shah (amit.shah@redhat.com) wrote:
>>>> From: "Denis V. Lunev" <den@openvz.org>
>>>>
>>>> There is a possibility to hit an assert in qcow2_get_specific_info that
>>>> s->qcow_version is undefined. This happens when VM in starting from
>>>> suspended state, i.e. it processes incoming migration, and in the same
>>>> time 'info block' is called.
>>>>
>>>> The problem is that qcow2_invalidate_cache() closes the image and
>>>> memset()s BDRVQcowState in the middle.
>>>>
>>>> The patch moves processing of bdrv_invalidate_cache_all out of
>>>> coroutine context for postcopy migration to avoid that. This function
>>>> is called with the following stack:
>>>>    process_incoming_migration_co
>>>>    qemu_loadvm_state
>>>>    qemu_loadvm_state_main
>>>>    loadvm_process_command
>>>>    loadvm_postcopy_handle_run
>>>>
>>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>>> Tested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>> hmm; actually - this segs in a variety of different ways;
>>> there are two problems:
>>>
>>>     a) +    bh = qemu_bh_new(loadvm_postcopy_handle_run_bh, NULL);
>>>       That's the easy one; that NULL should be 'mis', because
>>>       the bh is expecting to use it as a MigrationIncomingState
>>>       so it segs fairly reliably in the qemu_bh_delete(mis->bh)
>>>
>>>     b) The harder problem is that there's a race where qemu_bh_delete
>>>        segs, and I'm not 100% sure why yet - it only does it sometime
>>>        (i.e. run virt-test and leave it and it occasionally does it).
>>>        From the core it looks like qemu->bh is corrupt (0x10101010...)
>>>        so maybe mis has been freed at that point?
>>>        I'm suspecting this is the postcopy_ram_listen_thread freeing
>>>        mis at the end of it, but I don't know yet.
>>>
>>> Dave
>> Yes. this is exactly use-after-free. I have looked into the code
>> and this seems correct.
>>
>> Could you try this simple patch?
> Hmm no, that's not right.
> The order for postcopy is that we are running the listen thread and then
> receive the 'run', and the listening thread is still running - so you
> can't destroy the incoming state during the run.
> It can't get destroyed until both the main thread has finished loading
> the migration AND the listen thread has finished.
>
> Hmm - that does give me an idea about the other seg I saw; I need to check it;
> but I think the problem is probably the case of a very short postcopy
> where the listen thread exits before the handle_run_bh is triggered;
> (and since I've only seen it in my virt-test setup, and I know it can do
> very short postcopies)
> I think the fix here is to pass loadvm_postcopy_handle_run_bh a pointer to it's
> own bh structure rather than store it in mis->bh; that way it doesn't use mis
> at all.
>
> Dave
>
>> Den
>>
>>
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 96e7db5..9a020ef 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -1446,15 +1446,6 @@ static void *postcopy_ram_listen_thread(void *opaque)
>>   
>>       migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
>>                                      MIGRATION_STATUS_COMPLETED);
>> -    /*
>> -     * If everything has worked fine, then the main thread has waited
>> -     * for us to start, and we're the last use of the mis.
>> -     * (If something broke then qemu will have to exit anyway since it's
>> -     * got a bad migration state).
>> -     */
>> -    migration_incoming_state_destroy();
>> -
>> -
>>       return NULL;
>>   }
>>   
>> @@ -1533,6 +1524,14 @@ static void loadvm_postcopy_handle_run_bh(void *opaque)
>>       }
>>   
>>       qemu_bh_delete(mis->bh);
>> +
>> +    /*
>> +     * If everything has worked fine, then the main thread has waited
>> +     * for us to start, and we're the last use of the mis.
>> +     * (If something broke then qemu will have to exit anyway since it's
>> +     * got a bad migration state).
>> +     */
>> +    migration_incoming_state_destroy();
>>   }
>>   
>>   /* After all discards we can start running and asking for pages */
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
This will help for sure. The idea to reuse migration state seems wrong.

Den