Re: [PATCH 2/2] KVM: Convert read-only users of vm_list to RCU

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2011-02-10 13:34, Avi Kivity wrote:
> On 02/10/2011 01:31 PM, Jan Kiszka wrote:
>>>
>>>>
>>>>  @@ -3607,10 +3607,14 @@ static int mmu_shrink(struct shrinker *shrink, int nr_to_scan, gfp_t gfp_mask)
>>>>    		spin_unlock(&kvm->mmu_lock);
>>>>    		srcu_read_unlock(&kvm->srcu, idx);
>>>>    	}
>>>>  -	if (kvm_freed)
>>>>  -		list_move_tail(&kvm_freed->vm_list,&vm_list);
>>>>  +	if (kvm_freed) {
>>>>  +		raw_spin_lock(&kvm_lock);
>>>>  +		if (!kvm->deleted)
>>>>  +			list_move_tail(&kvm_freed->vm_list,&vm_list);
>>>
>>>  There is no list_move_tail_rcu().
>>
>> ...specifically not for this one.
> 
> Well, we can add one if needed (and if possible).

I can have a look, at least at the lower hanging fruits.

> 
>>>
>>>  Why check kvm->deleted?  it's in the process of being torn down anyway,
>>>  it doesn't matter if mmu_shrink or kvm_destroy_vm pulls the trigger.
>>
>> kvm_destroy_vm removes a vm from the list while mmu_shrink is running.
>> Then mmu_shrink's list_move_tail will re-add that vm to the list tail
>> again (unless already the removal in move_tail produces a crash).
> 
> It's too subtle.  Communication across threads with a variable needs 
> memory barriers (even though they're nops on x86) and documentation.

The barriers are provided by this spin lock we acquire for testing are
modifying deleted.

> 
> btw, not even sure if it's legal: you have a mutating call within an rcu 
> read critical section for the same object.  If synchronize_rcu() were 
> called there, would it ever terminate?

Why not? kvm_destroy_vm is not preventing blocking mmu_shrink to acquire
the kvm_lock where we then find the vm deleted and release both kvm_lock
and the rcu read "lock" afterwards.

> 
> (not that synchronize_rcu() is a good thing there, better do it with 
> call_rcu()).

What's the benefit? The downside is a bit more complexity as you need an
additional callback handler.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux