On Thu, 13 Jan 2022 22:18:28 +0000,
Jing Zhang <jingzhangos@xxxxxxxxxx> wrote:
>
> To reduce MMU lock contention during dirty logging, all permission
> relaxation operations would be performed under read lock.
>
> Signed-off-by: Jing Zhang <jingzhangos@xxxxxxxxxx>
> ---
> arch/arm64/kvm/mmu.c | 20 ++++++++++++++++++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index cafd5813c949..15393cb61a3f 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -1084,6 +1084,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> unsigned long vma_pagesize, fault_granule;
> enum kvm_pgtable_prot prot = KVM_PGTABLE_PROT_R;
> struct kvm_pgtable *pgt;
> + bool use_mmu_readlock = false;
Group this with the rest of the flags. It would also be better if it
described the condition this represent rather than what we use it for.
For example, 'perm_fault_while_logging', or something along those
lines.
>
> fault_granule = 1UL << ARM64_HW_PGTABLE_LEVEL_SHIFT(fault_level);
> write_fault = kvm_is_write_fault(vcpu);
> @@ -1212,7 +1213,19 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> if (exec_fault && device)
> return -ENOEXEC;
>
> - write_lock(&kvm->mmu_lock);
> + if (fault_status == FSC_PERM && fault_granule == PAGE_SIZE
> + && logging_active && write_fault)
> + use_mmu_readlock = true;
This looks a bit clumsy, and would be better if this was kept together
with the rest of the logging_active==true code. Something like:
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index bc2aba953299..59b1d5f46b06 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -1114,6 +1114,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
if (logging_active) {
force_pte = true;
vma_shift = PAGE_SHIFT;
+ use_readlock = (fault_status == FSC_PERM && write_fault);
} else {
vma_shift = get_vma_page_shift(vma, hva);
}
I don't think we have to check for fault_granule here, as I don't see
how you could get a permission fault for something other than a page
size mapping.
> + /*
> + * To reduce MMU contentions and enhance concurrency during dirty
> + * logging dirty logging, only acquire read lock for permission
> + * relaxation. This fast path would greatly reduce the performance
> + * degradation of guest workloads.
> + */
This comment makes more sense with the previous hunk. Drop the last
sentence though, as it doesn't bring much information.
> + if (use_mmu_readlock)
> + read_lock(&kvm->mmu_lock);
> + else
> + write_lock(&kvm->mmu_lock);
> pgt = vcpu->arch.hw_mmu->pgt;
> if (mmu_notifier_retry(kvm, mmu_seq))
> goto out_unlock;
> @@ -1271,7 +1284,10 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> }
>
> out_unlock:
> - write_unlock(&kvm->mmu_lock);
> + if (use_mmu_readlock)
> + read_unlock(&kvm->mmu_lock);
> + else
> + write_unlock(&kvm->mmu_lock);
> kvm_set_pfn_accessed(pfn);
> kvm_release_pfn_clean(pfn);
> return ret != -EAGAIN ? ret : 0;
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
