Message-ID: <20060823102932.GA697@frankl.hpl.hp.com>
Date:	Wed, 23 Aug 2006 03:29:32 -0700
From:	Stephane Eranian <eranian@....hp.com>
To:	Andi Kleen <ak@...e.de>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 18/18] 2.6.17.9 perfmon2 patch for review: new x86_64 files

Andi,

On Wed, Aug 23, 2006 at 12:19:44PM +0200, Andi Kleen wrote:
> > +
> > +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? 
> 
This overs P4 in 64-bit mode only!

What you have in i386 is the architectural perfmon support as documented
by Intel for Core Duo/Core Solo and possibly others. Core 2 is something
different but hopfully backward compatible with this.

> 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.
> 
Agreed.

> 
> > + *
> > + * 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.
> 
Well, no. the PEBS record format is differnet between 32 and 64-bit mode.
In 64-bit they add r8-r15 in each sample. The rest of the logic is
exactly the same. Until now, I have kept the 32 and 64 bit format
completely separate. But I will merge them in my next patch to cut
down on the amount of code. 

> 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?
> 
Every sampling format is identified by a UUID. This  is how an
application can identify the format it wants to use when it
creates a context.


> > +
> > +/*
> > + * 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.

As of now this file (perfmon_em64t_pebs_smpl.c) is only compiled in
64-bit mode. Once I merge with 32-bit I will force types to avoid
alignment problems.

> Where is the actual code that implements the code that you hooked 
> into arch/x86_64/*? I must have missed that.
> 
It is in the patch I call modified x86_64 files.

Thanks for you quick and valuable feedback.

I will make the change you suggested on this part.

-- 
-Stephane
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists