From: Maynard J. <may...@us...> - 2013-06-25 22:17:45
|
On 06/25/2013 04:10 PM, Wainer Moschetta wrote: > On 06/25/2013 11:00 AM, Maynard Johnson wrote: >> New ocount tool and associated ocount_counter classes >> >> This patch implements the executable 'ocount' tool, as >> well as some classes used for opening and managing the >> "perf_events" file descriptors (opened via perf_event_open). > Hi Maynard, > > I've built oprofile with ocount support on an PowerPC machine, made some > pretty simple tests and, as far as I can tell you, it all seems to work > very well. > > However, I've found some inconsistencies between the manpage and actual > ocount options. Please, see comments below. Wainer, Thanks very much for taking the time to test and review. You have a keen eye to catch the inconsistencies below. Unless I get more review comments requiring some more extensive rework of the patch set, I won't bother submitting a version 2 of the set. I'll fix the issues you raised here and in your other review posting when I commit. -Maynard > >> >> Signed-off-by: Maynard Johnson <may...@us...> >> --- >> Makefile.am | 1 + >> configure.ac | 1 + >> pe_counting/Makefile.am | 28 ++ >> pe_counting/ocount.cpp | 817 ++++++++++++++++++++++++++++++++++++++++ >> pe_counting/ocount_counter.cpp | 614 ++++++++++++++++++++++++++++++ >> pe_counting/ocount_counter.h | 113 ++++++ >> 6 files changed, 1574 insertions(+), 0 deletions(-) >> create mode 100644 pe_counting/Makefile.am >> create mode 100644 pe_counting/ocount.cpp >> create mode 100644 pe_counting/ocount_counter.cpp >> create mode 100644 pe_counting/ocount_counter.h >> > <snip> >> + >> +static enum op_runmode runmode = OP_MAX_RUNMODE; >> +static string runmode_options[] = { "<command> [command-args]", "--system-wide", "--cpu-list", >> + "--process-list", "--thread-list" >> +}; >> + >> + >> +struct option long_options [] = >> +{ >> + {"verbose", no_argument, NULL, 'V'}, >> + {"system-wide", no_argument, NULL, 's'}, >> + {"cpu-list", required_argument, NULL, 'C'}, >> + {"process-list", required_argument, NULL, 'p'}, >> + {"thread-list", required_argument, NULL, 'r'}, >> + {"events", required_argument, NULL, 'e'}, >> + {"output-file", required_argument, NULL, 'f'}, >> + {"separate-cpu", no_argument, NULL, 'c'}, >> + {"separate-thread", no_argument, NULL, 't'}, >> + {"brief-format", no_argument, NULL, 'b'}, >> + {"time-interval", required_argument, NULL, 'i'}, > > The ocount's man says "--time-intervals" but actually "--time-interval" > is the valid option (as defined above). Need to either adjust the option > name or change the manpage. > > <snip> >> + >> + >> +static void __print_usage_and_exit(const char * extra_msg) >> +{ >> + if (extra_msg) >> + cerr << extra_msg << endl; >> + cerr << "usage: ocount [ options ] [ --system-wide | --pid <pid> | [ command [ args ] ] ]" << endl; > > The "--pid" flag doesn't exit and should be "--process-list". It is > also missing "--thread-list" and "--cpu-list" on usage message as well. > >> + cerr << "See ocount man page for details." << endl; >> + exit(EXIT_FAILURE); >> +} >> + >> + > <snip> > > Hey, nice job! I'm sure it will be lot of useful to performance > engineers using Oprofile. > > Thanks, > Wainer dos Santos Moschetta > > > ------------------------------------------------------------------------------ > This SF.net email is sponsored by Windows: > > Build for Windows Store. > > http://p.sf.net/sfu/windows-dev2dev > _______________________________________________ > oprofile-list mailing list > opr...@li... > https://lists.sourceforge.net/lists/listinfo/oprofile-list > |