Re: [PATCH 3/4] New ocount tool and associated ocount_counter classes

[Wainer M. <wai...@li...>]

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.

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