Re: [PATCH 3/3] perf tool: give user better message if precise is not supported

[Robert Richter <robert.richter@amd.com>]

On 11.09.12 07:22:32, David Ahern wrote:
> On 9/11/12 3:20 AM, Robert Richter wrote:
> > On 10.09.12 10:40:16, David Ahern wrote:
> >> --- a/tools/perf/builtin-record.c
> >> +++ b/tools/perf/builtin-record.c
> >> @@ -294,6 +294,11 @@ try_again:
> >>   					  perf_evsel__name(pos));
> >>   				rc = -err;
> >>   				goto out;
> >> +			} else if ((err == ENOTSUP) && (attr->precise_ip)) {
> >
> > It is EOPNOTSUPP, did you test this?

Ok, wrong question. Better would have been: Did you run it on a
non-pebs Intel machine of an non-ibs AMD machine?

> I do not post patches without testing them. This particular patch was 
> verified in a Virtual Machine (no PEBS) and using :pG modifier.
> 
> 'egrep -r ENOTSUP tools/perf' shows hits in 3 other files, so I am not 
> the only one using the shortcut. I'll change it in the follow up with 
> better commit messages to make it consistent with patch 2.

For VM this might be valid. Don't know where ENOTSUP comes from. It is
neither in kernel/events/ nor arch/x86/kernel/cpu/perf.

If you run this bare-metal on older machines which do not support pebs
or ibs, the syscall returns EOPNOTSUPP. You can trigger the same
behaviour on newer systems with:

 # perf record -e cycles:ppp -c 2097120 -R -a sleep 1
 
   Error: sys_perf_event_open() syscall returned with 95 (Operation not supported).  /bin/dmesg may provide additional information.
 ...

It should work in this case too.

> > To avoid adding more duplicate code, maybe we should start to unify
> > the code by implementing this in a shared helper function.
> 
> Doing that requires additional modifications to not break perl and 
> python scripts. Adding it in both commands here is consistent with all 
> other open counter failures. Consolidation of those loops into a common 
> base is known to do item.

I am fine with that too.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center