From: Maynard J. <may...@us...> - 2013-01-04 23:06:32
|
*Andi*, could you please review this patch? I finally opened a bug to track this issue: https://sourceforge.net/tracker/?func=detail&aid=3599530&group_id=16191&atid=116191 Thanks! -Maynard ------------------------------------------------------------------ Unit mask bitmasks containing non-unique values should fail When named unit mask support was added by Andi Kleen (Intel) in the 0.9.7 timeframe, it became possible for multiple unit masks to have the same hex value for a given event. The (only) way to disambiguate these non-unique unit mask values is to specify them by name. It was pointed out during patch reviews that OR'ing together such unit masks (for bitmask types of unit masks) was problematic. Andi asserted that bitmasks containing any of the non-unique values simly would not be supported. Unfortunately, we dropped the ball and did not document this restriction, and neither did we put any checks into the code to prevent users from doing this. Example: On i386/sandybridge, the int_misc event has the following possible unit mask values that can be put into a bitmask: Unit masks (default 0x40) ---------- 0x40: rat_stall_cycles Cycles Resource Allocation Table (RAT) external stall is sent to Instruction Decode Queue (IDQ) for this thread. 0x03: recovery_cycles Number of cycles waiting to be recover after Nuke due to all other cases except JEClear. (extra: cmask=1) 0x03: recovery_stalls_count Edge applied to recovery_cycles, thus counts occurrences. (extra: edge cmask=1) The event specification of int_misc:2000000:43 will be incorrectly accepted as valid even though it is clearly ambiguous. With this patch, such unit mask bitmask specifications will be detected, and we will exit gracefully with an informative error message. Signed-off-by: Maynard Johnson <may...@us...> --- doc/opcontrol.1.in | 6 ++-- doc/operf.1.in | 6 ++-- doc/oprofile.xml | 6 ++-- libop/op_events.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++---- 4 files changed, 95 insertions(+), 17 deletions(-) diff --git a/doc/opcontrol.1.in b/doc/opcontrol.1.in index 0b595f3..8bc9907 100644 --- a/doc/opcontrol.1.in +++ b/doc/opcontrol.1.in @@ -106,12 +106,12 @@ 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. The numerical unit mask -can also be a string which matches the first word in the unit mask +them on the same opcontrol invocation. The unit mask value +can also be specified as a string (i.e, symbolic name) which matches the first word in the unit mask description, but only for events with "extra:" parameters shown. Unit masks with "extra:" parameters .I must -be specified by first word. +be specified by symbolic name, thus it is not possible to include them in bitmask values. .br .TP .BI "--separate="[none,lib,kernel,thread,cpu,all] diff --git a/doc/operf.1.in b/doc/operf.1.in index b109324..bb87f6b 100644 --- a/doc/operf.1.in +++ b/doc/operf.1.in @@ -98,7 +98,7 @@ for profiling. Each event spec is of the form: .br .I " name:count[:unitmask[:kernel[:user]]]" .br -When specifying a unit mask value, it may be either a hexadecimal value (which +When specifying a unit mask value, it may be either a numerical value (hex values .I must begin with "0x") or a string (i.e, symbolic name) which matches the first word in the unit mask description. Specifying a symbolic name for the unit mask is valid only @@ -106,8 +106,8 @@ for unit masks having "extra:" parameters, as shown by the output of .B ophelp. Unit masks with "extra:" parameters .I must -be specified using the symbolic name. If no unit mask is specified, 0x0 will be -used as the default. +be specified using the symbolic name, thus it is not possible to include them in bitmask values. +If no unit mask is specified, the default unit mask value for the event is used. .P .RS When no event specification is given, the default event for the running diff --git a/doc/oprofile.xml b/doc/oprofile.xml index 6bbab72..5327186 100644 --- a/doc/oprofile.xml +++ b/doc/oprofile.xml @@ -1098,12 +1098,12 @@ they will be set to the default values (a unit mask of 0, and profiling both ker userspace code). Note that some events require a unit mask. </para> <para> -When specifying a unit mask value, it may be either a hexadecimal value (which +When specifying a unit mask value, it may be either a numerical value (hex values <emphasis>must</emphasis> begin with "0x") or a string (i.e, symbolic name) which matches the first word in the unit mask description. Specifying a symbolic name for the unit mask is valid only for unit masks having "extra:" parameters, as -shown by the output of <command>ophelp</command>. Unit masks with "extra:" parameters must be -specified using the symbolic name. +shown by the output of <command>ophelp</command>. Unit masks with "extra:" parameters <emphasis>must</emphasis> +be specified using the symbolic name, thus it is not possible to include them in bitmask values. </para> <note><para> When using legacy mode <command>opcontrol</command> on PowerPC platforms, all events specified must be in the same group; diff --git a/libop/op_events.c b/libop/op_events.c index 64b1dfc..a2beae7 100644 --- a/libop/op_events.c +++ b/libop/op_events.c @@ -1011,11 +1011,94 @@ struct op_event * op_find_event_any(op_cpu cpu_type, u32 nr) return find_event_any(nr); } +static int _is_um_valid_bitmask(struct op_event * event, u32 passed_um) +{ + int duped_um[MAX_UNIT_MASK]; + int retval = 0; + u32 masked_val = 0; + u32 i, k; + int dup_value_used = 0; + + struct op_event evt; + struct op_unit_mask * tmp_um = xmalloc(sizeof(struct op_unit_mask)); + struct op_unit_mask * tmp_um_no_dups = xmalloc(sizeof(struct op_unit_mask)); + memset(tmp_um, '\0', sizeof(struct op_unit_mask));; + memset(tmp_um_no_dups, '\0', sizeof(struct op_unit_mask)); + memset(duped_um, '\0', sizeof(int) * MAX_UNIT_MASK); + + // First, we make a copy of the event, with just its unit mask values. + evt.unit = tmp_um; + evt.unit->num = event->unit->num; + for (i = 0; i < event->unit->num; i++) + evt.unit->um[i].value = event->unit->um[i].value; + + // Next, we sort the unit mask values in ascending order. + for (i = 1; i < evt.unit->num; i++) { + int j = i - 1; + u32 tmp = evt.unit->um[i].value; + while (tmp < evt.unit->um[j].value && j >= 0) { + evt.unit->um[j + 1].value = evt.unit->um[j].value; + j -= 1; + } + evt.unit->um[j + 1].value = tmp; + } + + /* Now we remove duplicates. Duplicate unit mask values were not + * allowed until the "named unit mask" support was added in + * release 0.9.7. The downside to this is that if the user passed + * a unit mask value that includes one of the duplicated values, + * we have no way of differentiating between the duplicates, so + * the meaning of the bitmask would be ambiguous if we were to + * allow it. Thus, we must prevent the user from specifying such + * bitmasks. + */ + for (i = 0, k = 0; k < evt.unit->num; i++) { + tmp_um_no_dups->um[i].value = evt.unit->um[k].value; + tmp_um_no_dups->num++; + k++; + while ((evt.unit->um[i].value == evt.unit->um[k].value) && i < evt.unit->num) { + k++; + duped_um[i] = 1; + } + } + evt.unit = tmp_um_no_dups; + + /* Finally, we'll see if the passed unit mask value can be matched with a + * mask of available unit mask values. We check for this by determining + * whether the exact bits set in the current um are also set in the + * passed um; if so, we OR those bits into a cumulative masked_val variable. + * Simultaneously, we check if the passed um contains a non-unique unit + * mask value, in which case, it's invalid.. + */ + for (i = 0; i < evt.unit->num; i++) { + if ((evt.unit->um[i].value & passed_um) == evt.unit->um[i].value) { + masked_val |= evt.unit->um[i].value; + if (duped_um[i]) { + dup_value_used = 1; + break; + } + } + } + + free(tmp_um); + free(tmp_um_no_dups); + if (dup_value_used) { + fprintf(stderr, "Ambiguous bitmask: Unit mask values" + " cannot include non-unique numerical values (i.e., 0x%x).\n", + evt.unit->um[i].value); + fprintf(stderr, "Use ophelp to see the unit mask values for event %s.\n", + event->name); + } else if (masked_val == passed_um && passed_um != 0) { + retval = 1; + } + return retval; +} + int op_check_events(int ctr, u32 nr, u32 um, op_cpu cpu_type) { int ret = OP_INVALID_EVENT; size_t i; - u32 masked_val, ctr_mask = 1 << ctr; + u32 ctr_mask = 1 << ctr; struct list_head * pos; load_events(cpu_type); @@ -1031,12 +1114,7 @@ int op_check_events(int ctr, u32 nr, u32 um, op_cpu cpu_type) ret |= OP_INVALID_COUNTER; if (event->unit->unit_type_mask == utm_bitmask) { - masked_val = 0; - for (i = 0; i < event->unit->num; i++) { - if ((event->unit->um[i].value & um) == event->unit->um[i].value) - masked_val |= event->unit->um[i].value; - } - if (!(masked_val == um && um != 0)) + if (!_is_um_valid_bitmask(event, um)) ret |= OP_INVALID_UM; } else { for (i = 0; i < event->unit->num; ++i) { @@ -1281,7 +1359,7 @@ static void do_resolve_unit_mask(struct op_event *e, struct parsed_event *pe, } if (found > 1 && had_unit_mask) { fprintf(stderr, - "Non unique numerical unit mask.\n" + "Non-unique numerical unit mask.\n" "Please specify the unit mask using the first word of the description\n"); exit(EXIT_FAILURE); } -- 1.7.1 |