From: William C. <wc...@re...> - 2013-10-15 05:22:58
|
From: Lars Friend <l.f...@f5...> Older distributions may be running kernels that still use the /dev/opcontrol interface. On an Intel Ivy Bridge machine and similar processors may want to do something like: opcontrol --setup --no-vmlinux \ --event CPU_CLK_UNHALTED:2000000:0:0:1 \ --event uops_executed:2000000:stall_cycles:0:1 For the uops_executed event in the above example need to both set the extra and the unit_mask bits. The current code in opcontrol would never set the unit_mask bits when the extra bits were set. This change allows both to be set when required. Signed-off-by: William Cohen <wc...@re...> --- doc/ophelp.1.in | 4 ++++ utils/opcontrol | 9 +++++++-- utils/ophelp.c | 27 ++++++++++++++++++++++++++- 3 files changed, 37 insertions(+), 3 deletions(-) diff --git a/doc/ophelp.1.in b/doc/ophelp.1.in index 083cc85..97383bf 100644 --- a/doc/ophelp.1.in +++ b/doc/ophelp.1.in @@ -49,6 +49,10 @@ Show the default unit mask for the given event. Show the extra unit mask for given event. .br .TP +.BI "--symbolic-unit-mask / -U [event]" +Show the numerical unit and extra mask for given event. +.br +.TP .BI "--xml / -X" List events in XML format. .br diff --git a/utils/opcontrol b/utils/opcontrol index 38bb1ac..a3a6a3c 100644 --- a/utils/opcontrol +++ b/utils/opcontrol @@ -1522,9 +1522,14 @@ do_param_setup() set_ctr_param $CTR count $COUNT set_ctr_param $CTR kernel $KERNEL set_ctr_param $CTR user $USER - set_ctr_param $CTR unit_mask $UNIT_MASK - EXTRA=`$OPHELP --extra-mask $EVENT:$COUNT:$UNIT_MASK_NAMED` + # Resolve a [potentially] symbolic unit mask to a numeric + # unit mask and extra mask. + TMP_SYMBOLIC="`$OPHELP --symbolic-unit-mask $EVENT:$COUNT:$UNIT_MASK`" + UNIT_MASK_NUM=`echo $TMP_SYMBOLIC | awk '{print $1}'` + EXTRA=`echo $TMP_SYMBOLIC | awk '{print $2}'` + set_ctr_param $CTR unit_mask $UNIT_MASK_NUM + if test "$EXTRA" -ne 0 ; then # A value >= 0x40000 returned by 'ophelp --extra-mask' (EXTRA_MIN_VAL) is interpreted # as a valid extra value; otherwise we interpret as a simple unit mask value diff --git a/utils/ophelp.c b/utils/ophelp.c index 7543c6f..f77a19a 100644 --- a/utils/ophelp.c +++ b/utils/ophelp.c @@ -282,6 +282,21 @@ static void resolve_events(void) free(counter_map); } +static void resolve_symbolic_unit_mask(void) +{ + size_t count; + unsigned extra = 0; + + count = parse_events(parsed_events, num_chosen_events, chosen_events, ignore_count ? 0 : 1); + if (count > 1) { + fprintf(stderr, "More than one event specified.\n"); + exit(EXIT_FAILURE); + } + + op_resolve_unit_mask(parsed_events, &extra); + + printf("%d %d\n", parsed_events[0].unit_mask, extra); +} static void show_unit_mask(void) { @@ -334,6 +349,7 @@ static int check_events; static int unit_mask; static int get_default_event; static int extra_mask; +static int symbolic_unit_mask; static struct poptOption options[] = { { "cpu-type", 'c', POPT_ARG_STRING, &cpu_string, 0, @@ -356,6 +372,9 @@ static struct poptOption options[] = { "list events as XML", NULL, }, { "extra-mask", 'E', POPT_ARG_NONE, &extra_mask, 0, "print extra mask for event", NULL, }, + { "symbolic-unit-mask", 'U', POPT_ARG_NONE, &symbolic_unit_mask, 0, + "resolve an event with symbolic unit mask into numeric unit " + "and extra masks", NULL, }, POPT_AUTOHELP { NULL, 0, 0, NULL, 0, NULL, NULL, }, }; @@ -457,11 +476,17 @@ int main(int argc, char const * argv[]) events = op_events(cpu_type); - if (!chosen_events && (unit_mask || check_events || extra_mask)) { + if (!chosen_events && (unit_mask || check_events || extra_mask || + symbolic_unit_mask)) { fprintf(stderr, "No events given.\n"); exit(EXIT_FAILURE); } + if (symbolic_unit_mask) { + resolve_symbolic_unit_mask(); + exit(EXIT_SUCCESS); + } + if (unit_mask) { show_unit_mask(); exit(EXIT_SUCCESS); -- 1.8.3.1 |
From: Maynard J. <may...@us...> - 2013-10-16 16:27:26
|
Suravee, can you review this, please? Thanks. -Maynard On 10/15/2013 12:22 AM, William Cohen wrote: > From: Lars Friend <l.f...@f5...> > > Older distributions may be running kernels that still use the > /dev/opcontrol interface. On an Intel Ivy Bridge machine and similar > processors may want to do something like: > > opcontrol --setup --no-vmlinux \ > --event CPU_CLK_UNHALTED:2000000:0:0:1 \ > --event uops_executed:2000000:stall_cycles:0:1 > > For the uops_executed event in the above example need to both set the > extra and the unit_mask bits. The current code in opcontrol would > never set the unit_mask bits when the extra bits were set. This > change allows both to be set when required. > > Signed-off-by: William Cohen <wc...@re...> > --- > doc/ophelp.1.in | 4 ++++ > utils/opcontrol | 9 +++++++-- > utils/ophelp.c | 27 ++++++++++++++++++++++++++- > 3 files changed, 37 insertions(+), 3 deletions(-) > > diff --git a/doc/ophelp.1.in b/doc/ophelp.1.in > index 083cc85..97383bf 100644 > --- a/doc/ophelp.1.in > +++ b/doc/ophelp.1.in > @@ -49,6 +49,10 @@ Show the default unit mask for the given event. > Show the extra unit mask for given event. > .br > .TP > +.BI "--symbolic-unit-mask / -U [event]" > +Show the numerical unit and extra mask for given event. > +.br > +.TP > .BI "--xml / -X" > List events in XML format. > .br > diff --git a/utils/opcontrol b/utils/opcontrol > index 38bb1ac..a3a6a3c 100644 > --- a/utils/opcontrol > +++ b/utils/opcontrol > @@ -1522,9 +1522,14 @@ do_param_setup() > set_ctr_param $CTR count $COUNT > set_ctr_param $CTR kernel $KERNEL > set_ctr_param $CTR user $USER > - set_ctr_param $CTR unit_mask $UNIT_MASK > > - EXTRA=`$OPHELP --extra-mask $EVENT:$COUNT:$UNIT_MASK_NAMED` > + # Resolve a [potentially] symbolic unit mask to a numeric > + # unit mask and extra mask. > + TMP_SYMBOLIC="`$OPHELP --symbolic-unit-mask $EVENT:$COUNT:$UNIT_MASK`" > + UNIT_MASK_NUM=`echo $TMP_SYMBOLIC | awk '{print $1}'` > + EXTRA=`echo $TMP_SYMBOLIC | awk '{print $2}'` > + set_ctr_param $CTR unit_mask $UNIT_MASK_NUM > + > if test "$EXTRA" -ne 0 ; then > # A value >= 0x40000 returned by 'ophelp --extra-mask' (EXTRA_MIN_VAL) is interpreted > # as a valid extra value; otherwise we interpret as a simple unit mask value > diff --git a/utils/ophelp.c b/utils/ophelp.c > index 7543c6f..f77a19a 100644 > --- a/utils/ophelp.c > +++ b/utils/ophelp.c > @@ -282,6 +282,21 @@ static void resolve_events(void) > free(counter_map); > } > > +static void resolve_symbolic_unit_mask(void) > +{ > + size_t count; > + unsigned extra = 0; > + > + count = parse_events(parsed_events, num_chosen_events, chosen_events, ignore_count ? 0 : 1); > + if (count > 1) { > + fprintf(stderr, "More than one event specified.\n"); > + exit(EXIT_FAILURE); > + } > + > + op_resolve_unit_mask(parsed_events, &extra); > + > + printf("%d %d\n", parsed_events[0].unit_mask, extra); > +} > > static void show_unit_mask(void) > { > @@ -334,6 +349,7 @@ static int check_events; > static int unit_mask; > static int get_default_event; > static int extra_mask; > +static int symbolic_unit_mask; > > static struct poptOption options[] = { > { "cpu-type", 'c', POPT_ARG_STRING, &cpu_string, 0, > @@ -356,6 +372,9 @@ static struct poptOption options[] = { > "list events as XML", NULL, }, > { "extra-mask", 'E', POPT_ARG_NONE, &extra_mask, 0, > "print extra mask for event", NULL, }, > + { "symbolic-unit-mask", 'U', POPT_ARG_NONE, &symbolic_unit_mask, 0, > + "resolve an event with symbolic unit mask into numeric unit " > + "and extra masks", NULL, }, > POPT_AUTOHELP > { NULL, 0, 0, NULL, 0, NULL, NULL, }, > }; > @@ -457,11 +476,17 @@ int main(int argc, char const * argv[]) > > events = op_events(cpu_type); > > - if (!chosen_events && (unit_mask || check_events || extra_mask)) { > + if (!chosen_events && (unit_mask || check_events || extra_mask || > + symbolic_unit_mask)) { > fprintf(stderr, "No events given.\n"); > exit(EXIT_FAILURE); > } > > + if (symbolic_unit_mask) { > + resolve_symbolic_unit_mask(); > + exit(EXIT_SUCCESS); > + } > + > if (unit_mask) { > show_unit_mask(); > exit(EXIT_SUCCESS); > |
From: Suravee S. <sur...@am...> - 2013-11-01 14:12:09
|
The patches overall look and test out fine. However, I have a minor suggestion in the utils/ophelp.c. With the new "resolve_symbolic_unit_mask" function added, there are 3 versions of the "show_unit_mask" function. This should be consolidated. I am sending out a follow up patch to consolidate these functions. Suravee On 10/16/2013 11:27 AM, Maynard Johnson wrote: > Suravee, can you review this, please? Thanks. > > -Maynard > > On 10/15/2013 12:22 AM, William Cohen wrote: >> From: Lars Friend <l.f...@f5...> >> >> Older distributions may be running kernels that still use the >> /dev/opcontrol interface. On an Intel Ivy Bridge machine and similar >> processors may want to do something like: >> >> opcontrol --setup --no-vmlinux \ >> --event CPU_CLK_UNHALTED:2000000:0:0:1 \ >> --event uops_executed:2000000:stall_cycles:0:1 >> >> For the uops_executed event in the above example need to both set the >> extra and the unit_mask bits. The current code in opcontrol would >> never set the unit_mask bits when the extra bits were set. This >> change allows both to be set when required. >> >> Signed-off-by: William Cohen <wc...@re...> >> --- >> doc/ophelp.1.in | 4 ++++ >> utils/opcontrol | 9 +++++++-- >> utils/ophelp.c | 27 ++++++++++++++++++++++++++- >> 3 files changed, 37 insertions(+), 3 deletions(-) >> >> diff --git a/doc/ophelp.1.in b/doc/ophelp.1.in >> index 083cc85..97383bf 100644 >> --- a/doc/ophelp.1.in >> +++ b/doc/ophelp.1.in >> @@ -49,6 +49,10 @@ Show the default unit mask for the given event. >> Show the extra unit mask for given event. >> .br >> .TP >> +.BI "--symbolic-unit-mask / -U [event]" >> +Show the numerical unit and extra mask for given event. >> +.br >> +.TP >> .BI "--xml / -X" >> List events in XML format. >> .br >> diff --git a/utils/opcontrol b/utils/opcontrol >> index 38bb1ac..a3a6a3c 100644 >> --- a/utils/opcontrol >> +++ b/utils/opcontrol >> @@ -1522,9 +1522,14 @@ do_param_setup() >> set_ctr_param $CTR count $COUNT >> set_ctr_param $CTR kernel $KERNEL >> set_ctr_param $CTR user $USER >> - set_ctr_param $CTR unit_mask $UNIT_MASK >> >> - EXTRA=`$OPHELP --extra-mask $EVENT:$COUNT:$UNIT_MASK_NAMED` >> + # Resolve a [potentially] symbolic unit mask to a numeric >> + # unit mask and extra mask. >> + TMP_SYMBOLIC="`$OPHELP --symbolic-unit-mask $EVENT:$COUNT:$UNIT_MASK`" >> + UNIT_MASK_NUM=`echo $TMP_SYMBOLIC | awk '{print $1}'` >> + EXTRA=`echo $TMP_SYMBOLIC | awk '{print $2}'` >> + set_ctr_param $CTR unit_mask $UNIT_MASK_NUM >> + >> if test "$EXTRA" -ne 0 ; then >> # A value >= 0x40000 returned by 'ophelp --extra-mask' (EXTRA_MIN_VAL) is interpreted >> # as a valid extra value; otherwise we interpret as a simple unit mask value >> diff --git a/utils/ophelp.c b/utils/ophelp.c >> index 7543c6f..f77a19a 100644 >> --- a/utils/ophelp.c >> +++ b/utils/ophelp.c >> @@ -282,6 +282,21 @@ static void resolve_events(void) >> free(counter_map); >> } >> >> +static void resolve_symbolic_unit_mask(void) >> +{ >> + size_t count; >> + unsigned extra = 0; >> + >> + count = parse_events(parsed_events, num_chosen_events, chosen_events, ignore_count ? 0 : 1); >> + if (count > 1) { >> + fprintf(stderr, "More than one event specified.\n"); >> + exit(EXIT_FAILURE); >> + } >> + >> + op_resolve_unit_mask(parsed_events, &extra); >> + >> + printf("%d %d\n", parsed_events[0].unit_mask, extra); >> +} >> >> static void show_unit_mask(void) >> { >> @@ -334,6 +349,7 @@ static int check_events; >> static int unit_mask; >> static int get_default_event; >> static int extra_mask; >> +static int symbolic_unit_mask; >> >> static struct poptOption options[] = { >> { "cpu-type", 'c', POPT_ARG_STRING, &cpu_string, 0, >> @@ -356,6 +372,9 @@ static struct poptOption options[] = { >> "list events as XML", NULL, }, >> { "extra-mask", 'E', POPT_ARG_NONE, &extra_mask, 0, >> "print extra mask for event", NULL, }, >> + { "symbolic-unit-mask", 'U', POPT_ARG_NONE, &symbolic_unit_mask, 0, >> + "resolve an event with symbolic unit mask into numeric unit " >> + "and extra masks", NULL, }, >> POPT_AUTOHELP >> { NULL, 0, 0, NULL, 0, NULL, NULL, }, >> }; >> @@ -457,11 +476,17 @@ int main(int argc, char const * argv[]) >> >> events = op_events(cpu_type); >> >> - if (!chosen_events && (unit_mask || check_events || extra_mask)) { >> + if (!chosen_events && (unit_mask || check_events || extra_mask || >> + symbolic_unit_mask)) { >> fprintf(stderr, "No events given.\n"); >> exit(EXIT_FAILURE); >> } >> >> + if (symbolic_unit_mask) { >> + resolve_symbolic_unit_mask(); >> + exit(EXIT_SUCCESS); >> + } >> + >> if (unit_mask) { >> show_unit_mask(); >> exit(EXIT_SUCCESS); >> > |
From: Lars F. <L.Friend@F5.com> - 2013-11-01 15:37:11
|
Sounds like an excellent idea. I thought of trying to merge those but I decided that in my particular case it made more sense to pick a solution that kept the change to the opcontrol script minimal because it was complex and I didn't want to break anything by accident. I mainly threw this together as a quick hack to scratch the itch so I could keep tracking down a performance bug so once I got it to the point of being functional I sort of left it be to go back to chasing the performance bugs since I was under the gun to win back some performance before a release deadline. There may be a couple places that lack a bit of polish (like I'm not sure of opreport correctly displays the counter desciptions for all of the named unit masks). In any case, thanks for taking the time to look at this, and I hope somebody other than myself will find having these extra counters to be useful. Cheers, -lars -----Original Message----- From: Suravee Suthikulpanit [mailto:sur...@am...] Sent: Friday, November 01, 2013 1:57 AM To: Maynard Johnson; William Cohen Cc: oprofile-list; Lars Friend Subject: Re: [PATCH] Allow events with extra flags to also set unit_mask The patches overall look and test out fine. However, I have a minor suggestion in the utils/ophelp.c. With the new "resolve_symbolic_unit_mask" function added, there are 3 versions of the "show_unit_mask" function. This should be consolidated. I am sending out a follow up patch to consolidate these functions. Suravee On 10/16/2013 11:27 AM, Maynard Johnson wrote: > Suravee, can you review this, please? Thanks. > > -Maynard > > On 10/15/2013 12:22 AM, William Cohen wrote: >> From: Lars Friend <l.f...@f5...> >> >> Older distributions may be running kernels that still use the >> /dev/opcontrol interface. On an Intel Ivy Bridge machine and similar >> processors may want to do something like: >> >> opcontrol --setup --no-vmlinux \ >> --event CPU_CLK_UNHALTED:2000000:0:0:1 \ >> --event uops_executed:2000000:stall_cycles:0:1 >> >> For the uops_executed event in the above example need to both set the >> extra and the unit_mask bits. The current code in opcontrol would >> never set the unit_mask bits when the extra bits were set. This >> change allows both to be set when required. >> >> Signed-off-by: William Cohen <wc...@re...> >> --- >> doc/ophelp.1.in | 4 ++++ >> utils/opcontrol | 9 +++++++-- >> utils/ophelp.c | 27 ++++++++++++++++++++++++++- >> 3 files changed, 37 insertions(+), 3 deletions(-) >> >> diff --git a/doc/ophelp.1.in b/doc/ophelp.1.in index 083cc85..97383bf >> 100644 >> --- a/doc/ophelp.1.in >> +++ b/doc/ophelp.1.in >> @@ -49,6 +49,10 @@ Show the default unit mask for the given event. >> Show the extra unit mask for given event. >> .br >> .TP >> +.BI "--symbolic-unit-mask / -U [event]" >> +Show the numerical unit and extra mask for given event. >> +.br >> +.TP >> .BI "--xml / -X" >> List events in XML format. >> .br >> diff --git a/utils/opcontrol b/utils/opcontrol index 38bb1ac..a3a6a3c >> 100644 >> --- a/utils/opcontrol >> +++ b/utils/opcontrol >> @@ -1522,9 +1522,14 @@ do_param_setup() >> set_ctr_param $CTR count $COUNT >> set_ctr_param $CTR kernel $KERNEL >> set_ctr_param $CTR user $USER >> - set_ctr_param $CTR unit_mask $UNIT_MASK >> >> - EXTRA=`$OPHELP --extra-mask $EVENT:$COUNT:$UNIT_MASK_NAMED` >> + # Resolve a [potentially] symbolic unit mask to a numeric >> + # unit mask and extra mask. >> + TMP_SYMBOLIC="`$OPHELP --symbolic-unit-mask $EVENT:$COUNT:$UNIT_MASK`" >> + UNIT_MASK_NUM=`echo $TMP_SYMBOLIC | awk '{print $1}'` >> + EXTRA=`echo $TMP_SYMBOLIC | awk '{print $2}'` >> + set_ctr_param $CTR unit_mask $UNIT_MASK_NUM >> + >> if test "$EXTRA" -ne 0 ; then >> # A value >= 0x40000 returned by 'ophelp --extra-mask' (EXTRA_MIN_VAL) is interpreted >> # as a valid extra value; otherwise we interpret as a simple >> unit mask value diff --git a/utils/ophelp.c b/utils/ophelp.c index >> 7543c6f..f77a19a 100644 >> --- a/utils/ophelp.c >> +++ b/utils/ophelp.c >> @@ -282,6 +282,21 @@ static void resolve_events(void) >> free(counter_map); >> } >> >> +static void resolve_symbolic_unit_mask(void) { >> + size_t count; >> + unsigned extra = 0; >> + >> + count = parse_events(parsed_events, num_chosen_events, chosen_events, ignore_count ? 0 : 1); >> + if (count > 1) { >> + fprintf(stderr, "More than one event specified.\n"); >> + exit(EXIT_FAILURE); >> + } >> + >> + op_resolve_unit_mask(parsed_events, &extra); >> + >> + printf("%d %d\n", parsed_events[0].unit_mask, extra); } >> >> static void show_unit_mask(void) >> { >> @@ -334,6 +349,7 @@ static int check_events; >> static int unit_mask; >> static int get_default_event; >> static int extra_mask; >> +static int symbolic_unit_mask; >> >> static struct poptOption options[] = { >> { "cpu-type", 'c', POPT_ARG_STRING, &cpu_string, 0, @@ -356,6 >> +372,9 @@ static struct poptOption options[] = { >> "list events as XML", NULL, }, >> { "extra-mask", 'E', POPT_ARG_NONE, &extra_mask, 0, >> "print extra mask for event", NULL, }, >> + { "symbolic-unit-mask", 'U', POPT_ARG_NONE, &symbolic_unit_mask, 0, >> + "resolve an event with symbolic unit mask into numeric unit " >> + "and extra masks", NULL, }, >> POPT_AUTOHELP >> { NULL, 0, 0, NULL, 0, NULL, NULL, }, >> }; >> @@ -457,11 +476,17 @@ int main(int argc, char const * argv[]) >> >> events = op_events(cpu_type); >> >> - if (!chosen_events && (unit_mask || check_events || extra_mask)) { >> + if (!chosen_events && (unit_mask || check_events || extra_mask || >> + symbolic_unit_mask)) { >> fprintf(stderr, "No events given.\n"); >> exit(EXIT_FAILURE); >> } >> >> + if (symbolic_unit_mask) { >> + resolve_symbolic_unit_mask(); >> + exit(EXIT_SUCCESS); >> + } >> + >> if (unit_mask) { >> show_unit_mask(); >> exit(EXIT_SUCCESS); >> > |
From: Maynard J. <may...@us...> - 2013-11-01 15:20:47
|
On 10/15/2013 12:22 AM, William Cohen wrote: > From: Lars Friend <l.f...@f5...> > > Older distributions may be running kernels that still use the > /dev/opcontrol interface. On an Intel Ivy Bridge machine and similar > processors may want to do something like: > > opcontrol --setup --no-vmlinux \ > --event CPU_CLK_UNHALTED:2000000:0:0:1 \ > --event uops_executed:2000000:stall_cycles:0:1 > > For the uops_executed event in the above example need to both set the > extra and the unit_mask bits. The current code in opcontrol would > never set the unit_mask bits when the extra bits were set. This > change allows both to be set when required. Andi, The current opcontrol behavior of only setting the "extra" bits is the analogous to the operf behavior, so on the face of it, I don't see why this patch is needed. In my testing on Sandybridge of the uops_issued event with the stall_cycles unit mask, I get roughly the same number of samples for a given app whether I'm using opcontrol or operf. So it seems to be they are either both wrong or both right. If the latter, then this patch is not needed. Please help us clear this up. Thanks! -Maynard > > Signed-off-by: William Cohen <wc...@re...> > --- > doc/ophelp.1.in | 4 ++++ > utils/opcontrol | 9 +++++++-- > utils/ophelp.c | 27 ++++++++++++++++++++++++++- > 3 files changed, 37 insertions(+), 3 deletions(-) > > diff --git a/doc/ophelp.1.in b/doc/ophelp.1.in > index 083cc85..97383bf 100644 > --- a/doc/ophelp.1.in > +++ b/doc/ophelp.1.in > @@ -49,6 +49,10 @@ Show the default unit mask for the given event. > Show the extra unit mask for given event. > .br > .TP > +.BI "--symbolic-unit-mask / -U [event]" > +Show the numerical unit and extra mask for given event. > +.br > +.TP > .BI "--xml / -X" > List events in XML format. > .br > diff --git a/utils/opcontrol b/utils/opcontrol > index 38bb1ac..a3a6a3c 100644 > --- a/utils/opcontrol > +++ b/utils/opcontrol > @@ -1522,9 +1522,14 @@ do_param_setup() > set_ctr_param $CTR count $COUNT > set_ctr_param $CTR kernel $KERNEL > set_ctr_param $CTR user $USER > - set_ctr_param $CTR unit_mask $UNIT_MASK > > - EXTRA=`$OPHELP --extra-mask $EVENT:$COUNT:$UNIT_MASK_NAMED` > + # Resolve a [potentially] symbolic unit mask to a numeric > + # unit mask and extra mask. > + TMP_SYMBOLIC="`$OPHELP --symbolic-unit-mask $EVENT:$COUNT:$UNIT_MASK`" > + UNIT_MASK_NUM=`echo $TMP_SYMBOLIC | awk '{print $1}'` > + EXTRA=`echo $TMP_SYMBOLIC | awk '{print $2}'` > + set_ctr_param $CTR unit_mask $UNIT_MASK_NUM > + > if test "$EXTRA" -ne 0 ; then > # A value >= 0x40000 returned by 'ophelp --extra-mask' (EXTRA_MIN_VAL) is interpreted > # as a valid extra value; otherwise we interpret as a simple unit mask value > diff --git a/utils/ophelp.c b/utils/ophelp.c > index 7543c6f..f77a19a 100644 > --- a/utils/ophelp.c > +++ b/utils/ophelp.c > @@ -282,6 +282,21 @@ static void resolve_events(void) > free(counter_map); > } > > +static void resolve_symbolic_unit_mask(void) > +{ > + size_t count; > + unsigned extra = 0; > + > + count = parse_events(parsed_events, num_chosen_events, chosen_events, ignore_count ? 0 : 1); > + if (count > 1) { > + fprintf(stderr, "More than one event specified.\n"); > + exit(EXIT_FAILURE); > + } > + > + op_resolve_unit_mask(parsed_events, &extra); > + > + printf("%d %d\n", parsed_events[0].unit_mask, extra); > +} > > static void show_unit_mask(void) > { > @@ -334,6 +349,7 @@ static int check_events; > static int unit_mask; > static int get_default_event; > static int extra_mask; > +static int symbolic_unit_mask; > > static struct poptOption options[] = { > { "cpu-type", 'c', POPT_ARG_STRING, &cpu_string, 0, > @@ -356,6 +372,9 @@ static struct poptOption options[] = { > "list events as XML", NULL, }, > { "extra-mask", 'E', POPT_ARG_NONE, &extra_mask, 0, > "print extra mask for event", NULL, }, > + { "symbolic-unit-mask", 'U', POPT_ARG_NONE, &symbolic_unit_mask, 0, > + "resolve an event with symbolic unit mask into numeric unit " > + "and extra masks", NULL, }, > POPT_AUTOHELP > { NULL, 0, 0, NULL, 0, NULL, NULL, }, > }; > @@ -457,11 +476,17 @@ int main(int argc, char const * argv[]) > > events = op_events(cpu_type); > > - if (!chosen_events && (unit_mask || check_events || extra_mask)) { > + if (!chosen_events && (unit_mask || check_events || extra_mask || > + symbolic_unit_mask)) { > fprintf(stderr, "No events given.\n"); > exit(EXIT_FAILURE); > } > > + if (symbolic_unit_mask) { > + resolve_symbolic_unit_mask(); > + exit(EXIT_SUCCESS); > + } > + > if (unit_mask) { > show_unit_mask(); > exit(EXIT_SUCCESS); > |
From: Andi K. <an...@fi...> - 2013-11-02 00:22:01
|
On Fri, Nov 01, 2013 at 10:20:35AM -0500, Maynard Johnson wrote: > On 10/15/2013 12:22 AM, William Cohen wrote: > > From: Lars Friend <l.f...@f5...> > > > > Older distributions may be running kernels that still use the > > /dev/opcontrol interface. On an Intel Ivy Bridge machine and similar > > processors may want to do something like: > > > > opcontrol --setup --no-vmlinux \ > > --event CPU_CLK_UNHALTED:2000000:0:0:1 \ > > --event uops_executed:2000000:stall_cycles:0:1 > > > > For the uops_executed event in the above example need to both set the > > extra and the unit_mask bits. The current code in opcontrol would > > never set the unit_mask bits when the extra bits were set. This > > change allows both to be set when required. > > Andi, > The current opcontrol behavior of only setting the "extra" bits is the analogous to the operf behavior, so on the face of it, I don't see why this patch is needed. In my testing on Sandybridge of the uops_issued event with the stall_cycles unit mask, I get roughly the same number of samples for a given app whether I'm using opcontrol or operf. So it seems to be they are either both wrong or both right. If the latter, then this patch is not needed. Please help us clear this up. Lars, how did you determine the bits were not set? -Andi |
From: Andi K. <an...@fi...> - 2013-11-02 00:33:51
|
> > The current opcontrol behavior of only setting the "extra" bits is the analogous to the operf behavior, so on the face of it, I don't see why this patch is needed. In my testing on Sandybridge of the uops_issued event with the stall_cycles unit mask, I get roughly the same number of samples for a given app whether I'm using opcontrol or operf. So it seems to be they are either both wrong or both right. If the latter, then this patch is not needed. Please help us clear this up. > > Lars, how did you determine the bits were not set? Ok I checked and it seems they are both wrong. So Lars patch should be applied to both. -Andi |
From: Lars F. <L.Friend@F5.com> - 2013-11-04 18:59:18
|
I examined the value returned by cat /dev/oprofile/0/extra -lars -----Original Message----- From: Andi Kleen [mailto:an...@fi...] Sent: Friday, November 01, 2013 5:22 PM To: Maynard Johnson Cc: William Cohen; opr...@li...; Lars Friend; Suravee Suthikulanit; Andi Kleen Subject: Re: [PATCH] Allow events with extra flags to also set unit_mask On Fri, Nov 01, 2013 at 10:20:35AM -0500, Maynard Johnson wrote: > On 10/15/2013 12:22 AM, William Cohen wrote: > > From: Lars Friend <l.f...@f5...> > > > > Older distributions may be running kernels that still use the > > /dev/opcontrol interface. On an Intel Ivy Bridge machine and > > similar processors may want to do something like: > > > > opcontrol --setup --no-vmlinux \ > > --event CPU_CLK_UNHALTED:2000000:0:0:1 \ --event > > uops_executed:2000000:stall_cycles:0:1 > > > > For the uops_executed event in the above example need to both set > > the extra and the unit_mask bits. The current code in opcontrol > > would never set the unit_mask bits when the extra bits were set. > > This change allows both to be set when required. > > Andi, > The current opcontrol behavior of only setting the "extra" bits is the analogous to the operf behavior, so on the face of it, I don't see why this patch is needed. In my testing on Sandybridge of the uops_issued event with the stall_cycles unit mask, I get roughly the same number of samples for a given app whether I'm using opcontrol or operf. So it seems to be they are either both wrong or both right. If the latter, then this patch is not needed. Please help us clear this up. Lars, how did you determine the bits were not set? -Andi |
From: Andi K. <an...@fi...> - 2013-11-04 19:18:11
|
On Mon, Nov 04, 2013 at 06:59:07PM +0000, Lars Friend wrote: > I examined the value returned by cat /dev/oprofile/0/extra I think your patch is correct (and I verified it), but as Maynard pointed out it also needs to be applied to the operf path. Could you update your patch to do that please? -Andi |
From: Andi K. <an...@fi...> - 2013-12-05 00:55:17
|
Andi Kleen <an...@fi...> writes: > On Mon, Nov 04, 2013 at 06:59:07PM +0000, Lars Friend wrote: >> I examined the value returned by cat /dev/oprofile/0/extra > > I think your patch is correct (and I verified it), > but as Maynard pointed out it also needs to be applied to the operf path. > > Could you update your patch to do that please? Hi, Where are we with this patch. I think it's correct and needed (plus needs the additional operf bits). Maynard, can you please apply it? -Andi -- ak...@li... -- Speaking for myself only |
From: Maynard J. <may...@us...> - 2013-12-20 18:14:44
|
On 12/04/2013 06:42 PM, Andi Kleen wrote: > Andi Kleen <an...@fi...> writes: > >> On Mon, Nov 04, 2013 at 06:59:07PM +0000, Lars Friend wrote: >>> I examined the value returned by cat /dev/oprofile/0/extra >> >> I think your patch is correct (and I verified it), >> but as Maynard pointed out it also needs to be applied to the operf path. >> >> Could you update your patch to do that please? > > Hi, > > Where are we with this patch. I think it's correct and needed (plus > needs the additional operf bits). Maynard, can you please apply it? Andi, Finally had a bit of downtime today (last day before Christmas break), and was checking the backlog and noticed this posting. Sorry I missed your question to me. As for applying it . . . Fixing it for opcontrol is fine, but since it's deprecated, I feel the issue should definitely be fixed on the operf side. So we need an update to the patch (or a second one) to fix operf. Lars, can you provide that? If not, then I would ask that either Suravee or Andi provide it. I'll be happy to review it. The location where the update is needed is libpe_utils/op_pe_utils.cpp:_get_event_code. Thanks. -Maynard > > -Andi > |
From: Andi K. <an...@fi...> - 2013-12-20 19:46:31
|
On Fri, Dec 20, 2013 at 12:14:33PM -0600, Maynard Johnson wrote: > On 12/04/2013 06:42 PM, Andi Kleen wrote: > > Andi Kleen <an...@fi...> writes: > > > >> On Mon, Nov 04, 2013 at 06:59:07PM +0000, Lars Friend wrote: > >>> I examined the value returned by cat /dev/oprofile/0/extra > >> > >> I think your patch is correct (and I verified it), > >> but as Maynard pointed out it also needs to be applied to the operf path. > >> > >> Could you update your patch to do that please? > > > > Hi, > > > > Where are we with this patch. I think it's correct and needed (plus > > needs the additional operf bits). Maynard, can you please apply it? > Andi, > Finally had a bit of downtime today (last day before Christmas break), and was checking the backlog and noticed this posting. Sorry I missed your question to me. As for applying it . . . Fixing it for opcontrol is fine, but since it's deprecated, I feel the issue should definitely be fixed on the operf side. So we need an update to the patch (or a second one) to fix operf. Lars, can you provide that? If not, then I would ask that either Suravee or Andi provide it. I'll be happy to review it. The location where the update is needed is libpe_utils/op_pe_utils.cpp:_get_event_code. This patch seems to work from a very quick test SUpport both extra mask and unit mask in operf Signed-off-by: Andi Kleen <ak...@li...> diff --git a/libpe_utils/op_pe_utils.cpp b/libpe_utils/op_pe_utils.cpp index 273fc61..f7a27ac 100644 --- a/libpe_utils/op_pe_utils.cpp +++ b/libpe_utils/op_pe_utils.cpp @@ -461,7 +461,8 @@ handle_named_um: config |= event->evt_um; else config |= ((event->evt_um & 0xFFULL) << 8); - } else if (!event->evt_um) { + } + if (!event->evt_um) { char * endptr; command.clear(); command = OP_BINDIR; |
From: Lars F. <L.Friend@F5.com> - 2013-12-20 19:48:34
|
Hi All, I'm afraid I don't have an execution environment that supports running operf (stuck firmly on 2.6.32 kernel for the foreseeable future) so I haven't investigated fixing it there, but looking ahead in my email it sounds like Andi has it under control. -lars -----Original Message----- From: Maynard Johnson [mailto:may...@us...] Sent: Friday, December 20, 2013 10:15 AM To: Andi Kleen; Lars Friend; Suravee Suthikulanit Cc: William Cohen; opr...@li... Subject: Re: [PATCH] Allow events with extra flags to also set unit_mask On 12/04/2013 06:42 PM, Andi Kleen wrote: > Andi Kleen <an...@fi...> writes: > >> On Mon, Nov 04, 2013 at 06:59:07PM +0000, Lars Friend wrote: >>> I examined the value returned by cat /dev/oprofile/0/extra >> >> I think your patch is correct (and I verified it), but as Maynard >> pointed out it also needs to be applied to the operf path. >> >> Could you update your patch to do that please? > > Hi, > > Where are we with this patch. I think it's correct and needed (plus > needs the additional operf bits). Maynard, can you please apply it? Andi, Finally had a bit of downtime today (last day before Christmas break), and was checking the backlog and noticed this posting. Sorry I missed your question to me. As for applying it . . . Fixing it for opcontrol is fine, but since it's deprecated, I feel the issue should definitely be fixed on the operf side. So we need an update to the patch (or a second one) to fix operf. Lars, can you provide that? If not, then I would ask that either Suravee or Andi provide it. I'll be happy to review it. The location where the update is needed is libpe_utils/op_pe_utils.cpp:_get_event_code. Thanks. -Maynard > > -Andi > |