Messages in this thread | | | Subject | Re: [PATCH 18/18] 2.6.17.9 perfmon2 patch for review: new x86_64 files | From | Andi Kleen <> | Date | 23 Aug 2006 12:19:44 +0200 |
| |
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |