[PATCH] perf: disable mmap2 support

[PATCH] perf: disable mmap2 support

[Stephane Eranian]

This patch disables the mmap2 record format support
for now. The support needs a bit more work to cover
VM_CLONE cases.

The patch leaves attr->mmap2 defined, but returns
an error if it is set. That ensures no PERF_RECORD_MMAP2
record can be generated. Yet it minimizes the reverts in
the perf tool.


Signed-off-by: Stephane Eranian <eranian@google.com>
---
 include/uapi/linux/perf_event.h |   18 +-----------------
 kernel/events/core.c            |    4 ++++
 2 files changed, 5 insertions(+), 17 deletions(-)

diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index da48837..8074b29 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -682,23 +682,7 @@ enum perf_event_type {
 	PERF_RECORD_SAMPLE			= 9,
 
 	/*
-	 * The MMAP2 records are an augmented version of MMAP, they add
-	 * maj, min, ino numbers to be used to uniquely identify each mapping
-	 *
-	 * struct {
-	 *	struct perf_event_header	header;
-	 *
-	 *	u32				pid, tid;
-	 *	u64				addr;
-	 *	u64				len;
-	 *	u64				pgoff;
-	 *	u32				maj;
-	 *	u32				min;
-	 *	u64				ino;
-	 *	u64				ino_generation;
-	 *	char				filename[];
-	 * 	struct sample_id		sample_id;
-	 * };
+	 * reserved, do not use
 	 */
 	PERF_RECORD_MMAP2			= 10,
 
diff --git a/kernel/events/core.c b/kernel/events/core.c
index c716385..d7152a6 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -6782,6 +6782,10 @@ static int perf_copy_attr(struct perf_event_attr __user *uattr,
 	if (attr->read_format & ~(PERF_FORMAT_MAX-1))
 		return -EINVAL;
 
+	/* disable for now */
+	if (attr->mmap2)
+		return -EINVAL;
+
 	if (attr->sample_type & PERF_SAMPLE_BRANCH_STACK) {
 		u64 mask = attr->branch_sample_type;
 
-- 
1.7.10.4

Re: [PATCH] perf: disable mmap2 support

[David Ahern]

On 10/9/13 10:17 AM, Stephane Eranian wrote:
>
> This patch disables the mmap2 record format support
> for now. The support needs a bit more work to cover
> VM_CLONE cases.
>
> The patch leaves attr->mmap2 defined, but returns
> an error if it is set. That ensures no PERF_RECORD_MMAP2
> record can be generated. Yet it minimizes the reverts in
> the perf tool.

The userspace code generates MMAP2 events for synthesized mmap events. 
That should be reverted to MMAP until the format of MMAP2 events is 
finalized.

David

Re: [PATCH] perf: disable mmap2 support

[Stephane Eranian]

On Wed, Oct 9, 2013 at 6:21 PM, David Ahern <dsahern@gmail.com> wrote:
> On 10/9/13 10:17 AM, Stephane Eranian wrote:
>>
>>
>> This patch disables the mmap2 record format support
>> for now. The support needs a bit more work to cover
>> VM_CLONE cases.
>>
>> The patch leaves attr->mmap2 defined, but returns
>> an error if it is set. That ensures no PERF_RECORD_MMAP2
>> record can be generated. Yet it minimizes the reverts in
>> the perf tool.
>
>
> The userspace code generates MMAP2 events for synthesized mmap events. That
> should be reverted to MMAP until the format of MMAP2 events is finalized.
>
Ah, yes, that is correct. I forgot about that ;-(
Let me send a revised patch then that covers both.

> David

Re: [PATCH] perf: disable mmap2 support

[Stephane Eranian]

On Wed, Oct 9, 2013 at 6:24 PM, Stephane Eranian <eranian@google.com> wrote:
> On Wed, Oct 9, 2013 at 6:21 PM, David Ahern <dsahern@gmail.com> wrote:
>> On 10/9/13 10:17 AM, Stephane Eranian wrote:
>>>
>>>
>>> This patch disables the mmap2 record format support
>>> for now. The support needs a bit more work to cover
>>> VM_CLONE cases.
>>>
>>> The patch leaves attr->mmap2 defined, but returns
>>> an error if it is set. That ensures no PERF_RECORD_MMAP2
>>> record can be generated. Yet it minimizes the reverts in
>>> the perf tool.
>>
>>
>> The userspace code generates MMAP2 events for synthesized mmap events. That
>> should be reverted to MMAP until the format of MMAP2 events is finalized.
>>
> Ah, yes, that is correct. I forgot about that ;-(
> Let me send a revised patch then that covers both.
>
Looks like the easiest thing to do for perf is to revert:

   Revert "perf tools: Add default handler for mmap2 events"
   git revert 6adb0b0ae26fcc35cfec068d71f13863faac5b44

   Revert "perf tools: Add attr->mmap2 support"
   git revert 5c5e854bc760a2e2c878df3cfcf2afa4febcd511

Do you agree?

Re: [PATCH] perf: disable mmap2 support

[David Ahern]

On 10/9/13 10:39 AM, Stephane Eranian wrote:
> Looks like the easiest thing to do for perf is to revert:
>
>     Revert "perf tools: Add default handler for mmap2 events"
>     git revert 6adb0b0ae26fcc35cfec068d71f13863faac5b44
>
>     Revert "perf tools: Add attr->mmap2 support"
>     git revert 5c5e854bc760a2e2c878df3cfcf2afa4febcd511
>
> Do you agree?

If you want to strip all of mmap2 you also need 
5654a0257a86c093c2b38ea5d3ceaa17affd735b.

David

Re: [PATCH] perf: disable mmap2 support

[Stephane Eranian]

On Wed, Oct 9, 2013 at 7:19 PM, David Ahern <dsahern@gmail.com> wrote:
> On 10/9/13 10:39 AM, Stephane Eranian wrote:
>>
>> Looks like the easiest thing to do for perf is to revert:
>>
>>     Revert "perf tools: Add default handler for mmap2 events"
>>     git revert 6adb0b0ae26fcc35cfec068d71f13863faac5b44
>>
>>     Revert "perf tools: Add attr->mmap2 support"
>>     git revert 5c5e854bc760a2e2c878df3cfcf2afa4febcd511
>>
>> Do you agree?
>
>
> If you want to strip all of mmap2 you also need
> 5654a0257a86c093c2b38ea5d3ceaa17affd735b.
>
You mean:
commit 384c671e33a7ddf905f2c60b433b9883f0e5a605
Author: David Ahern <dsahern@gmail.com>
Date:   Sun Sep 22 19:44:58 2013 -0600

    perf trace: Add mmap2 handler

Right?

Re: [PATCH] perf: disable mmap2 support

[David Ahern]

On 10/9/13 11:40 AM, Stephane Eranian wrote:
> On Wed, Oct 9, 2013 at 7:19 PM, David Ahern <dsahern@gmail.com> wrote:
>> On 10/9/13 10:39 AM, Stephane Eranian wrote:
>>>
>>> Looks like the easiest thing to do for perf is to revert:
>>>
>>>      Revert "perf tools: Add default handler for mmap2 events"
>>>      git revert 6adb0b0ae26fcc35cfec068d71f13863faac5b44
>>>
>>>      Revert "perf tools: Add attr->mmap2 support"
>>>      git revert 5c5e854bc760a2e2c878df3cfcf2afa4febcd511
>>>
>>> Do you agree?
>>
>>
>> If you want to strip all of mmap2 you also need
>> 5654a0257a86c093c2b38ea5d3ceaa17affd735b.
>>
> You mean:
> commit 384c671e33a7ddf905f2c60b433b9883f0e5a605
> Author: David Ahern <dsahern@gmail.com>
> Date:   Sun Sep 22 19:44:58 2013 -0600
>
>      perf trace: Add mmap2 handler
>
> Right?

Yes, a couple of those have 2 commit ids. I pulled the id's from acme's 
perf/core branch

David

Re: [PATCH] perf: disable mmap2 support

[Arnaldo Carvalho de Melo]

Em Wed, Oct 09, 2013 at 12:03:58PM -0600, David Ahern escreveu:
> On 10/9/13 11:40 AM, Stephane Eranian wrote:
> >On Wed, Oct 9, 2013 at 7:19 PM, David Ahern <dsahern@gmail.com> wrote:
> >>On 10/9/13 10:39 AM, Stephane Eranian wrote:
> >You mean:
> >commit 384c671e33a7ddf905f2c60b433b9883f0e5a605
> >Author: David Ahern <dsahern@gmail.com>
> >Date:   Sun Sep 22 19:44:58 2013 -0600
> >
> >     perf trace: Add mmap2 handler
> >
> >Right?
> 
> Yes, a couple of those have 2 commit ids. I pulled the id's from
> acme's perf/core branch

I'm merging my perf/core branch with Ingo's build speedup series, so
I've removed that patch already, will be ok as soon as the disabling is
done in perf/urgent and those branches get merged.
 
- Arnaldo

Re: [PATCH] perf: disable mmap2 support

[David Ahern]

Hi Stephane:

On 10/9/13 11:40 AM, Stephane Eranian wrote:
> On Wed, Oct 9, 2013 at 7:19 PM, David Ahern <dsahern@gmail.com> wrote:
>> On 10/9/13 10:39 AM, Stephane Eranian wrote:
>>>
>>> Looks like the easiest thing to do for perf is to revert:
>>>
>>>      Revert "perf tools: Add default handler for mmap2 events"
>>>      git revert 6adb0b0ae26fcc35cfec068d71f13863faac5b44
>>>
>>>      Revert "perf tools: Add attr->mmap2 support"
>>>      git revert 5c5e854bc760a2e2c878df3cfcf2afa4febcd511
>>>
>>> Do you agree?
>>
>>
>> If you want to strip all of mmap2 you also need
>> 5654a0257a86c093c2b38ea5d3ceaa17affd735b.
>>
> You mean:
> commit 384c671e33a7ddf905f2c60b433b9883f0e5a605
> Author: David Ahern <dsahern@gmail.com>
> Date:   Sun Sep 22 19:44:58 2013 -0600
>
>      perf trace: Add mmap2 handler
>

Given that 3.12 rc5 will be out on Sunday and it takes time to go from 
Arnaldo to Ingo to Linus the revert patches should go out today - in 
Arnaldo's next perf/urgent request.

David

Re: [PATCH] perf: disable mmap2 support

[Stephane Eranian]

Arnaldo,

Could you revert all the mmap2 patches in your perf/urgent tree for now?
Thanks.

On Fri, Oct 11, 2013 at 8:40 PM, David Ahern <dsahern@gmail.com> wrote:
> Hi Stephane:
>
>
> On 10/9/13 11:40 AM, Stephane Eranian wrote:
>>
>> On Wed, Oct 9, 2013 at 7:19 PM, David Ahern <dsahern@gmail.com> wrote:
>>>
>>> On 10/9/13 10:39 AM, Stephane Eranian wrote:
>>>>
>>>>
>>>> Looks like the easiest thing to do for perf is to revert:
>>>>
>>>>      Revert "perf tools: Add default handler for mmap2 events"
>>>>      git revert 6adb0b0ae26fcc35cfec068d71f13863faac5b44
>>>>
>>>>      Revert "perf tools: Add attr->mmap2 support"
>>>>      git revert 5c5e854bc760a2e2c878df3cfcf2afa4febcd511
>>>>
>>>> Do you agree?
>>>
>>>
>>>
>>> If you want to strip all of mmap2 you also need
>>> 5654a0257a86c093c2b38ea5d3ceaa17affd735b.
>>>
>> You mean:
>> commit 384c671e33a7ddf905f2c60b433b9883f0e5a605
>> Author: David Ahern <dsahern@gmail.com>
>> Date:   Sun Sep 22 19:44:58 2013 -0600
>>
>>      perf trace: Add mmap2 handler
>>
>
> Given that 3.12 rc5 will be out on Sunday and it takes time to go from
> Arnaldo to Ingo to Linus the revert patches should go out today - in
> Arnaldo's next perf/urgent request.
>
> David
>

Re: [PATCH] perf: disable mmap2 support

[Ingo Molnar]

* Stephane Eranian <eranian@google.com> wrote:

> Arnaldo,
> 
> Could you revert all the mmap2 patches in your perf/urgent tree for now?
> Thanks.

I think it would be helpful if you re-sent the final patch, with a full 
changelog, etc., in a separate thread, not inside an existing discussion 
and such.

Thanks,

	Ingo

Re: [PATCH] perf: disable mmap2 support

[Stephane Eranian]

On Tue, Oct 15, 2013 at 3:12 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Stephane Eranian <eranian@google.com> wrote:
>
>> Arnaldo,
>>
>> Could you revert all the mmap2 patches in your perf/urgent tree for now?
>> Thanks.
>
> I think it would be helpful if you re-sent the final patch, with a full
> changelog, etc., in a separate thread, not inside an existing discussion
> and such.
>
Are you talking about the perf tool or kernel patch?
I did not send any perf-related mmap2 revert patches.

> Thanks,
>
>         Ingo

Re: [PATCH] perf: disable mmap2 support

[Ingo Molnar]

* Stephane Eranian <eranian@google.com> wrote:

> On Tue, Oct 15, 2013 at 3:12 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Stephane Eranian <eranian@google.com> wrote:
> >
> >> Arnaldo,
> >>
> >> Could you revert all the mmap2 patches in your perf/urgent tree for now?
> >> Thanks.
> >
> > I think it would be helpful if you re-sent the final patch, with a full
> > changelog, etc., in a separate thread, not inside an existing discussion
> > and such.
>
> Are you talking about the perf tool or kernel patch?

Both.

> I did not send any perf-related mmap2 revert patches.

Would be nice to have a single patch that reverts both.

Thanks,

	Ingo

Re: [PATCH] perf: disable mmap2 support

[Stephane Eranian]

On Tue, Oct 15, 2013 at 3:34 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Stephane Eranian <eranian@google.com> wrote:
>
>> On Tue, Oct 15, 2013 at 3:12 PM, Ingo Molnar <mingo@kernel.org> wrote:
>> >
>> > * Stephane Eranian <eranian@google.com> wrote:
>> >
>> >> Arnaldo,
>> >>
>> >> Could you revert all the mmap2 patches in your perf/urgent tree for now?
>> >> Thanks.
>> >
>> > I think it would be helpful if you re-sent the final patch, with a full
>> > changelog, etc., in a separate thread, not inside an existing discussion
>> > and such.
>>
>> Are you talking about the perf tool or kernel patch?
>
> Both.
>
>> I did not send any perf-related mmap2 revert patches.
>
> Would be nice to have a single patch that reverts both.
>
I will send a single patch to remove everything (kernel + tool) then.

Re: [PATCH] perf: disable mmap2 support

[Ingo Molnar]

* Stephane Eranian <eranian@google.com> wrote:

> On Tue, Oct 15, 2013 at 3:34 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Stephane Eranian <eranian@google.com> wrote:
> >
> >> On Tue, Oct 15, 2013 at 3:12 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >> >
> >> > * Stephane Eranian <eranian@google.com> wrote:
> >> >
> >> >> Arnaldo,
> >> >>
> >> >> Could you revert all the mmap2 patches in your perf/urgent tree for now?
> >> >> Thanks.
> >> >
> >> > I think it would be helpful if you re-sent the final patch, with a full
> >> > changelog, etc., in a separate thread, not inside an existing discussion
> >> > and such.
> >>
> >> Are you talking about the perf tool or kernel patch?
> >
> > Both.
> >
> >> I did not send any perf-related mmap2 revert patches.
> >
> > Would be nice to have a single patch that reverts both.
> >
> I will send a single patch to remove everything (kernel + tool) then.

Thanks!

	Ingo

Re: [PATCH] perf: disable mmap2 support

[Arnaldo Carvalho de Melo]

Em Wed, Oct 09, 2013 at 06:17:43PM +0200, Stephane Eranian escreveu:
> This patch disables the mmap2 record format support
> for now. The support needs a bit more work to cover
> VM_CLONE cases.
> 
> The patch leaves attr->mmap2 defined, but returns
> an error if it is set. That ensures no PERF_RECORD_MMAP2
> record can be generated. Yet it minimizes the reverts in
> the perf tool.

Would be interesting to have a simple new tool that would detect all the
mmaps that are shared in a given perf.data file, using the
PERF_RECORD_MMAP2 information, and that tool should come together with
the reworked kernel ABI, showing how it is used.

A 'perf test' entry, based on the test programs you guys are using to
test this functionality would be even better, showing that all cases
you've been striving to cover are in fact covered by the new ABI.

- Arnaldo
 
> 
> Signed-off-by: Stephane Eranian <eranian@google.com>
> ---
>  include/uapi/linux/perf_event.h |   18 +-----------------
>  kernel/events/core.c            |    4 ++++
>  2 files changed, 5 insertions(+), 17 deletions(-)
> 
> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> index da48837..8074b29 100644
> --- a/include/uapi/linux/perf_event.h
> +++ b/include/uapi/linux/perf_event.h
> @@ -682,23 +682,7 @@ enum perf_event_type {
>  	PERF_RECORD_SAMPLE			= 9,
>  
>  	/*
> -	 * The MMAP2 records are an augmented version of MMAP, they add
> -	 * maj, min, ino numbers to be used to uniquely identify each mapping
> -	 *
> -	 * struct {
> -	 *	struct perf_event_header	header;
> -	 *
> -	 *	u32				pid, tid;
> -	 *	u64				addr;
> -	 *	u64				len;
> -	 *	u64				pgoff;
> -	 *	u32				maj;
> -	 *	u32				min;
> -	 *	u64				ino;
> -	 *	u64				ino_generation;
> -	 *	char				filename[];
> -	 * 	struct sample_id		sample_id;
> -	 * };
> +	 * reserved, do not use
>  	 */
>  	PERF_RECORD_MMAP2			= 10,
>  
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index c716385..d7152a6 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -6782,6 +6782,10 @@ static int perf_copy_attr(struct perf_event_attr __user *uattr,
>  	if (attr->read_format & ~(PERF_FORMAT_MAX-1))
>  		return -EINVAL;
>  
> +	/* disable for now */
> +	if (attr->mmap2)
> +		return -EINVAL;
> +
>  	if (attr->sample_type & PERF_SAMPLE_BRANCH_STACK) {
>  		u64 mask = attr->branch_sample_type;
>  
> -- 
> 1.7.10.4

Re: [PATCH] perf: disable mmap2 support

[Ingo Molnar]

* Arnaldo Carvalho de Melo <acme@redhat.com> wrote:

> Em Wed, Oct 09, 2013 at 06:17:43PM +0200, Stephane Eranian escreveu:

> > This patch disables the mmap2 record format support for now. The 
> > support needs a bit more work to cover VM_CLONE cases.
> > 
> > The patch leaves attr->mmap2 defined, but returns an error if it is 
> > set. That ensures no PERF_RECORD_MMAP2 record can be generated. Yet it 
> > minimizes the reverts in the perf tool.
> 
> Would be interesting to have a simple new tool that would detect all the 
> mmaps that are shared in a given perf.data file, using the 
> PERF_RECORD_MMAP2 information, and that tool should come together with 
> the reworked kernel ABI, showing how it is used.

Indeed. Let's make that a requirement for the next round of mmap2 patches. 

Issues like this is really why I hate not getting a fully functional 
usecase together with a new ABI - it's _so_ easy to come up with something 
subtly (or not so subtly) incomplete.

> A 'perf test' entry, based on the test programs you guys are using to 
> test this functionality would be even better, showing that all cases 
> you've been striving to cover are in fact covered by the new ABI.

That too, in addition to an actual tool that people can use.

Thanks,

	Ingo