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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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/[email protected]/
Could you please comment on that series ? I think it can be found in
your inbox :).
Thanks,
Yanan
>
> Will
> .
On Tue, 09 Mar 2021 08:34:43 +0000,
"wangyanan (Y)" <[email protected]> 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 <[email protected]>
> >> ---
> >> 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/[email protected]/
>
> 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.
On 2021/3/9 16:43, Marc Zyngier wrote:
> On Tue, 09 Mar 2021 08:34:43 +0000,
> "wangyanan (Y)" <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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/[email protected]/
>>
>> 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.
>