On Sun, Jan 16, 2022 at 3:14 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> 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.
>
Sure, will group with logging_active and rename it as "logging_perm_fault".
> >
> > 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.
>
You are right. Will do as you suggested.
> > + /*
> > + * 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.
>
Will do.
> > + 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.
Thanks,
Jing
