From: Maynard P. J. <may...@us...> - 2004-09-10 22:13:23
Attachments:
oprofPatchForLoP
|
Hi, As suggested, I've reworked the patch to genericize it as much as possible. Please take a look and let me know what you think. Thanks. -- Maynard Johnson |
From: John L. <le...@mo...> - 2004-09-11 00:32:03
|
On Fri, Sep 10, 2004 at 04:03:54PM -0500, Maynard P. Johnson wrote: > As suggested, I've reworked the patch to genericize it as much as > possible. Please take a look and let me know what you think. Thanks. OK, I think we're down to tweaking now... > +{ > + u64 value; Your patch has whitespace damage all over. Please double check it to ensure there's no trailing whitespace, spaces where tabs should be, extra spaces etc. > + dir = getenv("OPROFILE_EVENTS_DIR"); > + if (dir == NULL) > + dir = OP_DATADIR; > + > + event_dir = xmalloc(strlen(dir) + strlen("/") + strlen(cpu_name) + > + strlen("/") + 1); > + strcpy(event_dir, dir); > + strcat(event_dir, "/"); > + > + strcat(event_dir, cpu_name); > + strcat(event_dir, "/"); > + ev_map_file = xmalloc(strlen(event_dir) + strlen("event_mappings") + 1); > + strcpy(ev_map_file, event_dir); > + strcat(ev_map_file, "event_mappings"); > + filename = ev_map_file; > + return (fopen(ev_map_file, "r")); Odd way of doing it, why not one xmalloc ? > +char const * get_mapping(u8 nr, FILE * fp) { Wrong brace placement. > + if (evt==nr) { spaces between '==' > + free(line); > + line = op_get_line(fp); > + ++line_nr; > + } > + if (event_found) { > + if (!seen_mmcr0 || !seen_mmcr1 || !seen_mmcra) { > + fprintf(stderr, "Error: Missing information in line %d of event mapping file %s\n", line_nr, filename); > + exit(EXIT_FAILURE); > + } > + map = xmalloc(70); > + char * val = xmalloc(20); > + strcpy(map, "mmcr0:"); > + sprintf(val, "%u", mmcr0); > + strcat(map, val); > + free(val); Hmm, can't we just dump the read in line back out (after cutting any trailing comment) ? > +char const * find_mapping_for_event(u8 nr, op_cpu cpu_type) > +{ > + char const * cpu_name = op_get_cpu_name(cpu_type); > + FILE * fp = NULL; > + char const * map = NULL; > + switch (cpu_type) { > + case CPU_PPC64_POWER4: > + case CPU_PPC64_POWER5: > + fp = open_event_mapping_file(cpu_name); > + if (!fp) { > + fprintf(stderr, "oprofile: could not open event mapping file %s\n", filename); > + exit(EXIT_FAILURE); > + } else { > + map = get_mapping(nr, fp); > + } > + break; The event mapping file is not arch-specific: the fatality of its absence is. Try opening it always, and only make it a fatal error if we're ppc64 > +/** Find a mapping for a given event ID for architectures requiring additional information > + * from what is held in the events file. */ > +char const * find_mapping_for_event(u8 val, op_cpu cpu_type); /** * Find... * ...events file. */ please. > + if [ "$CPUTYPE" = "ppc64/power4" -o "$CPUTYPE" = "ppc64/power5" ]; then please use "if test", that's the style in this file. > + if [ $MMCR0_CK_VAL != $MMCR0_VAL -o $MMCR1_CK_VAL != $MMCR1_VAL -o $MMCRA_CK_VAL != $MMCRA_VAL ]; then consider wrapping this line > + > + EVT_MAP_DATA=`echo ${EVENT_STR} | awk '{print $2}'` > + let n=2 > + while [ "$EVT_MAP_DATA" != "" ] > + do > + KEY=`echo ${EVT_MAP_DATA} | awk -F: '{print $1}'` > + VAL=`echo ${EVT_MAP_DATA} | awk -F: '{print $2}'` > + set_ctr_param "" $KEY $VAL > + let n="$n"+1 > + EVT_MAP_DATA=`echo ${EVENT_STR} | awk "{print \$ $n}"` > + done How about using a for loop here for i in "$EVT_MAP_DATA"; do KEY=`echo $i | awk -F: '{print $1}'` VAL=`echo $i | awk -F: '{print $2}'` ... done > + case CPU_PPC64_POWER4: > + //printf("See Power 4 Performance Monitor Programmers Guide.\n"); > + break; > + case CPU_PPC64_POWER5: > + //printf("See Power 5 Performance Monitor Programmers Guide.\n"); > + break; You need to decide what to do with this. As you may have guessed, I think this is probably pretty ready to go in; the above are just some trivial tidy-ups I'd like to see first. regards john |
From: Maynard P. J. <may...@us...> - 2004-09-13 16:51:55
Attachments:
oprofPatchForLoP
|
John, Thanks for the review. I've attached the patch with most of the fix-ups you suggested. See my responses to your comments below. John Levon wrote: > On Fri, Sep 10, 2004 at 04:03:54PM -0500, Maynard P. Johnson wrote: > > >>As suggested, I've reworked the patch to genericize it as much as >>possible. Please take a look and let me know what you think. Thanks. > > > OK, I think we're down to tweaking now... > > >>+{ >>+ u64 value; > > > Your patch has whitespace damage all over. Please double check it to > ensure there's no trailing whitespace, spaces where tabs should be, > extra spaces etc. I think I've fixed up all the whitespace problems now. > > >>+ dir = getenv("OPROFILE_EVENTS_DIR"); >>+ if (dir == NULL) >>+ dir = OP_DATADIR; >>+ >>+ event_dir = xmalloc(strlen(dir) + strlen("/") + strlen(cpu_name) + >>+ strlen("/") + 1); >>+ strcpy(event_dir, dir); >>+ strcat(event_dir, "/"); >>+ >>+ strcat(event_dir, cpu_name); >>+ strcat(event_dir, "/"); >>+ ev_map_file = xmalloc(strlen(event_dir) + strlen("event_mappings") + 1); >>+ strcpy(ev_map_file, event_dir); >>+ strcat(ev_map_file, "event_mappings"); >>+ filename = ev_map_file; >>+ return (fopen(ev_map_file, "r")); > > > Odd way of doing it, why not one xmalloc ? Yeah, right. Fixed. > > >>+char const * get_mapping(u8 nr, FILE * fp) { > > > Wrong brace placement. done > > >>+ if (evt==nr) { > > > spaces between '==' done > > >>+ free(line); >>+ line = op_get_line(fp); >>+ ++line_nr; >>+ } >>+ if (event_found) { >>+ if (!seen_mmcr0 || !seen_mmcr1 || !seen_mmcra) { >>+ fprintf(stderr, "Error: Missing information in line %d of event mapping file %s\n", line_nr, filename); >>+ exit(EXIT_FAILURE); >>+ } >>+ map = xmalloc(70); >>+ char * val = xmalloc(20); >>+ strcpy(map, "mmcr0:"); >>+ sprintf(val, "%u", mmcr0); >>+ strcat(map, val); >>+ free(val); > > > Hmm, can't we just dump the read in line back out (after cutting any > trailing comment) ? Not sure what you're getting at here. Since it works, I left it alone rather than guessing at what you meant. > > >>+char const * find_mapping_for_event(u8 nr, op_cpu cpu_type) >>+{ >>+ char const * cpu_name = op_get_cpu_name(cpu_type); >>+ FILE * fp = NULL; >>+ char const * map = NULL; >>+ switch (cpu_type) { >>+ case CPU_PPC64_POWER4: >>+ case CPU_PPC64_POWER5: >>+ fp = open_event_mapping_file(cpu_name); >>+ if (!fp) { >>+ fprintf(stderr, "oprofile: could not open event mapping file %s\n", filename); >>+ exit(EXIT_FAILURE); >>+ } else { >>+ map = get_mapping(nr, fp); >>+ } >>+ break; > > > The event mapping file is not arch-specific: the fatality of its absence > is. Try opening it always, and only make it a fatal error if we're ppc64 > done > >>+/** Find a mapping for a given event ID for architectures requiring additional information >>+ * from what is held in the events file. */ >>+char const * find_mapping_for_event(u8 val, op_cpu cpu_type); > > > /** > * Find... > * ...events file. > */ > > please. done > > >>+ if [ "$CPUTYPE" = "ppc64/power4" -o "$CPUTYPE" = "ppc64/power5" ]; then > > > please use "if test", that's the style in this file. done > > >>+ if [ $MMCR0_CK_VAL != $MMCR0_VAL -o $MMCR1_CK_VAL != $MMCR1_VAL -o $MMCRA_CK_VAL != $MMCRA_VAL ]; then > > > consider wrapping this line done > > >>+ >>+ EVT_MAP_DATA=`echo ${EVENT_STR} | awk '{print $2}'` >>+ let n=2 >>+ while [ "$EVT_MAP_DATA" != "" ] >>+ do >>+ KEY=`echo ${EVT_MAP_DATA} | awk -F: '{print $1}'` >>+ VAL=`echo ${EVT_MAP_DATA} | awk -F: '{print $2}'` >>+ set_ctr_param "" $KEY $VAL >>+ let n="$n"+1 >>+ EVT_MAP_DATA=`echo ${EVENT_STR} | awk "{print \$ $n}"` >>+ done > > > How about using a for loop here > > for i in "$EVT_MAP_DATA"; do > KEY=`echo $i | awk -F: '{print $1}'` > VAL=`echo $i | awk -F: '{print $2}'` > ... > done no change. > > >>+ case CPU_PPC64_POWER4: >>+ //printf("See Power 4 Performance Monitor Programmers Guide.\n"); >>+ break; >>+ case CPU_PPC64_POWER5: >>+ //printf("See Power 5 Performance Monitor Programmers Guide.\n"); >>+ break; > > > You need to decide what to do with this. Until the PMU programmer's guides are declassified, the help text will just list the supported events, so the case statement for POWER4/5 will do nothing. I removed the commented out line and combined the two cases. > > As you may have guessed, I think this is probably pretty ready to go in; > the above are just some trivial tidy-ups I'd like to see first. Hopefully, we're ready to move to the next step now. Please let me know how you'd like the doc updates handled. Thanks. > > regards > john > -- Maynard Johnson |
From: John L. <le...@mo...> - 2004-09-13 17:09:30
|
On Mon, Sep 13, 2004 at 11:51:45AM -0500, Maynard P. Johnson wrote: > >How about using a for loop here > > > > for i in "$EVT_MAP_DATA"; do > > KEY=`echo $i | awk -F: '{print $1}'` > > VAL=`echo $i | awk -F: '{print $2}'` > > ... > > done > no change. Do feel free to explain why you didn't feel it should be changed... regards john |
From: Maynard P. J. <may...@us...> - 2004-09-13 18:44:28
|
John Levon wrote: > On Mon, Sep 13, 2004 at 11:51:45AM -0500, Maynard P. Johnson wrote: > > >>>How about using a for loop here >>> >>> for i in "$EVT_MAP_DATA"; do >>> KEY=`echo $i | awk -F: '{print $1}'` >>> VAL=`echo $i | awk -F: '{print $2}'` >>> ... >>> done >> >>no change. > > > Do feel free to explain why you didn't feel it should be changed... > > regards > john Actually, I felt that the while loop was more self-explanatory for those of us who aren't so familiar with shell programming. -Maynard > > > ------------------------------------------------------- > This SF.Net email is sponsored by: YOU BE THE JUDGE. Be one of 170 > Project Admins to receive an Apple iPod Mini FREE for your judgement on > who ports your project to Linux PPC the best. Sponsored by IBM. > Deadline: Sept. 13. Go here: http://sf.net/ppc_contest.php > _______________________________________________ > oprofile-list mailing list > opr...@li... > https://lists.sourceforge.net/lists/listinfo/oprofile-list > |
From: John L. <le...@mo...> - 2004-09-13 19:30:41
|
On Mon, Sep 13, 2004 at 01:44:13PM -0500, Maynard P. Johnson wrote: > Actually, I felt that the while loop was more self-explanatory for those > of us who aren't so familiar with shell programming. Going to have to wield the big stick here I'm afraid. IFS-using for loops are very much standard shell style, you really should be using them where the fit is natural, such as here. (rest of the patch looks great btw) regards john |
From: Maynard P. J. <may...@us...> - 2004-09-14 17:22:51
Attachments:
oprofPatchForLoP
|
Here's that patch with the change in opcontrol to use a for-loop. Regards, -Maynard John Levon wrote: > On Mon, Sep 13, 2004 at 01:44:13PM -0500, Maynard P. Johnson wrote: > > >>Actually, I felt that the while loop was more self-explanatory for those >>of us who aren't so familiar with shell programming. > > > Going to have to wield the big stick here I'm afraid. IFS-using for > loops are very much standard shell style, you really should be using > them where the fit is natural, such as here. > > (rest of the patch looks great btw) > > regards > john > > > ------------------------------------------------------- > This SF.Net email is sponsored by: YOU BE THE JUDGE. Be one of 170 > Project Admins to receive an Apple iPod Mini FREE for your judgement on > who ports your project to Linux PPC the best. Sponsored by IBM. > Deadline: Sept. 13. Go here: http://sf.net/ppc_contest.php > _______________________________________________ > oprofile-list mailing list > opr...@li... > https://lists.sourceforge.net/lists/listinfo/oprofile-list > -- Maynard Johnson |
From: John L. <le...@mo...> - 2004-09-15 01:27:12
|
On Tue, Sep 14, 2004 at 12:22:30PM -0500, Maynard P. Johnson wrote: > Here's that patch with the change in opcontrol to use a for-loop. Applied - thanks for your hard work here. regards john |
From: John L. <le...@mo...> - 2004-09-15 01:29:36
|
On Tue, Sep 14, 2004 at 12:22:30PM -0500, Maynard P. Johnson wrote: > Here's that patch with the change in opcontrol to use a for-loop. > BTW, changelogs inline in the email from now on please. I took pity the first time :) cheers john |
From: Maynard P. J. <may...@us...> - 2004-09-15 22:07:33
|
John Levon wrote: > On Tue, Sep 14, 2004 at 12:22:30PM -0500, Maynard P. Johnson wrote: > > >>Here's that patch with the change in opcontrol to use a for-loop. >> > > > BTW, changelogs inline in the email from now on please. I took pity the > first time :) John, OK, will do. As you guessed, I'm a newbie at this. Thanks for the help. As I mentioned, I will write up some documentation on the new function to be added to sections of the oprofile user manual. One of the pieces of information to be documented is the specific kernel version that is required. My colleague, Carl Love, has been working that end of things, but as of today, not all changes have gotten into the mainline kernel yet. Once we have that info, I'll send you the documentation updates. Do you want the doc updates in the form of a patch? The oprofile website states "You will need to have an XSL stylesheets installation (and xsltproc etc.) installed to change the documentation." Can anyone point me to any place where I can find out more about how to do this? Thanks. Regards, Maynard > > cheers > john > > > ------------------------------------------------------- > This SF.Net email is sponsored by: thawte's Crypto Challenge Vl > Crack the code and win a Sony DCRHC40 MiniDV Digital Handycam > Camcorder. More prizes in the weekly Lunch Hour Challenge. > Sign up NOW http://ad.doubleclick.net/clk;10740251;10262165;m > _______________________________________________ > oprofile-list mailing list > opr...@li... > https://lists.sourceforge.net/lists/listinfo/oprofile-list > -- Maynard Johnson |
From: John L. <le...@mo...> - 2004-09-16 00:11:28
|
On Wed, Sep 15, 2004 at 05:07:18PM -0500, Maynard P. Johnson wrote: > OK, will do. As you guessed, I'm a newbie at this. Thanks for the help. Glad to help. > yet. Once we have that info, I'll send you the documentation updates. > Do you want the doc updates in the form of a patch? Yes. > The oprofile website states "You will need to have an XSL stylesheets > installation (and xsltproc etc.) installed to change the > documentation." Can anyone point me to any place where I can find out > more about how to do this? If you're running a relatively recent linux distribution you shouldn't have any problem, just install the RPMS or whatever if they're not already (Omake sure xsltproc is on your path). Then do "./configure" and oprofile should find your stylesheets etc - check the output. After that, a make / make chunk in the doc/ directory will build HTML docs from the oprofile.xml regards john |
From: William C. <wc...@nc...> - 2004-10-05 19:40:50
|
Maynard P. Johnson wrote: > John Levon wrote: > >> On Tue, Sep 14, 2004 at 12:22:30PM -0500, Maynard P. Johnson wrote: >> >> >>> Here's that patch with the change in opcontrol to use a for-loop. >>> >> >> >> BTW, changelogs inline in the email from now on please. I took pity the >> first time :) > > John, > OK, will do. As you guessed, I'm a newbie at this. Thanks for the help. > > As I mentioned, I will write up some documentation on the new function > to be added to sections of the oprofile user manual. One of the pieces > of information to be documented is the specific kernel version that is > required. My colleague, Carl Love, has been working that end of things, > but as of today, not all changes have gotten into the mainline kernel > yet. Once we have that info, I'll send you the documentation updates. > Do you want the doc updates in the form of a patch? The oprofile > website states "You will need to have an XSL stylesheets installation > (and xsltproc etc.) installed to change the documentation." Can anyone > point me to any place where I can find out more about how to do this? > > Thanks. > Regards, > Maynard I have been looking over the patch for power4 and power5. Why the grouping into small groups each with one value for each performance register and the restriction that events from different groups cannot be run concurrently? It seems like there are events from different groups that would be useful to run concurrently. For example on power5 L1 loads AND l1 stores with L1 D cache load miss AMD L1 D cache store miss (groups 43 and 44 in the ppc64/power5/events). Do you have URLs to publically available documentation on the performance monitoring hardware in the power5? I haven't seen any around. -Will |
From: William C. <wc...@nc...> - 2004-10-06 15:31:14
|
Maynard P. Johnson wrote: > John Levon wrote: > >> On Tue, Sep 14, 2004 at 12:22:30PM -0500, Maynard P. Johnson wrote: >> >> >>> Here's that patch with the change in opcontrol to use a for-loop. >>> >> >> >> BTW, changelogs inline in the email from now on please. I took pity the >> first time :) > > John, > OK, will do. As you guessed, I'm a newbie at this. Thanks for the help. > > As I mentioned, I will write up some documentation on the new function > to be added to sections of the oprofile user manual. One of the pieces > of information to be documented is the specific kernel version that is > required. My colleague, Carl Love, has been working that end of things, > but as of today, not all changes have gotten into the mainline kernel > yet. Once we have that info, I'll send you the documentation updates. > Do you want the doc updates in the form of a patch? The oprofile > website states "You will need to have an XSL stylesheets installation > (and xsltproc etc.) installed to change the documentation." Can anyone > point me to any place where I can find out more about how to do this? > > Thanks. > Regards, > Maynard > Looking through the oprofile code for the kernel for the ppc64 oprofile support oprof_ppc64_ops.cpu_type can be set to the following in arch/ppc64/oprofile/common.c: "ppc64/power3"; "ppc64/rs64"; "ppc64/power4"; "ppc64/970"; "ppc64/power5"; The patch only handles "ppc64/power4" and "ppc64/power5". On one machine got: # more /dev/oprofile/cpu_type ppc64/970 As a result got non-helpful answer (should print out the reported cpu_type): # op_help cpu_type '(null)' is not valid Are there similar patches for the power3, rs64, and 970? Is the 970 the same as power4? Why the distinction between power4 and 970 in the kernel's oprofile driver? -Will |
From: William C. <wc...@nc...> - 2004-10-07 15:07:20
Attachments:
oprofile-0.8.1-power45sign.patch
|
Maynard P. Johnson wrote: > John Levon wrote: > >> On Tue, Sep 14, 2004 at 12:22:30PM -0500, Maynard P. Johnson wrote: >> >> >>> Here's that patch with the change in opcontrol to use a for-loop. >>> >> >> >> BTW, changelogs inline in the email from now on please. I took pity the >> first time :) > > John, > OK, will do. As you guessed, I'm a newbie at this. Thanks for the help. > > As I mentioned, I will write up some documentation on the new function > to be added to sections of the oprofile user manual. One of the pieces > of information to be documented is the specific kernel version that is > required. My colleague, Carl Love, has been working that end of things, > but as of today, not all changes have gotten into the mainline kernel > yet. Once we have that info, I'll send you the documentation updates. > Do you want the doc updates in the form of a patch? The oprofile > website states "You will need to have an XSL stylesheets installation > (and xsltproc etc.) installed to change the documentation." Can anyone > point me to any place where I can find out more about how to do this? > > Thanks. > Regards, > Maynard > > I have been doing some testing and I found that checking for some event groups, e.g. 14 and 52 on the power5, did not work correctly. In opcontrol the msb bit of the value was set when assigned using a "let" This was interpreted as a negative sign and the comparisons failed. I have a small patch for opcontrol that fixes the problem. 2004-10-07 Will Cohen <wc...@re...> * utils/opcontrol: Correct ppc64 checks that events in same group. -Will |
From: John L. <le...@mo...> - 2004-10-09 02:33:05
|
On Thu, Oct 07, 2004 at 11:01:34AM -0400, William Cohen wrote: > 2004-10-07 Will Cohen <wc...@re...> > > * utils/opcontrol: Correct ppc64 checks that events in same > group. OK john |
From: Maynard P. J. <may...@us...> - 2004-10-08 19:14:37
|
John Levon wrote: > On Thu, Oct 07, 2004 at 11:01:34AM -0400, William Cohen wrote: > > >>2004-10-07 Will Cohen <wc...@re...> >> >> * utils/opcontrol: Correct ppc64 checks that events in same >> group. > > > OK > > john > Sorry, I've been tied up and hadn't been able to respond to Will's bug report and his patch. I've done some testing on my own and concur with the changes. John, I see you've committed it to CVS already. Thanks! -- Maynard Johnson |
From: William C. <wc...@nc...> - 2004-10-15 18:13:47
|
Maynard P. Johnson wrote: > diff -paurNX ../diff_fileExclusionFilter oprofile/events/ppc64/power4/events ../workspace/oprofile0.8.1-forPOWER/events/ppc64/power4/events > --- oprofile/events/ppc64/power4/events 1969-12-31 18:00:00.000000000 -0600 > +++ ../workspace/oprofile0.8.1-forPOWER/events/ppc64/power4/events 2004-09-10 15:39:59.745141032 -0500 > +#Group 2 pm_basic > +event:0x20 counters:0 um:zero minimum:5000 name:PM_INST_CMPL_GRP2 : (Group 2 pm_basic) Instrucitons completed > +event:0x25 counters:5 um:zero minimum:5000 name:PM_INST_CMPL_GRP2 : (Group 2 pm_basic) Number of ELigible Instructions that completed Shouldn't these events have two different names? I found this when testing. Also noticed for power4 I couldn't have more than one event from group 4. For example: #/usr/local/bin/opcontrol --setup --no-vmlinux --event=PM_INST_CMPL_GRP4:500000:0 --event=PM_L2_DCACHE_RELOAD_VALID_GRP4:500000:0 --event=PM_CYC_GRP4:500000:0 --event=PM_DATA_FROM_L2_GRP4:500000:0 --event=PM_DATA_FROM_L25_SH_GRP4:500000:0 --event=PM_DATA_FROM_L275_SHR_GRP4:500000:0 --event=PM_DATA_FROM_L275_MOD_GRP4:500000:0 --event=PM_DATA_FROM_L25_MOD_GRP4:500000:0 (Couldn't allocate hardware counters for the selected events.) This appears to be a problem in libop/op_alloc_counter.c:map_event_to_counter() or things it calls. opcontrol uses op_help which in turn users map_event_to_counter(). -Will |
From: William C. <wc...@nc...> - 2004-10-15 18:41:43
|
William Cohen wrote: > > > Maynard P. Johnson wrote: > >> diff -paurNX ../diff_fileExclusionFilter >> oprofile/events/ppc64/power4/events >> ../workspace/oprofile0.8.1-forPOWER/events/ppc64/power4/events >> --- oprofile/events/ppc64/power4/events 1969-12-31 >> 18:00:00.000000000 -0600 >> +++ ../workspace/oprofile0.8.1-forPOWER/events/ppc64/power4/events >> 2004-09-10 15:39:59.745141032 -0500 > > >> +#Group 2 pm_basic >> +event:0x20 counters:0 um:zero minimum:5000 name:PM_INST_CMPL_GRP2 : >> (Group 2 pm_basic) Instrucitons completed > > >> +event:0x25 counters:5 um:zero minimum:5000 name:PM_INST_CMPL_GRP2 : >> (Group 2 pm_basic) Number of ELigible Instructions that completed > > > Shouldn't these events have two different names? I found this when testing. > > Also noticed for power4 I couldn't have more than one event from group > 4. For example: > > #/usr/local/bin/opcontrol --setup --no-vmlinux > --event=PM_INST_CMPL_GRP4:500000:0 > --event=PM_L2_DCACHE_RELOAD_VALID_GRP4:500000:0 > --event=PM_CYC_GRP4:500000:0 --event=PM_DATA_FROM_L2_GRP4:500000:0 > --event=PM_DATA_FROM_L25_SH_GRP4:500000:0 > --event=PM_DATA_FROM_L275_SHR_GRP4:500000:0 > --event=PM_DATA_FROM_L275_MOD_GRP4:500000:0 > --event=PM_DATA_FROM_L25_MOD_GRP4:500000:0 > (Couldn't allocate hardware counters for the selected events.) > > This appears to be a problem in > libop/op_alloc_counter.c:map_event_to_counter() or things it calls. > opcontrol uses op_help which in turn users map_event_to_counter(). The second problem is due to all the "counters:" fields being 0 for the group 4. Corrected the problem in power4/events file. -Will |
From: Maynard P. J. <may...@us...> - 2004-10-15 18:42:41
|
William Cohen wrote: > > > Maynard P. Johnson wrote: > >> diff -paurNX ../diff_fileExclusionFilter >> oprofile/events/ppc64/power4/events >> ../workspace/oprofile0.8.1-forPOWER/events/ppc64/power4/events >> --- oprofile/events/ppc64/power4/events 1969-12-31 >> 18:00:00.000000000 -0600 >> +++ ../workspace/oprofile0.8.1-forPOWER/events/ppc64/power4/events >> 2004-09-10 15:39:59.745141032 -0500 > > >> +#Group 2 pm_basic >> +event:0x20 counters:0 um:zero minimum:5000 name:PM_INST_CMPL_GRP2 : >> (Group 2 pm_basic) Instrucitons completed > > >> +event:0x25 counters:5 um:zero minimum:5000 name:PM_INST_CMPL_GRP2 : >> (Group 2 pm_basic) Number of ELigible Instructions that completed > > > Shouldn't these events have two different names? I found this when testing. The duplicates shouldn't hurt anything, but are certainly unnecessary. I will fix this at the same time I make the changes to minimum count values. > > Also noticed for power4 I couldn't have more than one event from group > 4. For example: > > #/usr/local/bin/opcontrol --setup --no-vmlinux > --event=PM_INST_CMPL_GRP4:500000:0 > --event=PM_L2_DCACHE_RELOAD_VALID_GRP4:500000:0 > --event=PM_CYC_GRP4:500000:0 --event=PM_DATA_FROM_L2_GRP4:500000:0 > --event=PM_DATA_FROM_L25_SH_GRP4:500000:0 > --event=PM_DATA_FROM_L275_SHR_GRP4:500000:0 > --event=PM_DATA_FROM_L275_MOD_GRP4:500000:0 > --event=PM_DATA_FROM_L25_MOD_GRP4:500000:0 > (Couldn't allocate hardware counters for the selected events.) This was a problem in the events file. The counter # for all 8 events in group 4 was set to '0'. I will fix this, too. > > -Will > > > > > ------------------------------------------------------- > This SF.net email is sponsored by: IT Product Guide on ITManagersJournal > Use IT products in your business? Tell us what you think of them. Give us > Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more > http://productguide.itmanagersjournal.com/guidepromo.tmpl > _______________________________________________ > oprofile-list mailing list > opr...@li... > https://lists.sourceforge.net/lists/listinfo/oprofile-list > Thanks for spotting these problems. -- Maynard Johnson |
From: William C. <wc...@nc...> - 2004-10-15 18:48:45
|
Maynard P. Johnson wrote: > William Cohen wrote: > >> >> >> Maynard P. Johnson wrote: >> >>> diff -paurNX ../diff_fileExclusionFilter >>> oprofile/events/ppc64/power4/events >>> ../workspace/oprofile0.8.1-forPOWER/events/ppc64/power4/events >>> --- oprofile/events/ppc64/power4/events 1969-12-31 >>> 18:00:00.000000000 -0600 >>> +++ ../workspace/oprofile0.8.1-forPOWER/events/ppc64/power4/events >>> 2004-09-10 15:39:59.745141032 -0500 >> >> >> >>> +#Group 2 pm_basic >>> +event:0x20 counters:0 um:zero minimum:5000 name:PM_INST_CMPL_GRP2 : >>> (Group 2 pm_basic) Instrucitons completed >> >> >> >>> +event:0x25 counters:5 um:zero minimum:5000 name:PM_INST_CMPL_GRP2 : >>> (Group 2 pm_basic) Number of ELigible Instructions that completed >> >> >> >> Shouldn't these events have two different names? I found this when >> testing. > > The duplicates shouldn't hurt anything, but are certainly unnecessary. I > will fix this at the same time I make the changes to minimum count values. The descriptions and event numbers were different, so I wasn't sure if this was a typo due to a cutting an pasting. Keep in mind I don't have access to any of the detailed event information and names, so this looked like they could have been different events. -Will |
From: Maynard P. J. <may...@us...> - 2004-10-15 19:16:12
|
William Cohen wrote: > > > > The descriptions and event numbers were different, so I wasn't sure if > this was a typo due to a cutting an pasting. Keep in mind I don't have > access to any of the detailed event information and names, so this > looked like they could have been different events. Right. Without going into all the gory details, from a hardware perspectve, counters 0 and 5 are both capable of counting instructions compelted when the other counters are used for counting the events listed in group 2. From the user's perspective, there's no extra information that can be gotten from this redundant counter, so that's why I say it's unneccesary and can be removed from OProfile's events file. Once the 970 stuff is committed into CVS, I'll create and submit the patch that fixes this (and updates the minimum count values). Thanks. > > -Will > > -- Maynard Johnson |
From: Maynard P. J. <may...@us...> - 2004-10-15 21:55:14
Attachments:
oprof-ppc64-event-fixes-patch
|
Maynard P. Johnson wrote: Attached is a patch that fixes several minor problems with PPC64 event files. Thanks. > William Cohen wrote: > >> >> >> >> The descriptions and event numbers were different, so I wasn't sure if >> this was a typo due to a cutting an pasting. Keep in mind I don't have >> access to any of the detailed event information and names, so this >> looked like they could have been different events. > > Right. Without going into all the gory details, from a hardware > perspectve, counters 0 and 5 are both capable of counting instructions > compelted when the other counters are used for counting the events > listed in group 2. From the user's perspective, there's no extra > information that can be gotten from this redundant counter, so that's > why I say it's unneccesary and can be removed from OProfile's events file. > > Once the 970 stuff is committed into CVS, I'll create and submit the > patch that fixes this (and updates the minimum count values). Thanks. > >> >> -Will >> >> > > -- Maynard Johnson |