Menu
▾
▴
Re: [oprofile] Patch welcome ?
|
From: Philippe E. <ph...@cl...> - 2001-06-03 01:11:22
|
>
> I prefer diff -u (and also inline in the mail rather than attachment).
ok
>
> Why this :
>
> + { 7, utm_bitmask, { 0x1, 0x2, 0x4, 0x8, 0x10, 0x20 }, },
>
> Why have you changed allowed from 6 to 7 in the second entry here ? That
> looks wrong to me. I'll look at the rest of the patch after you respond.
A stupid "last minute change" error.
Note than :
> + { 5, utm_bitmask, { 0x1, 0x2, 0x4, 0x8, 0xf, 0x0 }, },
would be
> + { 4, utm_bitmask, { 0x1, 0x2, 0x4, 0x8, 0x0, 0x0 }, },
but I prefer to keep the original code, the two version work but
your code is more near the intel documentation.
> I would also prefer to leave out utm_error altogether - there is too small
> an amount of code here for such a coding error to happen I think.
I agree, utm_mandatory could be suppressed also and changed
to utm_exclusive, but I think than mandatory um would be silently
added by the profiler.
> If you're doing this checking, how about adding a check that a utm_exclusive
> um really is exclusive ? That would be nice ...
/* KNI PIII events */
was utm_bitmask, a typo error.
+ { 4, utm_exclusive, { 0x0, 0x1, 0x2, 0x3, 0x0, 0x0 }, },
+ { 2, utm_bitmask, { 0x0, 0x1, 0x0, 0x0, 0x0, 0x0 }, },
utm_exclusive are really exclusive, because value allowed in
exclusive mode entry cannot be or'ed without loss of information.
I have rather tested than utm_bitmask are really bitmask (for segment
related events and MMX insn related events) and by a carefull reads of
the intel documentation. Other bitmask allowed (MESI state) are more
clearly documented in IA manual as real bit mask wich can be or'ed.
I cannot make test for PIII KNI events, my poor PII have some problems
with KNI insns :)
I will post two C test-file in near future for segment related event and
mmx events but I have no automatic way (script makefile) to make the
real test. I use the tcl/tk for preparing the test and some other thing to
complete the test.
>
> This patch looks good otherwise though, thanks for the fix.
>
> > I have checked also than the documentation from intel that say
> > FS, FS (twice time stated) is a typo error it is really FS GS in fact.
>
> Do you mean you have actually tested this on a real box ? If so, can you
> put the change in your next patch ? ;)
It's tested (I used "checked" for "tested" in the previous mail)
> > I have a tcl/tk interface to start the profiler, and some patch
> > in preparation to allow interpretation of profiler result (mixing asm
> > or source file with profiling data). This needs some new output
> > format from oprofpp. The tcl/tk interface is ready to work I think.
>
> This sounds really cool, I'd love to see this !
I send in a separate mail with a few comment.
I have also patch for interpretation of results, in a few days probably,
(mixing asm and/or source with the samples). This need adding some
new output option to oprofpp
> > I do not really understand why but I get really strange counter 1
> > result from oprofpp it seems to be always :
> > 16384 * (real counter 1 result).
>
> I think you mean 16384 + (real counter 1 result) ? This was a really
> stupid bug - I had a function opd_get_count() and never actually
> used it !
I have think about 16384 + but result are more strange.
For example use of counter0, counter1 for count mmx ops
with the same um show results like (real example from oprofpp) :
counter0 256 counter1 49372.
not really near 16384 + 256 ...
but probably your cvs update will fix the problem.
I'll will retry and send more precise information,or at least the c
source file wich allow to reproduce the problem, if it persist.
> I'm about to commit the change to CVS, thanks for the bug report.
uhum, I must learn how to use cvs under windows (no linux modem's driver)
> > I do not know if it is a good thing to speak about more than
> > one subjects in this mail list ? Must I repost more clearly or
> > just seperate subject in future?
>
> no, this is fine ;) just put your patches inline if you can
Yes, but I post from windows and I hope than there is no problem
of carriage return/linefeed/tabulation translation, I follow the gcc
convention, all stuff after my signature is the patch, In case of problem
I will send a link to the diff file.
cat patch_name | patch -p1
make install
have work at home.
regards,
--
Philippe Elie, with a little apprehension for the patch : with non-fixed
font under windows it's look like a very dirty patch...
diff -u oprofile-0.0.3/op_events.c oprofile-0.0.3.phe/op_events.c
--- oprofile-0.0.3/op_events.c Fri Apr 6 16:16:46 2001
+++ oprofile-0.0.3.phe/op_events.c Sun Jun 3 00:36:09 2001
@@ -38,6 +38,15 @@
#define u8 unsigned char
#define uint unsigned int
+enum unit_mask_type {
+ /* useless but required by the hardaware */
+ utm_mandatory,
+ /* one of the value are correct */
+ utm_exclusive,
+ /* value can be combined by bitwise or to form new value */
+ utm_bitmask
+};
+
struct op_event {
uint allowed;
u8 val; /* event number */
@@ -47,6 +56,7 @@
struct op_unit_mask {
uint num; /* number of possible unit masks */
+ enum unit_mask_type unit_type_mask;
/* up to six allowed unit masks */
u8 um[6];
};
@@ -59,19 +69,19 @@
static struct op_unit_mask op_unit_masks[] = {
/* not used */
- { 1, { 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 }, },
+ { 1, 0, { 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 }, },
/* MESI counters */
- { 5, { 0x1, 0x2, 0x4, 0x8, 0xf, 0x0 }, },
+ { 5, utm_bitmask, { 0x1, 0x2, 0x4, 0x8, 0xf, 0x0 }, },
/* EBL self/any */
- { 2, { 0x0, 0x20, 0x0, 0x0, 0x0, 0x0 }, },
+ { 2, utm_exclusive, { 0x0, 0x20, 0x0, 0x0, 0x0, 0x0 }, },
/* MMX PII events */
- { 1, { 0xf, 0x0, 0x0, 0x0, 0x0, 0x0 }, },
- { 6, { 0x1, 0x2, 0x4, 0x8, 0x10, 0x20 }, },
- { 2, { 0x0, 0x1, 0x0, 0x0, 0x0, 0x0 }, },
- { 5, { 0x1, 0x2, 0x4, 0x8, 0xf, 0x0 }, },
+ { 1, utm_mandatory, { 0xf, 0x0, 0x0, 0x0, 0x0, 0x0 }, },
+ { 6, utm_bitmask, { 0x1, 0x2, 0x4, 0x8, 0x10, 0x20 }, },
+ { 2, utm_exclusive, { 0x0, 0x1, 0x0, 0x0, 0x0, 0x0 }, },
+ { 5, utm_bitmask, { 0x1, 0x2, 0x4, 0x8, 0xf, 0x0 }, },
/* KNI PIII events */
- { 4, { 0x0, 0x1, 0x2, 0x3, 0x0, 0x0 }, },
- { 2, { 0x0, 0x1, 0x0, 0x0, 0x0, 0x0 }, },
+ { 4, utm_exclusive, { 0x0, 0x1, 0x2, 0x3, 0x0, 0x0 }, },
+ { 2, utm_bitmask, { 0x0, 0x1, 0x0, 0x0, 0x0, 0x0 }, },
};
static struct op_event op_events[] = {
@@ -198,11 +208,27 @@
*/
static int op_check_unit_mask(struct op_unit_mask *allow, u8 um)
{
- u8 i;
+ uint i, mask;
+
+ switch (allow->unit_type_mask) {
+ case utm_exclusive:
+ case utm_mandatory:
+ for (i=0; i < allow->num; i++)
+ if (allow->um[i]==um)
+ return 0;
+ break;
+
+ case utm_bitmask:
+ /* Must reject 0 bit mask because it can count nothing */
+ if (um != 0) {
+ mask = 0;
+ for (i=0; i < allow->num; i++)
+ mask |= allow->um[i];
- for (i=0; i < allow->num; i++) {
- if (allow->um[i]==um)
- return 0;
+ if ((mask & um) == um)
+ return 0;
+ }
+ break;
}
return 1;
@@ -407,7 +433,8 @@
"DS register",
"FS register",
/* IA manual says this is actually FS again - no mention in errata */
- "FS register",
+ /* but test show that is really a typo error from IA manual */
+ "GS register",
"ES,DS,FS,GS registers", NULL, }, },
{ { "prefetch NTA",
"prefetch T1",
|