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

[Jan Kiszka]

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.

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

Jan

signature.asc
Description: OpenPGP digital signature