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

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

Hi Ian,

On 14/01/15 14:33, Ian Campbell wrote:
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 25ecf1d..604fc81 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -179,7 +179,12 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
>  {
>      struct irq_desc *desc = irq_to_desc(irq);
>  
> -    /* TODO: perfc_incr(irqs); */
> +    perfc_incr(irqs);
> +
> +    if (irq < 32) /* SGIs do not come down this path */

if ( irq < 32 )

Maybe an ASSERT(irq >= 16) to check the validity of the comment.

[..]

> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 4c93250..68208d6 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -30,6 +30,7 @@
>  #include <xen/hypercall.h>
>  #include <xen/softirq.h>
>  #include <xen/domain_page.h>
> +#include <xen/perfc.h>
>  #include <public/sched.h>
>  #include <public/xen.h>
>  #include <asm/debugger.h>
> @@ -1240,6 +1241,7 @@ static void do_trap_psci(struct cpu_user_regs *regs)
>      case PSCI_cpu_off:
>          {
>              uint32_t pstate = PSCI_ARG32(regs,1);
> +            perfc_incr(psci_cpu_off);
>              PSCI_RESULT_REG(regs) = do_psci_cpu_off(pstate);
>          }
>          break;
> @@ -1247,33 +1249,41 @@ static void do_trap_psci(struct cpu_user_regs *regs)
>          {
>              uint32_t vcpuid = PSCI_ARG32(regs,1);
>              register_t epoint = PSCI_ARG(regs,2);
> +            perfc_incr(psci_cpu_on);

I find confusing to use psci_cpu_on because it may refer to the host psci.

Maybe renaming to vpsci_ would be better?

[..]

> @@ -2113,6 +2146,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
>  
>      default:
>   bad_trap:
> +        perfc_incr(trap_bad);

I don't think this perf counters is useful. When the CPU is receiving a
bad trap Xen will panic in do_unexpected_trap.

[..]

> diff --git a/xen/include/asm-arm/perfc.h b/xen/include/asm-arm/perfc.h
> new file mode 100644
> index 0000000..a1a591e
> --- /dev/null
> +++ b/xen/include/asm-arm/perfc.h
> @@ -0,0 +1,12 @@
> +#ifndef __ASM_PERFC_H__
> +#define __ASM_PERFC_H__
> +
> +static inline void arch_perfc_reset(void)
> +{
> +}
> +
> +static inline void arch_perfc_gather(void)
> +{
> +}
> +
> +#endif

Missing the Emacs local variables.

> diff --git a/xen/include/asm-arm/perfc_defn.h b/xen/include/asm-arm/perfc_defn.h
> new file mode 100644
> index 0000000..df86879
> --- /dev/null
> +++ b/xen/include/asm-arm/perfc_defn.h
> @@ -0,0 +1,74 @@
> +/* This file is legiimately included multiple times. */

Legacy for xen/include/perfc_defn.h?

> +/*#ifndef __XEN_PERFC_DEFN_H__*/

All headers in asm-arm use __ARCH_ARM_* or __ARM_*

This would avoid the strange reason to not use guard in xen/perfc_defn.h.

[..]

> +/*#endif*/ /* __XEN_PERFC_DEFN_H__ */

Missing local variables for emacs.

Regards,

-- 
Julien Grall