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
