Re: perf script: Question: Python trace processing script contains the tid of the process in the common_pid attribute

Re: perf script: Question: Python trace processing script contains the tid of the process in the common_pid attribute

[Arnaldo Carvalho de Melo]

Em Fri, Jun 30, 2017 at 03:40:57PM -0700, Arun Kalyanasundaram escreveu:
> The handlers in the python script generated from "perf script" have an
> attribute: common_pid. This attribute contains the tid of the process
> instead of its pid. I would like to know if this is the expected behavior.
> There are no other attributes in the Python handler that provide the pid
> and knowing the process id is useful to be able to group all samples that
> belong to the same process that generated different threads.

Humm, you have:

def process_event(param_dict):
        event_attr = param_dict["attr"]
        sample     = param_dict["sample"]
        raw_buf    = param_dict["raw_buf"]
        comm       = param_dict["comm"]
        name       = param_dict["ev_name"]

And then, on sample you have (from a recent python script for processing
Intel PT samples):

def print_common_start(comm, sample, name):
       ts = sample["time"]
       cpu = sample["cpu"]
       pid = sample["pid"]
       tid = sample["tid"]
       print "%16s %5u/%-5u [%03u] %9u.%09u %7s:" % (comm, pid, tid, cpu, ts / 1000000000, ts %1000000000, name),

- Arnaldo

Re: perf script: Question: Python trace processing script contains the tid of the process in the common_pid attribute

[Arun Kalyanasundaram]

Hi Arnaldo,

Thank you for your reply.
I actually meant tracepoint event handlers: def
trace_unhandled(event_name, context, event_fields_dict)
The dict parameter contains an attribute "common_pid" which is
actually the "tid" of the thread. There are no other attributes that
contain the actual pid of the process. So, I was wondering if this is
something intentional? If not I can share a patch to fix this.

Best,
- Arun


On Sat, Jul 1, 2017 at 7:47 AM, Arnaldo Carvalho de Melo
<acme@redhat.com> wrote:
> Em Fri, Jun 30, 2017 at 03:40:57PM -0700, Arun Kalyanasundaram escreveu:
>> The handlers in the python script generated from "perf script" have an
>> attribute: common_pid. This attribute contains the tid of the process
>> instead of its pid. I would like to know if this is the expected behavior.
>> There are no other attributes in the Python handler that provide the pid
>> and knowing the process id is useful to be able to group all samples that
>> belong to the same process that generated different threads.
>
> Humm, you have:
>
> def process_event(param_dict):
>         event_attr = param_dict["attr"]
>         sample     = param_dict["sample"]
>         raw_buf    = param_dict["raw_buf"]
>         comm       = param_dict["comm"]
>         name       = param_dict["ev_name"]
>
> And then, on sample you have (from a recent python script for processing
> Intel PT samples):
>
> def print_common_start(comm, sample, name):
>        ts = sample["time"]
>        cpu = sample["cpu"]
>        pid = sample["pid"]
>        tid = sample["tid"]
>        print "%16s %5u/%-5u [%03u] %9u.%09u %7s:" % (comm, pid, tid, cpu, ts / 1000000000, ts %1000000000, name),
>
> - Arnaldo

Re: perf script: Question: Python trace processing script contains the tid of the process in the common_pid attribute

[Arnaldo Carvalho de Melo]

Em Wed, Jul 05, 2017 at 09:22:07AM -0700, Arun Kalyanasundaram escreveu:
> Hi Arnaldo,
> 
> Thank you for your reply.
> I actually meant tracepoint event handlers: def
> trace_unhandled(event_name, context, event_fields_dict)
> The dict parameter contains an attribute "common_pid" which is
> actually the "tid" of the thread. There are no other attributes that
> contain the actual pid of the process. So, I was wondering if this is
> something intentional? If not I can share a patch to fix this.

Yeah there is a problem in:

tools/perf/util/scripting-engines/trace-event-python.c

static void python_process_event(union perf_event *event,
                                 struct perf_sample *sample,
                                 struct perf_evsel *evsel,
                                 struct addr_location *al)
{
        struct tables *tables = &tables_global;

        switch (evsel->attr.type) {
        case PERF_TYPE_TRACEPOINT:
                python_process_tracepoint(sample, evsel, al);
                break;
        /* Reserve for future process_hw/sw/raw APIs */
        default:
                if (tables->db_export_mode)
                        db_export__sample(&tables->dbe, event, sample, evsel, al);
                else
                        python_process_general_event(sample, evsel, al);
        }
}

The python_process_tracepoint() thing predates
python_process_general_event(), and doesn't adds the dict with all the
perf_sample entries that python_process_general_event() passes to the
python method :-\

Both the per-tracepoint python hooks _and_ trace_unhandled() should get
that dict, is that what your patch does?

- Arnaldo
 
> Best,
> - Arun
> 
> 
> On Sat, Jul 1, 2017 at 7:47 AM, Arnaldo Carvalho de Melo
> <acme@redhat.com> wrote:
> > Em Fri, Jun 30, 2017 at 03:40:57PM -0700, Arun Kalyanasundaram escreveu:
> >> The handlers in the python script generated from "perf script" have an
> >> attribute: common_pid. This attribute contains the tid of the process
> >> instead of its pid. I would like to know if this is the expected behavior.
> >> There are no other attributes in the Python handler that provide the pid
> >> and knowing the process id is useful to be able to group all samples that
> >> belong to the same process that generated different threads.
> >
> > Humm, you have:
> >
> > def process_event(param_dict):
> >         event_attr = param_dict["attr"]
> >         sample     = param_dict["sample"]
> >         raw_buf    = param_dict["raw_buf"]
> >         comm       = param_dict["comm"]
> >         name       = param_dict["ev_name"]
> >
> > And then, on sample you have (from a recent python script for processing
> > Intel PT samples):
> >
> > def print_common_start(comm, sample, name):
> >        ts = sample["time"]
> >        cpu = sample["cpu"]
> >        pid = sample["pid"]
> >        tid = sample["tid"]
> >        print "%16s %5u/%-5u [%03u] %9u.%09u %7s:" % (comm, pid, tid, cpu, ts / 1000000000, ts %1000000000, name),
> >
> > - Arnaldo

Re: perf script: Question: Python trace processing script contains the tid of the process in the common_pid attribute

[Arnaldo Carvalho de Melo]

Em Wed, Jul 05, 2017 at 04:25:45PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Jul 05, 2017 at 09:22:07AM -0700, Arun Kalyanasundaram escreveu:
> > Hi Arnaldo,
> > 
> > Thank you for your reply.
> > I actually meant tracepoint event handlers: def
> > trace_unhandled(event_name, context, event_fields_dict)
> > The dict parameter contains an attribute "common_pid" which is
> > actually the "tid" of the thread. There are no other attributes that
> > contain the actual pid of the process. So, I was wondering if this is
> > something intentional? If not I can share a patch to fix this.
> 
> Yeah there is a problem in:
> 
> tools/perf/util/scripting-engines/trace-event-python.c
> 
> static void python_process_event(union perf_event *event,
>                                  struct perf_sample *sample,
>                                  struct perf_evsel *evsel,
>                                  struct addr_location *al)
> {
>         struct tables *tables = &tables_global;
> 
>         switch (evsel->attr.type) {
>         case PERF_TYPE_TRACEPOINT:
>                 python_process_tracepoint(sample, evsel, al);
>                 break;
>         /* Reserve for future process_hw/sw/raw APIs */
>         default:
>                 if (tables->db_export_mode)
>                         db_export__sample(&tables->dbe, event, sample, evsel, al);
>                 else
>                         python_process_general_event(sample, evsel, al);
>         }
> }
> 
> The python_process_tracepoint() thing predates
> python_process_general_event(), and doesn't adds the dict with all the
> perf_sample entries that python_process_general_event() passes to the
> python method :-\
> 
> Both the per-tracepoint python hooks _and_ trace_unhandled() should get
> that dict, is that what your patch does?

Well, for performance reasons I think perhaps we could take a look at
the signature of the python hook and provide the dictionary only if it
is in it?

- Arnaldo

Re: perf script: Question: Python trace processing script contains the tid of the process in the common_pid attribute

[Arun Kalyanasundaram]

I wasn't entirely sure if we should modify the signature of the python
hooks_  as this would make existing scripts incompatible. So the patch
only adds sample->pid to the event_fields_dict param in
trace_unhandled().


On Wed, Jul 5, 2017 at 12:26 PM, Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
> Em Wed, Jul 05, 2017 at 04:25:45PM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Wed, Jul 05, 2017 at 09:22:07AM -0700, Arun Kalyanasundaram escreveu:
>> > Hi Arnaldo,
>> >
>> > Thank you for your reply.
>> > I actually meant tracepoint event handlers: def
>> > trace_unhandled(event_name, context, event_fields_dict)
>> > The dict parameter contains an attribute "common_pid" which is
>> > actually the "tid" of the thread. There are no other attributes that
>> > contain the actual pid of the process. So, I was wondering if this is
>> > something intentional? If not I can share a patch to fix this.
>>
>> Yeah there is a problem in:
>>
>> tools/perf/util/scripting-engines/trace-event-python.c
>>
>> static void python_process_event(union perf_event *event,
>>                                  struct perf_sample *sample,
>>                                  struct perf_evsel *evsel,
>>                                  struct addr_location *al)
>> {
>>         struct tables *tables = &tables_global;
>>
>>         switch (evsel->attr.type) {
>>         case PERF_TYPE_TRACEPOINT:
>>                 python_process_tracepoint(sample, evsel, al);
>>                 break;
>>         /* Reserve for future process_hw/sw/raw APIs */
>>         default:
>>                 if (tables->db_export_mode)
>>                         db_export__sample(&tables->dbe, event, sample, evsel, al);
>>                 else
>>                         python_process_general_event(sample, evsel, al);
>>         }
>> }
>>
>> The python_process_tracepoint() thing predates
>> python_process_general_event(), and doesn't adds the dict with all the
>> perf_sample entries that python_process_general_event() passes to the
>> python method :-\
>>
>> Both the per-tracepoint python hooks _and_ trace_unhandled() should get
>> that dict, is that what your patch does?
>
> Well, for performance reasons I think perhaps we could take a look at
> the signature of the python hook and provide the dictionary only if it
> is in it?
>
> - Arnaldo

Re: perf script: Question: Python trace processing script contains the tid of the process in the common_pid attribute

[Arun Kalyanasundaram]

Arnaldo:
So, I think what you are suggesting is, we should check the signature
of the hook to determine if it has an additional attribute and only
then should we provide the dict. May be something like this:

PyObject* custom_dict = PyObject_GetAttrString(handler, "other_fields_dict");
if (custom_dict)
    //Add dict to PyTuple

Do you think this would be a better approach?

Thank you,
- Arun

On Wed, Jul 5, 2017 at 1:41 PM, Arun Kalyanasundaram
<arunkaly@google.com> wrote:
> I wasn't entirely sure if we should modify the signature of the python
> hooks_  as this would make existing scripts incompatible. So the patch
> only adds sample->pid to the event_fields_dict param in
> trace_unhandled().
>
>
> On Wed, Jul 5, 2017 at 12:26 PM, Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
>> Em Wed, Jul 05, 2017 at 04:25:45PM -0300, Arnaldo Carvalho de Melo escreveu:
>>> Em Wed, Jul 05, 2017 at 09:22:07AM -0700, Arun Kalyanasundaram escreveu:
>>> > Hi Arnaldo,
>>> >
>>> > Thank you for your reply.
>>> > I actually meant tracepoint event handlers: def
>>> > trace_unhandled(event_name, context, event_fields_dict)
>>> > The dict parameter contains an attribute "common_pid" which is
>>> > actually the "tid" of the thread. There are no other attributes that
>>> > contain the actual pid of the process. So, I was wondering if this is
>>> > something intentional? If not I can share a patch to fix this.
>>>
>>> Yeah there is a problem in:
>>>
>>> tools/perf/util/scripting-engines/trace-event-python.c
>>>
>>> static void python_process_event(union perf_event *event,
>>>                                  struct perf_sample *sample,
>>>                                  struct perf_evsel *evsel,
>>>                                  struct addr_location *al)
>>> {
>>>         struct tables *tables = &tables_global;
>>>
>>>         switch (evsel->attr.type) {
>>>         case PERF_TYPE_TRACEPOINT:
>>>                 python_process_tracepoint(sample, evsel, al);
>>>                 break;
>>>         /* Reserve for future process_hw/sw/raw APIs */
>>>         default:
>>>                 if (tables->db_export_mode)
>>>                         db_export__sample(&tables->dbe, event, sample, evsel, al);
>>>                 else
>>>                         python_process_general_event(sample, evsel, al);
>>>         }
>>> }
>>>
>>> The python_process_tracepoint() thing predates
>>> python_process_general_event(), and doesn't adds the dict with all the
>>> perf_sample entries that python_process_general_event() passes to the
>>> python method :-\
>>>
>>> Both the per-tracepoint python hooks _and_ trace_unhandled() should get
>>> that dict, is that what your patch does?
>>
>> Well, for performance reasons I think perhaps we could take a look at
>> the signature of the python hook and provide the dictionary only if it
>> is in it?
>>
>> - Arnaldo

Re: perf script: Question: Python trace processing script contains the tid of the process in the common_pid attribute

[Arnaldo Carvalho de Melo]

Em Wed, Jul 05, 2017 at 04:51:26PM -0700, Arun Kalyanasundaram escreveu:
> Arnaldo:
> So, I think what you are suggesting is, we should check the signature
> of the hook to determine if it has an additional attribute and only
> then should we provide the dict. May be something like this:
> 
> PyObject* custom_dict = PyObject_GetAttrString(handler, "other_fields_dict");
> if (custom_dict)
>     //Add dict to PyTuple
> 
> Do you think this would be a better approach?

I think that we should provide as much as possible the common info about
a sample at all the sample processing routines, be it the initial,
tracepoint specific one, the one for non-tracepoint samples and the
trace_unhandled one.

So scripts not using this optional argument will work with older
versions of the perf tool as well as with new ones that support it.

One could say that to use a script that wants this optional argument, a
new enough version of the tool is required.

So yeah, I think this is a better approach. The details of how to best
do this method signature signing need to be figured out, of course.

Thanks for working on this!

- Arnaldo