by Stephane Eranian

[permalink] [raw]

Subject: Re: [PATCH V8 10/15] perf: make events stream always parsable

On Thu, Jul 18, 2013 at 12:42 PM, Adrian Hunter <[email protected]> wrote:
> The event stream is not always parsable because the format of a sample
> is dependent on the sample_type of the selected event. When there
> is more than one selected event and the sample_types are not the
> same then parsing becomes problematic. A sample can be matched to its
> selected event using the ID that is allocated when the event is opened.
> Unfortunately, to get the ID from the sample means first parsing it.
>
> This patch adds a new sample format bit PERF_SAMPLE_IDENTIFER that puts
> the ID at a fixed position so that the ID can be retrieved without
> parsing the sample. For sample events, that is the first position
> immediately after the header. For non-sample events, that is the last
> position.
>

I think you should add to your description a paragraph explaining that
the reason
this solves the problem is because you always get the event description BEFORE
you get any sample records. Otherwise, how would you know you're dealing with
a perf.data file that is using PERF_SAMPLE_IDENTIFIER? And that holds for
both file and pipe modes.

> Signed-off-by: Adrian Hunter <[email protected]>
> ---
> include/uapi/linux/perf_event.h | 27 ++++++++++++++++++++-------
> kernel/events/core.c | 11 ++++++++++-
> 2 files changed, 30 insertions(+), 8 deletions(-)
>
> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> index 876a18c..71f1ad3 100644
> --- a/include/uapi/linux/perf_event.h
> +++ b/include/uapi/linux/perf_event.h
> @@ -134,8 +134,9 @@ enum perf_event_sample_format {
> PERF_SAMPLE_STACK_USER = 1U << 13,
> PERF_SAMPLE_WEIGHT = 1U << 14,
> PERF_SAMPLE_DATA_SRC = 1U << 15,
> + PERF_SAMPLE_IDENTIFIER = 1U << 16,
>
> - PERF_SAMPLE_MAX = 1U << 16, /* non-ABI */
> + PERF_SAMPLE_MAX = 1U << 17, /* non-ABI */
> };
>
> /*
> @@ -471,12 +472,12 @@ enum perf_event_type {
> /*
> * If perf_event_attr.sample_id_all is set then all event types will
> * have the sample_type selected fields related to where/when
> - * (identity) an event took place (TID, TIME, ID, CPU, STREAM_ID)
> - * described in PERF_RECORD_SAMPLE below, it will be stashed just after
> - * the perf_event_header and the fields already present for the existing
> - * fields, i.e. at the end of the payload. That way a newer perf.data
> - * file will be supported by older perf tools, with these new optional
> - * fields being ignored.
> + * (identity) an event took place (TID, TIME, ID, STREAM_ID, CPU,
> + * IDENTIFIER) described in PERF_RECORD_SAMPLE below, it will be stashed
> + * just after the perf_event_header and the fields already present for
> + * the existing fields, i.e. at the end of the payload. That way a newer
> + * perf.data file will be supported by older perf tools, with these new
> + * optional fields being ignored.
> *
> * struct sample_id {
> * { u32 pid, tid; } && PERF_SAMPLE_TID
> @@ -484,7 +485,12 @@ enum perf_event_type {
> * { u64 id; } && PERF_SAMPLE_ID
> * { u64 stream_id;} && PERF_SAMPLE_STREAM_ID
> * { u32 cpu, res; } && PERF_SAMPLE_CPU
> + * { u64 id; } && PERF_SAMPLE_IDENTIFIER
> * } && perf_event_attr.sample_id_all
> + *
> + * Note that PERF_SAMPLE_IDENTIFIER duplicates PERF_SAMPLE_ID. The
> + * advantage of PERF_SAMPLE_IDENTIFIER is that its position is fixed
> + * relative to header.size.
> */
>
> /*
> @@ -574,6 +580,13 @@ enum perf_event_type {
> * struct {
> * struct perf_event_header header;
> *
> + * #
> + * # Note that PERF_SAMPLE_IDENTIFIER duplicates PERF_SAMPLE_ID.
> + * # The advantage of PERF_SAMPLE_IDENTIFIER is that its position
> + * # is fixed relative to header.
> + * #
> + *
> + * { u64 id; } && PERF_SAMPLE_IDENTIFIER
> * { u64 ip; } && PERF_SAMPLE_IP
> * { u32 pid, tid; } && PERF_SAMPLE_TID
> * { u64 time; } && PERF_SAMPLE_TIME
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index c9ef899..91c241d 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -1216,6 +1216,9 @@ static void perf_event__id_header_size(struct perf_event *event)
> if (sample_type & PERF_SAMPLE_TIME)
> size += sizeof(data->time);
>
> + if (sample_type & PERF_SAMPLE_IDENTIFIER)
> + size += sizeof(data->id);
> +
> if (sample_type & PERF_SAMPLE_ID)
> size += sizeof(data->id);
>
> @@ -4251,7 +4254,7 @@ static void __perf_event_header__init_id(struct perf_event_header *header,
> if (sample_type & PERF_SAMPLE_TIME)
> data->time = perf_clock();
>
> - if (sample_type & PERF_SAMPLE_ID)
> + if (sample_type & (PERF_SAMPLE_ID | PERF_SAMPLE_IDENTIFIER))
> data->id = primary_event_id(event);
>
> if (sample_type & PERF_SAMPLE_STREAM_ID)
> @@ -4290,6 +4293,9 @@ static void __perf_event__output_id_sample(struct perf_output_handle *handle,
>
> if (sample_type & PERF_SAMPLE_CPU)
> perf_output_put(handle, data->cpu_entry);
> +
> + if (sample_type & PERF_SAMPLE_IDENTIFIER)
> + perf_output_put(handle, data->id);
> }
>
> void perf_event__output_id_sample(struct perf_event *event,
> @@ -4402,6 +4408,9 @@ void perf_output_sample(struct perf_output_handle *handle,
>
> perf_output_put(handle, *header);
>
> + if (sample_type & PERF_SAMPLE_IDENTIFIER)
> + perf_output_put(handle, data->id);
> +
> if (sample_type & PERF_SAMPLE_IP)
> perf_output_put(handle, data->ip);
>
> --
> 1.7.11.7
>

2013-07-29 22:19:18

by David Ahern

[permalink] [raw]

Subject: Re: [PATCH V8 04/15] perf tools: change machine__findnew_thread() to set thread pid

On 7/28/13 4:43 AM, Adrian Hunter wrote:

>>> - thread = machine__findnew_thread(machine, event->ip.pid);
>>> + thread = machine__findnew_thread(machine, event->ip.pid,
>>> event->ip.pid);
>>
>> event->ip.tid? at a minimum current functionality is retained with
>> machine__findnew_thread(machine, 0, event->ip.pid)
>
> perf tools records mmap information against the process's main thread
> for which pid == tid. So the correct values are being passed here.

Yes. It's a case by case -- if you want syms use pid, otherwise use tid.
It would be less confusing if the API was improved - always take pid and
tid along with an arg that says 'want syms' or a separate function name.
But I digress.

Looking at your patch does maintain the tid element so it should be
fine. I ran a few tests with perf-script and the output is the same
after the patch, so..

Acked-by: David Ahern <[email protected]>