[PATCH] perf tools: Exit recording if events have non matching sample type

[PATCH] perf tools: Exit recording if events have non matching sample type

[Jiri Olsa]

The event processing relies on all events having the same sample_type.

This is being checked when the session is being opened read only.
It also needs to be checked when we do record, since events could be
read during processing build IDs at the end of the record command.

If we process events with different sample_type the processing might
skip some events or hang.

Following command hangs on my setup:
  ./perf record -o perf.data -e ext4:ext4_mb_new_group_pa \
   -e LLC-loads -- date '+%F'

because hardware and tracepoint events have different sample type.

With the patch applied the record command displays
"Non matching sample_type" message and exits.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/builtin-record.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index f4c3fbe..71e2c4e 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -832,6 +832,12 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
 		goto out_symbol_exit;
 	}
 
+	if (!perf_evlist__valid_sample_type(evsel_list)) {
+		pr_err("Non matching sample_type\n");
+		err = -EINVAL;
+		goto out_symbol_exit;
+	}
+
 	if (target_pid != -1)
 		target_tid = target_pid;
 
-- 
1.7.4

Re: [PATCH] perf tools: Exit recording if events have non matching sample type

[Arnaldo Carvalho de Melo]

Em Thu, Sep 29, 2011 at 06:01:08PM +0200, Jiri Olsa escreveu:
> The event processing relies on all events having the same sample_type.
> 
> This is being checked when the session is being opened read only.
> It also needs to be checked when we do record, since events could be
> read during processing build IDs at the end of the record command.
> 
> If we process events with different sample_type the processing might
> skip some events or hang.
> 
> Following command hangs on my setup:
>   ./perf record -o perf.data -e ext4:ext4_mb_new_group_pa \
>    -e LLC-loads -- date '+%F'
> 
> because hardware and tracepoint events have different sample type.
> 
> With the patch applied the record command displays
> "Non matching sample_type" message and exits.

That is way too cryptic :-\

What is that makes the sample type not match in this case? Can we make
it match instead?

This is something to be properly fixed by _allowing_ non matching sample
types, the evsel/evlist abstractions are getting we close but not there
yet, multiple files in a perf.data/ directory are needed.

Till then can you check if we can make it match or else just say that
the mix of events is not yet supported, please collect them in different
sessions or something along those lines?

- Arnaldo

Re: [PATCH] perf tools: Exit recording if events have non matching sample type

[David Ahern]

On 09/29/2011 04:05 PM, Arnaldo Carvalho de Melo wrote:
> Em Thu, Sep 29, 2011 at 06:01:08PM +0200, Jiri Olsa escreveu:
>> The event processing relies on all events having the same sample_type.
>>
>> This is being checked when the session is being opened read only.
>> It also needs to be checked when we do record, since events could be
>> read during processing build IDs at the end of the record command.
>>
>> If we process events with different sample_type the processing might
>> skip some events or hang.
>>
>> Following command hangs on my setup:
>>   ./perf record -o perf.data -e ext4:ext4_mb_new_group_pa \
>>    -e LLC-loads -- date '+%F'
>>
>> because hardware and tracepoint events have different sample type.
>>
>> With the patch applied the record command displays
>> "Non matching sample_type" message and exits.
> 
> That is way too cryptic :-\
> 
> What is that makes the sample type not match in this case? Can we make
> it match instead?
> 
> This is something to be properly fixed by _allowing_ non matching sample
> types, the evsel/evlist abstractions are getting we close but not there
> yet, multiple files in a perf.data/ directory are needed.

>From what I can see sample_type has to be the same for all samples:
https://lkml.org/lkml/2011/8/15/6

Re: [PATCH] perf tools: Exit recording if events have non matching sample type

[Jiri Olsa]

On Thu, Sep 29, 2011 at 07:05:48PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Sep 29, 2011 at 06:01:08PM +0200, Jiri Olsa escreveu:
> > The event processing relies on all events having the same sample_type.
> > 
> > This is being checked when the session is being opened read only.
> > It also needs to be checked when we do record, since events could be
> > read during processing build IDs at the end of the record command.
> > 
> > If we process events with different sample_type the processing might
> > skip some events or hang.
> > 
> > Following command hangs on my setup:
> >   ./perf record -o perf.data -e ext4:ext4_mb_new_group_pa \
> >    -e LLC-loads -- date '+%F'
> > 
> > because hardware and tracepoint events have different sample type.
> > 
> > With the patch applied the record command displays
> > "Non matching sample_type" message and exits.
> 
> That is way too cryptic :-\
> 
> What is that makes the sample type not match in this case? Can we make
> it match instead?
> 
> This is something to be properly fixed by _allowing_ non matching sample
> types, the evsel/evlist abstractions are getting we close but not there
> yet, multiple files in a perf.data/ directory are needed.
> 
> Till then can you check if we can make it match or else just say that
> the mix of events is not yet supported, please collect them in different
> sessions or something along those lines?
> 
> - Arnaldo
hi,
thanks for comments..
I just was this got in the tip tree git://tesla.tglx.de/git/linux-2.6-tip

perf tools: Exit record if events have non matching sample type
commit 6f9a4869e15de8af2fd715af876bc3e6b00757a7
Author: Jiri Olsa <jolsa@redhat.com>
Date:   Thu Sep 29 19:49:50 2011 +0200

I've read the email trail.. I'll check the sources and try to come up
with something.. ;)

thanks,
jirka

Re: [PATCH] perf tools: Exit recording if events have non matching sample type

[Arnaldo Carvalho de Melo]

Em Mon, Oct 03, 2011 at 12:22:35PM +0200, Jiri Olsa escreveu:
> On Thu, Sep 29, 2011 at 07:05:48PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Thu, Sep 29, 2011 at 06:01:08PM +0200, Jiri Olsa escreveu:
> > > The event processing relies on all events having the same sample_type.
> > > 
> > > This is being checked when the session is being opened read only.
> > > It also needs to be checked when we do record, since events could be
> > > read during processing build IDs at the end of the record command.
> > > 
> > > If we process events with different sample_type the processing might
> > > skip some events or hang.
> > > 
> > > Following command hangs on my setup:
> > >   ./perf record -o perf.data -e ext4:ext4_mb_new_group_pa \
> > >    -e LLC-loads -- date '+%F'
> > > 
> > > because hardware and tracepoint events have different sample type.
> > > 
> > > With the patch applied the record command displays
> > > "Non matching sample_type" message and exits.
> > 
> > That is way too cryptic :-\
> > 
> > What is that makes the sample type not match in this case? Can we make
> > it match instead?
> > 
> > This is something to be properly fixed by _allowing_ non matching sample
> > types, the evsel/evlist abstractions are getting we close but not there
> > yet, multiple files in a perf.data/ directory are needed.
> > 
> > Till then can you check if we can make it match or else just say that
> > the mix of events is not yet supported, please collect them in different
> > sessions or something along those lines?
> > 
> > - Arnaldo
> hi,
> thanks for comments..
> I just was this got in the tip tree git://tesla.tglx.de/git/linux-2.6-tip
> 
> perf tools: Exit record if events have non matching sample type
> commit 6f9a4869e15de8af2fd715af876bc3e6b00757a7
> Author: Jiri Olsa <jolsa@redhat.com>
> Date:   Thu Sep 29 19:49:50 2011 +0200
> 
> I've read the email trail.. I'll check the sources and try to come up
> with something.. ;)

Couldn't understand, will check if the patch was already applied, if so,
why bother commenting, right? :-)

- Arnaldo

Re: [PATCH] perf tools: Exit recording if events have non matching sample type

[Jiri Olsa]

On Mon, Oct 03, 2011 at 01:27:04PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Oct 03, 2011 at 12:22:35PM +0200, Jiri Olsa escreveu:
> > On Thu, Sep 29, 2011 at 07:05:48PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Thu, Sep 29, 2011 at 06:01:08PM +0200, Jiri Olsa escreveu:
> > > > The event processing relies on all events having the same sample_type.
> > > > 
> > > > This is being checked when the session is being opened read only.
> > > > It also needs to be checked when we do record, since events could be
> > > > read during processing build IDs at the end of the record command.
> > > > 
> > > > If we process events with different sample_type the processing might
> > > > skip some events or hang.
> > > > 
> > > > Following command hangs on my setup:
> > > >   ./perf record -o perf.data -e ext4:ext4_mb_new_group_pa \
> > > >    -e LLC-loads -- date '+%F'
> > > > 
> > > > because hardware and tracepoint events have different sample type.
> > > > 
> > > > With the patch applied the record command displays
> > > > "Non matching sample_type" message and exits.
> > > 
> > > That is way too cryptic :-\
> > > 
> > > What is that makes the sample type not match in this case? Can we make
> > > it match instead?
> > > 
> > > This is something to be properly fixed by _allowing_ non matching sample
> > > types, the evsel/evlist abstractions are getting we close but not there
> > > yet, multiple files in a perf.data/ directory are needed.
> > > 
> > > Till then can you check if we can make it match or else just say that
> > > the mix of events is not yet supported, please collect them in different
> > > sessions or something along those lines?
> > > 
> > > - Arnaldo
> > hi,
> > thanks for comments..
> > I just was this got in the tip tree git://tesla.tglx.de/git/linux-2.6-tip
> > 
> > perf tools: Exit record if events have non matching sample type
> > commit 6f9a4869e15de8af2fd715af876bc3e6b00757a7
> > Author: Jiri Olsa <jolsa@redhat.com>
> > Date:   Thu Sep 29 19:49:50 2011 +0200
> > 
> > I've read the email trail.. I'll check the sources and try to come up
> > with something.. ;)
> 
> Couldn't understand, will check if the patch was already applied, if so,
> why bother commenting, right? :-)

I meant solving the 'sample_type issue' some other 'better' way,
probably along the lines you guys suggested.. ;)

jirka

Re: [PATCH] perf tools: Exit recording if events have non matching sample type

[Jiri Olsa]

On Mon, Oct 03, 2011 at 01:27:04PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Oct 03, 2011 at 12:22:35PM +0200, Jiri Olsa escreveu:
> > On Thu, Sep 29, 2011 at 07:05:48PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Thu, Sep 29, 2011 at 06:01:08PM +0200, Jiri Olsa escreveu:
> > > > The event processing relies on all events having the same sample_type.
> > > > 
> > > > This is being checked when the session is being opened read only.
> > > > It also needs to be checked when we do record, since events could be
> > > > read during processing build IDs at the end of the record command.
> > > > 
> > > > If we process events with different sample_type the processing might
> > > > skip some events or hang.
> > > > 
> > > > Following command hangs on my setup:
> > > >   ./perf record -o perf.data -e ext4:ext4_mb_new_group_pa \
> > > >    -e LLC-loads -- date '+%F'
> > > > 
> > > > because hardware and tracepoint events have different sample type.
> > > > 
> > > > With the patch applied the record command displays
> > > > "Non matching sample_type" message and exits.
> > > 
> > > That is way too cryptic :-\
> > > 
> > > What is that makes the sample type not match in this case? Can we make
> > > it match instead?
> > > 
> > > This is something to be properly fixed by _allowing_ non matching sample
> > > types, the evsel/evlist abstractions are getting we close but not there
> > > yet, multiple files in a perf.data/ directory are needed.
> > > 
> > > Till then can you check if we can make it match or else just say that
> > > the mix of events is not yet supported, please collect them in different
> > > sessions or something along those lines?
> > > 
> > > - Arnaldo
> > hi,
> > thanks for comments..
> > I just was this got in the tip tree git://tesla.tglx.de/git/linux-2.6-tip
> > 
> > perf tools: Exit record if events have non matching sample type
> > commit 6f9a4869e15de8af2fd715af876bc3e6b00757a7
> > Author: Jiri Olsa <jolsa@redhat.com>
> > Date:   Thu Sep 29 19:49:50 2011 +0200
> > 
> > I've read the email trail.. I'll check the sources and try to come up
> > with something.. ;)
> 
> Couldn't understand, will check if the patch was already applied, if so,
> why bother commenting, right? :-)

ops, I mixed my branches, the change was not commited yet

sry, jirka

RE: [PATCH] perf tools: Exit recording if events have non matching sample type

[Xu, Anhua]

I installed the latest kernel and found without such patch( not sure if other patches were added besides this one). That following command works fine.
perf record -o perf.data -e ext3:ext3_request_inode -e LLC-loads -- date '+%F'

-----Original Message-----
From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of Jiri Olsa
Sent: Friday, September 30, 2011 12:01 AM
To: acme@redhat.com; a.p.zijlstra@chello.nl; mingo@elte.hu; paulus@samba.org
Cc: linux-kernel@vger.kernel.org; Jiri Olsa
Subject: [PATCH] perf tools: Exit recording if events have non matching sample type

The event processing relies on all events having the same sample_type.

This is being checked when the session is being opened read only.
It also needs to be checked when we do record, since events could be
read during processing build IDs at the end of the record command.

If we process events with different sample_type the processing might
skip some events or hang.

Following command hangs on my setup:
  ./perf record -o perf.data -e ext4:ext4_mb_new_group_pa \
   -e LLC-loads -- date '+%F'

because hardware and tracepoint events have different sample type.

With the patch applied the record command displays
"Non matching sample_type" message and exits.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/builtin-record.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index f4c3fbe..71e2c4e 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -832,6 +832,12 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
 		goto out_symbol_exit;
 	}
 
+	if (!perf_evlist__valid_sample_type(evsel_list)) {
+		pr_err("Non matching sample_type\n");
+		err = -EINVAL;
+		goto out_symbol_exit;
+	}
+
 	if (target_pid != -1)
 		target_tid = target_pid;
 
-- 
1.7.4

--
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/

Re: [PATCH] perf tools: Exit recording if events have non matching sample type

[Jiri Olsa]

On Fri, Oct 21, 2011 at 03:18:31PM +0800, Xu, Anhua wrote:
> I installed the latest kernel and found without such patch( not sure if other patches were added besides this one). That following command works fine.
> perf record -o perf.data -e ext3:ext3_request_inode -e LLC-loads -- date '+%F'
hi,
was there any ext3 event stored? The issue should be hit when
processing stored events.. either in the atexit code or during
report.

Following command freezes for me on 3.1.0-rc10-tip+:
./perf record -o perf.data -e ext4:ext4_request_inode -e LLC-loads -- date '+%F'


wbr,
jirka

> 
> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of Jiri Olsa
> Sent: Friday, September 30, 2011 12:01 AM
> To: acme@redhat.com; a.p.zijlstra@chello.nl; mingo@elte.hu; paulus@samba.org
> Cc: linux-kernel@vger.kernel.org; Jiri Olsa
> Subject: [PATCH] perf tools: Exit recording if events have non matching sample type
> 
> The event processing relies on all events having the same sample_type.
> 
> This is being checked when the session is being opened read only.
> It also needs to be checked when we do record, since events could be
> read during processing build IDs at the end of the record command.
> 
> If we process events with different sample_type the processing might
> skip some events or hang.
> 
> Following command hangs on my setup:
>   ./perf record -o perf.data -e ext4:ext4_mb_new_group_pa \
>    -e LLC-loads -- date '+%F'
> 
> because hardware and tracepoint events have different sample type.
> 
> With the patch applied the record command displays
> "Non matching sample_type" message and exits.
> 
> Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> ---
>  tools/perf/builtin-record.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index f4c3fbe..71e2c4e 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -832,6 +832,12 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
>  		goto out_symbol_exit;
>  	}
>  
> +	if (!perf_evlist__valid_sample_type(evsel_list)) {
> +		pr_err("Non matching sample_type\n");
> +		err = -EINVAL;
> +		goto out_symbol_exit;
> +	}
> +
>  	if (target_pid != -1)
>  		target_tid = target_pid;
>  
> -- 
> 1.7.4
> 
> --
> 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/

Re: [PATCH] perf tools: Exit recording if events have non matching sample type

[David Ahern]

On 10/21/2011 02:10 AM, Jiri Olsa wrote:
> On Fri, Oct 21, 2011 at 03:18:31PM +0800, Xu, Anhua wrote:
>> I installed the latest kernel and found without such patch( not sure if other patches were added besides this one). That following command works fine.
>> perf record -o perf.data -e ext3:ext3_request_inode -e LLC-loads -- date '+%F'
> hi,
> was there any ext3 event stored? The issue should be hit when
> processing stored events.. either in the atexit code or during
> report.
> 
> Following command freezes for me on 3.1.0-rc10-tip+:
> ./perf record -o perf.data -e ext4:ext4_request_inode -e LLC-loads -- date '+%F'
> 

With -B and it doesn't for me. Without the -B argument the backtrace
shows it stuck here:

#0  0x0000000000460d91 in fetch_mmaped_event (session=0x13dd510,
head=3464, mmap_size=21112,
    buf=0x7fbd1d016000 "") at util/session.c:988
#1  0x0000000000461074 in __perf_session__process_events
(session=0x13dd510, data_offset=456,
    data_size=20656, file_size=21112, ops=0x77ad40) at util/session.c:1063
#2  0x000000000041dcc4 in process_buildids () at builtin-record.c:377
#3  0x000000000041dd2e in atexit_header () at builtin-record.c:388
#4  0x0000003e96e38991 in __run_exit_handlers () from /lib64/libc.so.6
#5  0x0000003e96e38a15 in exit () from /lib64/libc.so.6
#6  0x00000000004126f8 in handle_internal_command (argc=11,
argv=0x7fffade3f3d0) at perf.c:358
#7  0x000000000041283d in run_argv (argcp=0x7fffade3f2bc,
argv=0x7fffade3f2b0) at perf.c:402
#8  0x0000000000412b24 in main (argc=11, argv=0x7fffade3f3d0) at perf.c:512


It's stuck in a loop on this:
remap:
	buf = mmap(NULL, mmap_size, mmap_prot, mmap_flags, session->fd,
		   file_offset);
	if (buf == MAP_FAILED) {
		pr_err("failed to mmap file\n");
		err = -errno;
		goto out_err;
	}
	mmaps[map_idx] = buf;
	map_idx = (map_idx + 1) & (ARRAY_SIZE(mmaps) - 1);
	file_pos = file_offset + head;

more:
	event = fetch_mmaped_event(session, head, mmap_size, buf);
	if (!event) {
		if (mmaps[map_idx]) {
			munmap(mmaps[map_idx], mmap_size);
			mmaps[map_idx] = NULL;
		}

		page_offset = page_size * (head / page_size);
		file_offset += page_offset;
		head -= page_offset;
		goto remap;
	}

head < page_size, page_offset stays 0.

David


> 
> wbr,
> jirka
> 
>>
>> -----Original Message-----
>> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of Jiri Olsa
>> Sent: Friday, September 30, 2011 12:01 AM
>> To: acme@redhat.com; a.p.zijlstra@chello.nl; mingo@elte.hu; paulus@samba.org
>> Cc: linux-kernel@vger.kernel.org; Jiri Olsa
>> Subject: [PATCH] perf tools: Exit recording if events have non matching sample type
>>
>> The event processing relies on all events having the same sample_type.
>>
>> This is being checked when the session is being opened read only.
>> It also needs to be checked when we do record, since events could be
>> read during processing build IDs at the end of the record command.
>>
>> If we process events with different sample_type the processing might
>> skip some events or hang.
>>
>> Following command hangs on my setup:
>>   ./perf record -o perf.data -e ext4:ext4_mb_new_group_pa \
>>    -e LLC-loads -- date '+%F'
>>
>> because hardware and tracepoint events have different sample type.
>>
>> With the patch applied the record command displays
>> "Non matching sample_type" message and exits.
>>
>> Signed-off-by: Jiri Olsa <jolsa@redhat.com>
>> ---
>>  tools/perf/builtin-record.c |    6 ++++++
>>  1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
>> index f4c3fbe..71e2c4e 100644
>> --- a/tools/perf/builtin-record.c
>> +++ b/tools/perf/builtin-record.c
>> @@ -832,6 +832,12 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
>>  		goto out_symbol_exit;
>>  	}
>>  
>> +	if (!perf_evlist__valid_sample_type(evsel_list)) {
>> +		pr_err("Non matching sample_type\n");
>> +		err = -EINVAL;
>> +		goto out_symbol_exit;
>> +	}
>> +
>>  	if (target_pid != -1)
>>  		target_tid = target_pid;
>>  
>> -- 
>> 1.7.4
>>
>> --
>> 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/
> --
> 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/

Re: [PATCH] perf tools: Exit recording if events have non matching sample type

[David Ahern]

On 10/21/2011 08:16 AM, David Ahern wrote:
> 
> 
> On 10/21/2011 02:10 AM, Jiri Olsa wrote:
>> On Fri, Oct 21, 2011 at 03:18:31PM +0800, Xu, Anhua wrote:
>>> I installed the latest kernel and found without such patch( not sure if other patches were added besides this one). That following command works fine.
>>> perf record -o perf.data -e ext3:ext3_request_inode -e LLC-loads -- date '+%F'
>> hi,
>> was there any ext3 event stored? The issue should be hit when
>> processing stored events.. either in the atexit code or during
>> report.
>>
>> Following command freezes for me on 3.1.0-rc10-tip+:
>> ./perf record -o perf.data -e ext4:ext4_request_inode -e LLC-loads -- date '+%F'
>>
> 
> With -B and it doesn't for me. Without the -B argument the backtrace
> shows it stuck here:

While it doesn't hang with the -B, the data file is worthless --
non-matching sample type. :-)

David

Re: [PATCH] perf tools: Exit recording if events have non matching sample type

[Jiri Olsa]

On Fri, Oct 21, 2011 at 08:16:14AM -0600, David Ahern wrote:
> 
> 
> On 10/21/2011 02:10 AM, Jiri Olsa wrote:
> > On Fri, Oct 21, 2011 at 03:18:31PM +0800, Xu, Anhua wrote:
> >> I installed the latest kernel and found without such patch( not sure if other patches were added besides this one). That following command works fine.
> >> perf record -o perf.data -e ext3:ext3_request_inode -e LLC-loads -- date '+%F'
> > hi,
> > was there any ext3 event stored? The issue should be hit when
> > processing stored events.. either in the atexit code or during
> > report.
> > 
> > Following command freezes for me on 3.1.0-rc10-tip+:
> > ./perf record -o perf.data -e ext4:ext4_request_inode -e LLC-loads -- date '+%F'
> > 
> 
> With -B and it doesn't for me. Without the -B argument the backtrace
> shows it stuck here:

yep, it get's stuck in processig build ids.

All the events are traversed and processing fails with event which does
not support configured sample_type.. the event is skipped, but with no
idea about the actual event length (not sure about the details right now),
thus screwing processing of following events.. ending up in infinite loop ;)

jirka

> 
> #0  0x0000000000460d91 in fetch_mmaped_event (session=0x13dd510,
> head=3464, mmap_size=21112,
>     buf=0x7fbd1d016000 "") at util/session.c:988
> #1  0x0000000000461074 in __perf_session__process_events
> (session=0x13dd510, data_offset=456,
>     data_size=20656, file_size=21112, ops=0x77ad40) at util/session.c:1063
> #2  0x000000000041dcc4 in process_buildids () at builtin-record.c:377
> #3  0x000000000041dd2e in atexit_header () at builtin-record.c:388
> #4  0x0000003e96e38991 in __run_exit_handlers () from /lib64/libc.so.6
> #5  0x0000003e96e38a15 in exit () from /lib64/libc.so.6
> #6  0x00000000004126f8 in handle_internal_command (argc=11,
> argv=0x7fffade3f3d0) at perf.c:358
> #7  0x000000000041283d in run_argv (argcp=0x7fffade3f2bc,
> argv=0x7fffade3f2b0) at perf.c:402
> #8  0x0000000000412b24 in main (argc=11, argv=0x7fffade3f3d0) at perf.c:512
> 
> 
> It's stuck in a loop on this:
> remap:
> 	buf = mmap(NULL, mmap_size, mmap_prot, mmap_flags, session->fd,
> 		   file_offset);
> 	if (buf == MAP_FAILED) {
> 		pr_err("failed to mmap file\n");
> 		err = -errno;
> 		goto out_err;
> 	}
> 	mmaps[map_idx] = buf;
> 	map_idx = (map_idx + 1) & (ARRAY_SIZE(mmaps) - 1);
> 	file_pos = file_offset + head;
> 
> more:
> 	event = fetch_mmaped_event(session, head, mmap_size, buf);
> 	if (!event) {
> 		if (mmaps[map_idx]) {
> 			munmap(mmaps[map_idx], mmap_size);
> 			mmaps[map_idx] = NULL;
> 		}
> 
> 		page_offset = page_size * (head / page_size);
> 		file_offset += page_offset;
> 		head -= page_offset;
> 		goto remap;
> 	}
> 
> head < page_size, page_offset stays 0.
> 
> David
> 
> 
> > 
> > wbr,
> > jirka
> > 
> >>
> >> -----Original Message-----
> >> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of Jiri Olsa
> >> Sent: Friday, September 30, 2011 12:01 AM
> >> To: acme@redhat.com; a.p.zijlstra@chello.nl; mingo@elte.hu; paulus@samba.org
> >> Cc: linux-kernel@vger.kernel.org; Jiri Olsa
> >> Subject: [PATCH] perf tools: Exit recording if events have non matching sample type
> >>
> >> The event processing relies on all events having the same sample_type.
> >>
> >> This is being checked when the session is being opened read only.
> >> It also needs to be checked when we do record, since events could be
> >> read during processing build IDs at the end of the record command.
> >>
> >> If we process events with different sample_type the processing might
> >> skip some events or hang.
> >>
> >> Following command hangs on my setup:
> >>   ./perf record -o perf.data -e ext4:ext4_mb_new_group_pa \
> >>    -e LLC-loads -- date '+%F'
> >>
> >> because hardware and tracepoint events have different sample type.
> >>
> >> With the patch applied the record command displays
> >> "Non matching sample_type" message and exits.
> >>
> >> Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> >> ---
> >>  tools/perf/builtin-record.c |    6 ++++++
> >>  1 files changed, 6 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> >> index f4c3fbe..71e2c4e 100644
> >> --- a/tools/perf/builtin-record.c
> >> +++ b/tools/perf/builtin-record.c
> >> @@ -832,6 +832,12 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
> >>  		goto out_symbol_exit;
> >>  	}
> >>  
> >> +	if (!perf_evlist__valid_sample_type(evsel_list)) {
> >> +		pr_err("Non matching sample_type\n");
> >> +		err = -EINVAL;
> >> +		goto out_symbol_exit;
> >> +	}
> >> +
> >>  	if (target_pid != -1)
> >>  		target_tid = target_pid;
> >>  
> >> -- 
> >> 1.7.4
> >>
> >> --
> >> 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/
> > --
> > 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/

Re: [PATCH] perf tools: Exit recording if events have non matching sample type

[Jiri Olsa]

On Fri, Oct 21, 2011 at 08:29:53AM -0600, David Ahern wrote:
> 
> 
> On 10/21/2011 08:16 AM, David Ahern wrote:
> > 
> > 
> > On 10/21/2011 02:10 AM, Jiri Olsa wrote:
> >> On Fri, Oct 21, 2011 at 03:18:31PM +0800, Xu, Anhua wrote:
> >>> I installed the latest kernel and found without such patch( not sure if other patches were added besides this one). That following command works fine.
> >>> perf record -o perf.data -e ext3:ext3_request_inode -e LLC-loads -- date '+%F'
> >> hi,
> >> was there any ext3 event stored? The issue should be hit when
> >> processing stored events.. either in the atexit code or during
> >> report.
> >>
> >> Following command freezes for me on 3.1.0-rc10-tip+:
> >> ./perf record -o perf.data -e ext4:ext4_request_inode -e LLC-loads -- date '+%F'
> >>
> > 
> > With -B and it doesn't for me. Without the -B argument the backtrace
> > shows it stuck here:
> 
> While it doesn't hang with the -B, the data file is worthless --
> non-matching sample type. :-)

right :) there's check when the session is opened for reading

jirka

Re: [PATCH] perf tools: Exit recording if events have non matching sample type

[Arnaldo Carvalho de Melo]

Em Tue, Oct 04, 2011 at 12:00:12PM +0200, Peter Zijlstra escreveu:
> On Mon, 2011-10-03 at 22:14 -0600, David Ahern wrote:
> > On 09/30/2011 01:39 AM, Peter Zijlstra wrote:
> > > On Thu, 2011-09-29 at 17:33 -0600, David Ahern wrote:
> > >>> For record that means we need to dump each per-sample_id mmap onto a
> > >>> separate file, i.e. use a directory, etc.

> > >> That seems like a major re-write of perf. 

> > > Well we want to go there anyway. A file per cpu stream can be a lot less
> > > overhead than one file for all cpu streams.

> > really it becomes file per cpu stream and per sample type. So a dual
> > socket, quad core with HT means 16 files per sample type. ie., the
> > number of files explodes quick.

> > I understand the allure for simplicity during the data collection. Has
> > any thought been given on management of the files in such a scenario?
> > User specifies a directory path instead of a file path - or both to
> > handle backward compatibility?

> I think the idea was a directory and a script to convert old data files
> to the new format. But it was a while ago acme and I talked about this.

I don't remember the script idea, but that would work too.

WRT major rewrites, well, as we go on developing any piece of software,
those kinds of things happen :-)

If it is to improve things, make them faster, cope with new hardware
realities, keep our jobs, so be it! ;-)

- Arnaldo

Re: [PATCH] perf tools: Exit recording if events have non matching sample type

[Peter Zijlstra]

On Mon, 2011-10-03 at 22:14 -0600, David Ahern wrote:
> On 09/30/2011 01:39 AM, Peter Zijlstra wrote:
> > On Thu, 2011-09-29 at 17:33 -0600, David Ahern wrote:
> >>> For record that means we need to dump each per-sample_id mmap onto a
> >>> separate file, i.e. use a directory, etc.
> >>
> >> That seems like a major re-write of perf. 
> > 
> > Well we want to go there anyway. A file per cpu stream can be a lot less
> > overhead than one file for all cpu streams.
> > 
> 
> really it becomes file per cpu stream and per sample type. So a dual
> socket, quad core with HT means 16 files per sample type. ie., the
> number of files explodes quick.
> 
> I understand the allure for simplicity during the data collection. Has
> any thought been given on management of the files in such a scenario?
> User specifies a directory path instead of a file path - or both to
> handle backward compatibility?

I think the idea was a directory and a script to convert old data files
to the new format. But it was a while ago acme and I talked about this.

Re: [PATCH] perf tools: Exit recording if events have non matching sample type

[David Ahern]

On 09/30/2011 01:39 AM, Peter Zijlstra wrote:
> On Thu, 2011-09-29 at 17:33 -0600, David Ahern wrote:
>>> For record that means we need to dump each per-sample_id mmap onto a
>>> separate file, i.e. use a directory, etc.
>>
>> That seems like a major re-write of perf. 
> 
> Well we want to go there anyway. A file per cpu stream can be a lot less
> overhead than one file for all cpu streams.
> 

really it becomes file per cpu stream and per sample type. So a dual
socket, quad core with HT means 16 files per sample type. ie., the
number of files explodes quick.

I understand the allure for simplicity during the data collection. Has
any thought been given on management of the files in such a scenario?
User specifies a directory path instead of a file path - or both to
handle backward compatibility?

David

Re: [PATCH] perf tools: Exit recording if events have non matching sample type

[Peter Zijlstra]

On Thu, 2011-09-29 at 17:33 -0600, David Ahern wrote:
> > For record that means we need to dump each per-sample_id mmap onto a
> > separate file, i.e. use a directory, etc.
> 
> That seems like a major re-write of perf. 

Well we want to go there anyway. A file per cpu stream can be a lot less
overhead than one file for all cpu streams.

Re: [PATCH] perf tools: Exit recording if events have non matching sample type

[David Ahern]

On 09/29/2011 08:21 PM, Arnaldo Carvalho de Melo wrote:
> Em Thu, Sep 29, 2011 at 06:58:20PM -0600, David Ahern escreveu:
>> On 09/29/2011 06:40 PM, Arnaldo Carvalho de Melo wrote:
>>> This comes from time to time:
>>>
>>> http://us.generation-nt.com/answer/bug-perf-event-sampling-buffer-format-cannot-handle-multi-event-sampling-help-202985832.html
>>>
>>> The whole point of sample_type is to ask for just what is needed for
>>> some specific event so that we reduce the per sample footprint.
>>>
>>> So we allow for multiple types of events to be on the same stream, but
>>> not for them to have just what each need, its a limitation, one that we
>>> can solve. I see no reason not to solve it :-)
>>>
>>> The evsel/evlist classes were designed to abstract away such details,
>>> i.e. perf_evlist__mmap should notice different sample_types and sort
>>> this out transparently.
>>>
>>> And then perf_evlist__mmap_read(evlist, idx), evlist->nr_mmaps should be
>>> enough to process the events :-)
>>>
>>> - Arnaldo
>>
>> An ABI change fixes the root cause of the current problem -- put an id
>> in perf_event_header. The id maps to the evsel which has the
>> sample_type. Right now you have to parse the sample to get the id to get
>> the evsel.
> 
> This id is not always needed, so it shouldn't go to perf_event_header.
> Peter, can you comment here? ;-)

I'm talking about a new 'id' that would always be needed *for samples*
-- it identifies the evsel it is associated with. The evsel has the
sample type. The sample_type is then used to parse it. Seems like a
simpler solution then having mmap's per sample type and then writing
separate files.

David

Re: [PATCH] perf tools: Exit recording if events have non matching sample type

[Arnaldo Carvalho de Melo]

Em Thu, Sep 29, 2011 at 06:58:20PM -0600, David Ahern escreveu:
> On 09/29/2011 06:40 PM, Arnaldo Carvalho de Melo wrote:
> > This comes from time to time:
> > 
> > http://us.generation-nt.com/answer/bug-perf-event-sampling-buffer-format-cannot-handle-multi-event-sampling-help-202985832.html
> > 
> > The whole point of sample_type is to ask for just what is needed for
> > some specific event so that we reduce the per sample footprint.
> > 
> > So we allow for multiple types of events to be on the same stream, but
> > not for them to have just what each need, its a limitation, one that we
> > can solve. I see no reason not to solve it :-)
> > 
> > The evsel/evlist classes were designed to abstract away such details,
> > i.e. perf_evlist__mmap should notice different sample_types and sort
> > this out transparently.
> > 
> > And then perf_evlist__mmap_read(evlist, idx), evlist->nr_mmaps should be
> > enough to process the events :-)
> > 
> > - Arnaldo
> 
> An ABI change fixes the root cause of the current problem -- put an id
> in perf_event_header. The id maps to the evsel which has the
> sample_type. Right now you have to parse the sample to get the id to get
> the evsel.

This id is not always needed, so it shouldn't go to perf_event_header.
Peter, can you comment here? ;-)

- Arnaldo

Re: [PATCH] perf tools: Exit recording if events have non matching sample type

[David Ahern]

On 09/29/2011 06:40 PM, Arnaldo Carvalho de Melo wrote:
> Em Thu, Sep 29, 2011 at 05:33:42PM -0600, David Ahern escreveu:
>> On 09/29/2011 05:06 PM, Arnaldo Carvalho de Melo wrote:
>>>> >From what I can see sample_type has to be the same for all samples:
>>>> https://lkml.org/lkml/2011/8/15/6
> 
>>> We could add a PERF_SAMPLE_ID2 that if present would be guaranteed
>>> to be the last, or we can, as PeterZ prefers/suggests, to use one
>>> mmap per sample id, then we know that if we're getting it on that
>>> mmap, it has that sample_type.
> 
>>> For record that means we need to dump each per-sample_id mmap onto a
>>> separate file, i.e. use a directory, etc.
>  
>> That seems like a major re-write of perf. Why not keep the existing
>> restriction and have users who want different sample_types run
>> multiple perf commands?
>  
>> What's to be gained by refactoring the code to support multiple
>> sample_types in a single command invocation?
> 
> This comes from time to time:
> 
> http://us.generation-nt.com/answer/bug-perf-event-sampling-buffer-format-cannot-handle-multi-event-sampling-help-202985832.html
> 
> The whole point of sample_type is to ask for just what is needed for
> some specific event so that we reduce the per sample footprint.
> 
> So we allow for multiple types of events to be on the same stream, but
> not for them to have just what each need, its a limitation, one that we
> can solve. I see no reason not to solve it :-)
> 
> The evsel/evlist classes were designed to abstract away such details,
> i.e. perf_evlist__mmap should notice different sample_types and sort
> this out transparently.
> 
> And then perf_evlist__mmap_read(evlist, idx), evlist->nr_mmaps should be
> enough to process the events :-)
> 
> - Arnaldo

An ABI change fixes the root cause of the current problem -- put an id
in perf_event_header. The id maps to the evsel which has the
sample_type. Right now you have to parse the sample to get the id to get
the evsel.

With that change I have a patch that (mostly?) covers the rest of perf
userspace to handle sample_type per evsel.

David

Re: [PATCH] perf tools: Exit recording if events have non matching sample type

[Arnaldo Carvalho de Melo]

Em Thu, Sep 29, 2011 at 05:33:42PM -0600, David Ahern escreveu:
> On 09/29/2011 05:06 PM, Arnaldo Carvalho de Melo wrote:
> >> >From what I can see sample_type has to be the same for all samples:
> >> https://lkml.org/lkml/2011/8/15/6

> > We could add a PERF_SAMPLE_ID2 that if present would be guaranteed
> > to be the last, or we can, as PeterZ prefers/suggests, to use one
> > mmap per sample id, then we know that if we're getting it on that
> > mmap, it has that sample_type.

> > For record that means we need to dump each per-sample_id mmap onto a
> > separate file, i.e. use a directory, etc.
 
> That seems like a major re-write of perf. Why not keep the existing
> restriction and have users who want different sample_types run
> multiple perf commands?
 
> What's to be gained by refactoring the code to support multiple
> sample_types in a single command invocation?

This comes from time to time:

http://us.generation-nt.com/answer/bug-perf-event-sampling-buffer-format-cannot-handle-multi-event-sampling-help-202985832.html

The whole point of sample_type is to ask for just what is needed for
some specific event so that we reduce the per sample footprint.

So we allow for multiple types of events to be on the same stream, but
not for them to have just what each need, its a limitation, one that we
can solve. I see no reason not to solve it :-)

The evsel/evlist classes were designed to abstract away such details,
i.e. perf_evlist__mmap should notice different sample_types and sort
this out transparently.

And then perf_evlist__mmap_read(evlist, idx), evlist->nr_mmaps should be
enough to process the events :-)

- Arnaldo

Re: [PATCH] perf tools: Exit recording if events have non matching sample type

[David Ahern]

On 09/29/2011 05:06 PM, Arnaldo Carvalho de Melo wrote:
>> >From what I can see sample_type has to be the same for all samples:
>> https://lkml.org/lkml/2011/8/15/6
> 
> We could add a PERF_SAMPLE_ID2 that if present would be guaranteed to be
> the last, or we can, as PeterZ prefers/suggests, to use one mmap per
> sample id, then we know that if we're getting it on that mmap, it has
> that sample_type.
> 
> For record that means we need to dump each per-sample_id mmap onto a
> separate file, i.e. use a directory, etc.

That seems like a major re-write of perf. Why not keep the existing
restriction and have users who want different sample_types run multiple
perf commands?

What's to be gained by refactoring the code to support multiple
sample_types in a single command invocation?

David

Re: [PATCH] perf tools: Exit recording if events have non matching sample type

[Arnaldo Carvalho de Melo]

Em Thu, Sep 29, 2011 at 04:55:23PM -0600, David Ahern escreveu:
> On 09/29/2011 04:05 PM, Arnaldo Carvalho de Melo wrote:
> > Em Thu, Sep 29, 2011 at 06:01:08PM +0200, Jiri Olsa escreveu:
> >> Following command hangs on my setup:
> >>   ./perf record -o perf.data -e ext4:ext4_mb_new_group_pa \
> >>    -e LLC-loads -- date '+%F'
> >>
> >> because hardware and tracepoint events have different sample type.
> >>
> >> With the patch applied the record command displays
> >> "Non matching sample_type" message and exits.
> > 
> > That is way too cryptic :-\
> > 
> > What is that makes the sample type not match in this case? Can we make
> > it match instead?
> > 
> > This is something to be properly fixed by _allowing_ non matching sample
> > types, the evsel/evlist abstractions are getting we close but not there
> > yet, multiple files in a perf.data/ directory are needed.
> 
> >From what I can see sample_type has to be the same for all samples:
> https://lkml.org/lkml/2011/8/15/6

We could add a PERF_SAMPLE_ID2 that if present would be guaranteed to be
the last, or we can, as PeterZ prefers/suggests, to use one mmap per
sample id, then we know that if we're getting it on that mmap, it has
that sample_type.

For record that means we need to dump each per-sample_id mmap onto a
separate file, i.e. use a directory, etc.

But what I suggested as a short term solution was to check what is the
difference in the above case and then make them use a single sample
type, i.e. bow to the current restriction.

- Arnaldo