From mboxrd@z Thu Jan  1 00:00:00 1970
From: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
Subject: Re: [PATCH 2/2] KVM: MMU: Flush TLBs only once in invlpg() before
 releasing mmu_lock
Date: Fri, 10 Feb 2012 16:21:53 +0900
Message-ID: <4F34C591.1030909@oss.ntt.co.jp>
References: <20120210152831.6ac3ac87.yoshikawa.takuya@oss.ntt.co.jp> <20120210152950.b54969be.yoshikawa.takuya@oss.ntt.co.jp> <4F34BF7E.4050009@linux.vnet.ibm.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Cc: avi@redhat.com, mtosatti@redhat.com, kvm@vger.kernel.org,
	aarcange@redhat.com
To: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from serv2.oss.ntt.co.jp ([222.151.198.100]:47202 "EHLO
	serv2.oss.ntt.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1752915Ab2BJHT6 (ORCPT <rfc822;kvm@vger.kernel.org>);
	Fri, 10 Feb 2012 02:19:58 -0500
In-Reply-To: <4F34BF7E.4050009@linux.vnet.ibm.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

(2012/02/10 15:55), Xiao Guangrong wrote:
> On 02/10/2012 02:29 PM, Takuya Yoshikawa wrote:
>
>> diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
>> index 1561028..69d06f5 100644
>> --- a/arch/x86/kvm/paging_tmpl.h
>> +++ b/arch/x86/kvm/paging_tmpl.h
>> @@ -682,6 +682,7 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
>>   	mmu_topup_memory_caches(vcpu);
>>
>>   	spin_lock(&vcpu->kvm->mmu_lock);
>> +
>>   	for_each_shadow_entry(vcpu, gva, iterator) {
>>   		level = iterator.level;
>>   		sptep = iterator.sptep;
>> @@ -697,8 +698,7 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
>>   			pte_gpa = FNAME(get_level1_sp_gpa)(sp);
>>   			pte_gpa += (sptep - sp->spt) * sizeof(pt_element_t);
>>
>> -			if (mmu_page_zap_pte(vcpu->kvm, sp, sptep))
>> -				kvm_flush_remote_tlbs(vcpu->kvm);
>> +			mmu_page_zap_pte(vcpu->kvm, sp, sptep);
>>
>>   			if (!rmap_can_add(vcpu))
>>   				break;
>> @@ -713,6 +713,8 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
>>   		if (!is_shadow_present_pte(*sptep) || !sp->unsync_children)
>>   			break;
>>   	}
>> +
>> +	kvm_flush_remote_tlbs(vcpu->kvm);
>>   	spin_unlock(&vcpu->kvm->mmu_lock);
>
>
> It is obvious wrong, i do not think all tlbs always need be flushed...
>

What do you mean by "obvious wrong" ?

Even before this patch, we were always flushing TLBs from the caller.

I have a question:  your patches apparently changed the timing of TLB flush
but all I could see from the changelogs were:


     KVM: MMU: cleanup FNAME(invlpg)

     Directly Use mmu_page_zap_pte to zap spte in FNAME(invlpg), also remove the
     same code between FNAME(invlpg) and FNAME(sync_page)


     KVM: MMU: fast prefetch spte on invlpg path

     Fast prefetch spte for the unsync shadow page on invlpg path


Please explain what your patches semantically changed before checking the
details of my patch.

Thanks,
	Takuya