Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1341954 > unrolled thread

[PATCH 09/12] KVM: MMU: coalesce zapping page after mmu_sync_children

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 09/12] KVM: MMU: coalesce zapping page after mmu_sync_children Paolo Bonzini <pbonzini@redhat.com> - 2016-02-24 14:20 +0100
    Re: [PATCH 09/12] KVM: MMU: coalesce zapping page after  mmu_sync_children Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp> - 2016-02-25 03:20 +0100
      Re: [PATCH 09/12] KVM: MMU: coalesce zapping page after  mmu_sync_children Xiao Guangrong <guangrong.xiao@linux.intel.com> - 2016-02-25 08:40 +0100
        Re: [PATCH 09/12] KVM: MMU: coalesce zapping page after  mmu_sync_children Paolo Bonzini <pbonzini@redhat.com> - 2016-02-25 09:50 +0100
          Re: [PATCH 09/12] KVM: MMU: coalesce zapping page after  mmu_sync_children Xiao Guangrong <guangrong.xiao@linux.intel.com> - 2016-02-25 10:20 +0100
            Re: [PATCH 09/12] KVM: MMU: coalesce zapping page after  mmu_sync_children Paolo Bonzini <pbonzini@redhat.com> - 2016-02-25 11:00 +0100
      Re: [PATCH 09/12] KVM: MMU: coalesce zapping page after  mmu_sync_children Paolo Bonzini <pbonzini@redhat.com> - 2016-02-25 09:50 +0100

#1341954 — [PATCH 09/12] KVM: MMU: coalesce zapping page after mmu_sync_children

Move the call to kvm_mmu_flush_or_zap outside the loop.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/mmu.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 725316df32ec..6d47b5c43246 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2029,24 +2029,27 @@ static void mmu_sync_children(struct kvm_vcpu *vcpu,
 	struct mmu_page_path parents;
 	struct kvm_mmu_pages pages;
 	LIST_HEAD(invalid_list);
+	bool flush = false;
 
 	while (mmu_unsync_walk(parent, &pages)) {
 		bool protected = false;
-		bool flush = false;
 
 		for_each_sp(pages, sp, parents, i)
 			protected |= rmap_write_protect(vcpu, sp->gfn);
 
-		if (protected)
+		if (protected) {
 			kvm_flush_remote_tlbs(vcpu->kvm);
+			flush = false;
+		}
 
 		for_each_sp(pages, sp, parents, i) {
 			flush |= kvm_sync_page(vcpu, sp, &invalid_list);
 			mmu_pages_clear_parents(&parents);
 		}
-		kvm_mmu_flush_or_zap(vcpu, &invalid_list, false, flush);
 		cond_resched_lock(&vcpu->kvm->mmu_lock);
 	}
+
+	kvm_mmu_flush_or_zap(vcpu, &invalid_list, false, flush);
 }
 
 static void __clear_sp_write_flooding_count(struct kvm_mmu_page *sp)
-- 
1.8.3.1

[toc] | [next] | [standalone]

#1342640 — Re: [PATCH 09/12] KVM: MMU: coalesce zapping page after mmu_sync_children

On 2016/02/24 22:17, Paolo Bonzini wrote:
> Move the call to kvm_mmu_flush_or_zap outside the loop.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   arch/x86/kvm/mmu.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 725316df32ec..6d47b5c43246 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -2029,24 +2029,27 @@ static void mmu_sync_children(struct kvm_vcpu *vcpu,
>   	struct mmu_page_path parents;
>   	struct kvm_mmu_pages pages;
>   	LIST_HEAD(invalid_list);
> +	bool flush = false;
>   
>   	while (mmu_unsync_walk(parent, &pages)) {
>   		bool protected = false;
> -		bool flush = false;
>   
>   		for_each_sp(pages, sp, parents, i)
>   			protected |= rmap_write_protect(vcpu, sp->gfn);
>   
> -		if (protected)
> +		if (protected) {
>   			kvm_flush_remote_tlbs(vcpu->kvm);
> +			flush = false;
> +		}
>   
>   		for_each_sp(pages, sp, parents, i) {
>   			flush |= kvm_sync_page(vcpu, sp, &invalid_list);
>   			mmu_pages_clear_parents(&parents);
>   		}
> -		kvm_mmu_flush_or_zap(vcpu, &invalid_list, false, flush);
>   		cond_resched_lock(&vcpu->kvm->mmu_lock);

This may release the mmu_lock before committing the zapping.
Is it safe?  If so, we may want to see the reason in the changelog.

  Takuya

>   	}
> +
> +	kvm_mmu_flush_or_zap(vcpu, &invalid_list, false, flush);
>   }
>   
>   static void __clear_sp_write_flooding_count(struct kvm_mmu_page *sp)
> 

[toc] | [prev] | [next] | [standalone]

#1342893 — Re: [PATCH 09/12] KVM: MMU: coalesce zapping page after mmu_sync_children

On 02/25/2016 10:15 AM, Takuya Yoshikawa wrote:
> On 2016/02/24 22:17, Paolo Bonzini wrote:
>> Move the call to kvm_mmu_flush_or_zap outside the loop.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>    arch/x86/kvm/mmu.c | 9 ++++++---
>>    1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>> index 725316df32ec..6d47b5c43246 100644
>> --- a/arch/x86/kvm/mmu.c
>> +++ b/arch/x86/kvm/mmu.c
>> @@ -2029,24 +2029,27 @@ static void mmu_sync_children(struct kvm_vcpu *vcpu,
>>    	struct mmu_page_path parents;
>>    	struct kvm_mmu_pages pages;
>>    	LIST_HEAD(invalid_list);
>> +	bool flush = false;
>>
>>    	while (mmu_unsync_walk(parent, &pages)) {
>>    		bool protected = false;
>> -		bool flush = false;
>>
>>    		for_each_sp(pages, sp, parents, i)
>>    			protected |= rmap_write_protect(vcpu, sp->gfn);
>>
>> -		if (protected)
>> +		if (protected) {
>>    			kvm_flush_remote_tlbs(vcpu->kvm);
>> +			flush = false;
>> +		}
>>
>>    		for_each_sp(pages, sp, parents, i) {
>>    			flush |= kvm_sync_page(vcpu, sp, &invalid_list);
>>    			mmu_pages_clear_parents(&parents);
>>    		}
>> -		kvm_mmu_flush_or_zap(vcpu, &invalid_list, false, flush);
>>    		cond_resched_lock(&vcpu->kvm->mmu_lock);
>
> This may release the mmu_lock before committing the zapping.
> Is it safe?  If so, we may want to see the reason in the changelog.

It is unsafe indeed, please do not do it.

[toc] | [prev] | [next] | [standalone]

#1342981 — Re: [PATCH 09/12] KVM: MMU: coalesce zapping page after mmu_sync_children

On 25/02/2016 08:35, Xiao Guangrong wrote:
>> This may release the mmu_lock before committing the zapping.
>> Is it safe?  If so, we may want to see the reason in the changelog.
> 
> It is unsafe indeed, please do not do it.

Can you explain why?  kvm_zap_obsolete_pages does the same.

Paolo

[toc] | [prev] | [next] | [standalone]

#1343011 — Re: [PATCH 09/12] KVM: MMU: coalesce zapping page after mmu_sync_children

On 02/25/2016 04:49 PM, Paolo Bonzini wrote:
>
>
> On 25/02/2016 08:35, Xiao Guangrong wrote:
>>> This may release the mmu_lock before committing the zapping.
>>> Is it safe?  If so, we may want to see the reason in the changelog.
>>
>> It is unsafe indeed, please do not do it.
>
> Can you explain why?  kvm_zap_obsolete_pages does the same.

It's not the same, please see the comment in  kvm_mmu_invalidate_zap_all_pages:
	/*
	 * Notify all vcpus to reload its shadow page table
	 * and flush TLB. Then all vcpus will switch to new
	 * shadow page table with the new mmu_valid_gen.
	 *
	 * Note: we should do this under the protection of
	 * mmu-lock, otherwise, vcpu would purge shadow page
	 * but miss tlb flush.
	 */
	kvm_reload_remote_mmus(kvm);

That means the tlb is flushed before releasing mmu-lock.

A example is in rmap_write_protect(), when KVM creates a shadow page table for
the the guest, it detects no spte pointing to the gfn, so tlb is not flushed so
that guest can freely updates its pte.

[toc] | [prev] | [next] | [standalone]

#1343058 — Re: [PATCH 09/12] KVM: MMU: coalesce zapping page after mmu_sync_children

On 25/02/2016 10:10, Xiao Guangrong wrote:
> 
> 
> On 02/25/2016 04:49 PM, Paolo Bonzini wrote:
>>
>>
>> On 25/02/2016 08:35, Xiao Guangrong wrote:
>>>> This may release the mmu_lock before committing the zapping.
>>>> Is it safe?  If so, we may want to see the reason in the changelog.
>>>
>>> It is unsafe indeed, please do not do it.
>>
>> Can you explain why?  kvm_zap_obsolete_pages does the same.
> 
> It's not the same, please see the comment in 
> kvm_mmu_invalidate_zap_all_pages:
>     /*
>      * Notify all vcpus to reload its shadow page table
>      * and flush TLB. Then all vcpus will switch to new
>      * shadow page table with the new mmu_valid_gen.
>      *
>      * Note: we should do this under the protection of
>      * mmu-lock, otherwise, vcpu would purge shadow page
>      * but miss tlb flush.
>      */
>     kvm_reload_remote_mmus(kvm);
> 
> That means the tlb is flushed before releasing mmu-lock.
> 
> A example is in rmap_write_protect(), when KVM creates a shadow page
> table for
> the the guest, it detects no spte pointing to the gfn, so tlb is not
> flushed so
> that guest can freely updates its pte.

Then I'll do a different patch that checks need_resched||spin_needbreak,
and if so does commit+cond_resched_lock.  I've removed 9/12 from
kvm/queue.  Again, sorry for giving the impression that these patches
were already final.

Paolo

[toc] | [prev] | [next] | [standalone]

#1342980 — Re: [PATCH 09/12] KVM: MMU: coalesce zapping page after mmu_sync_children

On 25/02/2016 03:15, Takuya Yoshikawa wrote:
> On 2016/02/24 22:17, Paolo Bonzini wrote:
>> Move the call to kvm_mmu_flush_or_zap outside the loop.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   arch/x86/kvm/mmu.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>> index 725316df32ec..6d47b5c43246 100644
>> --- a/arch/x86/kvm/mmu.c
>> +++ b/arch/x86/kvm/mmu.c
>> @@ -2029,24 +2029,27 @@ static void mmu_sync_children(struct kvm_vcpu *vcpu,
>>   	struct mmu_page_path parents;
>>   	struct kvm_mmu_pages pages;
>>   	LIST_HEAD(invalid_list);
>> +	bool flush = false;
>>   
>>   	while (mmu_unsync_walk(parent, &pages)) {
>>   		bool protected = false;
>> -		bool flush = false;
>>   
>>   		for_each_sp(pages, sp, parents, i)
>>   			protected |= rmap_write_protect(vcpu, sp->gfn);
>>   
>> -		if (protected)
>> +		if (protected) {
>>   			kvm_flush_remote_tlbs(vcpu->kvm);
>> +			flush = false;
>> +		}
>>   
>>   		for_each_sp(pages, sp, parents, i) {
>>   			flush |= kvm_sync_page(vcpu, sp, &invalid_list);
>>   			mmu_pages_clear_parents(&parents);
>>   		}
>> -		kvm_mmu_flush_or_zap(vcpu, &invalid_list, false, flush);
>>   		cond_resched_lock(&vcpu->kvm->mmu_lock);
> 
> This may release the mmu_lock before committing the zapping.
> Is it safe?  If so, we may want to see the reason in the changelog.

It should be safe; the page is already marked as invalid and hence the
role will not match in kvm_mmu_get_page.

The idea is simply that committing the zap is expensive (for example it
requires a remote TLB flush) so you want to do it as rarely as possible.
 I'll note this in the commit message.

Paolo

>   Takuya
> 
>>   	}
>> +
>> +	kvm_mmu_flush_or_zap(vcpu, &invalid_list, false, flush);
>>   }
>>   
>>   static void __clear_sp_write_flooding_count(struct kvm_mmu_page *sp)
>>
> 
> 
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web