Re: [PATCH 18/18] 2.6.17.9 perfmon2 patch for review: new x86_64 files

[Andi Kleen <ak@suse.de>]

Stephane Eranian <eranian@frankl.hpl.hp.com> writes:


Earlier comment about logical pieces applies too.

> 
> --- linux-2.6.17.9.base/arch/x86_64/perfmon/Kconfig	1969-12-31 16:00:00.000000000 -0800
> +++ linux-2.6.17.9/arch/x86_64/perfmon/Kconfig	2006-08-21 03:37:46.000000000 -0700
> @@ -0,0 +1,39 @@
> +menu "Hardware Performance Monitoring support"
> +config PERFMON
> + 	bool "Perfmon2 performance monitoring interface"
> +	select X86_LOCAL_APIC
> +	default y

No default y please unless the kernel doesn't boot without it.

> + 	help
> +  	Enables the perfmon2 interface to access the hardware
> +	performance counters. See <http://perfmon2.sf.net/> for
> + 	more details. If you're unsure, say Y.
> +
> +config X86_64_PERFMON_AMD64
> +	tristate "Support 64-bit mode AMD64 hardware performance counters"
> +	depends on PERFMON
> +	default m

No default m please.  If someone just presses return in make oldconfig
with a new kernel they don't want all kinds of new random optional drivers.

I think I would prefer to call it _K8, because in theory new AMD CPUs
might have difference performance counters.

> +	help
> +	Enables support for 64-bit mode AMD64 hardware performance
> +	counters. Does not work with Intel EM64T processors.
> +	If unsure, say m.

I would drop the if unsure ... too

> +
> +config X86_64_PERFMON_EM64T
> +	tristate "Support Intel EM64T hardware performance counters"
> +	depends on PERFMON
> +	default m
> +	help
> +	Enables support for the Intel EM64T hardware performance
> +	counters. Does not work with AMD64 processors.
> +	If unsure, say m.

Does that include the Core 2 support that you had in the i386 patch? 

In general I would prefer to call it P4, not EM64T which is just
a generic architecture name and at least on P4 performance counters
are not really architected yet.


> +
> +	if (cpu_data->x86 != 15) {
> +		PFM_INFO("unsupported family=%d", cpu_data->x86);
> +		return -1;
> +	}
> +
> +	if (cpu_data->x86_vendor != X86_VENDOR_AMD) {
> +		PFM_INFO("not an AMD processor");
> +		return -1;
> +	}

Doing the checks the other way round would be more logical.

> + *
> + * This file implements the PEBS sampling format for Intel
> + * EM64T Intel Pentium 4/Xeon processors. It does not work
> + * with Intel 32-bit P4/Xeon processors.

Why not anyways? The registers are basically the same. What's so different
in 64bit? oprofile shares that code too.

The file seems a bit underdocumented. At least some brief description
what PEBS is and maybe at least one sentence for each function?

> + */
> +#ifndef __PERFMON_EM64T_PEBS_SMPL_H__
> +#define __PERFMON_EM64T_PEBS_SMPL_H__ 1
> +
> +#define PFM_EM64T_PEBS_SMPL_UUID { \
> +	0x36, 0xbe, 0x97, 0x94, 0x1f, 0xbf, 0x41, 0xdf,\
> +	0xb4, 0x63, 0x10, 0x62, 0xeb, 0x72, 0x9b, 0xad}

What does it need the UUID for?

> +
> +/*
> + * format specific parameters (passed at context creation)
> + *
> + * intr_thres: index from start of buffer of entry where the
> + * PMU interrupt must be triggered. It must be several samples
> + * short of the end of the buffer.
> + */
> +struct pfm_em64t_pebs_smpl_arg {
> +	size_t	buf_size;	/* size of the buffer in bytes */
> +	size_t	intr_thres;	/* index of interrupt threshold entry */
> +	u32	flags;		/* buffer specific flags */
> +	u64	cnt_reset;	/* counter reset value */
> +	u32	res1;		/* for future use */
> +	u64	reserved[2];	/* for future use */

I hope you double checked the alignment comes up everywhere correctly.
u64 alignment is different on the 32bit and 64bit ABIs. That can screw

Normally it's safer to use aligned_u64 on files that can be used on 
32bit too, because that avoids that problem.


Where is the actual code that implements the code that you hooked 
into arch/x86_64/*? I must have missed that.

-Andi