perf tools: interface for improved PEBS ABI can accept wrong parameter

perf tools: interface for improved PEBS ABI can accept wrong parameter

[Xu, Anhua]

For the commit: ab60834 perf, x86: Improve the PEBS ABI
The precise_ip is restricted to 2 bits. The values larger than 3 are truncated less or equal to 3.
This can effectively restricted value passing to syscall. But the user space interface can accept other wrong values which are larger than 3 and lowest 2 bits are 0 can meet the requirement.

# Here is an example, Monitor "All branches" events on Sandybridge platform( Please refer to SDM 3B Table 30-21 to check Sandybridge specific events )

perf top -e r04c4:p  # precise_ip = 1 , Correct
perf top -e r04c4:pp  # precise_ip =2, Correct
perf top -e r04c4:ppp  # not supported, Correct
perf top -e r04c4:pppppp  # should not be supported, but after truncation , the number of 'p' treated as 2 which is accepted


diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index c816075..07d3cf9 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -216,6 +216,7 @@ struct perf_event_attr {
 				 *
 				 *  See also PERF_RECORD_MISC_EXACT_IP
 				 */
+#define SAMPLE_IP_MAX	3 
 				precise_ip     :  2, /* skid constraint       */
 				mmap_data      :  1, /* non-exec mmap data    */
 				sample_id_all  :  1, /* sample_type all events */
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 928918b..a2068cc 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -775,7 +775,10 @@ parse_event_modifier(const char **strp, struct perf_event_attr *attr)
 	attr->exclude_user   = eu;
 	attr->exclude_kernel = ek;
 	attr->exclude_hv     = eh;
-	attr->precise_ip     = precise;
+	if ( precise < SAMPLE_IP_MAX )
+		attr->precise_ip = precise;
+	else
+		return -1;
 
 	return 0;
 }

Re: perf tools: interface for improved PEBS ABI can accept wrong parameter

[Ingo Molnar]

Added more Cc:s of maintainers.

thanks,

	Ingo

* Xu, Anhua <anhua.xu@intel.com> wrote:

> For the commit: ab60834 perf, x86: Improve the PEBS ABI
> The precise_ip is restricted to 2 bits. The values larger than 3 are truncated less or equal to 3.
> This can effectively restricted value passing to syscall. But the user space interface can accept other wrong values which are larger than 3 and lowest 2 bits are 0 can meet the requirement.
> 
> # Here is an example, Monitor "All branches" events on Sandybridge platform( Please refer to SDM 3B Table 30-21 to check Sandybridge specific events )
> 
> perf top -e r04c4:p  # precise_ip = 1 , Correct
> perf top -e r04c4:pp  # precise_ip =2, Correct
> perf top -e r04c4:ppp  # not supported, Correct
> perf top -e r04c4:pppppp  # should not be supported, but after truncation , the number of 'p' treated as 2 which is accepted
> 
> 
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index c816075..07d3cf9 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -216,6 +216,7 @@ struct perf_event_attr {
>  				 *
>  				 *  See also PERF_RECORD_MISC_EXACT_IP
>  				 */
> +#define SAMPLE_IP_MAX	3 
>  				precise_ip     :  2, /* skid constraint       */
>  				mmap_data      :  1, /* non-exec mmap data    */
>  				sample_id_all  :  1, /* sample_type all events */
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 928918b..a2068cc 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -775,7 +775,10 @@ parse_event_modifier(const char **strp, struct perf_event_attr *attr)
>  	attr->exclude_user   = eu;
>  	attr->exclude_kernel = ek;
>  	attr->exclude_hv     = eh;
> -	attr->precise_ip     = precise;
> +	if ( precise < SAMPLE_IP_MAX )
> +		attr->precise_ip = precise;
> +	else
> +		return -1;
>  
>  	return 0;
>  }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Thanks,

	Ingo

Re: perf tools: interface for improved PEBS ABI can accept wrong parameter

[Peter Zijlstra]

On Tue, 2011-10-18 at 20:19 +0200, Ingo Molnar wrote:
> > +#define SAMPLE_IP_MAX        3 
> >                               precise_ip     :  2, /* skid constraint       */
> >                               mmap_data      :  1, /* non-exec mmap data    */
> >                               sample_id_all  :  1, /* sample_type all events */
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index 928918b..a2068cc 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -775,7 +775,10 @@ parse_event_modifier(const char **strp, struct perf_event_attr *attr)
> >       attr->exclude_user   = eu;
> >       attr->exclude_kernel = ek;
> >       attr->exclude_hv     = eh;
> > -     attr->precise_ip     = precise;
> > +     if ( precise < SAMPLE_IP_MAX )
> > +             attr->precise_ip = precise;
> > +     else
> > +             return -1; 

That name is horrid, how about PRECISE_IP_MAX? Also, I suspect acme will
want a better error return than -1, but I'll leave that up to him.

RE: perf tools: interface for improved PEBS ABI can accept wrong parameter

[Xu, Anhua]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1714 bytes --]

Thanks Peter:). The key point for this is that the detailed information about "PRECISE_IP" may not be exposed to user space.
Error reporting may come from syscall. Anyway, expect acme's suggestions.


-----Original Message-----
From: Peter Zijlstra [mailto:a.p.zijlstra@chello.nl] 
Sent: Wednesday, October 19, 2011 2:28 AM
To: Ingo Molnar
Cc: Xu, Anhua; Arnaldo Carvalho de Melo; linux-kernel@vger.kernel.org
Subject: Re: perf tools: interface for improved PEBS ABI can accept wrong parameter

On Tue, 2011-10-18 at 20:19 +0200, Ingo Molnar wrote:
> > +#define SAMPLE_IP_MAX        3 
> >                               precise_ip     :  2, /* skid constraint       */
> >                               mmap_data      :  1, /* non-exec mmap data    */
> >                               sample_id_all  :  1, /* sample_type all events */
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index 928918b..a2068cc 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -775,7 +775,10 @@ parse_event_modifier(const char **strp, struct perf_event_attr *attr)
> >       attr->exclude_user   = eu;
> >       attr->exclude_kernel = ek;
> >       attr->exclude_hv     = eh;
> > -     attr->precise_ip     = precise;
> > +     if ( precise < SAMPLE_IP_MAX )
> > +             attr->precise_ip = precise;
> > +     else
> > +             return -1; 

That name is horrid, how about PRECISE_IP_MAX? Also, I suspect acme will
want a better error return than -1, but I'll leave that up to him.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: perf tools: interface for improved PEBS ABI can accept wrong parameter

[Chen Gong]

于 2011/10/19 10:23, Xu, Anhua 写道:
> Thanks Peter:). The key point for this is that the detailed information about "PRECISE_IP" may not be exposed to user space.
> Error reporting may come from syscall. Anyway, expect acme's suggestions.
>
>
> -----Original Message-----
> From: Peter Zijlstra [mailto:a.p.zijlstra@chello.nl]
> Sent: Wednesday, October 19, 2011 2:28 AM
> To: Ingo Molnar
> Cc: Xu, Anhua; Arnaldo Carvalho de Melo; linux-kernel@vger.kernel.org
> Subject: Re: perf tools: interface for improved PEBS ABI can accept wrong parameter
>
> On Tue, 2011-10-18 at 20:19 +0200, Ingo Molnar wrote:
>>> +#define SAMPLE_IP_MAX        3
>>>                                precise_ip     :  2, /* skid constraint       */
>>>                                mmap_data      :  1, /* non-exec mmap data    */
>>>                                sample_id_all  :  1, /* sample_type all events */
>>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>>> index 928918b..a2068cc 100644
>>> --- a/tools/perf/util/parse-events.c
>>> +++ b/tools/perf/util/parse-events.c
>>> @@ -775,7 +775,10 @@ parse_event_modifier(const char **strp, struct perf_event_attr *attr)
>>>        attr->exclude_user   = eu;
>>>        attr->exclude_kernel = ek;
>>>        attr->exclude_hv     = eh;
>>> -     attr->precise_ip     = precise;
>>> +     if ( precise<  SAMPLE_IP_MAX )
>>> +             attr->precise_ip = precise;
>>> +     else
>>> +             return -1;
>
> That name is horrid, how about PRECISE_IP_MAX? Also, I suspect acme will
> want a better error return than -1, but I'll leave that up to him.
> N�����r��y���b�X��ǧv�^�)޺{.n�+����{����zX��\x17��ܨ}���Ơz�&j:+v���\a����zZ+��+zf���h���~����i���z�\x1e�w���?����&�)ߢ^[f��^jǫy�m��@A�a��\x7f�\f0��h�\x0f�i\x7f


Looks like your mail client configuration is bad. Change to another one?

Re: perf tools: interface for improved PEBS ABI can accept wrong parameter

[Arnaldo Carvalho de Melo]

Em Wed, Oct 19, 2011 at 10:23:12AM +0800, Xu, Anhua escreveu:
> Thanks Peter:). The key point for this is that the detailed information about "PRECISE_IP" may not be exposed to user space.
> Error reporting may come from syscall. Anyway, expect acme's suggestions.


Well, at least we can do as you did and avoid requests completely
invalid per the ABI or look the other way, truncate and silently provide
different behaviour than requested.

Point is how to propagate back from parse_event_modifier so that we
provide a sensible error message.

One could try to reuse errno and find things like -EINVAL, ELEVEL or
define some enum.
 
> 
> -----Original Message-----
> From: Peter Zijlstra [mailto:a.p.zijlstra@chello.nl] 
> Sent: Wednesday, October 19, 2011 2:28 AM
> To: Ingo Molnar
> Cc: Xu, Anhua; Arnaldo Carvalho de Melo; linux-kernel@vger.kernel.org
> Subject: Re: perf tools: interface for improved PEBS ABI can accept wrong parameter
> 
> On Tue, 2011-10-18 at 20:19 +0200, Ingo Molnar wrote:
> > > +#define SAMPLE_IP_MAX        3 
> > >                               precise_ip     :  2, /* skid constraint       */
> > >                               mmap_data      :  1, /* non-exec mmap data    */
> > >                               sample_id_all  :  1, /* sample_type all events */
> > > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > > index 928918b..a2068cc 100644
> > > --- a/tools/perf/util/parse-events.c
> > > +++ b/tools/perf/util/parse-events.c
> > > @@ -775,7 +775,10 @@ parse_event_modifier(const char **strp, struct perf_event_attr *attr)
> > >       attr->exclude_user   = eu;
> > >       attr->exclude_kernel = ek;
> > >       attr->exclude_hv     = eh;
> > > -     attr->precise_ip     = precise;
> > > +     if ( precise < SAMPLE_IP_MAX )
> > > +             attr->precise_ip = precise;
> > > +     else
> > > +             return -1; 
> 
> That name is horrid, how about PRECISE_IP_MAX? Also, I suspect acme will
> want a better error return than -1, but I'll leave that up to him.

RE: perf tools: interface for improved PEBS ABI can accept wrong parameter

[Xu, Anhua]

I have update my patch. Hope it is not so horrid :)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index c816075..837f416 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -238,6 +238,17 @@ struct perf_event_attr {
 	};
 };
 
+enum perf_sample_ip_constraints {
+	PERF_SAMPLE_IP_ARBITRARY_SKID,
+	PERF_SAMPLE_IP_CONSTANT_SKID,
+	PERF_SAMPLE_IP_REQUEST_NOSKID,
+
+	/*
+	 *  The PEBS implementation now supports up to 2.
+	 */
+	PERF_SMAPLE_IP_CONSTRAINT_MAX,
+};
+
 /*
  * Ioctls that can be done on a perf event fd:
  */
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 928918b..55567ec 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -775,7 +775,10 @@ parse_event_modifier(const char **strp, struct perf_event_attr *attr)
 	attr->exclude_user   = eu;
 	attr->exclude_kernel = ek;
 	attr->exclude_hv     = eh;
-	attr->precise_ip     = precise;
+	if ( precise < PERF_SMAPLE_IP_CONSTRAINT_MAX )
+		attr->precise_ip = precise;
+	else
+		return -EOPNOTSUPP;
 
 	return 0;
 }

-----Original Message-----
From: Arnaldo Carvalho de Melo [mailto:acme@redhat.com] 
Sent: Wednesday, October 19, 2011 10:15 PM
To: Xu, Anhua
Cc: Peter Zijlstra; Ingo Molnar; linux-kernel@vger.kernel.org
Subject: Re: perf tools: interface for improved PEBS ABI can accept wrong parameter

Em Wed, Oct 19, 2011 at 10:23:12AM +0800, Xu, Anhua escreveu:
> Thanks Peter:). The key point for this is that the detailed information about "PRECISE_IP" may not be exposed to user space.
> Error reporting may come from syscall. Anyway, expect acme's suggestions.


Well, at least we can do as you did and avoid requests completely
invalid per the ABI or look the other way, truncate and silently provide
different behaviour than requested.

Point is how to propagate back from parse_event_modifier so that we
provide a sensible error message.

One could try to reuse errno and find things like -EINVAL, ELEVEL or
define some enum.
 
> 
> -----Original Message-----
> From: Peter Zijlstra [mailto:a.p.zijlstra@chello.nl] 
> Sent: Wednesday, October 19, 2011 2:28 AM
> To: Ingo Molnar
> Cc: Xu, Anhua; Arnaldo Carvalho de Melo; linux-kernel@vger.kernel.org
> Subject: Re: perf tools: interface for improved PEBS ABI can accept wrong parameter
> 
> On Tue, 2011-10-18 at 20:19 +0200, Ingo Molnar wrote:
> > > +#define SAMPLE_IP_MAX        3 
> > >                               precise_ip     :  2, /* skid constraint       */
> > >                               mmap_data      :  1, /* non-exec mmap data    */
> > >                               sample_id_all  :  1, /* sample_type all events */
> > > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > > index 928918b..a2068cc 100644
> > > --- a/tools/perf/util/parse-events.c
> > > +++ b/tools/perf/util/parse-events.c
> > > @@ -775,7 +775,10 @@ parse_event_modifier(const char **strp, struct perf_event_attr *attr)
> > >       attr->exclude_user   = eu;
> > >       attr->exclude_kernel = ek;
> > >       attr->exclude_hv     = eh;
> > > -     attr->precise_ip     = precise;
> > > +     if ( precise < SAMPLE_IP_MAX )
> > > +             attr->precise_ip = precise;
> > > +     else
> > > +             return -1; 
> 
> That name is horrid, how about PRECISE_IP_MAX? Also, I suspect acme will
> want a better error return than -1, but I'll leave that up to him.

perf tools: interface for improved PEBS ABI can accept wrong parameter

[Xu, Anhua]

For the commit: ab60834 perf, x86: Improve the PEBS ABI
The precise_ip is restricted to 2 bits. The values larger than 3 are truncated less or equal to 3. This can effectively restricted value passing to syscall. 
But the user space interface can accept other wrong values which are larger than 3 and lowest 2 bits are 0 can meet the requirement.

# Here is an example, Monitor "All branches" events on Sandybridge platform( Please refer to SDM 3B Table 30-21 to check Sandybridge specific events )
perf top -e r04c4:p  # precise_ip = 1 , Correct
perf top -e r04c4:pp  # precise_ip =2, Correct
perf top -e r04c4:ppp  # not supported, Correct 
perf top -e r04c4:pppppp  # should not be supported, but after truncation , the number of 'p' treated as 2 which is accepted

Here is the patch:

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index c816075..07d3cf9 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -216,6 +216,7 @@ struct perf_event_attr {
 				 *
 				 *  See also PERF_RECORD_MISC_EXACT_IP
 				 */
+#define SAMPLE_IP_MAX	3 
 				precise_ip     :  2, /* skid constraint       */
 				mmap_data      :  1, /* non-exec mmap data    */
 				sample_id_all  :  1, /* sample_type all events */
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 928918b..a2068cc 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -775,7 +775,10 @@ parse_event_modifier(const char **strp, struct perf_event_attr *attr)
 	attr->exclude_user   = eu;
 	attr->exclude_kernel = ek;
 	attr->exclude_hv     = eh;
-	attr->precise_ip     = precise;
+	if ( precise < SAMPLE_IP_MAX )
+		attr->precise_ip = precise;
+	else
+		return -1;
 
 	return 0;
 }