Messages in this thread | | | Date | Mon, 30 Mar 2015 12:29:46 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH 2/2] perf tool: Fix ppid for synthesized fork events |
| |
Em Mon, Mar 30, 2015 at 09:23:21AM -0600, David Ahern escreveu: > On 3/30/15 2:04 AM, Jiri Olsa wrote: > >On Sun, Mar 29, 2015 at 04:30:02PM -0600, David Ahern wrote: > > > >SNIP > > > >>-static pid_t perf_event__synthesize_comm(struct perf_tool *tool, > >>- union perf_event *event, pid_t pid, > >>- perf_event__handler_t process, > >>- struct machine *machine) > >>+static int perf_event__synthesize_comm(struct perf_tool *tool, > >>+ union perf_event *event, pid_t pid, > >>+ perf_event__handler_t process, > >>+ struct machine *machine, > >>+ pid_t *tgid, pid_t *ppid) > >> { > >>- pid_t tgid = perf_event__prepare_comm(event, pid, machine); > >>- > >>- if (tgid == -1) > >>- goto out; > >>+ if (perf_event__prepare_comm(event, pid, machine, tgid, ppid) != 0) > >>+ return -1; > > > >why dont we set ppid also for single comm event? seems to me > >like following assignments: > > > > event->fork.ppid = ppid; > > event->fork.ptid = ppid; > > event->fork.pid = tgid; > > event->fork.tid = pid; > > event->fork.header.type = PERF_RECORD_FORK; > > > >should be part of perf_event__prepare_comm function..? > > comm events versus fork events. Only fork has the ppid. That loop that > generates COMM and FORK events needs some work. e.g., FORK should come > before COMM. But that is not the point of this patch which is to fix ppid.
Yes, and COMM nowadays creates the thread (uses findnew_thread) where it should really use just find_thread(), i.e. the fork event is the only one that should create it, I guess it was like that because it made the code a bit more robust, i.e. would handle COMMs without FORKs.
This one:
commit 4aa5f4f7bb8bc41cba15bcd0d80c4fb085027d6b Author: Arnaldo Carvalho de Melo <acme@redhat.com> Date: Fri Feb 27 19:52:10 2015 -0300
perf tools: Fix FORK after COMM when synthesizing records for pre-existing threads
Should have fixed a bit of this...
- Arnaldo
| |