| Message ID | 20220113221829.2785604-3-jingzhangos@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM64: Guest performance improvement during dirty | expand |
On Thu, 13 Jan 2022 22:18:28 +0000, Jing Zhang <jingzhangos@google.com> 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@google.com> > --- > 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.
On Sun, Jan 16, 2022 at 3:14 AM Marc Zyngier <maz@kernel.org> wrote: > > On Thu, 13 Jan 2022 22:18:28 +0000, > Jing Zhang <jingzhangos@google.com> 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@google.com> > > --- > > 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
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; 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; + /* + * 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. + */ + 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;
To reduce MMU lock contention during dirty logging, all permission relaxation operations would be performed under read lock. Signed-off-by: Jing Zhang <jingzhangos@google.com> --- arch/arm64/kvm/mmu.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)