Re: [PATCH 1/1] Oprofile Extented Feature Interface (Revision1)

[Maynard J. <may...@us...>]

Suravee Suthikulpanit wrote:
> Please see my comments below.
> 
> Suravee
> 
>  > Maynard Johnson wrote:
>>  >
>>  > +static void resolve_ext(void)
>>  > +{
>>  > +     size_t count;
>>  > +     size_t i, j;
>>  > +     size_t nr_counters = op_get_nr_counters(cpu_type);
>>  > +
>>  > +     count = parse_events(parsed_events, nr_counters, chosen_events);
>> [Maynard] I assume that chosen events can include both normal events and extended 
>> events,
>> right?  If so, then nr_counters may be less than the number of chosen 
>> events,
>> and parse_events will fail.  It seems to me that you've copied over too 
>> much of
>> resolve_events into resolve_ext.  As I've mentioned before, I'm just not
>> convinced we need a separate --check-ext option (which is what drives the
>> calling of resolve_ext). I think what you need to do is modify 
>> resolve_events()
>> to handle the possibility that some of the chosen events may be extended 
>> events.
>>   And unless you strip out the ext events from chosen events first, 
>> you'll also
>> have to modify parse_events().  Also note that the parsed_events array is of
>> size nr_counter ... oops!
> 
> [Suravee] I will be making the suggested changes as following:
> 
> 1. Extend "ophelp --check-events" to support extended events. The output 
>   will be the appropriate counters assignment based on the provided 
> events.  For instance,
> 
> ophelp --check-events CPU_CLK_UNHALTED:100000 IBS_FETCH_ALL:100000 
> IBS_OP_ALL:100000 RETIRED_INSTRUCTIONS:100000 DATA_CACHE_MISSES:100000 
> DATA_CACHE_ACCESSES:100000
> 
> will generate output "0 ibs_fetch ibs_op 1 2 3".
> 
> 2. I will need to modify the function "map_event_to_counter" so that the 
>   entries of "counter_map" will contain "-1" for the extended events. 
> For instance, the above input will generate counter_map with contents "0 
> -1 -1 1 2 3". "ophelp" will then map the entry with counter_map value 
> "-1" to the appropriate value, which is specified with tag "ext" in the 
> events file.
> 
> How does this sound?
Yes, I think that sounds right to me.  Just remember, as I said above, the size of the parsed_events array must include the extended events.  Currently, it's size is based on nr_counter.

-Maynard 
> 
> Suravee
> 
>