Menu
▾
▴
Re: [PATCH 3/5] Add support for naming unit masks
|
From: Maynard J. <may...@us...> - 2011-04-29 15:13:43
|
Andi Kleen wrote:
> From: Andi Kleen <ak...@li...>
>
> With extra fields available just specifying the numerical unit_mask is often
> not enough to select the right event. This patch allows to specify unitmasks
> by name (first word in the description) in the event description.
>
> This also makes many unit masks easier to use because names are easier
> to remember than numbers.
>
> v2: Fixed review feedback, fix regression tester
>
> Signed-off-by: Andi Kleen <ak...@li...>
> ---
> doc/opcontrol.1.in | 5 ++-
> doc/oprofile.xml | 2 +-
> libop/op_events.c | 107 +++++++++++++++++++++++++++++++++++++++
> libop/op_events.h | 11 ++++
> libop/op_parse_event.c | 9 +++-
> libop/op_parse_event.h | 1 +
> libop/tests/parse_event_tests.c | 8 ++--
> utils/opcontrol | 13 ++---
> utils/ophelp.c | 40 ++++----------
> 9 files changed, 153 insertions(+), 43 deletions(-)
>
> diff --git a/doc/opcontrol.1.in b/doc/opcontrol.1.in
> index 5a78aa6..931d877 100644
> --- a/doc/opcontrol.1.in
> +++ b/doc/opcontrol.1.in
> @@ -107,7 +107,10 @@ or "default" for the default event. The event is of the form
> count, unit mask, kernel-space counting, user-space counting,
> respectively. Note that this over-rides all previous events selected;
> if you want two or more counters used simultaneously, you must specify
> -them on the same opcontrol invocation.
> +them on the same opcontrol invocation. The numerical unit mask
> +can also be a string which matches the first word in the unit mask
> +description, but only for events with extra parameters shown.
The code does not match your statement above. As I said before, I won't allow a semi-functional unit mask naming scheme where users have to remember the rules of when they can use names and when they can't. I was willing to make an exception for Sandybridge's um's with the extra parameters, and I *thought* you understood my decision. You have caused me to waste my time reviewing a patch that you knew would be unacceptable. This is not the first time, but it better be the last.
-Maynard
> +Events with extra parameters must be specified by first word.
> .br
> .TP
> .BI "--separate="[none,lib,kernel,thread,cpu,all]
> diff --git a/doc/oprofile.xml b/doc/oprofile.xml
> index 0d6407c..843d96b 100644
> --- a/doc/oprofile.xml
> +++ b/doc/oprofile.xml
> @@ -877,7 +877,7 @@ of the form <option><emphasis>name</emphasis>:<emphasis>count</emphasis>:<emphas
> <tbody>
> <row><entry><option>name</option></entry><entry>The symbolic event name, e.g. <constant>CPU_CLK_UNHALTED</constant></entry></row>
> <row><entry><option>count</option></entry><entry>The counter reset value, e.g. 100000</entry></row>
> -<row><entry><option>unitmask</option></entry><entry>The unit mask, as given in the events list, e.g. 0x0f</entry></row>
> +<row><entry><option>unitmask</option></entry><entry>The unit mask, as given in the events list, e.g. 0x0f or a symbolic name (first word of the description if unique)</entry></row>
> <row><entry><option>kernel</option></entry><entry>Whether to profile kernel code</entry></row>
> <row><entry><option>user</option></entry><entry>Whether to profile userspace code</entry></row>
> </tbody>
> diff --git a/libop/op_events.c b/libop/op_events.c
> index 8da023b..0f27288 100644
> --- a/libop/op_events.c
> +++ b/libop/op_events.c
> @@ -17,6 +17,7 @@
> #include "op_string.h"
> #include "op_cpufreq.h"
> #include "op_hw_specific.h"
> +#include "op_parse_event.h"
>
> #include <string.h>
> #include <stdlib.h>
> @@ -921,6 +922,18 @@ struct op_event * find_event_by_name(char const * name, unsigned um, int um_vali
> }
>
>
> +static struct op_event * find_next_event(struct op_event * e)
> +{
> + struct list_head * n;
> +
> + for (n = e->event_next.next; n != &events_list; n = n->next) {
> + struct op_event * ne = list_entry(n, struct op_event, event_next);
> + if (!strcmp(e->name, ne->name))
> + return ne;
> + }
> + return NULL;
> +}
> +
> struct op_event * op_find_event(op_cpu cpu_type, u32 nr, u32 um)
> {
> struct op_event * event;
> @@ -1120,3 +1133,97 @@ void op_default_event(op_cpu cpu_type, struct op_default_event_descr * descr)
> break;
> }
> }
> +
> +static void extra_check(struct op_event *e, u32 unit_mask)
> +{
> + unsigned i;
> + int found = 0;
> +
> + for (i = 0; i < e->unit->num; i++)
> + if (e->unit->um[i].value == unit_mask)
> + found++;
> + if (found > 1) {
> + fprintf(stderr,
> +"Unfortunately you are not allowed to use named unit masks on events\n"
> +"without extra values. Please specify the magic numbers for the unit mask\n"
> +"instead\n");
> + exit(EXIT_FAILURE);
> + }
> +}
> +
> +static void do_resolve_unit_mask(struct op_event *e, struct parsed_event *pe,
> + u32 *extra)
> +{
> + unsigned i;
> + int found;
> +
> + for (;;) {
> + if (pe->unit_mask_name == NULL) {
> + int fi = 0;
> + unsigned um = pe->unit_mask;
> + int had_unit_mask = pe->unit_mask_valid;
> +
> + found = 0;
> + for (i = 0; i < e->unit->num; i++) {
> + if (pe->unit_mask_valid &&
> + e->unit->um[i].value == um) {
> + if (found++ == 0)
> + fi = i;
> + }
> + if (!pe->unit_mask_valid &&
> + e->unit->um[i].value == e->unit->default_mask) {
> + pe->unit_mask_valid = 1;
> + pe->unit_mask = e->unit->default_mask;
> + break;
> + }
> + }
> + if (found > 1 && had_unit_mask) {
> + fprintf(stderr,
> + "Non unique numerical unit mask.\n"
> + "Please specify the unit mask using the first word of the description\n");
> + exit(EXIT_FAILURE);
> + }
> + extra_check(e, pe->unit_mask);
> + if (i == e->unit->num) {
> + e = find_next_event(e);
> + if (e != NULL)
> + continue;
> + } else {
> + if (extra)
> + *extra = e->unit->um[i].extra;
> + }
> + return;
> + }
> + for (i = 0; i < e->unit->num; i++) {
> + int len = strcspn(e->unit->um[i].desc, " \t");
> + if (!strncmp(pe->unit_mask_name, e->unit->um[i].desc,
> + len) && pe->unit_mask_name[len] == '\0')
> + break;
> + }
> + if (i == e->unit->num) {
> + e = find_next_event(e);
> + if (e != NULL)
> + continue;
> + fprintf(stderr, "Cannot find unit mask %s for %s\n",
> + pe->unit_mask_name, pe->name);
> + exit(EXIT_FAILURE);
> + }
> + pe->unit_mask_valid = 1;
> + pe->unit_mask = e->unit->um[i].value;
> + if (extra)
> + *extra = e->unit->um[i].extra;
> + return;
> + }
> +}
> +
> +void op_resolve_unit_mask(struct parsed_event *pe, u32 *extra)
> +{
> + struct op_event *e;
> +
> + e = find_event_by_name(pe->name, 0, 0);
> + if (!e) {
> + fprintf(stderr, "Cannot find event %s\n", pe->name);
> + exit(EXIT_FAILURE);
> + }
> + return do_resolve_unit_mask(e, pe, extra);
> +}
> diff --git a/libop/op_events.h b/libop/op_events.h
> index 3aaaba2..58d39da 100644
> --- a/libop/op_events.h
> +++ b/libop/op_events.h
> @@ -128,6 +128,17 @@ struct op_default_event_descr {
> */
> void op_default_event(op_cpu cpu_type, struct op_default_event_descr * descr);
>
> +/**
> + * op_resolve_unit_mask - resolve a unit mask in a parsed event.
> + * @pe parsed event
> + * @extra pointer to extra mask or NULL.
> + *
> + * Fills in the extra mask for the unit mask.
> + */
> +
> +struct parsed_event;
> +void op_resolve_unit_mask(struct parsed_event *pe, u32 *extra);
> +
> #ifdef __cplusplus
> }
> #endif
> diff --git a/libop/op_parse_event.c b/libop/op_parse_event.c
> index eb99a20..949730b 100644
> --- a/libop/op_parse_event.c
> +++ b/libop/op_parse_event.c
> @@ -13,6 +13,7 @@
>
> #include <stdio.h>
> #include <stdlib.h>
> +#include <ctype.h>
>
> #include "op_parse_event.h"
> #include "op_string.h"
> @@ -94,8 +95,12 @@ size_t parse_events(struct parsed_event * parsed_events, size_t max_events,
>
> if (part) {
> parsed_events[i].unit_mask_valid = 1;
> - parsed_events[i].unit_mask = parse_ulong(part);
> - free(part);
> + if (!isdigit(*part))
> + parsed_events[i].unit_mask_name = part;
> + else {
> + parsed_events[i].unit_mask = parse_ulong(part);
> + free(part);
> + }
> }
>
> parsed_events[i].kernel = 1;
> diff --git a/libop/op_parse_event.h b/libop/op_parse_event.h
> index c8d4144..2519b0d 100644
> --- a/libop/op_parse_event.h
> +++ b/libop/op_parse_event.h
> @@ -20,6 +20,7 @@ struct parsed_event {
> char * name;
> int count;
> int unit_mask;
> + char * unit_mask_name;
> int kernel;
> int user;
> int unit_mask_valid;
> diff --git a/libop/tests/parse_event_tests.c b/libop/tests/parse_event_tests.c
> index b2fda3b..8e9dabe 100644
> --- a/libop/tests/parse_event_tests.c
> +++ b/libop/tests/parse_event_tests.c
> @@ -22,10 +22,10 @@ struct events_test {
>
> static struct events_test const events[] =
> {
> - { { "FOO:3000:0:0:0", 0 }, { "FOO", 3000, 0, 0, 0, 0 } },
> - { { "BAR:3000", 0 }, { "BAR", 3000, 0, 1, 1, 0 } },
> - { { "FOOBAR:3000:1:1:1", 0 }, { "FOOBAR", 3000, 1, 1, 1, 0 } },
> - { { NULL, NULL }, { 0, 0, 0, 0, 0, 0 } }
> + { { "FOO:3000:0:0:0", 0 }, { "FOO", 3000, 0, NULL, 0, 0, 0 } },
> + { { "BAR:3000", 0 }, { "BAR", 3000, 0, NULL, 1, 1, 0 } },
> + { { "FOOBAR:3000:1:1:1", 0 }, { "FOOBAR", 3000, 1, NULL, 1, 1, 0 } },
> + { { NULL, NULL }, { 0, 0, 0, NULL, 0, 0, 0 } }
> };
>
> static void do_test(struct events_test const * ev)
> diff --git a/utils/opcontrol b/utils/opcontrol
> index 603172d..dfe70f0 100644
> --- a/utils/opcontrol
> +++ b/utils/opcontrol
> @@ -638,12 +638,11 @@ normalise_events()
> UNIT_MASK=`echo $GOTEVENT | awk -F: '{print $3}'`
> KERNEL=`echo $GOTEVENT | awk -F: '{print $4}'`
> USER=`echo $GOTEVENT | awk -F: '{print $5}'`
> - if test -z "$UNIT_MASK"; then
> - TMPEVENT="$EVENT:$COUNT"
> - UNIT_MASK=`$OPHELP --unit-mask $TMPEVENT`
> - if test "$?" != 0; then
> - exit 1
> - fi
> + TMPEVENT="$EVENT:$COUNT:$UNIT_MASK"
> + UNIT_MASK_NAMED="$UNIT_MASK"
> + UNIT_MASK=`$OPHELP --unit-mask $TMPEVENT`
> + if test "$?" != 0; then
> + exit 1
> fi
> if test -z "$KERNEL"; then
> KERNEL=1
> @@ -1442,7 +1441,7 @@ do_param_setup()
> set_ctr_param $CTR user $USER
> set_ctr_param $CTR unit_mask $UNIT_MASK
>
> - EXTRA=`$OPHELP --extra-mask $GOTEVENT`
> + EXTRA=`$OPHELP --extra-mask $EVENT:$COUNT:$UNIT_MASK_NAMED`
> if test "$EXTRA" -ne 0 ; then
> if ! test -d $MOUNT/$CTR/extra ; then
> echo >&2 "Warning: $GOTEVENT has extra mask, but kernel does not support extra field"
> diff --git a/utils/ophelp.c b/utils/ophelp.c
> index 60b1662..b3aebde 100644
> --- a/utils/ophelp.c
> +++ b/utils/ophelp.c
> @@ -182,6 +182,8 @@ static void check_event(struct parsed_event * pev,
> exit(EXIT_FAILURE);
> }
>
> + op_resolve_unit_mask(pev, NULL);
> +
> ret = op_check_events(0, event->val, pev->unit_mask, cpu_type);
>
> if (ret & OP_INVALID_UM) {
> @@ -212,6 +214,7 @@ static void resolve_events(void)
> count = parse_events(parsed_events, num_chosen_events, chosen_events);
>
> for (i = 0; i < count; ++i) {
> + op_resolve_unit_mask(&parsed_events[i], NULL);
> for (j = i + 1; j < count; ++j) {
> struct parsed_event * pev1 = &parsed_events[i];
> struct parsed_event * pev2 = &parsed_events[j];
> @@ -270,7 +273,6 @@ static void resolve_events(void)
>
> static void show_unit_mask(void)
> {
> - struct op_event * event;
> size_t count;
>
> count = parse_events(parsed_events, num_chosen_events, chosen_events);
> @@ -279,20 +281,15 @@ static void show_unit_mask(void)
> exit(EXIT_FAILURE);
> }
>
> - event = find_event_by_name(parsed_events[0].name, 0, 0);
> -
> - if (!event) {
> - fprintf(stderr, "No such event found.\n");
> - exit(EXIT_FAILURE);
> - }
> -
> - printf("%d\n", event->unit->default_mask);
> + op_resolve_unit_mask(parsed_events, NULL);
> + if (parsed_events[0].unit_mask_name)
> + printf("%s\n", parsed_events[0].unit_mask_name);
> + else
> + printf("%d\n", parsed_events[0].unit_mask);
> }
>
> static void show_extra_mask(void)
> {
> - unsigned i;
> - struct op_event * event;
> size_t count;
> unsigned extra;
>
> @@ -302,22 +299,7 @@ static void show_extra_mask(void)
> exit(EXIT_FAILURE);
> }
>
> - event = find_event_by_name(parsed_events[0].name,
> - parsed_events[0].unit_mask,
> - 1);
> - if (!event) {
> - fprintf(stderr, "No such event found.\n");
> - exit(EXIT_FAILURE);
> - }
> -
> - /* Not exact match is nothing */
> - extra = 0;
> - for (i = 0; i < event->unit->num; i++)
> - if (event->unit->um[i].value == (unsigned)parsed_events[0].unit_mask) {
> - extra = event->unit->um[i].extra;
> - break;
> - }
> -
> + op_resolve_unit_mask(parsed_events, &extra);
> printf ("%d\n", extra);
> }
>
> @@ -761,8 +743,10 @@ int main(int argc, char const * argv[])
> sprintf(title, "oprofile: available events for CPU type \"%s\"\n\n", pretty);
> if (want_xml)
> open_xml_events(title, event_doc, cpu_type);
> - else
> + else {
> printf("%s%s", title, event_doc);
> + printf("You can use named events by specifying the first word of the description if unique\n\n");
> + }
>
> list_for_each(pos, events) {
> struct op_event * event = list_entry(pos, struct op_event, event_next);
|