Re: [RFC] KVM: x86: conditionally acquire/release slots_lock on entry/exit

[Avi Kivity <avi@redhat.com>]

On 08/28/2009 01:59 AM, Marcelo Tosatti wrote:
> On Thu, Aug 27, 2009 at 07:27:48PM +0300, Avi Kivity wrote:
>    
>> On 08/27/2009 06:54 PM, Marcelo Tosatti wrote:
>>      
>>> perf report shows heavy overhead from down/up of slots_lock.
>>>
>>> Attempted to remove slots_lock by having vcpus stop on a synchronization
>>> point, but this introduced further complexity (a vcpu can be scheduled
>>> out before reaching the synchronization point, and can sched back in at
>>> points which are slots_lock protected, etc).
>>>
>>> This patch changes vcpu_enter_guest to conditionally release/acquire
>>> slots_lock in case a vcpu state bit is set.
>>>
>>> vmexit performance improves by 5-10% on UP guest.
>>>
>>>        
>> Sorry, it looks pretty complex.
>>      
> Why?
>    

There's a new locking protocol in there.  I prefer sticking with the 
existing kernel plumbing, or it gets more and more difficult knowing who 
protects what and in what order you can do things.

>> Have you considered using srcu? It seems to me down/up_read() can
>> be replaced by srcu_read_lock/unlock(), and after proper conversion
>> of memslots and io_bus to rcu_assign_pointer(), we can just add
>> synchronize_srcu() immediately after changing stuff (of course
>> mmu_lock still needs to be held when updating slots).
>>      
> I don't see RCU as being suitable because in certain operations you
> want to stop writers (on behalf of vcpus), do something, and let them
> continue afterwards. The dirty log, for example. Or any operation that
> wants to modify lockless vcpu specific data.
>    

kvm_flush_remote_tlbs() (which you'd call after mmu operations), will 
get cpus out of guest mode, and synchronize_srcu() will wait for them to 
drop the srcu "read lock".  So it really happens naturally: do an RCU 
update, send some request to all vcpus, synchronize_srcu(), done.

> Also, synchronize_srcu() is limited to preemptible sections.
>
> io_bus could use RCU, but I think being able to stop vcpus is also a
> different requirement. Do you have any suggestion on how to do it in a
> better way?
>    

We don't need to stop vcpus, just kick them out of guest mode to let 
them notice the new data.  SRCU does that well.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.