Re: [PATCH] oprofile intel arch perfmon userland v2

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

Andi Kleen wrote:
> On Tue, Oct 14, 2008 at 03:43:14PM -0500, Maynard Johnson wrote:
>> Andi Kleen wrote:
>>> The kernel patches I posted recently to support Intel architectural perfmon
>>> also require some userland changes.
>>>
>> Your new find_event_um function does not handle events where the unit mask 
>> the user is specifying is a combintation of multiple unit mask values.  See 
>> oprofile bug 2161762:
>> https://sourceforge.net/tracker/index.php?func=detail&aid=2161762&group_id=16191&atid=116191
>>
>> Can you take a look?
> 
> The old code didn't handle this correctly either (it would just return
> a random event). But it didn't error out. It's also hard to handle
Where the old find_event() was used, the unit mask value was processed afterwards, so it did what it needed to do.  Your patch below looks fine and tested good.  Applied.  Thank you!

-Maynard

> fully because oprofile doesn't know if the unit mask is a mask or a number.
> 
> I guess I can just return the first event again like the old code.
> It's not correct, but also not worse than before.
> 
> Here's the patch.
> 
> 
> Index: libop/op_events.c
> ===================================================================
> RCS file: /cvsroot/oprofile/oprofile/libop/op_events.c,v
> retrieving revision 1.88
> diff -u -r1.88 op_events.c
> --- libop/op_events.c	28 Aug 2008 21:57:46 -0000	1.88
> +++ libop/op_events.c	7 Nov 2008 19:33:21 -0000
> @@ -545,6 +545,19 @@
>  	}
>  }
> 
> +/* There can be actually multiple events here, so this is not quite correct */
> +static struct op_event * find_event_any(u32 nr)
> +{
> +	struct list_head * pos;
> +
> +	list_for_each(pos, &events_list) {
> +		struct op_event * event = list_entry(pos, struct op_event, event_next);
> +		if (event->val == nr)
> +			return event;
> +	}
> +
> +	return NULL;
> +}
> 
>  static struct op_event * find_event_um(u32 nr, u32 um)
>  {
> @@ -726,6 +739,12 @@
>  	return event;
>  }
> 
> +struct op_event * op_find_event_any(op_cpu cpu_type, u32 nr)
> +{
> +	load_events(cpu_type);
> +
> +	return find_event_any(nr);
> +}
> 
>  int op_check_events(int ctr, u32 nr, u32 um, op_cpu cpu_type)
>  {
> Index: libop/op_events.h
> ===================================================================
> RCS file: /cvsroot/oprofile/oprofile/libop/op_events.h,v
> retrieving revision 1.28
> diff -u -r1.28 op_events.h
> --- libop/op_events.h	28 Aug 2008 21:57:46 -0000	1.28
> +++ libop/op_events.h	7 Nov 2008 19:33:21 -0000
> @@ -65,6 +65,7 @@
> 
>  /** Find a given event, returns NULL on error */
>  struct op_event * op_find_event(op_cpu cpu_type, u32 nr, u32 um);
> +struct op_event * op_find_event_any(op_cpu cpu_type, u32 nr);
> 
>  /** Find a given event by name */
>  struct op_event * find_event_by_name(char const * name);
> Index: libpp/op_header.cpp
> ===================================================================
> RCS file: /cvsroot/oprofile/oprofile/libpp/op_header.cpp,v
> retrieving revision 1.30
> diff -u -r1.30 op_header.cpp
> --- libpp/op_header.cpp	28 Aug 2008 21:57:46 -0000	1.30
> +++ libpp/op_header.cpp	7 Nov 2008 19:33:21 -0000
> @@ -168,8 +168,12 @@
>  	struct op_event * event = op_find_event(cpu_type, type, um);
> 
>  	if (!event) {
> -		cerr << "Could not locate event " << int(type) << endl;
> -		return str;
> +		event = op_find_event_any(cpu_type, type);
> +		if (!event) { 
> +			cerr << "Could not locate event " << int(type) << endl;
> +			str = "Unknown event";
> +			return str;
> +		}
>  	}
> 
>  	char const * um_desc = 0;
> @@ -207,8 +211,11 @@
> 
>  	struct op_event * event = op_find_event(cpu_type, type, um);
>  	if (!event) {
> -		cerr << "Could not locate event " << int(type) << endl;
> -		return "";
> +		event = op_find_event_any(cpu_type, type);
> +		if (!event) { 
> +			cerr << "Could not locate event " << int(type) << endl;
> +			return "";
> +		}
>  	}
> 
>  	if (cpu_type != CPU_RTC) {
> 
> -Andi