qemu_thread_create: propagate errors to callers to check

RFC PATCH Superseded Fei Li 3 participants 17 messages Oct 29, 2018 – Nov 1, 2018 RISC-V

lore

Share & tools

lore

Fei Li

author

2018-10-29 12:58 UTC #0 lore raw

v6 cover qemu_thread_create: propagate errors to callers to check

Hi,

This idea comes from BiteSizedTasks, and this patch series implement the error checking of qemu_thread_create: make qemu_thread_create return a flag to indicate if it succeeded rather than failing with an error; make all callers check it.

The first and the third patch fixes some segmentation faults occured
during the debugging. The second patch paves the way for the last
patch as that patch is too long. The last patch modifies the
qemu_thread_create() and makes it return a bool to all direct callers
to indicate if it succeeds. The middle three fixes some migration
issues.

Please help to review, thanks. :)

v6:
- Add a new migration-multifd related patch. BTW, delete the previous vnc related patch as it has been upstreamed.
- Use error_setg_errno() to set the errno when qemu_thread_create() fails for both Linux and Windows implementation.
- Optimize the first patch, less codes are needed

v5:
- Remove errno = err in qemu_thread_create() for Linux, and change return errno to return -1 in qemu_signal_init() to indicate the error in case qemu_thread_create() fails.
- Delete the v4-added qemu_cond/mutex_destroy() in iothread_complete() as the destroy() will be done by its callers' object_unref().

v4:
- Separate the migration compression patch from this series
- Add one more error handling patch related with migration
- Add more cleaning up code for touched functions

v3:
- Add two migration related patches to fix the segmentaion fault
- Extract the segmentation fault fix from v2's last patch to be a separate patch
- Add cleaning up code for touched functions
- Update some error messages

v2:
- Pass errp straightly instead of using a local_err & error_propagate
- Return a bool: false/true to indicate if one function succeeds
- Merge v1's last two patches into one to avoid the compile error
- Fix one omitted error in patch1 and update some error messages

Fei Li (7):
Fix segmentation fault when qemu_signal_init fails
qemu_init_vcpu: add a new Error parameter to propagate
qemu_thread_join: fix segmentation fault
migration: fix some segmentation faults when using multifd
migration: fix the multifd code when receiving less channels
migration: fix some error handling
qemu_thread_create: propagate the error to callers to handle

 accel/tcg/user-exec-stub.c      |  3 +-
 cpus.c                          | 79 +++++++++++++++++++++-------------
 dump.c                          |  6 ++-
 hw/misc/edu.c                   |  6 ++-
 hw/ppc/spapr_hcall.c            | 10 ++++-
 hw/rdma/rdma_backend.c          |  4 +-
 hw/usb/ccid-card-emulated.c     | 13 ++++--
 include/qemu/thread.h           |  4 +-
 include/qom/cpu.h               |  2 +-
 io/task.c                       |  3 +-
 iothread.c                      | 16 ++++---
 migration/channel.c             |  7 +++-
 migration/migration.c           | 66 ++++++++++++++++++-----------
 migration/migration.h           |  2 +-
 migration/postcopy-ram.c        | 17 +++++++-
 migration/ram.c                 | 93 ++++++++++++++++++++++++++++++-----------
 migration/ram.h                 |  4 +-
 migration/savevm.c              | 11 +++--
 target/alpha/cpu.c              |  4 +-
 target/arm/cpu.c                |  4 +-
 target/cris/cpu.c               |  4 +-
 target/hppa/cpu.c               |  4 +-
 target/i386/cpu.c               |  4 +-
 target/lm32/cpu.c               |  4 +-
 target/m68k/cpu.c               |  4 +-
 target/microblaze/cpu.c         |  4 +-
 target/mips/cpu.c               |  4 +-
 target/moxie/cpu.c              |  4 +-
 target/nios2/cpu.c              |  4 +-
 target/openrisc/cpu.c           |  4 +-
 target/ppc/translate_init.inc.c |  4 +-
 target/riscv/cpu.c              |  4 +-
 target/s390x/cpu.c              |  4 +-
 target/sh4/cpu.c                |  4 +-
 target/sparc/cpu.c              |  4 +-
 target/tilegx/cpu.c             |  4 +-
 target/tricore/cpu.c            |  4 +-
 target/unicore32/cpu.c          |  4 +-
 target/xtensa/cpu.c             |  4 +-
 tests/atomic_add-bench.c        |  3 +-
 tests/iothread.c                |  2 +-
 tests/qht-bench.c               |  3 +-
 tests/rcutorture.c              |  3 +-
 tests/test-aio.c                |  2 +-
 tests/test-rcu-list.c           |  3 +-
 ui/vnc-jobs.c                   | 17 +++++---
 ui/vnc-jobs.h                   |  2 +-
 ui/vnc.c                        |  4 +-
 util/compatfd.c                 | 12 +++++-
 util/main-loop.c                |  8 ++--
 util/oslib-posix.c              | 17 ++++++--
 util/qemu-thread-posix.c        | 27 ++++++++----
 util/qemu-thread-win32.c        | 18 +++++---
 util/rcu.c                      |  3 +-
 util/thread-pool.c              |  4 +-
 55 files changed, 393 insertions(+), 165 deletions(-)

Signature · 1 line

2.13.7

Fei Li

author

2018-10-29 12:58 UTC #1 lore raw

RFC PATCH v6 1/7 Fix segmentation fault when qemu_signal_init fails

When qemu_signal_init() fails in qemu_init_main_loop(), we return without setting an error. Its callers crash then when they try to report the error with error_report_err().

To avoid such segmentation fault, add a new Error parameter to make the call trace to propagate the err to the final caller.

Cc Markus Armbruster <[email protected]>

Cc Fam Zheng <[email protected]>

Signed-off-by Fei Li <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

---
 util/main-loop.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

util/main-loop.c +4 -4

index affe0403c5..443cb4cfe8 100644
@@ -71,7 +71,7 @@ static void sigfd_handler(void *opaque)
     }
 }
 
-static int qemu_signal_init(void)
+static int qemu_signal_init(Error **errp)
 {
     int sigfd;
     sigset_t set;

@@ -96,7 +96,7 @@ static int qemu_signal_init(void)
     sigdelset(&set, SIG_IPI);
     sigfd = qemu_signalfd(&set);
     if (sigfd == -1) {
-        fprintf(stderr, "failed to create signalfd\n");
+        error_setg_errno(errp, errno, "failed to create signalfd");
         return -errno;
     }

@@ -109,7 +109,7 @@ static int qemu_signal_init(void)
 
 #else /* _WIN32 */
 
-static int qemu_signal_init(void)
+static int qemu_signal_init(Error **errp)
 {
     return 0;
 }

@@ -148,7 +148,7 @@ int qemu_init_main_loop(Error **errp)
 
     init_clocks(qemu_timer_notify_cb);
 
-    ret = qemu_signal_init();
+    ret = qemu_signal_init(errp);
     if (ret) {
         return ret;
     }

Signature · 1 line

2.13.7

Fei Li

author

2018-10-29 12:58 UTC #2 lore raw

RFC PATCH v6 2/7 qemu_init_vcpu: add a new Error parameter to propagate

In reply to Fei Li

This patch is to pave the way for a later patch as it is too long:
"qemu_thread_create: propagate the error to callers to handle."

The callers of qemu_init_vcpu() already passed the **errp to handle errors. In view of this, add a new Error parameter to all the functions called by qemu_init_vcpu() to propagate the error and let the further callers check it.

Besides, make qemu_init_vcpu() return a Boolean value to let its callers know whether it succeeds.

Signed-off-by Fei Li <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

---
 accel/tcg/user-exec-stub.c      |  3 ++-
 cpus.c                          | 34 +++++++++++++++++++++-------------
 include/qom/cpu.h               |  2 +-
 target/alpha/cpu.c              |  4 +++-
 target/arm/cpu.c                |  4 +++-
 target/cris/cpu.c               |  4 +++-
 target/hppa/cpu.c               |  4 +++-
 target/i386/cpu.c               |  4 +++-
 target/lm32/cpu.c               |  4 +++-
 target/m68k/cpu.c               |  4 +++-
 target/microblaze/cpu.c         |  4 +++-
 target/mips/cpu.c               |  4 +++-
 target/moxie/cpu.c              |  4 +++-
 target/nios2/cpu.c              |  4 +++-
 target/openrisc/cpu.c           |  4 +++-
 target/ppc/translate_init.inc.c |  4 +++-
 target/riscv/cpu.c              |  4 +++-
 target/s390x/cpu.c              |  4 +++-
 target/sh4/cpu.c                |  4 +++-
 target/sparc/cpu.c              |  4 +++-
 target/tilegx/cpu.c             |  4 +++-
 target/tricore/cpu.c            |  4 +++-
 target/unicore32/cpu.c          |  4 +++-
 target/xtensa/cpu.c             |  4 +++-
 24 files changed, 87 insertions(+), 36 deletions(-)

accel/tcg/user-exec-stub.c +2 -1

index a32b4496af..f8c38a375c 100644
@@ -10,8 +10,9 @@ void cpu_resume(CPUState *cpu)
 {
 }
 
-void qemu_init_vcpu(CPUState *cpu)
+bool qemu_init_vcpu(CPUState *cpu, Error **errp)
 {
+    return true;
 }
 
 /* User mode emulation does not support record/replay yet.  */

cpus.c +21 ~~-13~~

index 3978f63d8f..ed71618e1f 100644
@@ -1919,7 +1919,7 @@ void cpu_remove_sync(CPUState *cpu)
 /* For temporary buffers for forming a name */
 #define VCPU_THREAD_NAME_SIZE 16
 
-static void qemu_tcg_init_vcpu(CPUState *cpu)
+static void qemu_tcg_init_vcpu(CPUState *cpu, Error **errp)
 {
     char thread_name[VCPU_THREAD_NAME_SIZE];
     static QemuCond *single_tcg_halt_cond;

@@ -1975,7 +1975,7 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
     }
 }
 
-static void qemu_hax_start_vcpu(CPUState *cpu)
+static void qemu_hax_start_vcpu(CPUState *cpu, Error **errp)
 {
     char thread_name[VCPU_THREAD_NAME_SIZE];

@@ -1992,7 +1992,7 @@ static void qemu_hax_start_vcpu(CPUState *cpu)
 #endif
 }
 
-static void qemu_kvm_start_vcpu(CPUState *cpu)
+static void qemu_kvm_start_vcpu(CPUState *cpu, Error **errp)
 {
     char thread_name[VCPU_THREAD_NAME_SIZE];

@@ -2005,7 +2005,7 @@ static void qemu_kvm_start_vcpu(CPUState *cpu)
                        cpu, QEMU_THREAD_JOINABLE);
 }
 
-static void qemu_hvf_start_vcpu(CPUState *cpu)
+static void qemu_hvf_start_vcpu(CPUState *cpu, Error **errp)
 {
     char thread_name[VCPU_THREAD_NAME_SIZE];

@@ -2023,7 +2023,7 @@ static void qemu_hvf_start_vcpu(CPUState *cpu)
                        cpu, QEMU_THREAD_JOINABLE);
 }
 
-static void qemu_whpx_start_vcpu(CPUState *cpu)
+static void qemu_whpx_start_vcpu(CPUState *cpu, Error **errp)
 {
     char thread_name[VCPU_THREAD_NAME_SIZE];

@@ -2039,7 +2039,7 @@ static void qemu_whpx_start_vcpu(CPUState *cpu)
 #endif
 }
 
-static void qemu_dummy_start_vcpu(CPUState *cpu)
+static void qemu_dummy_start_vcpu(CPUState *cpu, Error **errp)
 {
     char thread_name[VCPU_THREAD_NAME_SIZE];

@@ -2052,11 +2052,12 @@ static void qemu_dummy_start_vcpu(CPUState *cpu)
                        QEMU_THREAD_JOINABLE);
 }
 
-void qemu_init_vcpu(CPUState *cpu)
+bool qemu_init_vcpu(CPUState *cpu, Error **errp)
 {
     cpu->nr_cores = smp_cores;
     cpu->nr_threads = smp_threads;
     cpu->stopped = true;
+    Error *local_err = NULL;
 
     if (!cpu->as) {
         /* If the target cpu hasn't set up any address spaces itself,

@@ -2067,22 +2068,29 @@ void qemu_init_vcpu(CPUState *cpu)
     }
 
     if (kvm_enabled()) {
-        qemu_kvm_start_vcpu(cpu);
+        qemu_kvm_start_vcpu(cpu, &local_err);
     } else if (hax_enabled()) {
-        qemu_hax_start_vcpu(cpu);
+        qemu_hax_start_vcpu(cpu, &local_err);
     } else if (hvf_enabled()) {
-        qemu_hvf_start_vcpu(cpu);
+        qemu_hvf_start_vcpu(cpu, &local_err);
     } else if (tcg_enabled()) {
-        qemu_tcg_init_vcpu(cpu);
+        qemu_tcg_init_vcpu(cpu, &local_err);
     } else if (whpx_enabled()) {
-        qemu_whpx_start_vcpu(cpu);
+        qemu_whpx_start_vcpu(cpu, &local_err);
     } else {
-        qemu_dummy_start_vcpu(cpu);
+        qemu_dummy_start_vcpu(cpu, &local_err);
+    }
+
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return false;
     }
 
     while (!cpu->created) {
         qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
     }
+
+    return true;
 }
 
 void cpu_stop_current(void)

include/qom/cpu.h +1 -1

index 4e238b0d9f..6254cdb2bf 100644
@@ -1012,7 +1012,7 @@ void end_exclusive(void);
  *
  * Initializes a vCPU.
  */
-void qemu_init_vcpu(CPUState *cpu);
+bool qemu_init_vcpu(CPUState *cpu, Error **errp);
 
 #define SSTEP_ENABLE  0x1  /* Enable simulated HW single stepping */
 #define SSTEP_NOIRQ   0x2  /* Do not use IRQ while single stepping */

target/alpha/cpu.c +3 -1

index a953897fcc..bf3c34516d 100644
@@ -66,7 +66,9 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
     acc->parent_realize(dev, errp);
 }

target/arm/cpu.c +3 -1

index 8f16e96b6c..85a96554fb 100644
@@ -1036,7 +1036,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 #endif
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
     cpu_reset(cs);
 
     acc->parent_realize(dev, errp);

target/cris/cpu.c +3 -1

index a23aba2688..ec92d69781 100644
@@ -140,7 +140,9 @@ static void cris_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 
     cpu_reset(cs);
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
     ccc->parent_realize(dev, errp);
 }

target/hppa/cpu.c +3 -1

index 00bf444620..08f600ced9 100644
@@ -98,7 +98,9 @@ static void hppa_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
     acc->parent_realize(dev, errp);
 
 #ifndef CONFIG_USER_ONLY

target/i386/cpu.c +3 -1

index 1469a1be01..81fd4a365c 100644
@@ -5112,7 +5112,9 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 #endif
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
     /*
      * Most Intel and certain AMD CPUs support hyperthreading. Even though QEMU

target/lm32/cpu.c +3 -1

index b7499cb627..d50b1e4a43 100644
@@ -139,7 +139,9 @@ static void lm32_cpu_realizefn(DeviceState *dev, Error **errp)
 
     cpu_reset(cs);
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
     lcc->parent_realize(dev, errp);
 }

target/m68k/cpu.c +3 -1

index 582e3a73b3..4ab53f2d58 100644
@@ -231,7 +231,9 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp)
     m68k_cpu_init_gdb(cpu);
 
     cpu_reset(cs);
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
     mcc->parent_realize(dev, errp);
 }

target/microblaze/cpu.c +3 -1

index 9b546a2c18..3906c864a3 100644
@@ -161,7 +161,9 @@ static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
     env->pvr.regs[0] = PVR0_USE_EXC_MASK \
                        | PVR0_USE_ICACHE_MASK \

target/mips/cpu.c +3 -1

index e217fb3e36..1e5aa69c57 100644
@@ -145,7 +145,9 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
     cpu_mips_realize_env(&cpu->env);
 
     cpu_reset(cs);
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
     mcc->parent_realize(dev, errp);
 }

target/moxie/cpu.c +3 -1

index 8d67eb6727..8581a6d922 100644
@@ -66,7 +66,9 @@ static void moxie_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
     cpu_reset(cs);
 
     mcc->parent_realize(dev, errp);

target/nios2/cpu.c +3 -1

index fbfaa2ce26..5c7b4b486e 100644
@@ -94,7 +94,9 @@ static void nios2_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
     cpu_reset(cs);
 
     ncc->parent_realize(dev, errp);

target/openrisc/cpu.c +3 -1

index fb7cb5c507..a6dcdb9df9 100644
@@ -83,7 +83,9 @@ static void openrisc_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
     cpu_reset(cs);
 
     occ->parent_realize(dev, errp);

target/ppc/translate_init.inc.c +3 -1

index ee9432eb15..a4c392eb21 100644
@@ -9694,7 +9694,9 @@ static void ppc_cpu_realize(DeviceState *dev, Error **errp)
                                  32, "power-vsx.xml", 0);
     }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        goto unrealize;
+    }
 
     pcc->parent_realize(dev, errp);

target/riscv/cpu.c +3 -1

index a025a0a3ba..9829fd9bc4 100644
@@ -305,7 +305,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
     cpu_reset(cs);
 
     mcc->parent_realize(dev, errp);

target/s390x/cpu.c +3 -1

index 18ba7f85a5..2a3eac9761 100644
@@ -222,7 +222,9 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
 #endif
     s390_cpu_gdb_init(cs);
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
     /*
      * KVM requires the initial CPU reset ioctl to be executed on the target

target/sh4/cpu.c +3 -1

index b9f393b7c7..d32ef2e1cb 100644
@@ -196,7 +196,9 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 
     cpu_reset(cs);
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
     scc->parent_realize(dev, errp);
 }

target/sparc/cpu.c +3 -1

index 0f090ece54..9c22f6a7df 100644
@@ -773,7 +773,9 @@ static void sparc_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
     scc->parent_realize(dev, errp);
 }

target/tilegx/cpu.c +3 -1

index bfe9be59b5..234148fabd 100644
@@ -92,7 +92,9 @@ static void tilegx_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 
     cpu_reset(cs);
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
     tcc->parent_realize(dev, errp);
 }

target/tricore/cpu.c +3 -1

index 2edaef1aef..5482d6ea3f 100644
@@ -96,7 +96,9 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp)
         set_feature(env, TRICORE_FEATURE_13);
     }
     cpu_reset(cs);
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
     tcc->parent_realize(dev, errp);
 }

target/unicore32/cpu.c +3 -1

index 2b49d1ca40..0c737c3187 100644
@@ -96,7 +96,9 @@ static void uc32_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
     ucc->parent_realize(dev, errp);
 }

target/xtensa/cpu.c +3 -1

index a54dbe4260..d2351c9b20 100644
@@ -131,7 +131,9 @@ static void xtensa_cpu_realizefn(DeviceState *dev, Error **errp)
 
     cs->gdb_num_regs = xcc->config->gdb_regmap.num_regs;
 
-    qemu_init_vcpu(cs);
+    if (!qemu_init_vcpu(cs, errp)) {
+        return;
+    }
 
     xcc->parent_realize(dev, errp);
 }

Signature · 1 line

2.13.7

Fei Li

author

2018-10-29 12:58 UTC #3 lore raw

RFC PATCH v6 3/7 qemu_thread_join: fix segmentation fault

In reply to Fei Li

To avoid the segmentation fault in qemu_thread_join(), just directly return when the QemuThread *thread failed to be created in either qemu-thread-posix.c or qemu-thread-win32.c.

Signed-off-by Fei Li <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

---
 util/qemu-thread-posix.c | 3 +++
 util/qemu-thread-win32.c | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

util/qemu-thread-posix.c +3 -0

index dfa66ff2fb..289af4fab5 100644
@@ -562,6 +562,9 @@ void *qemu_thread_join(QemuThread *thread)
     int err;
     void *ret;
 
+    if (!thread->thread) {
+        return NULL;
+    }
     err = pthread_join(thread->thread, &ret);
     if (err) {
         error_exit(err, __func__);

util/qemu-thread-win32.c +1 -1

index 4a363ca675..1a27e1cf6f 100644
@@ -366,7 +366,7 @@ void *qemu_thread_join(QemuThread *thread)
     HANDLE handle;
 
     data = thread->data;
-    if (data->mode == QEMU_THREAD_DETACHED) {
+    if (data == NULL || data->mode == QEMU_THREAD_DETACHED) {
         return NULL;
     }

Signature · 1 line

2.13.7

Fei Li

author

2018-10-29 12:59 UTC #4 lore raw

RFC PATCH v6 4/7 migration: fix some segmentation faults when using multifd

In reply to Fei Li

When multifd is used during migration, a segmentaion fault will occur in the source when multifd_save_cleanup() is called again if the multifd_send_state has been freed in earlier error handling. This can happen when migrate_fd_connect() fails and multifd_fd_cleanup() is called, and then multifd_new_send_channel_async() fails and multifd_save_cleanup() is called again.

If the QIOChannel *c of multifd_recv_state->params[i] (p->c) is not initialized, there is no need to close the channel. Or else a segmentation fault will occur in multifd_recv_terminate_threads() when multifd_recv_initial_packet() fails.

Signed-off-by Fei Li <[email protected]>

---
 migration/ram.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

migration/ram.c +21 -7

index 7e7deec4d8..4db3b3e8f4 100644
@@ -907,6 +907,11 @@ static void multifd_send_terminate_threads(Error *err)
         }
     }
 
+    /* in case multifd_send_state has been freed earlier */
+    if (!multifd_send_state) {
+        return;
+    }
+
     for (i = 0; i < migrate_multifd_channels(); i++) {
         MultiFDSendParams *p = &multifd_send_state->params[i];

@@ -922,7 +927,7 @@ int multifd_save_cleanup(Error **errp)
     int i;
     int ret = 0;
 
-    if (!migrate_use_multifd()) {
+    if (!migrate_use_multifd() || !multifd_send_state) {
         return 0;
     }
     multifd_send_terminate_threads(NULL);

@@ -960,7 +965,7 @@ static void multifd_send_sync_main(void)
 {
     int i;
 
-    if (!migrate_use_multifd()) {
+    if (!migrate_use_multifd() || !multifd_send_state) {
         return;
     }
     if (multifd_send_state->pages->used) {

@@ -1070,6 +1075,10 @@ static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
     QIOChannel *sioc = QIO_CHANNEL(qio_task_get_source(task));
     Error *local_err = NULL;
 
+    if (!multifd_send_state) {
+        return;
+    }
+
     if (qio_task_propagate_error(task, &local_err)) {
         if (multifd_save_cleanup(&local_err) != 0) {
             migrate_set_error(migrate_get_current(), local_err);

@@ -1131,7 +1140,7 @@ struct {
     uint64_t packet_num;
 } *multifd_recv_state;
 
-static void multifd_recv_terminate_threads(Error *err)
+static void multifd_recv_terminate_threads(Error *err, bool channel)
 {
     int i;

@@ -1145,6 +1154,11 @@ static void multifd_recv_terminate_threads(Error *err)
         }
     }
 
+    /* in case p->c is not initialized */
+    if (!channel) {
+        return;
+    }
+
     for (i = 0; i < migrate_multifd_channels(); i++) {
         MultiFDRecvParams *p = &multifd_recv_state->params[i];

@@ -1166,7 +1180,7 @@ int multifd_load_cleanup(Error **errp)
     if (!migrate_use_multifd()) {
         return 0;
     }
-    multifd_recv_terminate_threads(NULL);
+    multifd_recv_terminate_threads(NULL, true);
     for (i = 0; i < migrate_multifd_channels(); i++) {
         MultiFDRecvParams *p = &multifd_recv_state->params[i];

@@ -1269,7 +1283,7 @@ static void *multifd_recv_thread(void *opaque)
     }
 
     if (local_err) {
-        multifd_recv_terminate_threads(local_err);
+        multifd_recv_terminate_threads(local_err, true);
     }
     qemu_mutex_lock(&p->mutex);
     p->running = false;

@@ -1331,7 +1345,7 @@ bool multifd_recv_new_channel(QIOChannel *ioc)
 
     id = multifd_recv_initial_packet(ioc, &local_err);
     if (id < 0) {
-        multifd_recv_terminate_threads(local_err);
+        multifd_recv_terminate_threads(local_err, false);
         return false;
     }

@@ -1339,7 +1353,7 @@ bool multifd_recv_new_channel(QIOChannel *ioc)
     if (p->c != NULL) {
         error_setg(&local_err, "multifd: received id '%d' already setup'",
                    id);
-        multifd_recv_terminate_threads(local_err);
+        multifd_recv_terminate_threads(local_err, true);
         return false;
     }
     p->c = ioc;

Signature · 1 line

2.13.7

Fei Li

author

2018-10-29 12:59 UTC #5 lore raw

RFC PATCH v6 5/7 migration: fix the multifd code when receiving less channels

In reply to Fei Li

In our current code, when multifd is used during migration, if there is an error before the destination receives all new channels, the source keeps running, however the destination does not exit but keeps waiting until the source is killed deliberately.

Fix this by simply killing the destination when it fails to receive packet via some channel.

Cc Dr. David Alan Gilbert <[email protected]>

Cc Peter Xu <[email protected]>

Signed-off-by Fei Li <[email protected]>

---
 migration/channel.c   |  7 ++++++-
 migration/migration.c |  9 +++++++--
 migration/migration.h |  2 +-
 migration/ram.c       | 17 ++++++++++++++---
 migration/ram.h       |  2 +-
 5 files changed, 29 insertions(+), 8 deletions(-)

migration/channel.c +6 -1

index 33e0e9b82f..572be4245a 100644
@@ -44,7 +44,12 @@ void migration_channel_process_incoming(QIOChannel *ioc)
             error_report_err(local_err);
         }
     } else {
-        migration_ioc_process_incoming(ioc);
+        Error *local_err = NULL;
+        migration_ioc_process_incoming(ioc, &local_err);
+        if (local_err) {
+            error_report_err(local_err);
+            exit(EXIT_FAILURE);
+        }
     }
 }

migration/migration.c +7 -2

index 8b36e7f184..87dfc7374f 100644
@@ -541,7 +541,7 @@ void migration_fd_process_incoming(QEMUFile *f)
     migration_incoming_process();
 }
 
-void migration_ioc_process_incoming(QIOChannel *ioc)
+void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
 {
     MigrationIncomingState *mis = migration_incoming_get_current();
     bool start_migration;

@@ -563,9 +563,14 @@ void migration_ioc_process_incoming(QIOChannel *ioc)
          */
         start_migration = !migrate_use_multifd();
     } else {
+        Error *local_err = NULL;
         /* Multiple connections */
         assert(migrate_use_multifd());
-        start_migration = multifd_recv_new_channel(ioc);
+        start_migration = multifd_recv_new_channel(ioc, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
     }
 
     if (start_migration) {

migration/migration.h +1 -1

index f7813f8261..7df4d426d0 100644
@@ -229,7 +229,7 @@ struct MigrationState
 void migrate_set_state(int *state, int old_state, int new_state);
 
 void migration_fd_process_incoming(QEMUFile *f);
-void migration_ioc_process_incoming(QIOChannel *ioc);
+void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp);
 void migration_incoming_process(void);
 
 bool  migration_has_all_channels(void);

migration/ram.c +14 -3

index 4db3b3e8f4..8f03afe228 100644
@@ -1072,6 +1072,7 @@ out:
 static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
 {
     MultiFDSendParams *p = opaque;
+    MigrationState *s = migrate_get_current();
     QIOChannel *sioc = QIO_CHANNEL(qio_task_get_source(task));
     Error *local_err = NULL;

@@ -1080,6 +1081,7 @@ static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
     }
 
     if (qio_task_propagate_error(task, &local_err)) {
+        migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
         if (multifd_save_cleanup(&local_err) != 0) {
             migrate_set_error(migrate_get_current(), local_err);
         }

@@ -1337,16 +1339,20 @@ bool multifd_recv_all_channels_created(void)
 }
 
 /* Return true if multifd is ready for the migration, otherwise false */
-bool multifd_recv_new_channel(QIOChannel *ioc)
+bool multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
 {
+    MigrationIncomingState *mis = migration_incoming_get_current();
     MultiFDRecvParams *p;
     Error *local_err = NULL;
     int id;
 
     id = multifd_recv_initial_packet(ioc, &local_err);
     if (id < 0) {
+        error_propagate_prepend(errp, local_err,
+                        "failed to receive packet via multifd channel %x: ",
+                        multifd_recv_state->count);
         multifd_recv_terminate_threads(local_err, false);
-        return false;
+        goto fail;
     }
 
     p = &multifd_recv_state->params[id];

@@ -1354,7 +1360,8 @@ bool multifd_recv_new_channel(QIOChannel *ioc)
         error_setg(&local_err, "multifd: received id '%d' already setup'",
                    id);
         multifd_recv_terminate_threads(local_err, true);
-        return false;
+        error_propagate(errp, local_err);
+        goto fail;
     }
     p->c = ioc;
     object_ref(OBJECT(ioc));

@@ -1366,6 +1373,10 @@ bool multifd_recv_new_channel(QIOChannel *ioc)
                        QEMU_THREAD_JOINABLE);
     atomic_inc(&multifd_recv_state->count);
     return multifd_recv_state->count == migrate_multifd_channels();
+fail:
+    qemu_fclose(mis->from_src_file);
+    mis->from_src_file = NULL;
+    return false;
 }
 
 /**

migration/ram.h +1 -1

index 83ff1bc11a..046d3074be 100644
@@ -47,7 +47,7 @@ int multifd_save_cleanup(Error **errp);
 int multifd_load_setup(void);
 int multifd_load_cleanup(Error **errp);
 bool multifd_recv_all_channels_created(void);
-bool multifd_recv_new_channel(QIOChannel *ioc);
+bool multifd_recv_new_channel(QIOChannel *ioc, Error **errp);
 
 uint64_t ram_pagesize_summary(void);
 int ram_save_queue_pages(const char *rbname, ram_addr_t start, ram_addr_t len);

Signature · 1 line

2.13.7

Peter Xu

2018-10-30 06:05 UTC #6 lore raw

Re: [Qemu-devel] [PATCH RFC v6 5/7] migration: fix the multifd code when receiving less channels

Quoted 24 lines · Fei Li

In our current code, when multifd is used during migration, if there is an error before the destination receives all new channels, the source keeps running, however the destination does not exit but keeps waiting until the source is killed deliberately.
Fix this by simply killing the destination when it fails to receive packet via some channel.
Cc Dr. David Alan Gilbert <[email protected]>
Cc Peter Xu <[email protected]>
Signed-off-by Fei Li <[email protected]>
---
 migration/channel.c   |  7 ++++++-
 migration/migration.c |  9 +++++++--
 migration/migration.h |  2 +-
 migration/ram.c       | 17 ++++++++++++++---
 migration/ram.h       |  2 +-
 5 files changed, 29 insertions(+), 8 deletions(-)
migration/channel.c +0 -0
index 33e0e9b82f..572be4245a 100644

migration/channel.c void migration_channel_process_incoming(QIOChannel *ioc) · line 44

Quoted 1 line

migration/channel.c +0 -0

@@ -44,7 +44,12 @@ void migration_channel_process_incoming(QIOChannel *ioc)
             error_report_err(local_err);

[1]

migration/channel.c void migration_channel_process_incoming(QIOChannel *ioc) · line 51

Quoted 8 lines

migration/channel.c +5 -1

         }
     } else {
-        migration_ioc_process_incoming(ioc);
+        Error *local_err = NULL;
+        migration_ioc_process_incoming(ioc, &local_err);
+        if (local_err) {
+            error_report_err(local_err);
+            exit(EXIT_FAILURE);

I would still suggest that you don't quit. See TLS error at [1], it only dumps the error. IMHO users can quit easily for dst vm, I'll just let them decide if they want.

Then you can merge the error path for both.

Quoted 51 lines

migration/channel.c +1 -0

+        }
     }
 }

migration/migration.c +7 -2

index 8b36e7f184..87dfc7374f 100644
@@ -541,7 +541,7 @@ void migration_fd_process_incoming(QEMUFile *f)
     migration_incoming_process();
 }
 
-void migration_ioc_process_incoming(QIOChannel *ioc)
+void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
 {
     MigrationIncomingState *mis = migration_incoming_get_current();
     bool start_migration;

@@ -563,9 +563,14 @@ void migration_ioc_process_incoming(QIOChannel *ioc)
          */
         start_migration = !migrate_use_multifd();
     } else {
+        Error *local_err = NULL;
         /* Multiple connections */
         assert(migrate_use_multifd());
-        start_migration = multifd_recv_new_channel(ioc);
+        start_migration = multifd_recv_new_channel(ioc, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
     }
 
     if (start_migration) {

migration/migration.h +1 -1

index f7813f8261..7df4d426d0 100644
@@ -229,7 +229,7 @@ struct MigrationState
 void migrate_set_state(int *state, int old_state, int new_state);
 
 void migration_fd_process_incoming(QEMUFile *f);
-void migration_ioc_process_incoming(QIOChannel *ioc);
+void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp);
 void migration_incoming_process(void);
 
 bool  migration_has_all_channels(void);

migration/ram.c +0 -0

index 4db3b3e8f4..8f03afe228 100644

migration/ram.c out: · line 1075

Quoted 4 lines

migration/ram.c +1 -0

@@ -1072,6 +1072,7 @@ out:
 static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
 {
     MultiFDSendParams *p = opaque;
+    MigrationState *s = migrate_get_current();

This seems to be the source part, then I'll suggest you split the patch and keep this patch only touches the dest vm path.

Quoted 45 lines

migration/ram.c +9 -3

     QIOChannel *sioc = QIO_CHANNEL(qio_task_get_source(task));
     Error *local_err = NULL;

@@ -1080,6 +1081,7 @@ static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
     }
 
     if (qio_task_propagate_error(task, &local_err)) {
+        migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
         if (multifd_save_cleanup(&local_err) != 0) {
             migrate_set_error(migrate_get_current(), local_err);
         }

@@ -1337,16 +1339,20 @@ bool multifd_recv_all_channels_created(void)
 }
 
 /* Return true if multifd is ready for the migration, otherwise false */
-bool multifd_recv_new_channel(QIOChannel *ioc)
+bool multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
 {
+    MigrationIncomingState *mis = migration_incoming_get_current();
     MultiFDRecvParams *p;
     Error *local_err = NULL;
     int id;
 
     id = multifd_recv_initial_packet(ioc, &local_err);
     if (id < 0) {
+        error_propagate_prepend(errp, local_err,
+                        "failed to receive packet via multifd channel %x: ",
+                        multifd_recv_state->count);
         multifd_recv_terminate_threads(local_err, false);
-        return false;
+        goto fail;
     }
 
     p = &multifd_recv_state->params[id];

@@ -1354,7 +1360,8 @@ bool multifd_recv_new_channel(QIOChannel *ioc)
         error_setg(&local_err, "multifd: received id '%d' already setup'",
                    id);
         multifd_recv_terminate_threads(local_err, true);
-        return false;
+        error_propagate(errp, local_err);
+        goto fail;
     }
     p->c = ioc;
     object_ref(OBJECT(ioc));

migration/ram.c bool multifd_recv_new_channel(QIOChannel *ioc) · line 1379

Quoted 7 lines

migration/ram.c +4 -0

@@ -1366,6 +1373,10 @@ bool multifd_recv_new_channel(QIOChannel *ioc)
                        QEMU_THREAD_JOINABLE);
     atomic_inc(&multifd_recv_state->count);
     return multifd_recv_state->count == migrate_multifd_channels();
+fail:
+    qemu_fclose(mis->from_src_file);
+    mis->from_src_file = NULL;
+    return false;

Do we need this?

I'd suggest to put all cleanups into a single function. For dest vm I say it's process_incoming_migration_bh.

Regards,

Signature · 1 line

Peter Xu

Fei Li

author

2018-10-30 10:05 UTC #7 lore raw

Re: [Qemu-devel] [PATCH RFC v6 5/7] migration: fix the multifd code when receiving less channels

Quoted 36 lines · Peter Xu

Quoted 24 lines · Fei Li
In our current code, when multifd is used during migration, if there is an error before the destination receives all new channels, the source keeps running, however the destination does not exit but keeps waiting until the source is killed deliberately.
Fix this by simply killing the destination when it fails to receive packet via some channel.
Cc Dr. David Alan Gilbert <[email protected]>
Cc Peter Xu <[email protected]>
Signed-off-by Fei Li <[email protected]>
---
  migration/channel.c   |  7 ++++++-
  migration/migration.c |  9 +++++++--
  migration/migration.h |  2 +-
  migration/ram.c       | 17 ++++++++++++++---
  migration/ram.h       |  2 +-
  5 files changed, 29 insertions(+), 8 deletions(-)
migration/channel.c +0 -0
index 33e0e9b82f..572be4245a 100644
migration/channel.c void migration_channel_process_incoming(QIOChannel *ioc) · line 44
Quoted 1 line
migration/channel.c +0 -0
@@ -44,7 +44,12 @@ void migration_channel_process_incoming(QIOChannel *ioc)
              error_report_err(local_err);
[1]
migration/channel.c void migration_channel_process_incoming(QIOChannel *ioc) · line 51
Quoted 8 lines
migration/channel.c +5 -1
          }
      } else {
-        migration_ioc_process_incoming(ioc);
+        Error *local_err = NULL;
+        migration_ioc_process_incoming(ioc, &local_err);
+        if (local_err) {
+            error_report_err(local_err);
+            exit(EXIT_FAILURE);

Quoted 5 lines

I would still suggest that you don't quit. See TLS error at [1], it only dumps the error. IMHO users can quit easily for dst vm, I'll just let them decide if they want.
Then you can merge the error path for both.

Ok, got it, thanks :)

Quoted 46 lines

migration/channel.c +1 -0

+        }
      }
  }

migration/migration.c +7 -2

index 8b36e7f184..87dfc7374f 100644
@@ -541,7 +541,7 @@ void migration_fd_process_incoming(QEMUFile *f)
      migration_incoming_process();
  }
 
-void migration_ioc_process_incoming(QIOChannel *ioc)
+void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
  {
      MigrationIncomingState *mis = migration_incoming_get_current();
      bool start_migration;

@@ -563,9 +563,14 @@ void migration_ioc_process_incoming(QIOChannel *ioc)
           */
          start_migration = !migrate_use_multifd();
      } else {
+        Error *local_err = NULL;
          /* Multiple connections */
          assert(migrate_use_multifd());
-        start_migration = multifd_recv_new_channel(ioc);
+        start_migration = multifd_recv_new_channel(ioc, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
      }
 
      if (start_migration) {

migration/migration.h +1 -1

index f7813f8261..7df4d426d0 100644
@@ -229,7 +229,7 @@ struct MigrationState
  void migrate_set_state(int *state, int old_state, int new_state);
 
  void migration_fd_process_incoming(QEMUFile *f);
-void migration_ioc_process_incoming(QIOChannel *ioc);
+void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp);
  void migration_incoming_process(void);

Quoted 12 lines

migration/ram.c out: · line 1075
Quoted 10 lines
migration/migration.h +0 -0
  bool  migration_has_all_channels(void);
migration/ram.c +1 -0
index 4db3b3e8f4..8f03afe228 100644
@@ -1072,6 +1072,7 @@ out:
  static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
  {
      MultiFDSendParams *p = opaque;
+    MigrationState *s = migrate_get_current();
This seems to be the source part, then I'll suggest you split the patch and keep this patch only touches the dest vm path.

Quoted 55 lines

Quoted 46 lines

migration/ram.c +9 -3

      QIOChannel *sioc = QIO_CHANNEL(qio_task_get_source(task));
      Error *local_err = NULL;

@@ -1080,6 +1081,7 @@ static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
      }
 
      if (qio_task_propagate_error(task, &local_err)) {
+        migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
          if (multifd_save_cleanup(&local_err) != 0) {
              migrate_set_error(migrate_get_current(), local_err);
          }

@@ -1337,16 +1339,20 @@ bool multifd_recv_all_channels_created(void)
  }
 
  /* Return true if multifd is ready for the migration, otherwise false */
-bool multifd_recv_new_channel(QIOChannel *ioc)
+bool multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
  {
+    MigrationIncomingState *mis = migration_incoming_get_current();
      MultiFDRecvParams *p;
      Error *local_err = NULL;
      int id;
 
      id = multifd_recv_initial_packet(ioc, &local_err);
      if (id < 0) {
+        error_propagate_prepend(errp, local_err,
+                        "failed to receive packet via multifd channel %x: ",
+                        multifd_recv_state->count);
          multifd_recv_terminate_threads(local_err, false);
-        return false;
+        goto fail;
      }
 
      p = &multifd_recv_state->params[id];

@@ -1354,7 +1360,8 @@ bool multifd_recv_new_channel(QIOChannel *ioc)
          error_setg(&local_err, "multifd: received id '%d' already setup'",
                     id);
          multifd_recv_terminate_threads(local_err, true);
-        return false;
+        error_propagate(errp, local_err);
+        goto fail;
      }
      p->c = ioc;
      object_ref(OBJECT(ioc));

migration/ram.c bool multifd_recv_new_channel(QIOChannel *ioc) · line 1379

Quoted 7 lines

migration/ram.c +4 -0

@@ -1366,6 +1373,10 @@ bool multifd_recv_new_channel(QIOChannel *ioc)
                         QEMU_THREAD_JOINABLE);
      atomic_inc(&multifd_recv_state->count);
      return multifd_recv_state->count == migrate_multifd_channels();
+fail:
+    qemu_fclose(mis->from_src_file);
+    mis->from_src_file = NULL;
+    return false;

Do we need this?

Quoted 5 lines

I'd suggest to put all cleanups into a single function. For dest vm I say it's process_incoming_migration_bh.
Regards,

Not sure whether I understand correctly, if multifd_recv_new_channel() fails, that means migration_incoming_process() will not be called, then process_incoming_migration_co() and process_incoming_migration_bh() will not be called either. In that way, there is no cleanup.

Have a nice day, thanks
Fei

Peter Xu

2018-10-30 22:19 UTC #8 lore raw

Re: [Qemu-devel] [PATCH RFC v6 5/7] migration: fix the multifd code when receiving less channels

On Tue, Oct 30, 2018 at 06:05:18PM +0800, Fei Li wrote:

[...]

Quoted 13 lines

Quoted 13 lines
migration/ram.c bool multifd_recv_new_channel(QIOChannel *ioc) · line 1379
Quoted 8 lines
migration/ram.c +4 -0
@@ -1366,6 +1373,10 @@ bool multifd_recv_new_channel(QIOChannel *ioc)
                         QEMU_THREAD_JOINABLE);
      atomic_inc(&multifd_recv_state->count);
      return multifd_recv_state->count == migrate_multifd_channels();
+fail:
+    qemu_fclose(mis->from_src_file);
+    mis->from_src_file = NULL;
+    return false;
Do we need this?
I'd suggest to put all cleanups into a single function. For dest vm I say it's process_incoming_migration_bh.

Quoted 7 lines

Quoted 2 lines
Regards,
Not sure whether I understand correctly, if multifd_recv_new_channel() fails,
that means migration_incoming_process() will not be called, then process_incoming_migration_co() and process_incoming_migration_bh() will not be called either. In that way, there is no cleanup.

Sorry the funtion name I wanted to paste is something like migration_incoming_state_destroy()... Anyway I still don't feel that right to close the mis->from_src_file in a multifd special path.

For now, I'll either ignore the cleanup part (AFAIU the TLS failure will also ignore it when migration_tls_channel_process_incoming fails) and just print the extra error message, or you can also look into how to cleanup the dest vm in a better way. That could be someting like calling migration_incoming_state_destroy() somewhere in migration_channel_process_incoming() when failure happens but I'm not sure.

Regards,

Signature · 1 line

Peter Xu

Fei Li

author

2018-10-31 12:26 UTC #9 lore raw

Re: [Qemu-devel] [PATCH RFC v6 5/7] migration: fix the multifd code when receiving less channels

Quoted 24 lines · Peter Xu

On Tue, Oct 30, 2018 at 06:05:18PM +0800, Fei Li wrote:
[...]
Quoted 13 lines
Quoted 13 lines
migration/ram.c bool multifd_recv_new_channel(QIOChannel *ioc) · line 1379
Quoted 8 lines
migration/ram.c +4 -0
@@ -1366,6 +1373,10 @@ bool multifd_recv_new_channel(QIOChannel *ioc)
                          QEMU_THREAD_JOINABLE);
       atomic_inc(&multifd_recv_state->count);
       return multifd_recv_state->count == migrate_multifd_channels();
+fail:
+    qemu_fclose(mis->from_src_file);
+    mis->from_src_file = NULL;
+    return false;
Do we need this?
I'd suggest to put all cleanups into a single function. For dest vm I say it's process_incoming_migration_bh.
Quoted 7 lines
Quoted 2 lines
Regards,
Not sure whether I understand correctly, if multifd_recv_new_channel() fails,
that means migration_incoming_process() will not be called, then process_incoming_migration_co() and process_incoming_migration_bh() will not be called either. In that way, there is no cleanup.

Quoted 7 lines

Sorry the funtion name I wanted to paste is something like migration_incoming_state_destroy()... Anyway I still don't feel that right to close the mis->from_src_file in a multifd special path.
For now, I'll either ignore the cleanup part (AFAIU the TLS failure will also ignore it when migration_tls_channel_process_incoming fails) and just print the extra error message,

I will adopt this option, thanks for the suggestion :)

Have a nice day, thanks
Fei

Quoted 8 lines

or you can also look into how
to cleanup the dest vm in a better way. That could be someting like calling migration_incoming_state_destroy() somewhere in migration_channel_process_incoming() when failure happens but I'm not sure.
Regards,

Fei Li

author

2018-10-31 13:50 UTC #10 lore raw

Re: [Qemu-devel] [PATCH RFC v6 5/7] migration: fix the multifd code when receiving less channels

In reply to Fei Li

Hi all,

I create a new thread to inquiry one live migration issue when using multifd :) I am not so sure with the rule that when and how to use the multifd is correct, so I'd like to confirm. This is because when I use the current upstream qemu code and run into a failed case: "Migration status: failed (Unable to write to
socket: Connection reset by peer)".

The detailed is as follows and the "failed" situation can not reproduce 100%. But as far as I tested, if I do the live migration using multifd* just after the guest started for less than one minute*, I almost can reproduce this for 100%.

My steps are:
1. start the vm in the src side;
2. start the -incoming in the dst side;
3. after the vm started for a little while (After I open a file inside the vm), I begin the live migration, steps are:
- on src: migrate_set_capability x-multifd on
- on src: migrate_set_parameter x-multifd-channels 4
- on dst: migrate_set_capability x-multifd on
- on dst: migrate_set_parameter x-multifd-channels 4
- on src: migrate -d tcp:192.168.120.5:4444

Errors are:
[src]
linux-50ts:/mnt/live-migration # ./sle12-source.sh
QEMU 3.0.50 monitor - type 'help' for more information
(qemu) Running QEMU with SDL 1.2 is deprecated, and will be removed
in a future release. Please switch to SDL 2.0 instead
migrate_set_capability x-multifd on
(qemu) migrate_set_parameter x-multifd-channels 4
(qemu) migrate -d tcp:192.168.120.5:4444
(qemu) info migrate
globals:
store-global-state: on
only-migratable: off
send-configuration: on
send-section-footer: on
decompress-error-check: on
capabilities: xbzrle: off rdma-pin-all: off auto-converge: off zero-blocks: off compress: off events: off postcopy-ram: off x-colo: off release-ram: off block: off return-path: off pause-before-switchover: off x-multifd: on dirty-bitmaps: off postcopy-blocktime: off late-block-activate: off Migration status: failed (Unable to write to socket: Connection reset by peer)
total time: 0 milliseconds

[dst]
linux-p6v6:/mnt/live-migration # ./sle12-dest.sh QEMU 3.0.50 monitor - type 'help' for more information (qemu) migrate_set_capability x-multifd on (qemu) migrate_set_parameter x-multifd-channels 4 (qemu) info migrate
globals:
store-global-state: on
only-migratable: off
send-configuration: on
send-section-footer: on
decompress-error-check: on

Hope this does not bother you too much. ;)
Have a nice day, thanks again
Fei

Quoted 156 lines · Fei Li

Fix this by simply killing the destination when it fails to receive packet via some channel.

Cc Dr. David Alan Gilbert <[email protected]>

Cc Peter Xu <[email protected]>

Signed-off-by Fei Li <[email protected]>

---
  migration/channel.c   |  7 ++++++-
  migration/migration.c |  9 +++++++--
  migration/migration.h |  2 +-
  migration/ram.c       | 17 ++++++++++++++---
  migration/ram.h       |  2 +-
  5 files changed, 29 insertions(+), 8 deletions(-)

migration/channel.c +6 -1

index 33e0e9b82f..572be4245a 100644
@@ -44,7 +44,12 @@ void migration_channel_process_incoming(QIOChannel *ioc)
              error_report_err(local_err);
          }
      } else {
-        migration_ioc_process_incoming(ioc);
+        Error *local_err = NULL;
+        migration_ioc_process_incoming(ioc, &local_err);
+        if (local_err) {
+            error_report_err(local_err);
+            exit(EXIT_FAILURE);
+        }
      }
  }

migration/migration.c +7 -2

index 8b36e7f184..87dfc7374f 100644
@@ -541,7 +541,7 @@ void migration_fd_process_incoming(QEMUFile *f)
      migration_incoming_process();
  }
 
-void migration_ioc_process_incoming(QIOChannel *ioc)
+void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
  {
      MigrationIncomingState *mis = migration_incoming_get_current();
      bool start_migration;

@@ -563,9 +563,14 @@ void migration_ioc_process_incoming(QIOChannel *ioc)
           */
          start_migration = !migrate_use_multifd();
      } else {
+        Error *local_err = NULL;
          /* Multiple connections */
          assert(migrate_use_multifd());
-        start_migration = multifd_recv_new_channel(ioc);
+        start_migration = multifd_recv_new_channel(ioc, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
      }
 
      if (start_migration) {

migration/migration.h +1 -1

index f7813f8261..7df4d426d0 100644
@@ -229,7 +229,7 @@ struct MigrationState
  void migrate_set_state(int *state, int old_state, int new_state);
 
  void migration_fd_process_incoming(QEMUFile *f);
-void migration_ioc_process_incoming(QIOChannel *ioc);
+void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp);
  void migration_incoming_process(void);
 
  bool  migration_has_all_channels(void);

migration/ram.c +14 -3

index 4db3b3e8f4..8f03afe228 100644
@@ -1072,6 +1072,7 @@ out:
  static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
  {
      MultiFDSendParams *p = opaque;
+    MigrationState *s = migrate_get_current();
      QIOChannel *sioc = QIO_CHANNEL(qio_task_get_source(task));
      Error *local_err = NULL;

@@ -1080,6 +1081,7 @@ static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
      }
 
      if (qio_task_propagate_error(task, &local_err)) {
+        migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
          if (multifd_save_cleanup(&local_err) != 0) {
              migrate_set_error(migrate_get_current(), local_err);
          }

@@ -1337,16 +1339,20 @@ bool multifd_recv_all_channels_created(void)
  }
 
  /* Return true if multifd is ready for the migration, otherwise false */
-bool multifd_recv_new_channel(QIOChannel *ioc)
+bool multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
  {
+    MigrationIncomingState *mis = migration_incoming_get_current();
      MultiFDRecvParams *p;
      Error *local_err = NULL;
      int id;
 
      id = multifd_recv_initial_packet(ioc, &local_err);
      if (id < 0) {
+        error_propagate_prepend(errp, local_err,
+                        "failed to receive packet via multifd channel %x: ",
+                        multifd_recv_state->count);
          multifd_recv_terminate_threads(local_err, false);
-        return false;
+        goto fail;
      }
 
      p = &multifd_recv_state->params[id];

@@ -1354,7 +1360,8 @@ bool multifd_recv_new_channel(QIOChannel *ioc)
          error_setg(&local_err, "multifd: received id '%d' already setup'",
                     id);
          multifd_recv_terminate_threads(local_err, true);
-        return false;
+        error_propagate(errp, local_err);
+        goto fail;
      }
      p->c = ioc;
      object_ref(OBJECT(ioc));

@@ -1366,6 +1373,10 @@ bool multifd_recv_new_channel(QIOChannel *ioc)
                         QEMU_THREAD_JOINABLE);
      atomic_inc(&multifd_recv_state->count);
      return multifd_recv_state->count == migrate_multifd_channels();
+fail:
+    qemu_fclose(mis->from_src_file);
+    mis->from_src_file = NULL;
+    return false;
  }
 
  /**

migration/ram.h +1 -1

index 83ff1bc11a..046d3074be 100644
@@ -47,7 +47,7 @@ int multifd_save_cleanup(Error **errp);
  int multifd_load_setup(void);
  int multifd_load_cleanup(Error **errp);
  bool multifd_recv_all_channels_created(void);
-bool multifd_recv_new_channel(QIOChannel *ioc);
+bool multifd_recv_new_channel(QIOChannel *ioc, Error **errp);
 
  uint64_t ram_pagesize_summary(void);
  int ram_save_queue_pages(const char *rbname, ram_addr_t start, ram_addr_t len);

Fei Li

author

2018-10-29 12:59 UTC #11 lore raw

RFC PATCH v6 6/7 migration: fix some error handling

In reply to Fei Li

Add error handling for qemu_ram_foreach_migratable_block() when it fails.

Always call migrate_set_error() to set the error state without relying on whether multifd_save_cleanup() succeeds. As the passed &local_err is never used in multifd_save_cleanup(), remove it.

Signed-off-by Fei Li <[email protected]>

---
 migration/migration.c    | 5 +----
 migration/postcopy-ram.c | 3 +++
 migration/ram.c          | 7 +++----
 migration/ram.h          | 2 +-
 4 files changed, 8 insertions(+), 9 deletions(-)

migration/migration.c +1 -4

index 87dfc7374f..3b8b7ab4f9 100644
@@ -1377,7 +1377,6 @@ static void migrate_fd_cleanup(void *opaque)
     qemu_savevm_state_cleanup();
 
     if (s->to_dst_file) {
-        Error *local_err = NULL;
         QEMUFile *tmp;
 
         trace_migrate_fd_cleanup();

@@ -1388,9 +1387,7 @@ static void migrate_fd_cleanup(void *opaque)
         }
         qemu_mutex_lock_iothread();
 
-        if (multifd_save_cleanup(&local_err) != 0) {
-            error_report_err(local_err);
-        }
+        multifd_save_cleanup();
         qemu_mutex_lock(&s->qemu_file_lock);
         tmp = s->to_dst_file;
         s->to_dst_file = NULL;

migration/postcopy-ram.c +3 -0

index e5c02a32c5..4ca2bc02b3 100644
@@ -1117,6 +1117,9 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis)
 
     /* Mark so that we get notified of accesses to unwritten areas */
     if (qemu_ram_foreach_migratable_block(ram_block_enable_notify, mis)) {
+        error_report("ram_block_enable_notify failed");
+        close(mis->userfault_event_fd);
+        close(mis->userfault_fd);
         return -1;
     }

migration/ram.c +3 -4

index 8f03afe228..57cb1bee3d 100644
@@ -922,7 +922,7 @@ static void multifd_send_terminate_threads(Error *err)
     }
 }
 
-int multifd_save_cleanup(Error **errp)
+int multifd_save_cleanup(void)
 {
     int i;
     int ret = 0;

@@ -1082,9 +1082,8 @@ static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
 
     if (qio_task_propagate_error(task, &local_err)) {
         migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
-        if (multifd_save_cleanup(&local_err) != 0) {
-            migrate_set_error(migrate_get_current(), local_err);
-        }
+        multifd_save_cleanup();
+        migrate_set_error(migrate_get_current(), local_err);
     } else {
         p->c = QIO_CHANNEL(sioc);
         qio_channel_set_delay(p->c, false);

migration/ram.h +1 -1

index 046d3074be..0d1215209e 100644
@@ -43,7 +43,7 @@ uint64_t ram_bytes_remaining(void);
 uint64_t ram_bytes_total(void);
 
 int multifd_save_setup(void);
-int multifd_save_cleanup(Error **errp);
+int multifd_save_cleanup(void);
 int multifd_load_setup(void);
 int multifd_load_cleanup(Error **errp);
 bool multifd_recv_all_channels_created(void);

Signature · 1 line

2.13.7

Dr. David Alan Gilbert

2018-10-30 19:49 UTC #12 lore raw

Re: [Qemu-devel] [PATCH RFC v6 6/7] migration: fix some error handling

Quoted 43 lines · * Fei Li ([email protected]) wrote:

Add error handling for qemu_ram_foreach_migratable_block() when it fails.

Always call migrate_set_error() to set the error state without relying on whether multifd_save_cleanup() succeeds. As the passed &local_err is never used in multifd_save_cleanup(), remove it.

Signed-off-by Fei Li <[email protected]>

---
 migration/migration.c    | 5 +----
 migration/postcopy-ram.c | 3 +++
 migration/ram.c          | 7 +++----
 migration/ram.h          | 2 +-
 4 files changed, 8 insertions(+), 9 deletions(-)

migration/migration.c +1 -4

index 87dfc7374f..3b8b7ab4f9 100644
@@ -1377,7 +1377,6 @@ static void migrate_fd_cleanup(void *opaque)
     qemu_savevm_state_cleanup();
 
     if (s->to_dst_file) {
-        Error *local_err = NULL;
         QEMUFile *tmp;
 
         trace_migrate_fd_cleanup();

@@ -1388,9 +1387,7 @@ static void migrate_fd_cleanup(void *opaque)
         }
         qemu_mutex_lock_iothread();
 
-        if (multifd_save_cleanup(&local_err) != 0) {
-            error_report_err(local_err);
-        }
+        multifd_save_cleanup();
         qemu_mutex_lock(&s->qemu_file_lock);
         tmp = s->to_dst_file;
         s->to_dst_file = NULL;

migration/postcopy-ram.c +0 -0

index e5c02a32c5..4ca2bc02b3 100644

migration/postcopy-ram.c int postcopy_ram_enable_notify(MigrationIncomingState *mis) · line 1122

Quoted 6 lines

migration/postcopy-ram.c +3 -0

@@ -1117,6 +1117,9 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis)
 
     /* Mark so that we get notified of accesses to unwritten areas */
     if (qemu_ram_foreach_migratable_block(ram_block_enable_notify, mis)) {
+        error_report("ram_block_enable_notify failed");
+        close(mis->userfault_event_fd);
+        close(mis->userfault_fd);

I don't think these close() calls are safe. This code is just after starting the fault thread, and the fault thread has a poll() call on these fd's, so we can't close them until we've instructed that thread to exit.

We should fall out through postcopy_ram_incoming_cleanup, and because the thread exists it should do a notify to the thread, a join and then only later do the close calls.

Dave

Quoted 44 lines

migration/postcopy-ram.c +0 -0

         return -1;
     }

migration/ram.c +3 -4

index 8f03afe228..57cb1bee3d 100644
@@ -922,7 +922,7 @@ static void multifd_send_terminate_threads(Error *err)
     }
 }
 
-int multifd_save_cleanup(Error **errp)
+int multifd_save_cleanup(void)
 {
     int i;
     int ret = 0;

@@ -1082,9 +1082,8 @@ static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
 
     if (qio_task_propagate_error(task, &local_err)) {
         migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
-        if (multifd_save_cleanup(&local_err) != 0) {
-            migrate_set_error(migrate_get_current(), local_err);
-        }
+        multifd_save_cleanup();
+        migrate_set_error(migrate_get_current(), local_err);
     } else {
         p->c = QIO_CHANNEL(sioc);
         qio_channel_set_delay(p->c, false);

migration/ram.h +1 -1

index 046d3074be..0d1215209e 100644
@@ -43,7 +43,7 @@ uint64_t ram_bytes_remaining(void);
 uint64_t ram_bytes_total(void);
 
 int multifd_save_setup(void);
-int multifd_save_cleanup(Error **errp);
+int multifd_save_cleanup(void);
 int multifd_load_setup(void);
 int multifd_load_cleanup(Error **errp);
 bool multifd_recv_all_channels_created(void);

Signature · 1 line

2.13.7

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

Fei Li

author

2018-10-31 11:25 UTC #13 lore raw

Re: [Qemu-devel] [PATCH RFC v6 6/7] migration: fix some error handling

Quoted 55 lines · Dr. David Alan Gilbert

Quoted 43 lines · * Fei Li ([email protected]) wrote:
Add error handling for qemu_ram_foreach_migratable_block() when it fails.
Always call migrate_set_error() to set the error state without relying on whether multifd_save_cleanup() succeeds. As the passed &local_err is never used in multifd_save_cleanup(), remove it.
Signed-off-by Fei Li <[email protected]>
---
  migration/migration.c    | 5 +----
  migration/postcopy-ram.c | 3 +++
  migration/ram.c          | 7 +++----
  migration/ram.h          | 2 +-
  4 files changed, 8 insertions(+), 9 deletions(-)
migration/migration.c +1 -4
index 87dfc7374f..3b8b7ab4f9 100644
@@ -1377,7 +1377,6 @@ static void migrate_fd_cleanup(void *opaque)
      qemu_savevm_state_cleanup();
 
      if (s->to_dst_file) {
-        Error *local_err = NULL;
          QEMUFile *tmp;
 
          trace_migrate_fd_cleanup();
@@ -1388,9 +1387,7 @@ static void migrate_fd_cleanup(void *opaque)
          }
          qemu_mutex_lock_iothread();
 
-        if (multifd_save_cleanup(&local_err) != 0) {
-            error_report_err(local_err);
-        }
+        multifd_save_cleanup();
          qemu_mutex_lock(&s->qemu_file_lock);
          tmp = s->to_dst_file;
          s->to_dst_file = NULL;
migration/postcopy-ram.c +0 -0
index e5c02a32c5..4ca2bc02b3 100644
migration/postcopy-ram.c int postcopy_ram_enable_notify(MigrationIncomingState *mis) · line 1122
Quoted 6 lines
migration/postcopy-ram.c +3 -0
@@ -1117,6 +1117,9 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis)
 
      /* Mark so that we get notified of accesses to unwritten areas */
      if (qemu_ram_foreach_migratable_block(ram_block_enable_notify, mis)) {
+        error_report("ram_block_enable_notify failed");
+        close(mis->userfault_event_fd);
+        close(mis->userfault_fd);
I don't think these close() calls are safe. This code is just after starting the fault thread, and the fault thread has a poll() call on these fd's, so we can't close them until we've instructed that thread to exit.

Quoted 5 lines

We should fall out through postcopy_ram_incoming_cleanup, and because the thread exists it should do a notify to the thread, a join and then only later do the close calls.
Dave

I see the postcopy_ram_incoming_cleanup() already include the notify & join the fault thread & close these two fds and other more cleanup, thus that means directly replace the above two close() with postcopy_ram_incoming_cleanup() is ok, right? :)

BTW, does the parameter "&local_err" for multifd_save_cleanup(&local_err) is reserved for some reason? As I notice no code is using this error parameter, and if it is reserved for nothing I'd like to delete it just as the second paragraph of the commit message said. :)

Have a nice day, thanks
Fei

Quoted 48 lines

Quoted 45 lines

migration/postcopy-ram.c +0 -0

          return -1;
      }

migration/ram.c +3 -4

index 8f03afe228..57cb1bee3d 100644
@@ -922,7 +922,7 @@ static void multifd_send_terminate_threads(Error *err)
      }
  }
 
-int multifd_save_cleanup(Error **errp)
+int multifd_save_cleanup(void)
  {
      int i;
      int ret = 0;

@@ -1082,9 +1082,8 @@ static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
 
      if (qio_task_propagate_error(task, &local_err)) {
          migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
-        if (multifd_save_cleanup(&local_err) != 0) {
-            migrate_set_error(migrate_get_current(), local_err);
-        }
+        multifd_save_cleanup();
+        migrate_set_error(migrate_get_current(), local_err);
      } else {
          p->c = QIO_CHANNEL(sioc);
          qio_channel_set_delay(p->c, false);

migration/ram.h +1 -1

index 046d3074be..0d1215209e 100644
@@ -43,7 +43,7 @@ uint64_t ram_bytes_remaining(void);
  uint64_t ram_bytes_total(void);
 
  int multifd_save_setup(void);
-int multifd_save_cleanup(Error **errp);
+int multifd_save_cleanup(void);
  int multifd_load_setup(void);
  int multifd_load_cleanup(Error **errp);
  bool multifd_recv_all_channels_created(void);

Signature · 1 line

2.13.7

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

Dr. David Alan Gilbert

2018-10-31 16:31 UTC #14 lore raw

Re: [Qemu-devel] [PATCH RFC v6 6/7] migration: fix some error handling

Quoted 58 lines · * Fei Li ([email protected]) wrote:

Quoted 55 lines · Dr. David Alan Gilbert
Quoted 40 lines · * Fei Li ([email protected]) wrote:
Add error handling for qemu_ram_foreach_migratable_block() when it fails.
Always call migrate_set_error() to set the error state without relying on whether multifd_save_cleanup() succeeds. As the passed &local_err is never used in multifd_save_cleanup(), remove it.
Signed-off-by Fei Li <[email protected]>
---
  migration/migration.c    | 5 +----
  migration/postcopy-ram.c | 3 +++
  migration/ram.c          | 7 +++----
  migration/ram.h          | 2 +-
  4 files changed, 8 insertions(+), 9 deletions(-)
migration/migration.c +1 -4
index 87dfc7374f..3b8b7ab4f9 100644
@@ -1377,7 +1377,6 @@ static void migrate_fd_cleanup(void *opaque)
      qemu_savevm_state_cleanup();
      if (s->to_dst_file) {
-        Error *local_err = NULL;
          QEMUFile *tmp;
          trace_migrate_fd_cleanup();
@@ -1388,9 +1387,7 @@ static void migrate_fd_cleanup(void *opaque)
          }
          qemu_mutex_lock_iothread();
-        if (multifd_save_cleanup(&local_err) != 0) {
-            error_report_err(local_err);
-        }
+        multifd_save_cleanup();
          qemu_mutex_lock(&s->qemu_file_lock);
          tmp = s->to_dst_file;
          s->to_dst_file = NULL;
migration/postcopy-ram.c +0 -0
index e5c02a32c5..4ca2bc02b3 100644
migration/postcopy-ram.c int postcopy_ram_enable_notify(MigrationIncomingState *mis) · line 1121
Quoted 5 lines
migration/postcopy-ram.c +3 -0
@@ -1117,6 +1117,9 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis)
      /* Mark so that we get notified of accesses to unwritten areas */
      if (qemu_ram_foreach_migratable_block(ram_block_enable_notify, mis)) {
+        error_report("ram_block_enable_notify failed");
+        close(mis->userfault_event_fd);
+        close(mis->userfault_fd);
I don't think these close() calls are safe. This code is just after starting the fault thread, and the fault thread has a poll() call on these fd's, so we can't close them until we've instructed that thread to exit.
We should fall out through postcopy_ram_incoming_cleanup, and because the thread exists it should do a notify to the thread, a join and then only later do the close calls.

Quoted 5 lines

Quoted 1 line
Dave
I see the postcopy_ram_incoming_cleanup() already include the notify & join the fault thread & close these two fds and other more cleanup, thus that means directly replace the above two close() with postcopy_ram_incoming_cleanup() is ok, right? :)

Yes; I think I'd do that in loadvm_postcopy_handle_listen perhaps. It's a weird case, normally if it fails before 'listen' then we call cleanup in process_incoming_migration_co, but after listen we call it at the bottom of the listen thread. Here we've got a half-state where we're trying and failing to enter listen.

Quoted 5 lines

BTW, does the parameter "&local_err" for multifd_save_cleanup(&local_err) is reserved for some reason? As I notice no code is using this error parameter, and if it is reserved for nothing I'd like to delete it just as the second paragraph
of the commit message said. :)

Yes, that's OK; it might be better as a separate patch; I suspect maybe in an earlier version it had an error case.

Dave

Quoted 47 lines

Have a nice day, thanks
Fei

Quoted 44 lines

Quoted 41 lines

migration/postcopy-ram.c +0 -0

          return -1;
      }

migration/ram.c +3 -4

index 8f03afe228..57cb1bee3d 100644
@@ -922,7 +922,7 @@ static void multifd_send_terminate_threads(Error *err)
      }
  }
-int multifd_save_cleanup(Error **errp)
+int multifd_save_cleanup(void)
  {
      int i;
      int ret = 0;

@@ -1082,9 +1082,8 @@ static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
      if (qio_task_propagate_error(task, &local_err)) {
          migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
-        if (multifd_save_cleanup(&local_err) != 0) {
-            migrate_set_error(migrate_get_current(), local_err);
-        }
+        multifd_save_cleanup();
+        migrate_set_error(migrate_get_current(), local_err);
      } else {
          p->c = QIO_CHANNEL(sioc);
          qio_channel_set_delay(p->c, false);

migration/ram.h +1 -1

index 046d3074be..0d1215209e 100644
@@ -43,7 +43,7 @@ uint64_t ram_bytes_remaining(void);
  uint64_t ram_bytes_total(void);
  int multifd_save_setup(void);
-int multifd_save_cleanup(Error **errp);
+int multifd_save_cleanup(void);
  int multifd_load_setup(void);
  int multifd_load_cleanup(Error **errp);
  bool multifd_recv_all_channels_created(void);

Signature · 1 line

2.13.7

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

Fei Li

author

2018-11-01 05:20 UTC #15 lore raw

Re: [Qemu-devel] [PATCH RFC v6 6/7] migration: fix some error handling

Quoted 63 lines · Dr. David Alan Gilbert

Quoted 57 lines · * Fei Li ([email protected]) wrote:
Quoted 55 lines · Dr. David Alan Gilbert
Quoted 40 lines · * Fei Li ([email protected]) wrote:
Add error handling for qemu_ram_foreach_migratable_block() when it fails.
Always call migrate_set_error() to set the error state without relying on whether multifd_save_cleanup() succeeds. As the passed &local_err is never used in multifd_save_cleanup(), remove it.
Signed-off-by Fei Li <[email protected]>
---
   migration/migration.c    | 5 +----
   migration/postcopy-ram.c | 3 +++
   migration/ram.c          | 7 +++----
   migration/ram.h          | 2 +-
   4 files changed, 8 insertions(+), 9 deletions(-)
migration/migration.c +1 -4
index 87dfc7374f..3b8b7ab4f9 100644
@@ -1377,7 +1377,6 @@ static void migrate_fd_cleanup(void *opaque)
       qemu_savevm_state_cleanup();
       if (s->to_dst_file) {
-        Error *local_err = NULL;
           QEMUFile *tmp;
           trace_migrate_fd_cleanup();
@@ -1388,9 +1387,7 @@ static void migrate_fd_cleanup(void *opaque)
           }
           qemu_mutex_lock_iothread();
-        if (multifd_save_cleanup(&local_err) != 0) {
-            error_report_err(local_err);
-        }
+        multifd_save_cleanup();
           qemu_mutex_lock(&s->qemu_file_lock);
           tmp = s->to_dst_file;
           s->to_dst_file = NULL;
migration/postcopy-ram.c +0 -0
index e5c02a32c5..4ca2bc02b3 100644
migration/postcopy-ram.c int postcopy_ram_enable_notify(MigrationIncomingState *mis) · line 1121
Quoted 5 lines
migration/postcopy-ram.c +3 -0
@@ -1117,6 +1117,9 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis)
       /* Mark so that we get notified of accesses to unwritten areas */
       if (qemu_ram_foreach_migratable_block(ram_block_enable_notify, mis)) {
+        error_report("ram_block_enable_notify failed");
+        close(mis->userfault_event_fd);
+        close(mis->userfault_fd);
I don't think these close() calls are safe. This code is just after starting the fault thread, and the fault thread has a poll() call on these fd's, so we can't close them until we've instructed that thread to exit.
We should fall out through postcopy_ram_incoming_cleanup, and because the thread exists it should do a notify to the thread, a join and then only later do the close calls.
Quoted 5 lines
Quoted 1 line
Dave
I see the postcopy_ram_incoming_cleanup() already include the notify & join the fault thread & close these two fds and other more cleanup, thus that means directly replace the above two close() with postcopy_ram_incoming_cleanup() is ok, right? :)

Quoted 5 lines

Yes; I think I'd do that in loadvm_postcopy_handle_listen perhaps. It's a weird case, normally if it fails before 'listen' then we call cleanup in process_incoming_migration_co, but after listen we call it at the bottom of the listen thread. Here we've got a half-state where we're trying and failing to enter listen.

Ok, I will put the cleanup() inside of loadvm_postcopy_handle_liste(), thanks for the advise. :)

Quoted 10 lines

Quoted 6 lines
BTW, does the parameter "&local_err" for multifd_save_cleanup(&local_err) is reserved for some reason? As I notice no code is using this error parameter, and if it is reserved for nothing I'd like to delete it just as the second paragraph
of the commit message said. :)
Yes, that's OK; it might be better as a separate patch; I suspect maybe in an earlier version it had an error case.
Dave

Thanks for the suggestion, will separate into two patches in the next version.

Have a nice day
Fei

Quoted 49 lines

Quoted 46 lines

Have a nice day, thanks
Fei

Quoted 43 lines

Quoted 40 lines

migration/postcopy-ram.c +0 -0

           return -1;
       }

migration/ram.c +3 -4

index 8f03afe228..57cb1bee3d 100644
@@ -922,7 +922,7 @@ static void multifd_send_terminate_threads(Error *err)
       }
   }
-int multifd_save_cleanup(Error **errp)
+int multifd_save_cleanup(void)
   {
       int i;
       int ret = 0;

@@ -1082,9 +1082,8 @@ static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
       if (qio_task_propagate_error(task, &local_err)) {
           migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
-        if (multifd_save_cleanup(&local_err) != 0) {
-            migrate_set_error(migrate_get_current(), local_err);
-        }
+        multifd_save_cleanup();
+        migrate_set_error(migrate_get_current(), local_err);
       } else {
           p->c = QIO_CHANNEL(sioc);
           qio_channel_set_delay(p->c, false);

migration/ram.h +1 -1

index 046d3074be..0d1215209e 100644
@@ -43,7 +43,7 @@ uint64_t ram_bytes_remaining(void);
   uint64_t ram_bytes_total(void);
   int multifd_save_setup(void);
-int multifd_save_cleanup(Error **errp);
+int multifd_save_cleanup(void);
   int multifd_load_setup(void);
   int multifd_load_cleanup(Error **errp);
   bool multifd_recv_all_channels_created(void);

Signature · 1 line

2.13.7

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

Fei Li

author

2018-10-29 12:59 UTC #16 lore raw

RFC PATCH v6 7/7 qemu_thread_create: propagate the error to callers to handle

In reply to Fei Li

Make qemu_thread_create() return a Boolean to indicate if it succeeds rather than failing with an error. And add an Error parameter to hold the error message and let the callers handle it.

Signed-off-by Fei Li <[email protected]>

---
 cpus.c                      | 45 ++++++++++++++++++++++++++-------------
 dump.c                      |  6 ++++--
 hw/misc/edu.c               |  6 ++++--
 hw/ppc/spapr_hcall.c        | 10 +++++++--
 hw/rdma/rdma_backend.c      |  4 +++-
 hw/usb/ccid-card-emulated.c | 13 ++++++++----
 include/qemu/thread.h       |  4 ++--
 io/task.c                   |  3 ++-
 iothread.c                  | 16 +++++++++-----
 migration/migration.c       | 52 +++++++++++++++++++++++++++++----------------
 migration/postcopy-ram.c    | 14 ++++++++++--
 migration/ram.c             | 41 ++++++++++++++++++++++++++---------
 migration/savevm.c          | 11 +++++++---
 tests/atomic_add-bench.c    |  3 ++-
 tests/iothread.c            |  2 +-
 tests/qht-bench.c           |  3 ++-
 tests/rcutorture.c          |  3 ++-
 tests/test-aio.c            |  2 +-
 tests/test-rcu-list.c       |  3 ++-
 ui/vnc-jobs.c               | 17 ++++++++++-----
 ui/vnc-jobs.h               |  2 +-
 ui/vnc.c                    |  4 +++-
 util/compatfd.c             | 12 +++++++++--
 util/oslib-posix.c          | 17 +++++++++++----
 util/qemu-thread-posix.c    | 24 ++++++++++++++-------
 util/qemu-thread-win32.c    | 16 ++++++++++----
 util/rcu.c                  |  3 ++-
 util/thread-pool.c          |  4 +++-
 28 files changed, 240 insertions(+), 100 deletions(-)

cpus.c +30 ~~-15~~

index ed71618e1f..0510f90e06 100644
@@ -1949,15 +1949,20 @@ static void qemu_tcg_init_vcpu(CPUState *cpu, Error **errp)
             snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/TCG",
                  cpu->cpu_index);
 
-            qemu_thread_create(cpu->thread, thread_name, qemu_tcg_cpu_thread_fn,
-                               cpu, QEMU_THREAD_JOINABLE);
+            if (!qemu_thread_create(cpu->thread, thread_name,
+                                    qemu_tcg_cpu_thread_fn, cpu,
+                                    QEMU_THREAD_JOINABLE, errp)) {
+                return;
+            }
 
         } else {
             /* share a single thread for all cpus with TCG */
             snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "ALL CPUs/TCG");
-            qemu_thread_create(cpu->thread, thread_name,
-                               qemu_tcg_rr_cpu_thread_fn,
-                               cpu, QEMU_THREAD_JOINABLE);
+            if (!qemu_thread_create(cpu->thread, thread_name,
+                                    qemu_tcg_rr_cpu_thread_fn, cpu,
+                                    QEMU_THREAD_JOINABLE, errp)) {
+                return;
+            }
 
             single_tcg_halt_cond = cpu->halt_cond;
             single_tcg_cpu_thread = cpu->thread;

@@ -1985,8 +1990,10 @@ static void qemu_hax_start_vcpu(CPUState *cpu, Error **errp)
 
     snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/HAX",
              cpu->cpu_index);
-    qemu_thread_create(cpu->thread, thread_name, qemu_hax_cpu_thread_fn,
-                       cpu, QEMU_THREAD_JOINABLE);
+    if (!qemu_thread_create(cpu->thread, thread_name, qemu_hax_cpu_thread_fn,
+                            cpu, QEMU_THREAD_JOINABLE, errp)) {
+        return;
+    }
 #ifdef _WIN32
     cpu->hThread = qemu_thread_get_handle(cpu->thread);
 #endif

@@ -2001,8 +2008,10 @@ static void qemu_kvm_start_vcpu(CPUState *cpu, Error **errp)
     qemu_cond_init(cpu->halt_cond);
     snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/KVM",
              cpu->cpu_index);
-    qemu_thread_create(cpu->thread, thread_name, qemu_kvm_cpu_thread_fn,
-                       cpu, QEMU_THREAD_JOINABLE);
+    if (!qemu_thread_create(cpu->thread, thread_name, qemu_kvm_cpu_thread_fn,
+                            cpu, QEMU_THREAD_JOINABLE, errp)) {
+        /* keep 'if' here in case there is further error handling logic */
+    }
 }
 
 static void qemu_hvf_start_vcpu(CPUState *cpu, Error **errp)

@@ -2019,8 +2028,10 @@ static void qemu_hvf_start_vcpu(CPUState *cpu, Error **errp)
 
     snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/HVF",
              cpu->cpu_index);
-    qemu_thread_create(cpu->thread, thread_name, qemu_hvf_cpu_thread_fn,
-                       cpu, QEMU_THREAD_JOINABLE);
+    if (!qemu_thread_create(cpu->thread, thread_name, qemu_hvf_cpu_thread_fn,
+                            cpu, QEMU_THREAD_JOINABLE, errp)) {
+        /* keep 'if' here in case there is further error handling logic */
+    }
 }
 
 static void qemu_whpx_start_vcpu(CPUState *cpu, Error **errp)

@@ -2032,8 +2043,10 @@ static void qemu_whpx_start_vcpu(CPUState *cpu, Error **errp)
     qemu_cond_init(cpu->halt_cond);
     snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/WHPX",
              cpu->cpu_index);
-    qemu_thread_create(cpu->thread, thread_name, qemu_whpx_cpu_thread_fn,
-                       cpu, QEMU_THREAD_JOINABLE);
+    if (!qemu_thread_create(cpu->thread, thread_name, qemu_whpx_cpu_thread_fn,
+                            cpu, QEMU_THREAD_JOINABLE, errp)) {
+        return;
+    }
 #ifdef _WIN32
     cpu->hThread = qemu_thread_get_handle(cpu->thread);
 #endif

@@ -2048,8 +2061,10 @@ static void qemu_dummy_start_vcpu(CPUState *cpu, Error **errp)
     qemu_cond_init(cpu->halt_cond);
     snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/DUMMY",
              cpu->cpu_index);
-    qemu_thread_create(cpu->thread, thread_name, qemu_dummy_cpu_thread_fn, cpu,
-                       QEMU_THREAD_JOINABLE);
+    if (!qemu_thread_create(cpu->thread, thread_name, qemu_dummy_cpu_thread_fn,
+                           cpu, QEMU_THREAD_JOINABLE, errp)) {
+        /* keep 'if' here in case there is further error handling logic */
+    }
 }
 
 bool qemu_init_vcpu(CPUState *cpu, Error **errp)

dump.c +4 -2

index 4ec94c5e25..1f003aff9a 100644
@@ -2020,8 +2020,10 @@ void qmp_dump_guest_memory(bool paging, const char *file,
     if (detach_p) {
         /* detached dump */
         s->detached = true;
-        qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread,
-                           s, QEMU_THREAD_DETACHED);
+        if (!qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread,
+                                s, QEMU_THREAD_DETACHED, errp)) {
+            /* keep 'if' here in case there is further error handling logic */
+        }
     } else {
         /* sync dump */
         dump_process(s, errp);

hw/misc/edu.c +4 -2

index cdcf550dd7..6684c60a96 100644
@@ -355,8 +355,10 @@ static void pci_edu_realize(PCIDevice *pdev, Error **errp)
 
     qemu_mutex_init(&edu->thr_mutex);
     qemu_cond_init(&edu->thr_cond);
-    qemu_thread_create(&edu->thread, "edu", edu_fact_thread,
-                       edu, QEMU_THREAD_JOINABLE);
+    if (!qemu_thread_create(&edu->thread, "edu", edu_fact_thread,
+                            edu, QEMU_THREAD_JOINABLE, errp)) {
+        return;
+    }
 
     memory_region_init_io(&edu->mmio, OBJECT(edu), &edu_mmio_ops, edu,
                     "edu-mmio", 1 * MiB);

hw/ppc/spapr_hcall.c +8 -2

index ae913d070f..7c16ade04a 100644
@@ -478,6 +478,7 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu,
     sPAPRPendingHPT *pending = spapr->pending_hpt;
     uint64_t current_ram_size;
     int rc;
+    Error *local_err = NULL;
 
     if (spapr->resize_hpt == SPAPR_RESIZE_HPT_DISABLED) {
         return H_AUTHORITY;

@@ -538,8 +539,13 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu,
     pending->shift = shift;
     pending->ret = H_HARDWARE;
 
-    qemu_thread_create(&pending->thread, "sPAPR HPT prepare",
-                       hpt_prepare_thread, pending, QEMU_THREAD_DETACHED);
+    if (!qemu_thread_create(&pending->thread, "sPAPR HPT prepare",
+                            hpt_prepare_thread, pending,
+                            QEMU_THREAD_DETACHED, &local_err)) {
+        error_reportf_err(local_err, "failed to create hpt_prepare_thread: ");
+        g_free(pending);
+        return H_RESOURCE;
+    }
 
     spapr->pending_hpt = pending;

hw/rdma/rdma_backend.c +3 -1

index d7a4bbd91f..53a2bd0d85 100644
@@ -164,8 +164,10 @@ static void start_comp_thread(RdmaBackendDev *backend_dev)
     snprintf(thread_name, sizeof(thread_name), "rdma_comp_%s",
              ibv_get_device_name(backend_dev->ib_dev));
     backend_dev->comp_thread.run = true;
+    /* FIXME: let the further caller handle the error instead of abort() here */
     qemu_thread_create(&backend_dev->comp_thread.thread, thread_name,
-                       comp_handler_thread, backend_dev, QEMU_THREAD_DETACHED);
+                       comp_handler_thread, backend_dev,
+                       QEMU_THREAD_DETACHED, &error_abort);
 }
 
 void rdma_backend_register_comp_handler(void (*handler)(int status,

hw/usb/ccid-card-emulated.c +9 -4

index 5c8b3c9907..09682b5d30 100644
@@ -538,10 +538,15 @@ static void emulated_realize(CCIDCardState *base, Error **errp)
         error_setg(errp, "%s: failed to initialize vcard", TYPE_EMULATED_CCID);
         return;
     }
-    qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread,
-                       card, QEMU_THREAD_JOINABLE);
-    qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", handle_apdu_thread,
-                       card, QEMU_THREAD_JOINABLE);
+    if (!qemu_thread_create(&card->event_thread_id, "ccid/event", event_thread,
+                            card, QEMU_THREAD_JOINABLE, errp)) {
+        return;
+    }
+    if (!qemu_thread_create(&card->apdu_thread_id, "ccid/apdu",
+                            handle_apdu_thread, card,
+                            QEMU_THREAD_JOINABLE, errp)) {
+        /* keep 'if' here in case there is further error handling logic */
+    }
 }
 
 static void emulated_unrealize(CCIDCardState *base, Error **errp)

include/qemu/thread.h +2 -2

index b2661b6672..c230eb5d5b 100644
@@ -152,9 +152,9 @@ void qemu_event_reset(QemuEvent *ev);
 void qemu_event_wait(QemuEvent *ev);
 void qemu_event_destroy(QemuEvent *ev);
 
-void qemu_thread_create(QemuThread *thread, const char *name,
+bool qemu_thread_create(QemuThread *thread, const char *name,
                         void *(*start_routine)(void *),
-                        void *arg, int mode);
+                        void *arg, int mode, Error **errp);
 void *qemu_thread_join(QemuThread *thread);
 void qemu_thread_get_self(QemuThread *thread);
 bool qemu_thread_is_self(QemuThread *thread);

io/task.c +2 -1

index 2886a2c1bc..6d3a18ab80 100644
@@ -149,7 +149,8 @@ void qio_task_run_in_thread(QIOTask *task,
                        "io-task-worker",
                        qio_task_thread_worker,
                        data,
-                       QEMU_THREAD_DETACHED);
+                       QEMU_THREAD_DETACHED,
+                       &error_abort);
 }

iothread.c +11 -5

index 2fb1cdf55d..7335dacf0b 100644
@@ -164,9 +164,7 @@ static void iothread_complete(UserCreatable *obj, Error **errp)
                                 &local_error);
     if (local_error) {
         error_propagate(errp, local_error);
-        aio_context_unref(iothread->ctx);
-        iothread->ctx = NULL;
-        return;
+        goto fail;
     }
 
     qemu_mutex_init(&iothread->init_done_lock);

@@ -178,8 +176,12 @@ static void iothread_complete(UserCreatable *obj, Error **errp)
      */
     name = object_get_canonical_path_component(OBJECT(obj));
     thread_name = g_strdup_printf("IO %s", name);
-    qemu_thread_create(&iothread->thread, thread_name, iothread_run,
-                       iothread, QEMU_THREAD_JOINABLE);
+    if (!qemu_thread_create(&iothread->thread, thread_name, iothread_run,
+                            iothread, QEMU_THREAD_JOINABLE, errp)) {
+        g_free(thread_name);
+        g_free(name);
+        goto fail;
+    }
     g_free(thread_name);
     g_free(name);

@@ -190,6 +192,10 @@ static void iothread_complete(UserCreatable *obj, Error **errp)
                        &iothread->init_done_lock);
     }
     qemu_mutex_unlock(&iothread->init_done_lock);
+    return;
+fail:
+    aio_context_unref(iothread->ctx);
+    iothread->ctx = NULL;
 }
 
 typedef struct {

migration/migration.c +34 ~~-18~~

94 lines not on this page · raw message

migration/postcopy-ram.c +12 -2

index 4ca2bc02b3..56c13e3661 100644
@@ -1083,6 +1083,8 @@ retry:
 
 int postcopy_ram_enable_notify(MigrationIncomingState *mis)
 {
+    Error *local_err = NULL;
+
     /* Open the fd for the kernel to give us userfaults */
     mis->userfault_fd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
     if (mis->userfault_fd == -1) {

@@ -1109,8 +1111,16 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis)
     }
 
     qemu_sem_init(&mis->fault_thread_sem, 0);
-    qemu_thread_create(&mis->fault_thread, "postcopy/fault",
-                       postcopy_ram_fault_thread, mis, QEMU_THREAD_JOINABLE);
+    if (!qemu_thread_create(&mis->fault_thread, "postcopy/fault",
+                            postcopy_ram_fault_thread, mis,
+                            QEMU_THREAD_JOINABLE, &local_err)) {
+        error_reportf_err(local_err,
+                          "failed to create postcopy_ram_fault_thread: ");
+        close(mis->userfault_event_fd);
+        close(mis->userfault_fd);
+        qemu_sem_destroy(&mis->fault_thread_sem);
+        return -1;
+    }
     qemu_sem_wait(&mis->fault_thread_sem);
     qemu_sem_destroy(&mis->fault_thread_sem);
     mis->have_fault_thread = true;

migration/ram.c +31 ~~-10~~

77 lines not on this page · raw message

migration/savevm.c +8 -3

index 9992af4db4..127d9a1ae4 100644
@@ -1742,9 +1742,14 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
     mis->have_listen_thread = true;
     /* Start up the listening thread and wait for it to signal ready */
     qemu_sem_init(&mis->listen_thread_sem, 0);
-    qemu_thread_create(&mis->listen_thread, "postcopy/listen",
-                       postcopy_ram_listen_thread, NULL,
-                       QEMU_THREAD_DETACHED);
+    if (!qemu_thread_create(&mis->listen_thread, "postcopy/listen",
+                            postcopy_ram_listen_thread, NULL,
+                            QEMU_THREAD_DETACHED, &local_err)) {
+        error_reportf_err(local_err,
+                          "failed to create postcopy_ram_listen_thread: ");
+        qemu_sem_destroy(&mis->listen_thread_sem);
+        return -1;
+    }
     qemu_sem_wait(&mis->listen_thread_sem);
     qemu_sem_destroy(&mis->listen_thread_sem);

tests/atomic_add-bench.c +2 -1

index 2f6c72f63a..338b9563e3 100644
@@ -2,6 +2,7 @@
 #include "qemu/thread.h"
 #include "qemu/host-utils.h"
 #include "qemu/processor.h"
+#include "qapi/error.h"
 
 struct thread_info {
     uint64_t r;

@@ -110,7 +111,7 @@ static void create_threads(void)
 
         info->r = (i + 1) ^ time(NULL);
         qemu_thread_create(&threads[i], NULL, thread_func, info,
-                           QEMU_THREAD_JOINABLE);
+                           QEMU_THREAD_JOINABLE, &error_abort);
     }
 }

tests/iothread.c +1 -1

index 777d9eea46..f4ad992e61 100644
@@ -73,7 +73,7 @@ IOThread *iothread_new(void)
     qemu_mutex_init(&iothread->init_done_lock);
     qemu_cond_init(&iothread->init_done_cond);
     qemu_thread_create(&iothread->thread, NULL, iothread_run,
-                       iothread, QEMU_THREAD_JOINABLE);
+                       iothread, QEMU_THREAD_JOINABLE, &error_abort);
 
     /* Wait for initialization to complete */
     qemu_mutex_lock(&iothread->init_done_lock);

tests/qht-bench.c +2 -1

index 2089e2bed1..71df567ea2 100644
@@ -9,6 +9,7 @@
 #include "qemu/atomic.h"
 #include "qemu/qht.h"
 #include "qemu/rcu.h"
+#include "qapi/error.h"
 #include "exec/tb-hash-xx.h"
 
 struct thread_stats {

@@ -247,7 +248,7 @@ th_create_n(QemuThread **threads, struct thread_info **infos, const char *name,
         prepare_thread_info(&info[i], offset + i);
         info[i].func = func;
         qemu_thread_create(&th[i], name, thread_func, &info[i],
-                           QEMU_THREAD_JOINABLE);
+                           QEMU_THREAD_JOINABLE, &error_abort);
     }
 }

tests/rcutorture.c +2 -1

index 49311c82ea..0e799ff256 100644
@@ -64,6 +64,7 @@
 #include "qemu/atomic.h"
 #include "qemu/rcu.h"
 #include "qemu/thread.h"
+#include "qapi/error.h"
 
 long long n_reads = 0LL;
 long n_updates = 0L;

@@ -90,7 +91,7 @@ static void create_thread(void *(*func)(void *))
         exit(-1);
     }
     qemu_thread_create(&threads[n_threads], "test", func, &data[n_threads],
-                       QEMU_THREAD_JOINABLE);
+                       QEMU_THREAD_JOINABLE, &error_abort);
     n_threads++;
 }

tests/test-aio.c +1 -1

index 86fb73b3d5..b3ac261724 100644
@@ -154,7 +154,7 @@ static void test_acquire(void)
 
     qemu_thread_create(&thread, "test_acquire_thread",
                        test_acquire_thread,
-                       &data, QEMU_THREAD_JOINABLE);
+                       &data, QEMU_THREAD_JOINABLE, &error_abort);
 
     /* Block in aio_poll(), let other thread kick us and acquire context */
     aio_context_acquire(ctx);

tests/test-rcu-list.c +2 -1

index 2e6f70bd59..0f7da81291 100644
@@ -25,6 +25,7 @@
 #include "qemu/rcu.h"
 #include "qemu/thread.h"
 #include "qemu/rcu_queue.h"
+#include "qapi/error.h"
 
 /*
  * Test variables.

@@ -68,7 +69,7 @@ static void create_thread(void *(*func)(void *))
         exit(-1);
     }
     qemu_thread_create(&threads[n_threads], "test", func, &data[n_threads],
-                       QEMU_THREAD_JOINABLE);
+                       QEMU_THREAD_JOINABLE, &error_abort);
     n_threads++;
 }

ui/vnc-jobs.c +12 -5

index 929391f85d..35a652d1fd 100644
@@ -31,6 +31,7 @@
 #include "vnc-jobs.h"
 #include "qemu/sockets.h"
 #include "qemu/main-loop.h"
+#include "qapi/error.h"
 #include "block/aio.h"
 
 /*

@@ -331,15 +332,21 @@ static bool vnc_worker_thread_running(void)
     return queue; /* Check global queue */
 }
 
-void vnc_start_worker_thread(void)
+bool vnc_start_worker_thread(Error **errp)
 {
     VncJobQueue *q;
 
-    if (vnc_worker_thread_running())
-        return ;
+    if (vnc_worker_thread_running()) {
+        goto out;
+    }
 
     q = vnc_queue_init();
-    qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread, q,
-                       QEMU_THREAD_DETACHED);
+    if (!qemu_thread_create(&q->thread, "vnc_worker", vnc_worker_thread,
+                            q, QEMU_THREAD_DETACHED, errp)) {
+        vnc_queue_clear(q);
+        return false;
+    }
     queue = q; /* Set global queue */
+out:
+    return true;
 }

ui/vnc-jobs.h +1 -1

index 59f66bcc35..14640593db 100644
@@ -37,7 +37,7 @@ void vnc_job_push(VncJob *job);
 void vnc_jobs_join(VncState *vs);
 
 void vnc_jobs_consume_buffer(VncState *vs);
-void vnc_start_worker_thread(void);
+bool vnc_start_worker_thread(Error **errp);
 
 /* Locks */
 static inline int vnc_trylock_display(VncDisplay *vd)

ui/vnc.c +3 -1

index 0c1b477425..0ffe9e6a5d 100644
@@ -3236,7 +3236,9 @@ void vnc_display_init(const char *id, Error **errp)
     vd->connections_limit = 32;
 
     qemu_mutex_init(&vd->mutex);
-    vnc_start_worker_thread();
+    if (!vnc_start_worker_thread(errp)) {
+        return;
+    }
 
     vd->dcl.ops = &dcl_ops;
     register_displaychangelistener(&vd->dcl);

util/compatfd.c +10 -2

index 980bd33e52..886aa249f9 100644
@@ -16,6 +16,7 @@
 #include "qemu/osdep.h"
 #include "qemu-common.h"
 #include "qemu/thread.h"
+#include "qapi/error.h"
 
 #include <sys/syscall.h>

@@ -70,6 +71,7 @@ static int qemu_signalfd_compat(const sigset_t *mask)
     struct sigfd_compat_info *info;
     QemuThread thread;
     int fds[2];
+    Error *local_err = NULL;
 
     info = malloc(sizeof(*info));
     if (info == NULL) {

@@ -88,8 +90,14 @@ static int qemu_signalfd_compat(const sigset_t *mask)
     memcpy(&info->mask, mask, sizeof(*mask));
     info->fd = fds[1];
 
-    qemu_thread_create(&thread, "signalfd_compat", sigwait_compat, info,
-                       QEMU_THREAD_DETACHED);
+    if (!qemu_thread_create(&thread, "signalfd_compat", sigwait_compat,
+                            info, QEMU_THREAD_DETACHED, &local_err)) {
+        error_reportf_err(local_err, "failed to create sigwait_compat: ");
+        close(fds[0]);
+        close(fds[1]);
+        free(info);
+        return -1;
+    }
 
     return fds[0];
 }

util/oslib-posix.c +13 -4

index fbd0dc8c57..c05ed9d020 100644
@@ -437,9 +437,12 @@ static bool touch_all_pages(char *area, size_t hpagesize, size_t numpages,
     size_t size_per_thread;
     char *addr = area;
     int i = 0;
+    int started_thread = 0;
+    Error *local_err = NULL;
 
     memset_thread_failed = false;
     memset_num_threads = get_memset_num_threads(smp_cpus);
+    started_thread = memset_num_threads;
     memset_thread = g_new0(MemsetThread, memset_num_threads);
     numpages_per_thread = (numpages / memset_num_threads);
     size_per_thread = (hpagesize * numpages_per_thread);

@@ -448,13 +451,19 @@ static bool touch_all_pages(char *area, size_t hpagesize, size_t numpages,
         memset_thread[i].numpages = (i == (memset_num_threads - 1)) ?
                                     numpages : numpages_per_thread;
         memset_thread[i].hpagesize = hpagesize;
-        qemu_thread_create(&memset_thread[i].pgthread, "touch_pages",
-                           do_touch_pages, &memset_thread[i],
-                           QEMU_THREAD_JOINABLE);
+        if (!qemu_thread_create(&memset_thread[i].pgthread, "touch_pages",
+                                do_touch_pages, &memset_thread[i],
+                                QEMU_THREAD_JOINABLE, &local_err)) {
+            error_reportf_err(local_err, "failed to create do_touch_pages: ");
+            memset_thread_failed = true;
+            started_thread = i;
+            goto out;
+        }
         addr += size_per_thread;
         numpages -= numpages_per_thread;
     }
-    for (i = 0; i < memset_num_threads; i++) {
+out:
+    for (i = 0; i < started_thread; i++) {
         qemu_thread_join(&memset_thread[i].pgthread);
     }
     g_free(memset_thread);

util/qemu-thread-posix.c +16 -8

index 289af4fab5..f2246dac02 100644
@@ -15,6 +15,7 @@
 #include "qemu/atomic.h"
 #include "qemu/notify.h"
 #include "qemu-thread-common.h"
+#include "qapi/error.h"
 
 static bool name_threads;

@@ -504,9 +505,9 @@ static void *qemu_thread_start(void *args)
     return start_routine(arg);
 }
 
-void qemu_thread_create(QemuThread *thread, const char *name,
-                       void *(*start_routine)(void*),
-                       void *arg, int mode)
+bool qemu_thread_create(QemuThread *thread, const char *name,
+                        void *(*start_routine)(void *),
+                        void *arg, int mode, Error **errp)
 {
     sigset_t set, oldset;
     int err;

@@ -515,7 +516,9 @@ void qemu_thread_create(QemuThread *thread, const char *name,
 
     err = pthread_attr_init(&attr);
     if (err) {
-        error_exit(err, __func__);
+        error_setg_errno(errp, -err, "pthread_attr_init failed: %s",
+                         strerror(err));
+        return false;
     }
 
     if (mode == QEMU_THREAD_DETACHED) {

@@ -530,16 +533,21 @@ void qemu_thread_create(QemuThread *thread, const char *name,
     qemu_thread_args->name = g_strdup(name);
     qemu_thread_args->start_routine = start_routine;
     qemu_thread_args->arg = arg;
-
     err = pthread_create(&thread->thread, &attr,
                          qemu_thread_start, qemu_thread_args);
-
-    if (err)
-        error_exit(err, __func__);
+    if (err) {
+        error_setg_errno(errp, -err, "pthread_create failed: %s",
+                         strerror(err));
+        pthread_attr_destroy(&attr);
+        g_free(qemu_thread_args->name);
+        g_free(qemu_thread_args);
+        return false;
+    }
 
     pthread_sigmask(SIG_SETMASK, &oldset, NULL);
 
     pthread_attr_destroy(&attr);
+    return true;
 }
 
 void qemu_thread_get_self(QemuThread *thread)

util/qemu-thread-win32.c +12 -4

index 1a27e1cf6f..ca4d5329e3 100644
@@ -20,6 +20,7 @@
 #include "qemu/thread.h"
 #include "qemu/notify.h"
 #include "qemu-thread-common.h"
+#include "qapi/error.h"
 #include <process.h>
 
 static bool name_threads;

@@ -388,9 +389,9 @@ void *qemu_thread_join(QemuThread *thread)
     return ret;
 }
 
-void qemu_thread_create(QemuThread *thread, const char *name,
-                       void *(*start_routine)(void *),
-                       void *arg, int mode)
+bool qemu_thread_create(QemuThread *thread, const char *name,
+                        void *(*start_routine)(void *),
+                        void *arg, int mode, Error **errp)
 {
     HANDLE hThread;
     struct QemuThreadData *data;

@@ -409,10 +410,17 @@ void qemu_thread_create(QemuThread *thread, const char *name,
     hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine,
                                       data, 0, &thread->tid);
     if (!hThread) {
-        error_exit(GetLastError(), __func__);
+        if (data->mode != QEMU_THREAD_DETACHED) {
+            DeleteCriticalSection(&data->cs);
+        }
+        error_setg_errno(errp, errno,
+                         "failed to create win32_start_routine");
+        g_free(data);
+        return false;
     }
     CloseHandle(hThread);
     thread->data = data;
+    return true;
 }
 
 void qemu_thread_get_self(QemuThread *thread)

util/rcu.c +2 -1

index 5676c22bd1..145dcdb0c6 100644
@@ -32,6 +32,7 @@
 #include "qemu/atomic.h"
 #include "qemu/thread.h"
 #include "qemu/main-loop.h"
+#include "qapi/error.h"
 #if defined(CONFIG_MALLOC_TRIM)
 #include <malloc.h>
 #endif

@@ -325,7 +326,7 @@ static void rcu_init_complete(void)
      * must have been quiescent even after forking, just recreate it.
      */
     qemu_thread_create(&thread, "call_rcu", call_rcu_thread,
-                       NULL, QEMU_THREAD_DETACHED);
+                       NULL, QEMU_THREAD_DETACHED, &error_abort);
 
     rcu_register_thread();
 }

util/thread-pool.c +3 -1

index 610646d131..ad0f980783 100644
@@ -22,6 +22,7 @@
 #include "trace.h"
 #include "block/thread-pool.h"
 #include "qemu/main-loop.h"
+#include "qapi/error.h"
 
 static void do_spawn_thread(ThreadPool *pool);

@@ -132,7 +133,8 @@ static void do_spawn_thread(ThreadPool *pool)
     pool->new_threads--;
     pool->pending_threads++;
 
-    qemu_thread_create(&t, "worker", worker_thread, pool, QEMU_THREAD_DETACHED);
+    qemu_thread_create(&t, "worker", worker_thread, pool,
+                       QEMU_THREAD_DETACHED, &error_abort);
 }
 
 static void spawn_thread_bh_fn(void *opaque)

Signature · 1 line

2.13.7