[PATCH 0/2] Performance improvement about cache flush

[PATCH 0/2] Performance improvement about cache flush

[Yanan Wang]

Hi,
This two patches are posted to introduce a new method that can distinguish cases
of allocating memcache more precisely, and to elide some unnecessary cache flush.

For patch-1:
With a guest translation fault, we don't really need the memcache pages when
only installing a new entry to the existing page table or replacing the table
entry with a block entry. And with a guest permission fault, we also don't need
the memcache pages for a write_fault in dirty-logging time if VMs are not
configured with huge mappings. So a new method is introduced to distinguish cases
of allocating memcache more precisely.

For patch-2:
If migration of a VM with hugepages is canceled midway, KVM will adjust the
stage-2 table mappings back to block mappings. With multiple vCPUs accessing
guest pages within the same 1G range, there could be numbers of translation
faults to handle, and KVM will uniformly flush data cache for 1G range before
handling the faults. As it will cost a long time to flush the data cache for
1G range of memory(130ms on Kunpeng 920 servers, for example), the consequent
cache flush for each translation fault will finally lead to vCPU stuck for
seconds or even a soft lockup. I have met both the stuck and soft lockup on
Kunpeng servers with FWB not supported.

When KVM need to recover the table mappings back to block mappings, as we only
replace the existing page tables with a block entry and the cacheability has not
been changed, the cache maintenance opreations can be skipped.

Yanan Wang (2):
  KVM: arm64: Distinguish cases of allocating memcache more precisely
  KVM: arm64: Skip the cache flush when coalescing tables into a block

 arch/arm64/kvm/mmu.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

-- 
2.19.1

[PATCH 1/2] KVM: arm64: Distinguish cases of allocating memcache more precisely

[Yanan Wang]

With a guest translation fault, we don't really need the memcache pages
when only installing a new entry to the existing page table or replacing
the table entry with a block entry. And with a guest permission fault,
we also don't need the memcache pages for a write_fault in dirty-logging
time if VMs are not configured with huge mappings.

The cases where allocations from memcache are required can be much more
precisely distinguished by comparing fault_granule and vma_pagesize.

Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 arch/arm64/kvm/mmu.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 7d2257cc5438..8e8549ea1d70 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -820,19 +820,6 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 	gfn = fault_ipa >> PAGE_SHIFT;
 	mmap_read_unlock(current->mm);
 
-	/*
-	 * Permission faults just need to update the existing leaf entry,
-	 * and so normally don't require allocations from the memcache. The
-	 * only exception to this is when dirty logging is enabled at runtime
-	 * and a write fault needs to collapse a block entry into a table.
-	 */
-	if (fault_status != FSC_PERM || (logging_active && write_fault)) {
-		ret = kvm_mmu_topup_memory_cache(memcache,
-						 kvm_mmu_cache_min_pages(kvm));
-		if (ret)
-			return ret;
-	}
-
 	mmu_seq = vcpu->kvm->mmu_notifier_seq;
 	/*
 	 * Ensure the read of mmu_notifier_seq happens before we call
@@ -898,6 +885,18 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 	else if (cpus_have_const_cap(ARM64_HAS_CACHE_DIC))
 		prot |= KVM_PGTABLE_PROT_X;
 
+	/*
+	 * Allocations from the memcache are required only when granule of the
+	 * lookup level where a guest fault happened exceeds the vma_pagesize,
+	 * which means new page tables will be created in the fault handlers.
+	 */
+	if (fault_granule > vma_pagesize) {
+		ret = kvm_mmu_topup_memory_cache(memcache,
+						 kvm_mmu_cache_min_pages(kvm));
+		if (ret)
+			return ret;
+	}
+
 	/*
 	 * Under the premise of getting a FSC_PERM fault, we just need to relax
 	 * permissions only if vma_pagesize equals fault_granule. Otherwise,
-- 
2.19.1

[PATCH 2/2] KVM: arm64: Skip the cache flush when coalescing tables into a block

[Yanan Wang]

After dirty-logging is stopped for a VM configured with huge mappings,
KVM will recover the table mappings back to block mappings. As we only
replace the existing page tables with a block entry and the cacheability
has not been changed, the cache maintenance opreations can be skipped.

Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 arch/arm64/kvm/mmu.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 8e8549ea1d70..37b427dcbc4f 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -744,7 +744,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 {
 	int ret = 0;
 	bool write_fault, writable, force_pte = false;
-	bool exec_fault;
+	bool exec_fault, adjust_hugepage;
 	bool device = false;
 	unsigned long mmu_seq;
 	struct kvm *kvm = vcpu->kvm;
@@ -872,12 +872,18 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 		mark_page_dirty(kvm, gfn);
 	}
 
-	if (fault_status != FSC_PERM && !device)
+	/*
+	 * There is no necessity to perform cache maintenance operations if we
+	 * will only replace the existing table mappings with a block mapping.
+	 */
+	adjust_hugepage = fault_granule < vma_pagesize ? true : false;
+	if (fault_status != FSC_PERM && !device && !adjust_hugepage)
 		clean_dcache_guest_page(pfn, vma_pagesize);
 
 	if (exec_fault) {
 		prot |= KVM_PGTABLE_PROT_X;
-		invalidate_icache_guest_page(pfn, vma_pagesize);
+		if (!adjust_hugepage)
+			invalidate_icache_guest_page(pfn, vma_pagesize);
 	}
 
 	if (device)
-- 
2.19.1

Re: [PATCH 2/2] KVM: arm64: Skip the cache flush when coalescing tables into a block

[Will Deacon]

On Mon, Jan 25, 2021 at 10:10:44PM +0800, Yanan Wang wrote:
> After dirty-logging is stopped for a VM configured with huge mappings,
> KVM will recover the table mappings back to block mappings. As we only
> replace the existing page tables with a block entry and the cacheability
> has not been changed, the cache maintenance opreations can be skipped.
> 
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  arch/arm64/kvm/mmu.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 8e8549ea1d70..37b427dcbc4f 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -744,7 +744,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  {
>  	int ret = 0;
>  	bool write_fault, writable, force_pte = false;
> -	bool exec_fault;
> +	bool exec_fault, adjust_hugepage;
>  	bool device = false;
>  	unsigned long mmu_seq;
>  	struct kvm *kvm = vcpu->kvm;
> @@ -872,12 +872,18 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  		mark_page_dirty(kvm, gfn);
>  	}
>  
> -	if (fault_status != FSC_PERM && !device)
> +	/*
> +	 * There is no necessity to perform cache maintenance operations if we
> +	 * will only replace the existing table mappings with a block mapping.
> +	 */
> +	adjust_hugepage = fault_granule < vma_pagesize ? true : false;

nit: you don't need the '? true : false' part

That said, your previous patch checks for 'fault_granule > vma_pagesize',
so I'm not sure the local variable helps all that much here because it
obscures the size checks in my opinion. It would be more straight-forward
if we could structure the logic as:


	if (fault_granule < vma_pagesize) {

	} else if (fault_granule > vma_page_size) {

	} else {

	}

With some comments describing what we can infer about the memcache and cache
maintenance requirements for each case.

Will

Re: [PATCH 1/2] KVM: arm64: Distinguish cases of allocating memcache more precisely

[Will Deacon]

On Mon, Jan 25, 2021 at 10:10:43PM +0800, Yanan Wang wrote:
> With a guest translation fault, we don't really need the memcache pages
> when only installing a new entry to the existing page table or replacing
> the table entry with a block entry. And with a guest permission fault,
> we also don't need the memcache pages for a write_fault in dirty-logging
> time if VMs are not configured with huge mappings.
> 
> The cases where allocations from memcache are required can be much more
> precisely distinguished by comparing fault_granule and vma_pagesize.
> 
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  arch/arm64/kvm/mmu.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 7d2257cc5438..8e8549ea1d70 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -820,19 +820,6 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  	gfn = fault_ipa >> PAGE_SHIFT;
>  	mmap_read_unlock(current->mm);
>  
> -	/*
> -	 * Permission faults just need to update the existing leaf entry,
> -	 * and so normally don't require allocations from the memcache. The
> -	 * only exception to this is when dirty logging is enabled at runtime
> -	 * and a write fault needs to collapse a block entry into a table.
> -	 */
> -	if (fault_status != FSC_PERM || (logging_active && write_fault)) {
> -		ret = kvm_mmu_topup_memory_cache(memcache,
> -						 kvm_mmu_cache_min_pages(kvm));
> -		if (ret)
> -			return ret;
> -	}
> -
>  	mmu_seq = vcpu->kvm->mmu_notifier_seq;
>  	/*
>  	 * Ensure the read of mmu_notifier_seq happens before we call
> @@ -898,6 +885,18 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  	else if (cpus_have_const_cap(ARM64_HAS_CACHE_DIC))
>  		prot |= KVM_PGTABLE_PROT_X;
>  
> +	/*
> +	 * Allocations from the memcache are required only when granule of the
> +	 * lookup level where a guest fault happened exceeds the vma_pagesize,
> +	 * which means new page tables will be created in the fault handlers.
> +	 */
> +	if (fault_granule > vma_pagesize) {
> +		ret = kvm_mmu_topup_memory_cache(memcache,
> +						 kvm_mmu_cache_min_pages(kvm));
> +		if (ret)
> +			return ret;
> +	}

This feels like it could bite us in future as the code evolves but people
forget to reconsider this check. Maybe it would be better to extend this
patch so that we handle getting -ENOMEM back and try a second time after
topping up the memcache?

Will

Re: [PATCH 2/2] KVM: arm64: Skip the cache flush when coalescing tables into a block

[wangyanan (Y)]

On 2021/3/9 0:34, Will Deacon wrote:
> On Mon, Jan 25, 2021 at 10:10:44PM +0800, Yanan Wang wrote:
>> After dirty-logging is stopped for a VM configured with huge mappings,
>> KVM will recover the table mappings back to block mappings. As we only
>> replace the existing page tables with a block entry and the cacheability
>> has not been changed, the cache maintenance opreations can be skipped.
>>
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> ---
>>   arch/arm64/kvm/mmu.c | 12 +++++++++---
>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
>> index 8e8549ea1d70..37b427dcbc4f 100644
>> --- a/arch/arm64/kvm/mmu.c
>> +++ b/arch/arm64/kvm/mmu.c
>> @@ -744,7 +744,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>>   {
>>   	int ret = 0;
>>   	bool write_fault, writable, force_pte = false;
>> -	bool exec_fault;
>> +	bool exec_fault, adjust_hugepage;
>>   	bool device = false;
>>   	unsigned long mmu_seq;
>>   	struct kvm *kvm = vcpu->kvm;
>> @@ -872,12 +872,18 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>>   		mark_page_dirty(kvm, gfn);
>>   	}
>>   
>> -	if (fault_status != FSC_PERM && !device)
>> +	/*
>> +	 * There is no necessity to perform cache maintenance operations if we
>> +	 * will only replace the existing table mappings with a block mapping.
>> +	 */
>> +	adjust_hugepage = fault_granule < vma_pagesize ? true : false;
> nit: you don't need the '? true : false' part
>
> That said, your previous patch checks for 'fault_granule > vma_pagesize',
> so I'm not sure the local variable helps all that much here because it
> obscures the size checks in my opinion. It would be more straight-forward
> if we could structure the logic as:
>
>
> 	if (fault_granule < vma_pagesize) {
>
> 	} else if (fault_granule > vma_page_size) {
>
> 	} else {
>
> 	}
>
> With some comments describing what we can infer about the memcache and cache
> maintenance requirements for each case.
Thanks for your suggestion here, Will.
But I have resent another newer series [1] (KVM: arm64: Improve 
efficiency of stage2 page table)
recently, which has the same theme but different solutions that I think 
are better.
[1] 
https://lore.kernel.org/lkml/20210208112250.163568-1-wangyanan55@huawei.com/

Could you please comment on that series ?  I think it can be found in 
your inbox :).

Thanks,

Yanan
>
> Will
> .

Re: [PATCH 2/2] KVM: arm64: Skip the cache flush when coalescing tables into a block

[Marc Zyngier]

On Tue, 09 Mar 2021 08:34:43 +0000,
"wangyanan (Y)" <wangyanan55@huawei.com> wrote:
> 
> 
> On 2021/3/9 0:34, Will Deacon wrote:
> > On Mon, Jan 25, 2021 at 10:10:44PM +0800, Yanan Wang wrote:
> >> After dirty-logging is stopped for a VM configured with huge mappings,
> >> KVM will recover the table mappings back to block mappings. As we only
> >> replace the existing page tables with a block entry and the cacheability
> >> has not been changed, the cache maintenance opreations can be skipped.
> >> 
> >> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> >> ---
> >>   arch/arm64/kvm/mmu.c | 12 +++++++++---
> >>   1 file changed, 9 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> >> index 8e8549ea1d70..37b427dcbc4f 100644
> >> --- a/arch/arm64/kvm/mmu.c
> >> +++ b/arch/arm64/kvm/mmu.c
> >> @@ -744,7 +744,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> >>   {
> >>   	int ret = 0;
> >>   	bool write_fault, writable, force_pte = false;
> >> -	bool exec_fault;
> >> +	bool exec_fault, adjust_hugepage;
> >>   	bool device = false;
> >>   	unsigned long mmu_seq;
> >>   	struct kvm *kvm = vcpu->kvm;
> >> @@ -872,12 +872,18 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> >>   		mark_page_dirty(kvm, gfn);
> >>   	}
> >>   -	if (fault_status != FSC_PERM && !device)
> >> +	/*
> >> +	 * There is no necessity to perform cache maintenance operations if we
> >> +	 * will only replace the existing table mappings with a block mapping.
> >> +	 */
> >> +	adjust_hugepage = fault_granule < vma_pagesize ? true : false;
> > nit: you don't need the '? true : false' part
> > 
> > That said, your previous patch checks for 'fault_granule > vma_pagesize',
> > so I'm not sure the local variable helps all that much here because it
> > obscures the size checks in my opinion. It would be more straight-forward
> > if we could structure the logic as:
> > 
> > 
> > 	if (fault_granule < vma_pagesize) {
> > 
> > 	} else if (fault_granule > vma_page_size) {
> > 
> > 	} else {
> > 
> > 	}
> > 
> > With some comments describing what we can infer about the memcache and cache
> > maintenance requirements for each case.
> Thanks for your suggestion here, Will.
> But I have resent another newer series [1] (KVM: arm64: Improve
> efficiency of stage2 page table)
> recently, which has the same theme but different solutions that I
> think are better.
> [1]
> https://lore.kernel.org/lkml/20210208112250.163568-1-wangyanan55@huawei.com/
> 
> Could you please comment on that series ?  I think it can be found in
> your inbox :).

There were already a bunch of comments on that series, and I stopped
at the point where the cache maintenance was broken. Please respin
that series if you want further feedback on it.

In the future, if you deprecate a series (which is completely
understandable), please leave a note on the list with a pointer to the
new series so that people don't waste time reviewing an obsolete
series. Or post the new series with a new version number so that it is
obvious that the original series has been superseded.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 2/2] KVM: arm64: Skip the cache flush when coalescing tables into a block

[wangyanan (Y)]

On 2021/3/9 16:43, Marc Zyngier wrote:
> On Tue, 09 Mar 2021 08:34:43 +0000,
> "wangyanan (Y)" <wangyanan55@huawei.com> wrote:
>>
>> On 2021/3/9 0:34, Will Deacon wrote:
>>> On Mon, Jan 25, 2021 at 10:10:44PM +0800, Yanan Wang wrote:
>>>> After dirty-logging is stopped for a VM configured with huge mappings,
>>>> KVM will recover the table mappings back to block mappings. As we only
>>>> replace the existing page tables with a block entry and the cacheability
>>>> has not been changed, the cache maintenance opreations can be skipped.
>>>>
>>>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>>>> ---
>>>>    arch/arm64/kvm/mmu.c | 12 +++++++++---
>>>>    1 file changed, 9 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
>>>> index 8e8549ea1d70..37b427dcbc4f 100644
>>>> --- a/arch/arm64/kvm/mmu.c
>>>> +++ b/arch/arm64/kvm/mmu.c
>>>> @@ -744,7 +744,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>>>>    {
>>>>    	int ret = 0;
>>>>    	bool write_fault, writable, force_pte = false;
>>>> -	bool exec_fault;
>>>> +	bool exec_fault, adjust_hugepage;
>>>>    	bool device = false;
>>>>    	unsigned long mmu_seq;
>>>>    	struct kvm *kvm = vcpu->kvm;
>>>> @@ -872,12 +872,18 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>>>>    		mark_page_dirty(kvm, gfn);
>>>>    	}
>>>>    -	if (fault_status != FSC_PERM && !device)
>>>> +	/*
>>>> +	 * There is no necessity to perform cache maintenance operations if we
>>>> +	 * will only replace the existing table mappings with a block mapping.
>>>> +	 */
>>>> +	adjust_hugepage = fault_granule < vma_pagesize ? true : false;
>>> nit: you don't need the '? true : false' part
>>>
>>> That said, your previous patch checks for 'fault_granule > vma_pagesize',
>>> so I'm not sure the local variable helps all that much here because it
>>> obscures the size checks in my opinion. It would be more straight-forward
>>> if we could structure the logic as:
>>>
>>>
>>> 	if (fault_granule < vma_pagesize) {
>>>
>>> 	} else if (fault_granule > vma_page_size) {
>>>
>>> 	} else {
>>>
>>> 	}
>>>
>>> With some comments describing what we can infer about the memcache and cache
>>> maintenance requirements for each case.
>> Thanks for your suggestion here, Will.
>> But I have resent another newer series [1] (KVM: arm64: Improve
>> efficiency of stage2 page table)
>> recently, which has the same theme but different solutions that I
>> think are better.
>> [1]
>> https://lore.kernel.org/lkml/20210208112250.163568-1-wangyanan55@huawei.com/
>>
>> Could you please comment on that series ?  I think it can be found in
>> your inbox :).
> There were already a bunch of comments on that series, and I stopped
> at the point where the cache maintenance was broken. Please respin
> that series if you want further feedback on it.
Ok, I will send a new version later.
>
> In the future, if you deprecate a series (which is completely
> understandable), please leave a note on the list with a pointer to the
> new series so that people don't waste time reviewing an obsolete
> series. Or post the new series with a new version number so that it is
> obvious that the original series has been superseded.
I apologize for this, I will be more careful in the future.

Thanks,

Yanan
> Thanks,
>
> 	M.
>