Re: [PATCH] spapr: avoid overhead of finding vhyp class in critical operations

["Nicholas Piggin" <npiggin@gmail.com>]

On Tue Mar 12, 2024 at 6:56 PM AEST, Harsh Prateek Bora wrote:
>
>
> On 3/12/24 14:18, Nicholas Piggin wrote:
> > On Tue Mar 12, 2024 at 4:38 PM AEST, Harsh Prateek Bora wrote:
> >> Hi Nick,
> >>
> >> One minor comment below:
> >>
> >> On 2/24/24 13:03, Nicholas Piggin wrote:
> >>> PPC_VIRTUAL_HYPERVISOR_GET_CLASS is used in critical operations like
> >>> interrupts and TLB misses and is quite costly. Running the
> >>> kvm-unit-tests sieve program with radix MMU enabled thrashes the TCG
> >>> TLB and spends a lot of time in TLB and page table walking code. The
> >>> test takes 67 seconds to complete with a lot of time being spent in
> >>> code related to finding the vhyp class:
> >>>
> >>>      12.01%  [.] g_str_hash
> >>>       8.94%  [.] g_hash_table_lookup
> >>>       8.06%  [.] object_class_dynamic_cast
> >>>       6.21%  [.] address_space_ldq
> >>>       4.94%  [.] __strcmp_avx2
> >>>       4.28%  [.] tlb_set_page_full
> >>>       4.08%  [.] address_space_translate_internal
> >>>       3.17%  [.] object_class_dynamic_cast_assert
> >>>       2.84%  [.] ppc_radix64_xlate
> >>>
> >>> Keep a pointer to the class and avoid this lookup. This reduces the
> >>> execution time to 40 seconds.
> >>>
> >>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> >>> ---
> >>> This feels a bit ugly, but the performance problem of looking up the
> >>> class in fast paths can't be ignored. Is there a "nicer" way to get the
> >>> same result?
> >>>
> >>> Thanks,
> >>> Nick
> >>>
> >>>    target/ppc/cpu.h           |  3 ++-
> >>>    target/ppc/mmu-book3s-v3.h |  4 +---
> >>>    hw/ppc/pegasos2.c          |  1 +
> >>>    target/ppc/cpu_init.c      |  9 +++------
> >>>    target/ppc/excp_helper.c   | 16 ++++------------
> >>>    target/ppc/kvm.c           |  4 +---
> >>>    target/ppc/mmu-hash64.c    | 16 ++++------------
> >>>    target/ppc/mmu-radix64.c   |  4 +---
> >>>    8 files changed, 17 insertions(+), 40 deletions(-)
> >>>
> >>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> >>> index ec14574d14..eb85d9aa71 100644
> >>> --- a/target/ppc/cpu.h
> >>> +++ b/target/ppc/cpu.h
> >>> @@ -1437,6 +1437,7 @@ struct ArchCPU {
> >>>        int vcpu_id;
> >>>        uint32_t compat_pvr;
> >>>        PPCVirtualHypervisor *vhyp;
> >>> +    PPCVirtualHypervisorClass *vhyp_class;
> >>>        void *machine_data;
> >>>        int32_t node_id; /* NUMA node this CPU belongs to */
> >>>        PPCHash64Options *hash64_opts;
> >>> @@ -1535,7 +1536,7 @@ DECLARE_OBJ_CHECKERS(PPCVirtualHypervisor, PPCVirtualHypervisorClass,
> >>>    
> >>>    static inline bool vhyp_cpu_in_nested(PowerPCCPU *cpu)
> >>>    {
> >>> -    return PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp)->cpu_in_nested(cpu);
> >>> +    return cpu->vhyp_class->cpu_in_nested(cpu);
> >>>    }
> >>>    #endif /* CONFIG_USER_ONLY */
> >>>    
> >>> diff --git a/target/ppc/mmu-book3s-v3.h b/target/ppc/mmu-book3s-v3.h
> >>> index 674377a19e..f3f7993958 100644
> >>> --- a/target/ppc/mmu-book3s-v3.h
> >>> +++ b/target/ppc/mmu-book3s-v3.h
> >>> @@ -108,9 +108,7 @@ static inline hwaddr ppc_hash64_hpt_mask(PowerPCCPU *cpu)
> >>>        uint64_t base;
> >>>    
> >>>        if (cpu->vhyp) {
> >>
> >> All the checks for cpu->vhyp needs to be changed to check for
> >> cpu->vhyp_class now, for all such instances.
> > 
> > It wasn't supposed to, because vhyp != NULL implies vhyp_class != NULL.
> > It's supposed to be an equivalent transformation just changing the
> > lookup function.
>
> I agree, but not just it appears a bit odd, my only worry is if a future
> change cause vhyp_class to be NULL before the control reaches here, this
> check wont really serve the purpose. Anyways, not a mandatory
> requirement for now, so I shall leave it to your choice.

It does look like it should be the other way around without context
of s/PPC_V_H_G_C(cpu->vhyp)/cpu->vhyp_class/

We'll never have a vhyp != NULL && vhyp_class == NULL though, so
should be okay.

Thanks,
Nick