Re: [PATCH v1 2/3] KVM: arm64: Add fast path to handle permission relaxation during dirty logging

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux