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

[Jan Kiszka]

On 2012-06-22 23:44, Anthony Liguori wrote:
> 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()

Right, that's what we have here as well. The latter is modeled as a so
called "I/O pathway", a thread-based execution context for
frontend/backend pairs with some logic to transfer certain I/O requests
asynchronously to the pathway thread.

The tricky part was to get nested requests right, i.e. when a requests
triggers another one from within the device model. This is where things
get ugly. In theory, you can end up with a vm deadlock if you just apply
per-device locking. I'm currently trying to rebase our patches, review
and document the logic behind it.

> 
> 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

We skipped this as our NICs didn't do PIO, but you clearly need it for
virtio.

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

In the end, PIO and MMIO should use the same patterns - and will face
the same challenges. Ideally, we model things very similar right from
the start.

And then there is also

5) provide direct IRQ delivery from the device model to the IRQ chip.
That's much like what we need for VFIO and KVM device assignment. But
here we won't be able to cheat and ignore correct generation of vmstates
of the bypassed PCI host bridges etc... Which leads me to that other
thread about how to handle this for PCI device pass-through.
Contributions to that discussion are welcome as well.

> 
> 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.

I think we should mostly be fine with a "big hammer" rwlock: unlocked
read access from VCPUs and iothreads, and vmstop/resume around
modifications of fast path data structures (like the memory region
hierarchy or the PIO table). Where that's not sufficient, RCU will be
needed. Sleeping rwlocks have horrible semantics (specifically when
thread priorities come into play) and are performance-wise inferior. We
should avoid them completely.

> 
>>
>>>
>>> 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.

Good that we agree. :)

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

Yep, I think we have it now ;). Hope I can contribute some code bits to
it soon, though I didn't schedule this task for the next week.

Jan

signature.asc
Description: OpenPGP digital signature