[Qemu-devel] thread synchronization in qcow2.c and qcow2-cluster.c

[Qemu-devel] thread synchronization in qcow2.c and qcow2-cluster.c

[Chunqiang (CQ) Tang]

Hi there,

I just started to read the code of qemu-kvm-0.12.3 recently, and was
puzzled by the thread synchronization issue in qcow2.c and
qcow2-cluster.c. Could someone please enlighten me? Thanks!

Specifically, I found that BDRVQcowState.cluster_allocs, which is a
QLIST_HEAD, may be accessed concurrently by two threads, but I could
not figure out how the two thread synchronize with each other to avoid
race conditions (e.g., through a lock?).

Stack trace of thread 1:
main -> main_loop -> kvm_main_loop -> main_loop_wait -> posix_aio_read
-> posix_aio_process_queue -> qcow_aio_write_cb ->
qcow2_alloc_cluster_offset (which may modify
BDRVQcowState.cluster_allocs)

Stack trace of thread 2:
ap_main_loop -> ... -> kvm_handle_io -> ... -> qcow_aio_writev ->
qcow_aio_write_cb -> qcow2_alloc_cluster_offset (which may modify
BDRVQcowState.cluster_allocs)

-- 
Best regards,
CQ Tang

Re: [Qemu-devel] thread synchronization in qcow2.c and qcow2-cluster.c

[Stefan Hajnoczi]

On Tue, Apr 27, 2010 at 5:06 PM, Chunqiang (CQ) Tang <tangchq@gmail.com> wrote:
> I just started to read the code of qemu-kvm-0.12.3 recently, and was
> puzzled by the thread synchronization issue in qcow2.c and
> qcow2-cluster.c. Could someone please enlighten me? Thanks!

Is this what you are looking for:

kvm-all.c:kvm_cpu_exec:
        qemu_mutex_unlock_iothread();
        ret = kvm_vcpu_ioctl(env, KVM_RUN, 0);
        qemu_mutex_lock_iothread();

and

vl.c:main_loop_wait:
    ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv);
    qemu_mutex_lock_iothread();
    if (ret > 0) {
        IOHandlerRecord *pioh;

        QLIST_FOREACH_SAFE(ioh, &io_handlers, next, pioh) {

If I understand correctly the global iothread mutex prevents block
driver code from executing concurrently.

Stefan

Re: [Qemu-devel] thread synchronization in qcow2.c and qcow2-cluster.c

[Chunqiang (CQ) Tang]

> kvm-all.c:kvm_cpu_exec:
>        qemu_mutex_unlock_iothread();
>        ret = kvm_vcpu_ioctl(env, KVM_RUN, 0);
>        qemu_mutex_lock_iothread();

Thank you for the information. I also suspected that
qemu_mutex_lock_iothread() does the synchronization. However, my
profiling showed that qemu-kvm.c:kvm_cpu_exec() in is actually
executed, instead of kvm-all.c:kvm_cpu_exec(). Also I previously
profiled all executions of qemu_mutex_lock_iothread(), and found that
it only protects the vl.c:main_loop_wai() thread but does NOT protect
the qemu-kvm.c:kvm_cpu_exec() thread. Did I miss something or is this
a defect? I did extensive profiling but still don't know the source
code well enough to confidently draw a conclusion.

For example, see the profiled execution sequence below. The
kvm_cpu_exec() thread did not perform qemu_mutex_lock_iothread(). The
locking was only performed by the vl.c:main_loop_wai() thread.

home/ctang/kvm/qemu-kvm-0.12.3/qemu-kvm.c : 2530    thread: b7e056d0
        /home/ctang/kvm/bin/qemu-system-x86_64(qemu_mutex_unlock_iothread+0x1a)
[0x8092242]
        /home/ctang/kvm/bin/qemu-system-x86_64(main_loop_wait+0x221) [0x806edef]
        /home/ctang/kvm/bin/qemu-system-x86_64(kvm_main_loop+0x1ff) [0x80916a1]
        /home/ctang/kvm/bin/qemu-system-x86_64 [0x806f5c2]
        /home/ctang/kvm/bin/qemu-system-x86_64(main+0x2e2c) [0x80736d1]
        /lib/tls/i686/cmov/libc.so.6(__libc_start_main+0xe5) [0xb7e33775]
        /home/ctang/kvm/bin/qemu-system-x86_64 [0x8068bb1]

block/qcow2-cluster.c : 721    thread: b7dc2b90
        /home/ctang/kvm/bin/qemu-system-x86_64(qcow2_alloc_cluster_offset+0x3c)
[0x81175fa]
        /home/ctang/kvm/bin/qemu-system-x86_64(qcow_aio_write_cb+0x158)
[0x8111d73]
        /home/ctang/kvm/bin/qemu-system-x86_64(qcow_aio_writev+0x94) [0x8112054]
        /home/ctang/kvm/bin/qemu-system-x86_64(bdrv_aio_writev+0xe1) [0x80fa8e9]
        /home/ctang/kvm/bin/qemu-system-x86_64 [0x81f4a96]
        /home/ctang/kvm/bin/qemu-system-x86_64 [0x81f4c04]
        /home/ctang/kvm/bin/qemu-system-x86_64(dma_bdrv_write+0x48) [0x81f4cbf]
        /home/ctang/kvm/bin/qemu-system-x86_64 [0x80a437c]
        /home/ctang/kvm/bin/qemu-system-x86_64(bmdma_cmd_writeb+0x73)
[0x80a9503]
        /home/ctang/kvm/bin/qemu-system-x86_64 [0x812b1eb]
        /home/ctang/kvm/bin/qemu-system-x86_64(cpu_outb+0x27) [0x812b4e6]
        /home/ctang/kvm/bin/qemu-system-x86_64 [0x808d267]
        /home/ctang/kvm/bin/qemu-system-x86_64(kvm_run+0x2f4) [0x808f4b8]
        /home/ctang/kvm/bin/qemu-system-x86_64(kvm_cpu_exec+0x56) [0x80907b2]
        /home/ctang/kvm/bin/qemu-system-x86_64 [0x8090f4d]
        /home/ctang/kvm/bin/qemu-system-x86_64 [0x8091098]
        /lib/tls/i686/cmov/libpthread.so.0 [0xb7fd24ff]
        /lib/tls/i686/cmov/libc.so.6(clone+0x5e) [0xb7f0149e]

/home/ctang/kvm/qemu-kvm-0.12.3/qemu-kvm.c : 2537    thread: b7e056d0
        /home/ctang/kvm/bin/qemu-system-x86_64(qemu_mutex_lock_iothread+0x1a)
[0x809229d]
        /home/ctang/kvm/bin/qemu-system-x86_64(main_loop_wait+0x25c) [0x806ee2a]
        /home/ctang/kvm/bin/qemu-system-x86_64(kvm_main_loop+0x1ff) [0x80916a1]
        /home/ctang/kvm/bin/qemu-system-x86_64 [0x806f5c2]
        /home/ctang/kvm/bin/qemu-system-x86_64(main+0x2e2c) [0x80736d1]
        /lib/tls/i686/cmov/libc.so.6(__libc_start_main+0xe5) [0xb7e33775]
        /home/ctang/kvm/bin/qemu-system-x86_64 [0x8068bb1]

Re: [Qemu-devel] thread synchronization in qcow2.c and qcow2-cluster.c

[Stefan Hajnoczi]

On Tue, Apr 27, 2010 at 9:39 PM, Chunqiang (CQ) Tang <tangchq@gmail.com> wrote:
> Thank you for the information. I also suspected that
> qemu_mutex_lock_iothread() does the synchronization. However, my
> profiling showed that qemu-kvm.c:kvm_cpu_exec() in is actually
> executed, instead of kvm-all.c:kvm_cpu_exec().

Are you using qemu-kvm.git?

Can you double check that qemu-kvm.o is being linked in instead of kvm-all.o?

I looked at the link map for qemu-system-x86_64 and verified that
kvm-all.o is linked in and qemu-kvm.o is not linked in.

Stefan

diff --git a/Makefile.target b/Makefile.target
index 1ffd802..122b951 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -30,6 +30,7 @@ endif

 PROGS=$(QEMU_PROG)

+LDFLAGS+=-Wl,-Map=$(QEMU_PROG).map
 LIBS+=-lm

 kvm.o kvm-all.o: QEMU_CFLAGS+=$(KVM_CFLAGS)

Re: [Qemu-devel] thread synchronization in qcow2.c and qcow2-cluster.c

[Stefan Hajnoczi]

On Tue, Apr 27, 2010 at 10:26 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> Can you double check that qemu-kvm.o is being linked in instead of kvm-all.o?

I looked again and noticed that qemu-kvm.c is included from kvm-all.c.
 Please ignore my question of whether qemu-kvm.c is being linked in
:).

Perhaps you'd like to post your latest findings to the KVM mailing
list at kvm@vger.kernel.org since this is a qemu-kvm question?

Stefan

Re: [Qemu-devel] thread synchronization in qcow2.c and qcow2-cluster.c

[Kevin Wolf]

Am 27.04.2010 22:39, schrieb Chunqiang (CQ) Tang:
>> kvm-all.c:kvm_cpu_exec:
>>        qemu_mutex_unlock_iothread();
>>        ret = kvm_vcpu_ioctl(env, KVM_RUN, 0);
>>        qemu_mutex_lock_iothread();
> 
> Thank you for the information. I also suspected that
> qemu_mutex_lock_iothread() does the synchronization. However, my
> profiling showed that qemu-kvm.c:kvm_cpu_exec() in is actually
> executed, instead of kvm-all.c:kvm_cpu_exec(). Also I previously
> profiled all executions of qemu_mutex_lock_iothread(), and found that
> it only protects the vl.c:main_loop_wai() thread but does NOT protect
> the qemu-kvm.c:kvm_cpu_exec() thread. Did I miss something or is this
> a defect? I did extensive profiling but still don't know the source
> code well enough to confidently draw a conclusion.

That code path exists only in qemu-kvm, but yes, looks wrong to me. The
block drivers are definitely not prepared to run in parallel in multiple
threads.

Avi, something missing there in qemu-kvm?

Kevin

Re: [Qemu-devel] thread synchronization in qcow2.c and qcow2-cluster.c

[Chunqiang (CQ) Tang]

On Wed, Apr 28, 2010 at 4:13 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Apr 27, 2010 at 10:26 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> Can you double check that qemu-kvm.o is being linked in instead of kvm-all.o?
>
> I looked again and noticed that qemu-kvm.c is included from kvm-all.c.
>  Please ignore my question of whether qemu-kvm.c is being linked in
> :).
>
> Perhaps you'd like to post your latest findings to the KVM mailing
> list at kvm@vger.kernel.org since this is a qemu-kvm question?
>
> Stefan

Yes, this is a qemu-kvm question. I will summarize my finding and send
it to kvm@vger.kernel.org . Thank you for the answers and for
informing the KVM mailing list.

Regards,
CQ Tang