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 <[email protected]>
Signed-off-by: Chun-Tse Shao <[email protected]>
---
arch/arm64/kvm/mmu.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
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));
--
2.41.0.162.gfafddb0af9-goog
On Thu, 08 Jun 2023 23:05:41 +0100,
Chun-Tse Shao <[email protected]> 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.
--
Without deviation from the norm, progress is not possible.
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.
--
Thanks,
Oliver
On Fri, Jun 9, 2023 at 12:44 AM Marc Zyngier <[email protected]> wrote:
>
> On Thu, 08 Jun 2023 23:05:41 +0100,
> Chun-Tse Shao <[email protected]> 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