Re: [Qemu-devel] [RFC] use little granularity lock to substitue qemu_mutex_lock_iothread

[Anthony Liguori]

On 06/22/2012 04:14 PM, Jan Kiszka wrote:
> On 2012-06-22 22:11, Anthony Liguori wrote:
> > On 06/22/2012 05:37 AM, Jan Kiszka wrote:
> > > On 2012-06-22 12:24, liu ping fan wrote:
> > > > On Thu, Jun 21, 2012 at 11:23 PM, Jan Kiszka<address@hidden>
> > > > wrote:
> > > > > On 2012-06-21 16:49, Liu Ping Fan wrote:
> > > > > > Nowadays, we use
> > > > > > qemu_mutex_lock_iothread()/qemu_mutex_unlock_iothread() to
> > > > > > protect the race to access the emulated dev launched by vcpu
> > > > > > threads&   iothread.
> > > > > >
> > > > > > But this lock is too big. We can break it down.
> > > > > > These patches separate the CPUArchState's protection from the other
> > > > > > devices, so we
> > > > > > can have a per-cpu lock for each CPUArchState, not the big lock any
> > > > > > longer.
> > > > >
> > > > > Anything that reduces lock dependencies is generally welcome. But can
> > > > > you specify in more details what you gain, and under which conditions?
> > > > In fact, there are several steps to break down the Qemu big lock. This
> > > > step just aims to shrink the code area protected by
> > > > qemu_mutex_lock_iothread()/qemu_mutex_unlock_iothread(). And I am
> > > > working on the following steps, which focus on breaking down the big
> > > > lock when calling handle_{io,mmio}
> > >
> > > Then let us discuss the strategy. This is important as it is unrealistic
> > > to break up the lock for all code paths. We really need to focus on
> > > goals that provide benefits for relevant use cases.
> >
> > Stefan put together a proof of concept that implemented the data-plane
> > portion of virtio-blk in a separate thread.  This is possible because of
> > I/O eventfd (we were able to select() on that fd in a separate thread).
> >
> > The performance difference between virtio-blk-pci and
> > virtio-blk-data-plane is staggering when dealing with a very large
> > storage system.
> >
> > So we'd like to get the infrastructure in place where we can start
> > multithreading devices in QEMU to we can integrate this work.
>
> Can you name the primary bits? We really need to see the whole picture
> before adding new locks. They alone are not the solution.

Sorry, not sure what you mean by "the primary bits".

At a high level, the plan is to:

1) unlock iothread before entering the do {} look in kvm_cpu_exec()
   a) reacquire the lock after the loop
   b) reacquire the lock in kvm_handle_io()
   c) introduce an unlocked memory accessor that for now, just requires the 
iothread lock() and calls cpu_physical_memory_rw()

2) focus initially on killing the lock in kvm_handle_io()
   a) the ioport table is pretty simplistic so adding fine grain locking won't 
be hard.
   b) reacquire lock right before ioport dispatch

3) allow for register ioport handlers w/o the dispatch function carrying a 
iothread
   a) this is mostly memory API plumbing

4) focus on going back and adding fine grain locking to the 
cpu_physical_memory_rw() accessor

Note that whenever possible, we should be using rwlocks instead of a normal 
mutex.  In particular, for the ioport data structures, a rwlock seems pretty 
obvious.

> > The basic plan is introduce granular locking starting at the KVM
> > dispatch level until we can get to MemoryRegion dispatch.  We'll then
> > have some way to indicate that a MemoryRegion's callbacks should be
> > invoked without holding the qemu global mutex.
>
> I don't disagree, but this end really looks like starting at the wrong
> edge. The changes are not isolated and surely not yet correct
> (run_on_cpu is broken for tcg e.g.).
>
> Then, none of this locking should be needed for in-kernel irqchips. All
> touched states are thread local or should be modifiable atomically - if
> not let's fix *that*, it's more beneficial.
>
> Actually, cpu_lock is counterproductive as it adds locking ops to a path
> where we will not need them later on in the normal configuration. User
> space irqchip is a slow path and perfectly fine to handle under BQL. So
> is VCPU control (pause/resume/run-on). It's better to focus on the fast
> path first.

To be clear, I'm not advocating introducing cpu_lock.  We should do whatever 
makes the most sense to not have to hold iothread lock while processing an exit 
from KVM.

Note that this is an RFC, the purpose of this series is to have this discussion 
:-)

Regards,

Anthony Liguori

> Jan