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

Jan Kiszka
2011-02-10T11:32:10Z

On 2011-02-10 11:16, Avi Kivity wrote:
> On 02/08/2011 01:55 PM, Jan Kiszka wrote:
>> Only for walking the list of VMs, we do not need to hold the preemption
>> disabling kvm_lock. Convert stat services, the cpufreq callback and
>> mmu_shrink to RCU. For the latter, special care is required to
>> synchronize its list_move_tail with kvm_destroy_vm.
>>
>>
>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>> index b6a9963..e9d0ed8 100644
>> --- a/arch/x86/kvm/mmu.c
>> +++ b/arch/x86/kvm/mmu.c
>> @@ -3587,9 +3587,9 @@ static int mmu_shrink(struct shrinker *shrink, int nr_to_scan, gfp_t gfp_mask)
>> if (nr_to_scan == 0)
>> goto out;
>>
>> - raw_spin_lock(&kvm_lock);
>> + rcu_read_lock();
>>
>> - list_for_each_entry(kvm,&vm_list, vm_list) {
>> + list_for_each_entry_rcu(kvm,&vm_list, vm_list) {
>> int idx, freed_pages;
>> LIST_HEAD(invalid_list);
>
> Have to #include rculist.h,

OK.

> and to change all list operations on vm_list
> to rcu variants.

Not sure if we have such variants for all cases...

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

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

Jan

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