[PATCH] perf parse events: Fix invalid precise_ip handling

[PATCH] perf parse events: Fix invalid precise_ip handling

[Mengting Zhang]

When the user set a precise_ip greater than the highest precision available,
perf should return EINVAL to indicate the invalid precise_ip setting.

Currently, in get_event_modifier(), perf compares the user-specified precise_ip
with 3, instead of the highest precision available in the current processor.
It is incorrect if the maximum precision available is less than 3.

For example, with a highest precision perf_event_attr.precise_ip = 2, the
user-specified precise_ip greater than 2 should be handled as invalid argument.

Introduce perf_get_max_precise_ip() to get max precision available and compare
it with the user-specified precise_ip. Also show the available max precision in
an error message when the user-specified precise_ip is invalid.

Before:
$./perf record -e cycles:ppp sleep 1
Error:
PMU Hardware doesn't support sampling/overflow-interrupts.

After:
$./perf record -e cycles:ppp sleep 1
Error: Invalid argument for precise_ip setting.
The highest level of precise ip is 2
invalid or unsupported event: 'cycles:ppp'
Run 'perf list' for a list of valid events

 Usage: perf record [<options>] [<command>]
    or: perf record [<options>] -- <command> [<options>]

    -e, --event <event>   event selector. use 'perf list' to list available events

Signed-off-by: Mengting Zhang <zhangmengting@huawei.com>
---
 tools/perf/util/parse-events.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 39b1596..25225f4 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1369,6 +1369,32 @@ struct event_modifier {
 	int pinned;
 };
 
+static int perf_get_max_precise_ip(void)
+{
+       int max_precise_ip = 0;
+       struct perf_event_attr attr = {
+               .type   = PERF_TYPE_HARDWARE,
+               .config = PERF_COUNT_HW_CPU_CYCLES,
+       };
+
+       event_attr_init(&attr);
+       
+       attr.precise_ip = 3;
+       attr.sample_period = 1;
+
+       while (attr.precise_ip != 0) {
+               int fd = sys_perf_event_open(&attr, 0, -1, -1, 0);
+               if (fd != -1){
+                       close(fd);
+                       break;
+               }
+               --attr.precise_ip;
+       }
+       max_precise_ip = attr.precise_ip;
+
+       return max_precise_ip;
+}
+
 static int get_event_modifier(struct event_modifier *mod, char *str,
 			       struct perf_evsel *evsel)
 {
@@ -1382,6 +1408,7 @@ static int get_event_modifier(struct event_modifier *mod, char *str,
 	int precise_max = 0;
 	int sample_read = 0;
 	int pinned = evsel ? evsel->attr.pinned : 0;
+	int max_precise_ip = 0;
 
 	int exclude = eu | ek | eh;
 	int exclude_GH = evsel ? evsel->exclude_GH : 0;
@@ -1438,8 +1465,12 @@ static int get_event_modifier(struct event_modifier *mod, char *str,
 	 *
 	 *  See also PERF_RECORD_MISC_EXACT_IP
 	 */
-	if (precise > 3)
+	max_precise_ip = perf_get_max_precise_ip();
+	if (precise > max_precise_ip) {
+		pr_err("Error: Invalid argument for precise_ip setting. \n"
+                       "The highest level of precise ip is %d\n", max_precise_ip);
 		return -EINVAL;
+	}
 
 	mod->eu = eu;
 	mod->ek = ek;
-- 
1.7.12.4

Re: [PATCH] perf parse events: Fix invalid precise_ip handling

[Jiri Olsa]

On Fri, Nov 10, 2017 at 04:28:37PM +0800, Mengting Zhang wrote:

SNIP

> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 39b1596..25225f4 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1369,6 +1369,32 @@ struct event_modifier {
>  	int pinned;
>  };
>  
> +static int perf_get_max_precise_ip(void)
> +{
> +       int max_precise_ip = 0;
> +       struct perf_event_attr attr = {
> +               .type   = PERF_TYPE_HARDWARE,
> +               .config = PERF_COUNT_HW_CPU_CYCLES,
> +       };
> +
> +       event_attr_init(&attr);
> +       
> +       attr.precise_ip = 3;
> +       attr.sample_period = 1;
> +
> +       while (attr.precise_ip != 0) {
> +               int fd = sys_perf_event_open(&attr, 0, -1, -1, 0);
> +               if (fd != -1){
> +                       close(fd);
> +                       break;
> +               }
> +               --attr.precise_ip;
> +       }
> +       max_precise_ip = attr.precise_ip;
> +
> +       return max_precise_ip;
> +}

we already have a function for that, please check perf_event_attr__set_max_precise_ip

also I think the precise level is not generic for all the events,
so you should check it for specific perf_event_attr later, when
the attr is ready, not in modifier parsing

thanks,
jirka

Re: [PATCH] perf parse events: Fix invalid precise_ip handling

[zhangmengting]

Hi Jiri, thanks for your detailed review, please see my comments inline.


On 2017/11/10 18:39, Jiri Olsa wrote:
> On Fri, Nov 10, 2017 at 04:28:37PM +0800, Mengting Zhang wrote:
>
> SNIP
>
>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>> index 39b1596..25225f4 100644
>> --- a/tools/perf/util/parse-events.c
>> +++ b/tools/perf/util/parse-events.c
>> @@ -1369,6 +1369,32 @@ struct event_modifier {
>>   	int pinned;
>>   };
>>   
>> +static int perf_get_max_precise_ip(void)
>> +{
>> +       int max_precise_ip = 0;
>> +       struct perf_event_attr attr = {
>> +               .type   = PERF_TYPE_HARDWARE,
>> +               .config = PERF_COUNT_HW_CPU_CYCLES,
>> +       };
>> +
>> +       event_attr_init(&attr);
>> +
>> +       attr.precise_ip = 3;
>> +       attr.sample_period = 1;
>> +
>> +       while (attr.precise_ip != 0) {
>> +               int fd = sys_perf_event_open(&attr, 0, -1, -1, 0);
>> +               if (fd != -1){
>> +                       close(fd);
>> +                       break;
>> +               }
>> +               --attr.precise_ip;
>> +       }
>> +       max_precise_ip = attr.precise_ip;
>> +
>> +       return max_precise_ip;
>> +}
> we already have a function for that, please check perf_event_attr__set_max_precise_ip
Yeah, I've checked that function. But 
perf_event_attr__set_max_precise_ip() will change attr.precise_ip
into the max precise ip available.

In this case, perf should only check whether the user-specified 
precise_ip is greater than the max
precise_ip without changing it into maximum.  Here, introduce 
perf_get_max_precise_ip() to return
the max precise ip and do not change attr.precise_ip.

But you reminds me that perf_get_max_precise_ip() can be simplied.
> also I think the precise level is not generic for all the events,
> so you should check it for specific perf_event_attr later, when
> the attr is ready, not in modifier parsing
You are right, and I would check it for specific perf_event_attr.

BTW, I have a question. If the user-specified precise_ip is greater than 
the max precise_ip, I wonder
whether it is better to adjust the user-specified precise_ip to the 
maximum available.

Thanks
Mengting Zhang

> thanks,
> jirka
> --
> To unsubscribe from this list: send the line "unsubscribe linux-perf-users" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> .
>

Re: [PATCH] perf parse events: Fix invalid precise_ip handling

[Jiri Olsa]

On Wed, Nov 15, 2017 at 09:00:03AM +0800, zhangmengting wrote:
> Hi Jiri, thanks for your detailed review, please see my comments inline.
> 
> 
> On 2017/11/10 18:39, Jiri Olsa wrote:
> > On Fri, Nov 10, 2017 at 04:28:37PM +0800, Mengting Zhang wrote:
> > 
> > SNIP
> > 
> > > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > > index 39b1596..25225f4 100644
> > > --- a/tools/perf/util/parse-events.c
> > > +++ b/tools/perf/util/parse-events.c
> > > @@ -1369,6 +1369,32 @@ struct event_modifier {
> > >   	int pinned;
> > >   };
> > > +static int perf_get_max_precise_ip(void)
> > > +{
> > > +       int max_precise_ip = 0;
> > > +       struct perf_event_attr attr = {
> > > +               .type   = PERF_TYPE_HARDWARE,
> > > +               .config = PERF_COUNT_HW_CPU_CYCLES,
> > > +       };
> > > +
> > > +       event_attr_init(&attr);
> > > +
> > > +       attr.precise_ip = 3;
> > > +       attr.sample_period = 1;
> > > +
> > > +       while (attr.precise_ip != 0) {
> > > +               int fd = sys_perf_event_open(&attr, 0, -1, -1, 0);
> > > +               if (fd != -1){
> > > +                       close(fd);
> > > +                       break;
> > > +               }
> > > +               --attr.precise_ip;
> > > +       }
> > > +       max_precise_ip = attr.precise_ip;
> > > +
> > > +       return max_precise_ip;
> > > +}
> > we already have a function for that, please check perf_event_attr__set_max_precise_ip
> Yeah, I've checked that function. But perf_event_attr__set_max_precise_ip()
> will change attr.precise_ip
> into the max precise ip available.
> 
> In this case, perf should only check whether the user-specified precise_ip
> is greater than the max
> precise_ip without changing it into maximum.  Here, introduce
> perf_get_max_precise_ip() to return
> the max precise ip and do not change attr.precise_ip.
> 
> But you reminds me that perf_get_max_precise_ip() can be simplied.

well both do the same.. probe kernel for max precise level,
so we can keep just one function for that

> > also I think the precise level is not generic for all the events,
> > so you should check it for specific perf_event_attr later, when
> > the attr is ready, not in modifier parsing
> You are right, and I would check it for specific perf_event_attr.
> 
> BTW, I have a question. If the user-specified precise_ip is greater than the
> max precise_ip, I wonder
> whether it is better to adjust the user-specified precise_ip to the maximum
> available.

no, I think that user defined precise level should stay the
way the user wants it.. we don't want more angry users ;-)

jirka

Re: [PATCH] perf parse events: Fix invalid precise_ip handling

[zhangmengting]

On 2017/11/20 15:33, Jiri Olsa wrote:
> On Wed, Nov 15, 2017 at 09:00:03AM +0800, zhangmengting wrote:
>> Hi Jiri, thanks for your detailed review, please see my comments inline.
>>
>>
>> On 2017/11/10 18:39, Jiri Olsa wrote:
>>> On Fri, Nov 10, 2017 at 04:28:37PM +0800, Mengting Zhang wrote:
>>>
>>> SNIP
>>>
>>>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>>>> index 39b1596..25225f4 100644
>>>> --- a/tools/perf/util/parse-events.c
>>>> +++ b/tools/perf/util/parse-events.c
>>>> @@ -1369,6 +1369,32 @@ struct event_modifier {
>>>>    	int pinned;
>>>>    };
>>>> +static int perf_get_max_precise_ip(void)
>>>> +{
>>>> +       int max_precise_ip = 0;
>>>> +       struct perf_event_attr attr = {
>>>> +               .type   = PERF_TYPE_HARDWARE,
>>>> +               .config = PERF_COUNT_HW_CPU_CYCLES,
>>>> +       };
>>>> +
>>>> +       event_attr_init(&attr);
>>>> +
>>>> +       attr.precise_ip = 3;
>>>> +       attr.sample_period = 1;
>>>> +
>>>> +       while (attr.precise_ip != 0) {
>>>> +               int fd = sys_perf_event_open(&attr, 0, -1, -1, 0);
>>>> +               if (fd != -1){
>>>> +                       close(fd);
>>>> +                       break;
>>>> +               }
>>>> +               --attr.precise_ip;
>>>> +       }
>>>> +       max_precise_ip = attr.precise_ip;
>>>> +
>>>> +       return max_precise_ip;
>>>> +}
>>> we already have a function for that, please check perf_event_attr__set_max_precise_ip
>> Yeah, I've checked that function. But perf_event_attr__set_max_precise_ip()
>> will change attr.precise_ip
>> into the max precise ip available.
>>
>> In this case, perf should only check whether the user-specified precise_ip
>> is greater than the max
>> precise_ip without changing it into maximum.  Here, introduce
>> perf_get_max_precise_ip() to return
>> the max precise ip and do not change attr.precise_ip.
>>
>> But you reminds me that perf_get_max_precise_ip() can be simplied.
> well both do the same.. probe kernel for max precise level,
> so we can keep just one function for that

OKay, I will just keep that function for probing max precise level.

>>> also I think the precise level is not generic for all the events,
>>> so you should check it for specific perf_event_attr later, when
>>> the attr is ready, not in modifier parsing
>> You are right, and I would check it for specific perf_event_attr.
>>
>> BTW, I have a question. If the user-specified precise_ip is greater than the
>> max precise_ip, I wonder
>> whether it is better to adjust the user-specified precise_ip to the maximum
>> available.
> no, I think that user defined precise level should stay the
> way the user wants it.. we don't want more angry users ;-)

Humm, I am sorry for being unclear.
If the user defined precise level is greater than the max precise level,
I think there are two ways to deal with it.
1.  return EINVAL to indicate the invalid precise_ip setting;
2.  adjust to the max precise level available and give message to 
indicate the adjustment.

Since we should check user-defined precise level in perf_evsel__config(),
when the attr is ready, I think there is a problem with method 1, if we 
keep the
user defined precise level stay the way the user wants it.

With method 1, we have to let perf_evsel__config() return value and show 
errno.
And this change will affect many related functions, such as 
perf_evlist__config(), and files.

With method 2, we don't need to change the return type of 
perf_evsel__config().

Am I right?
>
> jirka
>
> .
>

Re: [PATCH] perf parse events: Fix invalid precise_ip handling

[Jiri Olsa]

On Tue, Nov 21, 2017 at 04:30:09PM +0800, zhangmengting wrote:

SNIP

> > > > also I think the precise level is not generic for all the events,
> > > > so you should check it for specific perf_event_attr later, when
> > > > the attr is ready, not in modifier parsing
> > > You are right, and I would check it for specific perf_event_attr.
> > > 
> > > BTW, I have a question. If the user-specified precise_ip is greater than the
> > > max precise_ip, I wonder
> > > whether it is better to adjust the user-specified precise_ip to the maximum
> > > available.
> > no, I think that user defined precise level should stay the
> > way the user wants it.. we don't want more angry users ;-)
> 
> Humm, I am sorry for being unclear.
> If the user defined precise level is greater than the max precise level,
> I think there are two ways to deal with it.
> 1.  return EINVAL to indicate the invalid precise_ip setting;

and warn user about the reason

> 2.  adjust to the max precise level available and give message to indicate
> the adjustment.

we do that (or should) only if the precise_ip is not defined by user
because we want the max precise level by default

> Since we should check user-defined precise level in perf_evsel__config(),
> when the attr is ready, I think there is a problem with method 1, if we keep
> the
> user defined precise level stay the way the user wants it.
> 
> With method 1, we have to let perf_evsel__config() return value and show
> errno.
> And this change will affect many related functions, such as
> perf_evlist__config(), and files.
> 
> With method 2, we don't need to change the return type of
> perf_evsel__config().
> 
> Am I right?

not sure.. let's discuss over the code changes

jirka