Re: [Qemu-devel] [RFC PATCH v5 13/24] kvm: remove BQL lock/unlock

[Paolo Bonzini]

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