Re: [PATCH 2/3] powerpc: Instrument Hypervisor Calls: add wrappers

[Nathan Lynch <ntl@pobox.com>]

Mike Kravetz wrote:
> +config HCALL_STATS
> +	bool "Hypervisor call instrumentation"
> +	depends on PPC_PSERIES
> +	help
> +	  Adds code to keep track of number of hypervisor calls made and
> +	  the amount of time spent in hypervisor calls.
> +
> +	  This option will add a small amount of overhead to all hypervisor
> +	  calls.

Would be good to say how the stats are made available to userspace
here.


> +DEFINE_PER_CPU(struct hcall_stats[MAX_HCALL_OPCODES+1], hcall_stats);
> +
> +static inline void update_stats(unsigned long opcode, unsigned long t_before)
> +{
> +	unsigned long op_index= opcode >> 2;
> +	struct hcall_stats *hs = &__get_cpu_var(hcall_stats[op_index]);
> +
> +	/* racey, but acceptable for non-critical stats */
> +	hs->total_time += (mfspr(SPRN_PURR) - t_before);
> +	hs->num_calls++;
> +}
> +
> +extern long plpar_hcall_base (unsigned long opcode,
> +                 unsigned long arg1,
> +                 unsigned long arg2,
> +                 unsigned long arg3,
> +                 unsigned long arg4,
> +                 unsigned long *out1,
> +                 unsigned long *out2,
> +                 unsigned long *out3);
> +
> +long plpar_hcall(unsigned long opcode,
> +                 unsigned long arg1,
> +                 unsigned long arg2,
> +                 unsigned long arg3,
> +                 unsigned long arg4,
> +                 unsigned long *out1,
> +                 unsigned long *out2,
> +                 unsigned long *out3)
> +{
> +	long rc;
> +	unsigned long t_before = mfspr(SPRN_PURR);
> +
> +	rc = plpar_hcall_base(opcode, arg1, arg2, arg3, arg4, out1, out2, out3);
> +
> +	update_stats(opcode, t_before);
> +	return rc;
> +}

Without disabling preemption around the mfspr ... update_stats section
in these hcall wrappers, you risk updating the stats on the wrong cpu.


> +struct hcall_stats {
> +	unsigned long	num_calls;
> +	unsigned long	total_time;
> +};

A comment explaining the unit of time used for the total_time field
would be nice.