Remove redundant check for CPU feature S2FWB in dcache flush code
to save some CPU cycles for every memslot flush and unmapping.
And move the S2FWB check to outer functions to avoid future
redundancy and keep consistent with other usage like in
access_dcsw and kvm_arch_prepare_memory_region.
Signed-off-by: Jing Zhang <redacted>
---
arch/arm64/kvm/hyp/pgtable.c | 9 ++-------
arch/arm64/kvm/mmu.c | 3 ++-
2 files changed, 4 insertions(+), 8 deletions(-)
@@ -1458,7 +1458,8 @@ void kvm_toggle_cache(struct kvm_vcpu *vcpu, bool was_enabled)*Ifswitchingitoff,needtocleanthecaches.*Clean+invalidatedoesthetrickalways.*/-if(now_enabled!=was_enabled)+if(now_enabled!=was_enabled&&+!cpus_have_const_cap(ARM64_HAS_STAGE2_FWB))stage2_flush_vm(vcpu->kvm);/* Caches are now on, stop trapping VM ops (until a S/W op) */
base-commit: a8ac864a7d6dbc2fc43081b1eecd9e0183065d47
--
2.30.0.365.g02bc693789-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Marc Zyngier <maz@kernel.org> Date: 2021-02-05 17:25:17
Hi Jing,
On 2021-02-05 04:44, Jing Zhang wrote:
Remove redundant check for CPU feature S2FWB in dcache flush code
to save some CPU cycles for every memslot flush and unmapping.
What CPU cycles? This is only a static branch. Can you actually
measure the overhead? What does it represent in the face of
a full memslot unmapping?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Marc,
Thanks for the comment.
On Fri, Feb 5, 2021 at 11:24 AM Marc Zyngier [off-list ref] wrote:
Hi Jing,
On 2021-02-05 04:44, Jing Zhang wrote:
quoted
Remove redundant check for CPU feature S2FWB in dcache flush code
to save some CPU cycles for every memslot flush and unmapping.
What CPU cycles? This is only a static branch. Can you actually
measure the overhead? What does it represent in the face of
a full memslot unmapping?
For CPU cycles, I mean CPU time spent for S2FWB check.
For memslot unmapping, there is actually no improvement, just move the
check to the stage2_unmap_walker since we removed the S2FWB check in
stage2_flush_dcache.
The saving is from the code path of memslot flush. The S2FWB check was
in stage2_flush_dcache, in which case, for a memslot flush, the check
was done for every page. Now it will save some CPU time if we do the
check at a higher level, like in kvm_toggle_cache, access_dcsw,
kvm_arch_prepare_memory_region.
The redundant check is as follows (Only the first check is necessary):
kvm_arch_prepare_memory_region -> S2FWB check -> stage2_flush_memslot
-> kvm_pgtable_stage2_flush -> S2FWB check -> stage2_flush_walker ->
S2FWB check -> __flush_dcache_area
Thanks,
M.
--
Jazz is not dead. It just smells funny...
From: Will Deacon <will@kernel.org> Date: 2021-03-08 16:50:52
On Fri, Feb 05, 2021 at 04:44:03AM +0000, Jing Zhang wrote:
quoted hunk
Remove redundant check for CPU feature S2FWB in dcache flush code
to save some CPU cycles for every memslot flush and unmapping.
And move the S2FWB check to outer functions to avoid future
redundancy and keep consistent with other usage like in
access_dcsw and kvm_arch_prepare_memory_region.
Signed-off-by: Jing Zhang <redacted>
---
arch/arm64/kvm/hyp/pgtable.c | 9 ++-------
arch/arm64/kvm/mmu.c | 3 ++-
2 files changed, 4 insertions(+), 8 deletions(-)
I think we should leave pgtable.c as it is: there's no benefit from this
change on the unmap path, and the other path involves the case where the
caller has asked for a flush and we can elide it.
This change looks fine, but I don't grok the justification in your follow-up
email. You say:
| The saving is from the code path of memslot flush. The S2FWB check was
| in stage2_flush_dcache, in which case, for a memslot flush, the check
| was done for every page.
but I don't see where this is called for every page. It looks to me like it's
called for every pgd in the range, which is a very different kettle of frogs.
What am I missing?
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Will,
On Mon, Mar 8, 2021 at 10:49 AM Will Deacon [off-list ref] wrote:
On Fri, Feb 05, 2021 at 04:44:03AM +0000, Jing Zhang wrote:
quoted
Remove redundant check for CPU feature S2FWB in dcache flush code
to save some CPU cycles for every memslot flush and unmapping.
And move the S2FWB check to outer functions to avoid future
redundancy and keep consistent with other usage like in
access_dcsw and kvm_arch_prepare_memory_region.
Signed-off-by: Jing Zhang <redacted>
---
arch/arm64/kvm/hyp/pgtable.c | 9 ++-------
arch/arm64/kvm/mmu.c | 3 ++-
2 files changed, 4 insertions(+), 8 deletions(-)
I think we should leave pgtable.c as it is: there's no benefit from this
change on the unmap path, and the other path involves the case where the
caller has asked for a flush and we can elide it.
This change looks fine, but I don't grok the justification in your follow-up
email. You say:
| The saving is from the code path of memslot flush. The S2FWB check was
| in stage2_flush_dcache, in which case, for a memslot flush, the check
| was done for every page.
but I don't see where this is called for every page. It looks to me like it's
called for every pgd in the range, which is a very different kettle of frogs.
What am I missing?
You are right. It is called for every pgd in the range instead of for
every page.