| Message ID | 20230608220558.39094-4-ctshao@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/3] KVM: arm64: Consistently use free_removed_table() for stage-2 | expand |
On Thu, 08 Jun 2023 23:05:41 +0100, Chun-Tse Shao <ctshao@google.com> wrote: > > Access bit is RCU safe and can be set without taking kvm->mmu_lock(). Please explain why. What happens when the page tables are *not* RCU controlled, such as in the pKVM case? > Replacing existing kvm->mmu_lock() with rcu_read_lock() for better > performance. Please define "better performance", quote workloads, figures, HW setup and point to a reproducer. Please add a cover letter to your patch series explaining the context this happens in. Also, I'm getting increasingly annoyed by the lack of coordination between seamingly overlapping patch series (this, Yu's, Anish's and Vipin's), all from a single company. Surely you can talk to each other and devise a coordinated approach? Thanks, M.
On Thu, Jun 08, 2023 at 03:05:41PM -0700, Chun-Tse Shao wrote: > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c > index 3b9d4d24c361..0f7ea66fb894 100644 > --- a/arch/arm64/kvm/mmu.c > +++ b/arch/arm64/kvm/mmu.c > @@ -1437,10 +1437,10 @@ static void handle_access_fault(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa) > > trace_kvm_access_fault(fault_ipa); > > - read_lock(&vcpu->kvm->mmu_lock); > + rcu_read_lock(); > mmu = vcpu->arch.hw_mmu; > pte = kvm_pgtable_stage2_mkyoung(mmu->pgt, fault_ipa); > - read_unlock(&vcpu->kvm->mmu_lock); > + rcu_read_unlock(); What is the point of acquiring the RCU read lock here? kvm_pgtable_walk_{begin,end}() already do the exact same for any 'shared' walk. I agree with Marc that this warrants some very clear benchmark data showing the value of the change. As I had mentioned to Yu, I already implemented this for my own purposes, but wasn't able to see a significant improvement over acquiring the MMU lock for read.
On Fri, Jun 9, 2023 at 12:44 AM Marc Zyngier <maz@kernel.org> wrote: > > On Thu, 08 Jun 2023 23:05:41 +0100, > Chun-Tse Shao <ctshao@google.com> wrote: > > > > Access bit is RCU safe and can be set without taking kvm->mmu_lock(). > > Please explain why. What happens when the page tables are *not* RCU > controlled, such as in the pKVM case? > > > Replacing existing kvm->mmu_lock() with rcu_read_lock() for better > > performance. > > Please define "better performance", quote workloads, figures, HW setup > and point to a reproducer. Please add a cover letter to your patch > series explaining the context this happens in. Thanks for the suggestion, we are currently working on the performance test in parallel and will update after gathering more data. > > Also, I'm getting increasingly annoyed by the lack of coordination > between seamingly overlapping patch series (this, Yu's, Anish's and > Vipin's), all from a single company. > > Surely you can talk to each other and devise a coordinated approach? Sure, I will start internal meeting as needed. > > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible. Thanks, CT
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c index 3b9d4d24c361..0f7ea66fb894 100644 --- a/arch/arm64/kvm/mmu.c +++ b/arch/arm64/kvm/mmu.c @@ -1437,10 +1437,10 @@ static void handle_access_fault(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa) trace_kvm_access_fault(fault_ipa); - read_lock(&vcpu->kvm->mmu_lock); + rcu_read_lock(); mmu = vcpu->arch.hw_mmu; pte = kvm_pgtable_stage2_mkyoung(mmu->pgt, fault_ipa); - read_unlock(&vcpu->kvm->mmu_lock); + rcu_read_unlock(); if (kvm_pte_valid(pte)) kvm_set_pfn_accessed(kvm_pte_to_pfn(pte));
Access bit is RCU safe and can be set without taking kvm->mmu_lock(). Replacing existing kvm->mmu_lock() with rcu_read_lock() for better performance. Original disussion can be found in: https://lore.kernel.org/kvmarm/CAOUHufZrfnfcbrqSzmHkejR5MA2gmGKZ3LMRhbLHV+1427z=Tw@mail.gmail.com/ Suggested-by: Yu Zhao <yuzhao@google.com> Signed-off-by: Chun-Tse Shao <ctshao@google.com> --- arch/arm64/kvm/mmu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)