Re: [PATCH] xen: arm: enable perf counters

[Julien Grall <julien.grall@linaro.org>]

On 05/15/2014 03:17 PM, Ian Campbell wrote:
> As well as the existing common perf counters add a bunch of ARM specifics,
> including the various trap types, vuart/vgic/vtimer accesses and different
> types of interrupt.

Performance counters in vgic don't make sense for me as we need to trap
it in any case.

But we might want perf counter in p2m_lookup because this function is
costly.

I would also add one in flush_tlb_* functions, such as flush_tlb_domain.
It will help us optimizing TLBs.

>      PSCI_RESULT_REG(regs) = psci_call(PSCI_ARGS(regs));
>  }
>  
> @@ -1135,15 +1138,19 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr,
>      register_t orig_pc = regs->pc;
>  #endif
>  
> +    BUILD_BUG_ON(NR_hypercalls < ARRAY_SIZE(arm_hypercall_table) );
> +
>      if ( iss != XEN_HYPERCALL_TAG )
>          domain_crash_synchronous();
>  
>      if ( *nr >= ARRAY_SIZE(arm_hypercall_table) )
>      {
> +        perfc_incr(invalid_hypercalls);
>          HYPERCALL_RESULT_REG(regs) = -ENOSYS;
>          return;
>      }
>  
> +    perfc_incra(hypercalls, *nr);
>      call = arm_hypercall_table[*nr].fn;
>      if ( call == NULL )
>      {
> @@ -1283,8 +1290,10 @@ static int check_conditional_instr(struct cpu_user_regs *regs, union hsr hsr)
>      cpsr_cond = cpsr >> 28;
>  
>      if ( !((cc_map[cond] >> cpsr_cond) & 1) )
> +    {
> +        perfc_incr(trap_uncond);

trap_uncond alone doesn't have much meaning. Can you add a perf_counter
to count the number of call for this function (i.e check_conditional_instr)?

>          return 0;
> -
> +    }
>      return 1;
>  }
>  
> @@ -1664,6 +1673,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
>  
>      switch (hsr.ec) {
>      case HSR_EC_WFI_WFE:
> +        perfc_incr(trap_wfi_wfe);

Can you add a perf counter to count the number of WFI and WFE? (actually
the last one is not trap for now).

>          if ( !check_conditional_instr(regs, hsr) )
>          {
>              advance_pc(regs, hsr);
> @@ -1684,38 +1694,51 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
>      case HSR_EC_CP15_32:
>          if ( !is_32bit_domain(current->domain) )
>              goto bad_trap;
> +        perfc_incr(trap_cp15_32);
>          do_cp15_32(regs, hsr);
>          break;
>      case HSR_EC_CP15_64:
>          if ( !is_32bit_domain(current->domain) )
>              goto bad_trap;
> +        perfc_incr(trap_cp15_32);

Did you mean trap_cp15_64?

[..]

>      case HSR_EC_HVC32:
> +        perfc_incr(trap_hvc32);
>  #ifndef NDEBUG
>          if ( (hsr.iss & 0xff00) == 0xff00 )
>              return do_debug_trap(regs, hsr.iss & 0x00ff);
>  #endif
>          if ( hsr.iss == 0 )
>              return do_trap_psci(regs);
> +

Spurious change?

[..]

>      case HSR_EC_INSTR_ABORT_LOWER_EL:
> +        perfc_incr(trap_iabt);
>          do_trap_instr_abort_guest(regs, hsr);
>          break;
>      case HSR_EC_DATA_ABORT_LOWER_EL:
> +        perfc_incr(trap_dabt);
>          do_trap_data_abort_guest(regs, hsr);
>          break;
>      default:
>   bad_trap:
> +        perfc_incr(trap_bad);

The perfc_incr seems pointless here. Indeed, do_unexcepted_trap will
basically break the current PCPU and can be worst when it's occurs on CPU0.

[..]

> diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
> index ef291ff..0de6f7e 100644
> --- a/xen/include/asm-arm/config.h
> +++ b/xen/include/asm-arm/config.h
> @@ -178,6 +178,8 @@
>  #define PAGE_MASK           (~(PAGE_SIZE-1))
>  #define PAGE_FLAG_MASK      (~0)
>  
> +#define NR_hypercalls 64
> +

Should not it be define in common code?

Regards,

-- 
Julien Grall