[PATCH v1 1/3] KVM: arm64: Consistently use free_removed_table() for stage-2

[PATCH v1 1/3] KVM: arm64: Consistently use free_removed_table() for stage-2

[Chun-Tse Shao]

From: Oliver Upton <oliver.upton@linux.dev>

free_removed_table() is essential to the RCU-protected parallel walking
scheme, as behind the scenes the cleanup is deferred until an RCU grace
period. Nonetheless, the stage-2 unmap path calls put_page() directly,
which leads to table memory being freed inline with the table walk.

This is safe for the time being, as the stage-2 unmap walker is called
while holding the write lock. A future change to KVM will further relax
the locking mechanics around the stage-2 page tables to allow lock-free
walkers protected only by RCU. As such, switch to the RCU-safe mechanism
for freeing table memory.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
Signed-off-by: Chun-Tse Shao <ctshao@google.com>
---
 arch/arm64/kvm/hyp/pgtable.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index 5282cb9ca4cf..cc1af0286755 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -1042,7 +1042,7 @@ static int stage2_unmap_walker(const struct kvm_pgtable_visit_ctx *ctx,
 					       kvm_granule_size(ctx->level));
 
 	if (childp)
-		mm_ops->put_page(childp);
+		mm_ops->free_removed_table(childp, ctx->level);
 
 	return 0;
 }
-- 
2.41.0.162.gfafddb0af9-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 2/3] KVM: arm64: Only initiate walk if page_count() > 1 in free_removed_table()

[Chun-Tse Shao]

Page table walk is unnecessary in free_removed_table() being called from
the stage-2 unmap path while PTEs on the table is empty. It can be
fast-pathed by only initiating a walk if page_count() > 1

Original disussion can be found in:
https://lore.kernel.org/kvmarm/ZHfWzX04GlcNngdU@linux.dev/

Suggested-by: Yu Zhao <yuzhao@google.com>
Suggested-by: Oliver Upton <oliver.upton@linux.dev>
Signed-off-by: Chun-Tse Shao <ctshao@google.com>
---
 arch/arm64/kvm/hyp/pgtable.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index cc1af0286755..d8e570263388 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -1319,5 +1319,6 @@ void kvm_pgtable_stage2_free_removed(struct kvm_pgtable_mm_ops *mm_ops, void *pg
 		.end	= kvm_granule_size(level),
 	};
 
-	WARN_ON(__kvm_pgtable_walk(&data, mm_ops, ptep, level + 1));
+	if (mm_ops->page_count(pgtable) > 1)
+		WARN_ON(__kvm_pgtable_walk(&data, mm_ops, ptep, level + 1));
 }
-- 
2.41.0.162.gfafddb0af9-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 3/3] KVM: arm64: Using rcu_read_lock() for kvm_pgtable_stage2_mkyoung()

[Chun-Tse Shao]

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(-)

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 3/3] KVM: arm64: Using rcu_read_lock() for kvm_pgtable_stage2_mkyoung()

[Marc Zyngier]

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.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 3/3] KVM: arm64: Using rcu_read_lock() for kvm_pgtable_stage2_mkyoung()

[Chun-Tse Shao]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 3/3] KVM: arm64: Using rcu_read_lock() for kvm_pgtable_stage2_mkyoung()

[Oliver Upton]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/3] KVM: arm64: Consistently use free_removed_table() for stage-2

[Yu Zhao]

On Thu, Jun 8, 2023 at 4:06 PM Chun-Tse Shao <ctshao@google.com> wrote:
>
> From: Oliver Upton <oliver.upton@linux.dev>
>
> free_removed_table() is essential to the RCU-protected parallel walking
> scheme, as behind the scenes the cleanup is deferred until an RCU grace
> period. Nonetheless, the stage-2 unmap path calls put_page() directly,
> which leads to table memory being freed inline with the table walk.
>
> This is safe for the time being, as the stage-2 unmap walker is called
> while holding the write lock. A future change to KVM will further relax
> the locking mechanics around the stage-2 page tables to allow lock-free
> walkers protected only by RCU. As such, switch to the RCU-safe mechanism
> for freeing table memory.
>
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> Signed-off-by: Chun-Tse Shao <ctshao@google.com>

Acked-by: Yu Zhao <yuzhao@google.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel