| Message ID | 20180123085432.3419.84711.stgit@pasha-VirtualBox |
|---|---|
| State | New |
| Headers | show |
| Series | replay additions | expand |
On 23/01/2018 03:54, Pavel Dovgalyuk wrote: > @@ -1861,7 +1861,6 @@ int kvm_cpu_exec(CPUState *cpu) > return EXCP_HLT; > } > > - qemu_mutex_unlock_iothread(); > cpu_exec_start(cpu); > do { > MemTxAttrs attrs; So this means that kvm_cpu_exec is now called without taking the BQL. I'll leave aside the bisectability issue (patch 11 breaks kvm_cpu_exec, because this qemu_mutex_unlock_iothread now has an assertion failure), since they are easily fixed by squashing patches 11-13 together. The lines immediately above are if (kvm_arch_process_async_events(cpu)) { atomic_set(&cpu->exit_request, 0); return EXCP_HLT; } So this means that, after patch 11, kvm_arch_process_async_events went from "called with BQL taken" to "called with BQL not taken". And that is completely broken, because it accesses cs->interrupt_request just like cpu_has_work. Previous reviews have ascertained that accessing cs->interrupt_request requires taking the BQL; this is the same, except worse because now we can even *write* cs->interrupt_request (clear bits) without taking the lock. I don't need to explain to you why this is bad. .------------------------------------------------. | .--------------------------------------------. | | | This is not how you are supposed to modify | | | | multi-threaded code. | | | '--------------------------------------------' | '------------------------------------------------' If something can be accessed outside a lock, e.g. with atomics, that has to be documented. In addition, if it's not obvious whether a function is called with a lock or without, you add comments that make it clear. Take a lock at accel/tcg/translate-all.c or exec.c for examples. This is the last pass through this series that I make. I'll pick the patches that I consider ready, for everything else you'll have to find a reviewer that is willing to look through the series and vouch for it with a "Reviewed-by". Paolo
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index 9628512..d708c7f 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -1861,7 +1861,6 @@ int kvm_cpu_exec(CPUState *cpu) return EXCP_HLT; } - qemu_mutex_unlock_iothread(); cpu_exec_start(cpu); do { MemTxAttrs attrs; @@ -1992,7 +1991,6 @@ int kvm_cpu_exec(CPUState *cpu) } while (ret == 0); cpu_exec_end(cpu); - qemu_mutex_lock_iothread(); if (ret < 0) { cpu_dump_state(cpu, stderr, fprintf, CPU_DUMP_CODE);
BQL now is used only for waiting for IO events. This patch also removes lock/unlock from kvm module. Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> --- accel/kvm/kvm-all.c | 2 -- 1 file changed, 2 deletions(-)