* [PATCH 1/2] perf tool: Refactor comm/tgid lookup @ 2015-03-29 22:30 David Ahern 2015-03-29 22:30 ` [PATCH 2/2] perf tool: Fix ppid for synthesized fork events David Ahern 2015-03-30 8:01 ` [PATCH 1/2] perf tool: Refactor comm/tgid lookup Jiri Olsa 0 siblings, 2 replies; 11+ messages in thread From: David Ahern @ 2015-03-29 22:30 UTC (permalink / raw) To: acme; +Cc: linux-kernel, David Ahern, Don Zickus, Joe Mario, Jiri Olsa Rather than parsing /proc/pid/status file one line at a time, read it into a buffer in one shot and search for all strings in one pass. Signed-off-by: David Ahern <dsahern@gmail.com> Cc: Don Zickus <dzickus@redhat.com> Cc: Joe Mario <jmario@redhat.com> Cc: Jiri Olsa <jolsa@redhat.com> --- tools/perf/util/event.c | 80 +++++++++++++++++++++++++++++++------------------ 1 file changed, 51 insertions(+), 29 deletions(-) diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c index d5efa5092ce6..b49bee8a283d 100644 --- a/tools/perf/util/event.c +++ b/tools/perf/util/event.c @@ -49,48 +49,70 @@ static struct perf_sample synth_sample = { .period = 1, }; +/* + * Assumes that the first 4095 bytes of /proc/pid/stat contains + * the comm and tgid. + */ static pid_t perf_event__get_comm_tgid(pid_t pid, char *comm, size_t len) { char filename[PATH_MAX]; - char bf[BUFSIZ]; - FILE *fp; - size_t size = 0; + char bf[4096]; + int fd; + size_t size = 0, n; pid_t tgid = -1; + char *nl, *name, *tgids; snprintf(filename, sizeof(filename), "/proc/%d/status", pid); - fp = fopen(filename, "r"); - if (fp == NULL) { + fd = open(filename, O_RDONLY); + if (fd < 0) { pr_debug("couldn't open %s\n", filename); return 0; } - while (!comm[0] || (tgid < 0)) { - if (fgets(bf, sizeof(bf), fp) == NULL) { - pr_warning("couldn't get COMM and pgid, malformed %s\n", - filename); - break; - } - - if (memcmp(bf, "Name:", 5) == 0) { - char *name = bf + 5; - while (*name && isspace(*name)) - ++name; - size = strlen(name) - 1; - if (size >= len) - size = len - 1; - memcpy(comm, name, size); - comm[size] = '\0'; - - } else if (memcmp(bf, "Tgid:", 5) == 0) { - char *tgids = bf + 5; - while (*tgids && isspace(*tgids)) - ++tgids; - tgid = atoi(tgids); - } + n = read(fd, bf, sizeof(bf) - 1); + close(fd); + if (n <= 0) { + pr_warning("Couldn't get COMM and tgid for pid %d\n", + pid); + return -1; } + bf[n] = '\0'; - fclose(fp); + name = strstr(bf, "Name:"); + tgids = strstr(bf, "Tgid:"); + + if (name) { + name += 5; /* strlen("Name:") */ + + while (*name && isspace(*name)) + ++name; + + nl = strchr(name, '\n'); + if (nl) + *nl = '\0'; + + size = strlen(name); + if (size >= len) + size = len - 1; + memcpy(comm, name, size); + comm[size] = '\0'; + } else + pr_debug("Name: string not found for pid %d\n", pid); + + if (tgids) { + tgids += 5; /* strlen("Tgid:") */ + while (*tgids && isspace(*tgids)) + ++tgids; + + nl = strchr(tgids, '\n'); + if (nl) + *nl = '\0'; + + tgid = atoi(tgids); + + } else + pr_debug("Tgid: string not found for pid %d\n", pid); return tgid; } -- 2.2.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] perf tool: Fix ppid for synthesized fork events 2015-03-29 22:30 [PATCH 1/2] perf tool: Refactor comm/tgid lookup David Ahern @ 2015-03-29 22:30 ` David Ahern 2015-03-30 8:04 ` Jiri Olsa 2015-03-30 18:20 ` [PATCH v2 " David Ahern 2015-03-30 8:01 ` [PATCH 1/2] perf tool: Refactor comm/tgid lookup Jiri Olsa 1 sibling, 2 replies; 11+ messages in thread From: David Ahern @ 2015-03-29 22:30 UTC (permalink / raw) To: acme; +Cc: linux-kernel, David Ahern, Don Zickus, Joe Mario, Jiri Olsa 363b785f38 added synthesized fork events and set a thread's parent id to itself. Since we are already processing /proc/<pid>/status the ppid can be determined properly. Make it so. Signed-off-by: David Ahern <dsahern@gmail.com> Cc: Don Zickus <dzickus@redhat.com> Cc: Joe Mario <jmario@redhat.com> Cc: Jiri Olsa <jolsa@redhat.com> --- tools/perf/util/event.c | 106 ++++++++++++++++++++++++++++-------------------- 1 file changed, 63 insertions(+), 43 deletions(-) diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c index b49bee8a283d..648744a21410 100644 --- a/tools/perf/util/event.c +++ b/tools/perf/util/event.c @@ -51,29 +51,32 @@ static struct perf_sample synth_sample = { /* * Assumes that the first 4095 bytes of /proc/pid/stat contains - * the comm and tgid. + * the comm, tgid and ppid. */ -static pid_t perf_event__get_comm_tgid(pid_t pid, char *comm, size_t len) +static int perf_event__get_comm_ids(pid_t pid, char *comm, size_t len, + pid_t *tgid, pid_t *ppid) { char filename[PATH_MAX]; char bf[4096]; int fd; size_t size = 0, n; - pid_t tgid = -1; - char *nl, *name, *tgids; + char *nl, *name, *tgids, *ppids; + + *tgid = -1; + *ppid = -1; snprintf(filename, sizeof(filename), "/proc/%d/status", pid); fd = open(filename, O_RDONLY); if (fd < 0) { pr_debug("couldn't open %s\n", filename); - return 0; + return -1; } n = read(fd, bf, sizeof(bf) - 1); close(fd); if (n <= 0) { - pr_warning("Couldn't get COMM and tgid for pid %d\n", + pr_warning("Couldn't get COMM, tigd and ppid for pid %d\n", pid); return -1; } @@ -81,6 +84,7 @@ static pid_t perf_event__get_comm_tgid(pid_t pid, char *comm, size_t len) name = strstr(bf, "Name:"); tgids = strstr(bf, "Tgid:"); + ppids = strstr(bf, "PPid:"); if (name) { name += 5; /* strlen("Name:") */ @@ -109,32 +113,51 @@ static pid_t perf_event__get_comm_tgid(pid_t pid, char *comm, size_t len) if (nl) *nl = '\0'; - tgid = atoi(tgids); + *tgid = atoi(tgids); } else pr_debug("Tgid: string not found for pid %d\n", pid); - return tgid; + if (ppids) { + ppids += 5; /* strlen("PPid:") */ + + while (*ppids && isspace(*ppids)) + ++ppids; + + nl = strchr(ppids, '\n'); + if (nl) + *nl = '\0'; + + *ppid = atoi(ppids); + } else + pr_debug("PPid: string not found for pid %d\n", pid); + + return 0; } -static pid_t perf_event__prepare_comm(union perf_event *event, pid_t pid, - struct machine *machine) +static int perf_event__prepare_comm(union perf_event *event, pid_t pid, + struct machine *machine, + pid_t *tgid, pid_t *ppid) { size_t size; - pid_t tgid; + + *ppid = -1; memset(&event->comm, 0, sizeof(event->comm)); - if (machine__is_host(machine)) - tgid = perf_event__get_comm_tgid(pid, event->comm.comm, - sizeof(event->comm.comm)); - else - tgid = machine->pid; + if (machine__is_host(machine)) { + if (perf_event__get_comm_ids(pid, event->comm.comm, + sizeof(event->comm.comm), + tgid, ppid) != 0) { + return -1; + } + } else + *tgid = machine->pid; - if (tgid < 0) - goto out; + if (*tgid < 0) + return -1; - event->comm.pid = tgid; + event->comm.pid = *tgid; event->comm.header.type = PERF_RECORD_COMM; size = strlen(event->comm.comm) + 1; @@ -144,36 +167,35 @@ static pid_t perf_event__prepare_comm(union perf_event *event, pid_t pid, (sizeof(event->comm.comm) - size) + machine->id_hdr_size); event->comm.tid = pid; -out: - return tgid; + + return 0; } -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; if (process(tool, event, &synth_sample, machine) != 0) return -1; -out: - return tgid; + return 0; } static int perf_event__synthesize_fork(struct perf_tool *tool, - union perf_event *event, pid_t pid, - pid_t tgid, perf_event__handler_t process, + union perf_event *event, + pid_t pid, pid_t tgid, pid_t ppid, + perf_event__handler_t process, struct machine *machine) { memset(&event->fork, 0, sizeof(event->fork) + machine->id_hdr_size); - event->fork.ppid = tgid; - event->fork.ptid = tgid; + event->fork.ppid = ppid; + event->fork.ptid = ppid; event->fork.pid = tgid; event->fork.tid = pid; event->fork.header.type = PERF_RECORD_FORK; @@ -365,14 +387,12 @@ static int __event__synthesize_thread(union perf_event *comm_event, char filename[PATH_MAX]; DIR *tasks; struct dirent dirent, *next; - pid_t tgid; + pid_t tgid, ppid; /* special case: only send one comm event using passed in pid */ if (!full) { - tgid = perf_event__synthesize_comm(tool, comm_event, pid, - process, machine); - - if (tgid == -1) + if (perf_event__synthesize_comm(tool, comm_event, pid, + process, machine, &tgid, &ppid) != 0) return -1; return perf_event__synthesize_mmap_events(tool, mmap_event, pid, tgid, @@ -400,12 +420,12 @@ static int __event__synthesize_thread(union perf_event *comm_event, if (*end) continue; - tgid = perf_event__prepare_comm(comm_event, _pid, machine); - if (tgid == -1) + if (perf_event__prepare_comm(comm_event, _pid, machine, + &tgid, &ppid) != 0) return -1; if (perf_event__synthesize_fork(tool, fork_event, _pid, tgid, - process, machine) < 0) + ppid, process, machine) < 0) return -1; /* * Send the prepared comm event -- 2.2.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] perf tool: Fix ppid for synthesized fork events 2015-03-29 22:30 ` [PATCH 2/2] perf tool: Fix ppid for synthesized fork events David Ahern @ 2015-03-30 8:04 ` Jiri Olsa 2015-03-30 15:23 ` David Ahern 2015-03-30 18:20 ` [PATCH v2 " David Ahern 1 sibling, 1 reply; 11+ messages in thread From: Jiri Olsa @ 2015-03-30 8:04 UTC (permalink / raw) To: David Ahern; +Cc: acme, linux-kernel, Don Zickus, Joe Mario 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..? SNIP > @@ -365,14 +387,12 @@ static int __event__synthesize_thread(union perf_event *comm_event, > char filename[PATH_MAX]; > DIR *tasks; > struct dirent dirent, *next; > - pid_t tgid; > + pid_t tgid, ppid; > > /* special case: only send one comm event using passed in pid */ > if (!full) { > - tgid = perf_event__synthesize_comm(tool, comm_event, pid, > - process, machine); > - > - if (tgid == -1) > + if (perf_event__synthesize_comm(tool, comm_event, pid, > + process, machine, &tgid, &ppid) != 0) why did you add *ppid arg into perf_event__synthesize_comm? I see no use for it in the rest of the caller.. jirka ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] perf tool: Fix ppid for synthesized fork events 2015-03-30 8:04 ` Jiri Olsa @ 2015-03-30 15:23 ` David Ahern 2015-03-30 15:29 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 11+ messages in thread From: David Ahern @ 2015-03-30 15:23 UTC (permalink / raw) To: Jiri Olsa; +Cc: acme, linux-kernel, Don Zickus, Joe Mario 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. > > SNIP > >> @@ -365,14 +387,12 @@ static int __event__synthesize_thread(union perf_event *comm_event, >> char filename[PATH_MAX]; >> DIR *tasks; >> struct dirent dirent, *next; >> - pid_t tgid; >> + pid_t tgid, ppid; >> >> /* special case: only send one comm event using passed in pid */ >> if (!full) { >> - tgid = perf_event__synthesize_comm(tool, comm_event, pid, >> - process, machine); >> - >> - if (tgid == -1) >> + if (perf_event__synthesize_comm(tool, comm_event, pid, >> + process, machine, &tgid, &ppid) != 0) > > why did you add *ppid arg into perf_event__synthesize_comm? > I see no use for it in the rest of the caller.. Because of the twisted state that code is in. Yes, I can remove that one. David ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] perf tool: Fix ppid for synthesized fork events 2015-03-30 15:23 ` David Ahern @ 2015-03-30 15:29 ` Arnaldo Carvalho de Melo 0 siblings, 0 replies; 11+ messages in thread From: Arnaldo Carvalho de Melo @ 2015-03-30 15:29 UTC (permalink / raw) To: David Ahern; +Cc: Jiri Olsa, linux-kernel, Don Zickus, Joe Mario 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] perf tool: Fix ppid for synthesized fork events 2015-03-29 22:30 ` [PATCH 2/2] perf tool: Fix ppid for synthesized fork events David Ahern 2015-03-30 8:04 ` Jiri Olsa @ 2015-03-30 18:20 ` David Ahern 2015-03-30 19:40 ` Arnaldo Carvalho de Melo 1 sibling, 1 reply; 11+ messages in thread From: David Ahern @ 2015-03-30 18:20 UTC (permalink / raw) To: acme; +Cc: linux-kernel, David Ahern, Don Zickus, Joe Mario, Jiri Olsa 363b785f38 added synthesized fork events and set a thread's parent id to itself. Since we are already processing /proc/<pid>/status the ppid can be determined properly. Make it so. Signed-off-by: David Ahern <dsahern@gmail.com> Cc: Don Zickus <dzickus@redhat.com> Cc: Joe Mario <jmario@redhat.com> Cc: Jiri Olsa <jolsa@redhat.com> --- v2: - removed changes to function signature for perf_event__synthesize_comm as noted by Jiri tools/perf/util/event.c | 92 ++++++++++++++++++++++++++++++------------------- 1 file changed, 57 insertions(+), 35 deletions(-) diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c index b49bee8a283d..a31b0fd41c05 100644 --- a/tools/perf/util/event.c +++ b/tools/perf/util/event.c @@ -51,29 +51,32 @@ static struct perf_sample synth_sample = { /* * Assumes that the first 4095 bytes of /proc/pid/stat contains - * the comm and tgid. + * the comm, tgid and ppid. */ -static pid_t perf_event__get_comm_tgid(pid_t pid, char *comm, size_t len) +static int perf_event__get_comm_ids(pid_t pid, char *comm, size_t len, + pid_t *tgid, pid_t *ppid) { char filename[PATH_MAX]; char bf[4096]; int fd; size_t size = 0, n; - pid_t tgid = -1; - char *nl, *name, *tgids; + char *nl, *name, *tgids, *ppids; + + *tgid = -1; + *ppid = -1; snprintf(filename, sizeof(filename), "/proc/%d/status", pid); fd = open(filename, O_RDONLY); if (fd < 0) { pr_debug("couldn't open %s\n", filename); - return 0; + return -1; } n = read(fd, bf, sizeof(bf) - 1); close(fd); if (n <= 0) { - pr_warning("Couldn't get COMM and tgid for pid %d\n", + pr_warning("Couldn't get COMM, tigd and ppid for pid %d\n", pid); return -1; } @@ -81,6 +84,7 @@ static pid_t perf_event__get_comm_tgid(pid_t pid, char *comm, size_t len) name = strstr(bf, "Name:"); tgids = strstr(bf, "Tgid:"); + ppids = strstr(bf, "PPid:"); if (name) { name += 5; /* strlen("Name:") */ @@ -109,32 +113,51 @@ static pid_t perf_event__get_comm_tgid(pid_t pid, char *comm, size_t len) if (nl) *nl = '\0'; - tgid = atoi(tgids); + *tgid = atoi(tgids); } else pr_debug("Tgid: string not found for pid %d\n", pid); - return tgid; + if (ppids) { + ppids += 5; /* strlen("PPid:") */ + + while (*ppids && isspace(*ppids)) + ++ppids; + + nl = strchr(ppids, '\n'); + if (nl) + *nl = '\0'; + + *ppid = atoi(ppids); + } else + pr_debug("PPid: string not found for pid %d\n", pid); + + return 0; } -static pid_t perf_event__prepare_comm(union perf_event *event, pid_t pid, - struct machine *machine) +static int perf_event__prepare_comm(union perf_event *event, pid_t pid, + struct machine *machine, + pid_t *tgid, pid_t *ppid) { size_t size; - pid_t tgid; + + *ppid = -1; memset(&event->comm, 0, sizeof(event->comm)); - if (machine__is_host(machine)) - tgid = perf_event__get_comm_tgid(pid, event->comm.comm, - sizeof(event->comm.comm)); - else - tgid = machine->pid; + if (machine__is_host(machine)) { + if (perf_event__get_comm_ids(pid, event->comm.comm, + sizeof(event->comm.comm), + tgid, ppid) != 0) { + return -1; + } + } else + *tgid = machine->pid; - if (tgid < 0) - goto out; + if (*tgid < 0) + return -1; - event->comm.pid = tgid; + event->comm.pid = *tgid; event->comm.header.type = PERF_RECORD_COMM; size = strlen(event->comm.comm) + 1; @@ -144,8 +167,8 @@ static pid_t perf_event__prepare_comm(union perf_event *event, pid_t pid, (sizeof(event->comm.comm) - size) + machine->id_hdr_size); event->comm.tid = pid; -out: - return tgid; + + return 0; } static pid_t perf_event__synthesize_comm(struct perf_tool *tool, @@ -153,27 +176,27 @@ static pid_t perf_event__synthesize_comm(struct perf_tool *tool, perf_event__handler_t process, struct machine *machine) { - pid_t tgid = perf_event__prepare_comm(event, pid, machine); + pid_t tgid, ppid; - if (tgid == -1) - goto out; + if (perf_event__prepare_comm(event, pid, machine, &tgid, &ppid) != 0) + return -1; if (process(tool, event, &synth_sample, machine) != 0) return -1; -out: return tgid; } static int perf_event__synthesize_fork(struct perf_tool *tool, - union perf_event *event, pid_t pid, - pid_t tgid, perf_event__handler_t process, + union perf_event *event, + pid_t pid, pid_t tgid, pid_t ppid, + perf_event__handler_t process, struct machine *machine) { memset(&event->fork, 0, sizeof(event->fork) + machine->id_hdr_size); - event->fork.ppid = tgid; - event->fork.ptid = tgid; + event->fork.ppid = ppid; + event->fork.ptid = ppid; event->fork.pid = tgid; event->fork.tid = pid; event->fork.header.type = PERF_RECORD_FORK; @@ -365,13 +388,12 @@ static int __event__synthesize_thread(union perf_event *comm_event, char filename[PATH_MAX]; DIR *tasks; struct dirent dirent, *next; - pid_t tgid; + pid_t tgid, ppid; /* special case: only send one comm event using passed in pid */ if (!full) { tgid = perf_event__synthesize_comm(tool, comm_event, pid, - process, machine); - + process, machine); if (tgid == -1) return -1; @@ -400,12 +422,12 @@ static int __event__synthesize_thread(union perf_event *comm_event, if (*end) continue; - tgid = perf_event__prepare_comm(comm_event, _pid, machine); - if (tgid == -1) + if (perf_event__prepare_comm(comm_event, _pid, machine, + &tgid, &ppid) != 0) return -1; if (perf_event__synthesize_fork(tool, fork_event, _pid, tgid, - process, machine) < 0) + ppid, process, machine) < 0) return -1; /* * Send the prepared comm event -- 2.2.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] perf tool: Fix ppid for synthesized fork events 2015-03-30 18:20 ` [PATCH v2 " David Ahern @ 2015-03-30 19:40 ` Arnaldo Carvalho de Melo 2015-03-30 19:53 ` David Ahern 0 siblings, 1 reply; 11+ messages in thread From: Arnaldo Carvalho de Melo @ 2015-03-30 19:40 UTC (permalink / raw) To: David Ahern Cc: linux-kernel, Don Zickus, Joe Mario, Jiri Olsa, Ingo Molnar, Peter Zijlstra Em Mon, Mar 30, 2015 at 12:20:42PM -0600, David Ahern escreveu: Some nits below: > tools/perf/util/event.c | 92 ++++++++++++++++++++++++++++++------------------- > 1 file changed, 57 insertions(+), 35 deletions(-) > > diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c <SNIP> > @@ -81,6 +84,7 @@ static pid_t perf_event__get_comm_tgid(pid_t pid, char *comm, size_t len) > > name = strstr(bf, "Name:"); > tgids = strstr(bf, "Tgid:"); > + ppids = strstr(bf, "PPid:"); can't we make this: ppids = strstr(tgids, "PPid:"); To speed it up a teeny little bit? 8-) > if (name) { > name += 5; /* strlen("Name:") */ > @@ -109,32 +113,51 @@ static pid_t perf_event__get_comm_tgid(pid_t pid, char *comm, size_t len) > if (nl) > *nl = '\0'; > > - tgid = atoi(tgids); > + *tgid = atoi(tgids); > > } else > pr_debug("Tgid: string not found for pid %d\n", pid); > > - return tgid; > + if (ppids) { > + ppids += 5; /* strlen("PPid:") */ > + > + while (*ppids && isspace(*ppids)) > + ++ppids; The above could be simplified to: while (isspace(*ppids)) ++ppids; $ cat isspace.c #include <ctype.h> #include <stdio.h> int main(void) { return printf("isspace('\\0')=%d\n", isspace('\0')); } $ ./isspace isspace('\0')=0 $ > + nl = strchr(ppids, '\n'); > + if (nl) > + *nl = '\0'; We also don't need to find and zero this '\n', as: $ cat atoi.c #include <string.h> #include <stdio.h> int main(void) { return printf("atoi(\"1234\\n\")=%d\n", atoi("1234\n")); } $ ./atoi atoi("1234\n")=1234 $ <SNIP> > + if (machine__is_host(machine)) { > + if (perf_event__get_comm_ids(pid, event->comm.comm, > + sizeof(event->comm.comm), > + tgid, ppid) != 0) { > + return -1; > + } > + } else > + *tgid = machine->pid; Somebody, I think PeterZ and also Ingo, routinely asks for having {} on the else part of an if that has {}, please do so. - Arnaldo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] perf tool: Fix ppid for synthesized fork events 2015-03-30 19:40 ` Arnaldo Carvalho de Melo @ 2015-03-30 19:53 ` David Ahern 2015-03-30 20:00 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 11+ messages in thread From: David Ahern @ 2015-03-30 19:53 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: linux-kernel, Don Zickus, Joe Mario, Jiri Olsa, Ingo Molnar, Peter Zijlstra On 3/30/15 1:40 PM, Arnaldo Carvalho de Melo wrote: >> @@ -81,6 +84,7 @@ static pid_t perf_event__get_comm_tgid(pid_t pid, char *comm, size_t len) >> >> name = strstr(bf, "Name:"); >> tgids = strstr(bf, "Tgid:"); >> + ppids = strstr(bf, "PPid:"); > > can't we make this: > > ppids = strstr(tgids, "PPid:"); > > To speed it up a teeny little bit? 8-) Sure, I thought about that as well, but it puts an assumption on order of the data in the file. Why have the assumption? > >> if (name) { >> name += 5; /* strlen("Name:") */ >> @@ -109,32 +113,51 @@ static pid_t perf_event__get_comm_tgid(pid_t pid, char *comm, size_t len) >> if (nl) >> *nl = '\0'; >> >> - tgid = atoi(tgids); >> + *tgid = atoi(tgids); >> >> } else >> pr_debug("Tgid: string not found for pid %d\n", pid); >> >> - return tgid; >> + if (ppids) { >> + ppids += 5; /* strlen("PPid:") */ >> + >> + while (*ppids && isspace(*ppids)) >> + ++ppids; > > The above could be simplified to: > > while (isspace(*ppids)) > ++ppids; sure. > > $ cat isspace.c > #include <ctype.h> > #include <stdio.h> > int main(void) { return printf("isspace('\\0')=%d\n", isspace('\0')); } > $ ./isspace > isspace('\0')=0 > $ > >> + nl = strchr(ppids, '\n'); >> + if (nl) >> + *nl = '\0'; > > We also don't need to find and zero this '\n', as: > > $ cat atoi.c > #include <string.h> > #include <stdio.h> > int main(void) { return printf("atoi(\"1234\\n\")=%d\n", > atoi("1234\n")); } > $ ./atoi > atoi("1234\n")=1234 > $ ok. another assumption on implementation. fine with taking it out. > > <SNIP> > >> + if (machine__is_host(machine)) { >> + if (perf_event__get_comm_ids(pid, event->comm.comm, >> + sizeof(event->comm.comm), >> + tgid, ppid) != 0) { >> + return -1; >> + } >> + } else >> + *tgid = machine->pid; > > Somebody, I think PeterZ and also Ingo, routinely asks for having {} on > the else part of an if that has {}, please do so. ack ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] perf tool: Fix ppid for synthesized fork events 2015-03-30 19:53 ` David Ahern @ 2015-03-30 20:00 ` Arnaldo Carvalho de Melo 0 siblings, 0 replies; 11+ messages in thread From: Arnaldo Carvalho de Melo @ 2015-03-30 20:00 UTC (permalink / raw) To: David Ahern Cc: linux-kernel, Don Zickus, Joe Mario, Jiri Olsa, Ingo Molnar, Peter Zijlstra Em Mon, Mar 30, 2015 at 01:53:04PM -0600, David Ahern escreveu: > On 3/30/15 1:40 PM, Arnaldo Carvalho de Melo wrote: > >>@@ -81,6 +84,7 @@ static pid_t perf_event__get_comm_tgid(pid_t pid, char *comm, size_t len) > >> > >> name = strstr(bf, "Name:"); > >> tgids = strstr(bf, "Tgid:"); > >>+ ppids = strstr(bf, "PPid:"); > > > >can't we make this: > > > > ppids = strstr(tgids, "PPid:"); > > > >To speed it up a teeny little bit? 8-) > > Sure, I thought about that as well, but it puts an assumption on order of > the data in the file. Why have the assumption? Good question, perhaps we should leave it as is, should be in the noise, right? > >> if (name) { > >> name += 5; /* strlen("Name:") */ > >>@@ -109,32 +113,51 @@ static pid_t perf_event__get_comm_tgid(pid_t pid, char *comm, size_t len) > >> if (nl) > >> *nl = '\0'; > >> > >>- tgid = atoi(tgids); > >>+ *tgid = atoi(tgids); > >> > >> } else > >> pr_debug("Tgid: string not found for pid %d\n", pid); > >> > >>- return tgid; > >>+ if (ppids) { > >>+ ppids += 5; /* strlen("PPid:") */ > >>+ > >>+ while (*ppids && isspace(*ppids)) > >>+ ++ppids; > > > >The above could be simplified to: > > > > while (isspace(*ppids)) > > ++ppids; > > sure. And then, the this loop can be elided as well, as you can see below... > >$ cat isspace.c > >#include <ctype.h> > >#include <stdio.h> > >int main(void) { return printf("isspace('\\0')=%d\n", isspace('\0')); } > >$ ./isspace > >isspace('\0')=0 > >$ > > > >>+ nl = strchr(ppids, '\n'); > >>+ if (nl) > >>+ *nl = '\0'; > > > >We also don't need to find and zero this '\n', as: > > > >$ cat atoi.c > >#include <string.h> > >#include <stdio.h> > >int main(void) { return printf("atoi(\"1234\\n\")=%d\n", > >atoi("1234\n")); } > >$ ./atoi > >atoi("1234\n")=1234 > >$ > > ok. another assumption on implementation. fine with taking it out. Not really, POSIX sayz: http://pubs.opengroup.org/onlinepubs/9699919799/functions/atoi.html The call atoi(str) shall be equivalent to: (int) strtol(str, (char **)NULL, 10) ---- And: http://pubs.opengroup.org/onlinepubs/9699919799/functions/strtol.html These functions shall convert the initial portion of the string pointed to by str to a type long and long long representation, respectively. First, they decompose the input string into three parts: An initial, possibly empty, sequence of white-space characters (as specified by isspace()) A subject sequence interpreted as an integer represented in some radix determined by the value of base A final string of one or more unrecognized characters, including the terminating NUL character of the input string. ---------- So even that isspace loop can be ditched, no? > ><SNIP> > > > >>+ if (machine__is_host(machine)) { > >>+ if (perf_event__get_comm_ids(pid, event->comm.comm, > >>+ sizeof(event->comm.comm), > >>+ tgid, ppid) != 0) { > >>+ return -1; > >>+ } > >>+ } else > >>+ *tgid = machine->pid; > > > >Somebody, I think PeterZ and also Ingo, routinely asks for having {} on > >the else part of an if that has {}, please do so. > > ack Thanks! - Arnaldo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] perf tool: Refactor comm/tgid lookup 2015-03-29 22:30 [PATCH 1/2] perf tool: Refactor comm/tgid lookup David Ahern 2015-03-29 22:30 ` [PATCH 2/2] perf tool: Fix ppid for synthesized fork events David Ahern @ 2015-03-30 8:01 ` Jiri Olsa 2015-03-30 15:34 ` David Ahern 1 sibling, 1 reply; 11+ messages in thread From: Jiri Olsa @ 2015-03-30 8:01 UTC (permalink / raw) To: David Ahern; +Cc: acme, linux-kernel, Don Zickus, Joe Mario On Sun, Mar 29, 2015 at 04:30:01PM -0600, David Ahern wrote: SNIP > + fd = open(filename, O_RDONLY); > + if (fd < 0) { > pr_debug("couldn't open %s\n", filename); > return 0; > } > > - while (!comm[0] || (tgid < 0)) { > - if (fgets(bf, sizeof(bf), fp) == NULL) { > - pr_warning("couldn't get COMM and pgid, malformed %s\n", > - filename); > - break; > - } > - > - if (memcmp(bf, "Name:", 5) == 0) { > - char *name = bf + 5; > - while (*name && isspace(*name)) > - ++name; > - size = strlen(name) - 1; > - if (size >= len) > - size = len - 1; > - memcpy(comm, name, size); > - comm[size] = '\0'; > - > - } else if (memcmp(bf, "Tgid:", 5) == 0) { > - char *tgids = bf + 5; > - while (*tgids && isspace(*tgids)) > - ++tgids; > - tgid = atoi(tgids); > - } > + n = read(fd, bf, sizeof(bf) - 1); > + close(fd); we have filename__read_str, which will read whole file into buffer, and you dont need to worry about the file size.. not that there's any worry wrt /proc/<pid>/status file, but who knows.. ;-) jirka ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] perf tool: Refactor comm/tgid lookup 2015-03-30 8:01 ` [PATCH 1/2] perf tool: Refactor comm/tgid lookup Jiri Olsa @ 2015-03-30 15:34 ` David Ahern 0 siblings, 0 replies; 11+ messages in thread From: David Ahern @ 2015-03-30 15:34 UTC (permalink / raw) To: Jiri Olsa; +Cc: acme, linux-kernel, Don Zickus, Joe Mario On 3/30/15 2:01 AM, Jiri Olsa wrote: > On Sun, Mar 29, 2015 at 04:30:01PM -0600, David Ahern wrote: > > SNIP > >> + fd = open(filename, O_RDONLY); >> + if (fd < 0) { >> pr_debug("couldn't open %s\n", filename); >> return 0; >> } >> >> - while (!comm[0] || (tgid < 0)) { >> - if (fgets(bf, sizeof(bf), fp) == NULL) { >> - pr_warning("couldn't get COMM and pgid, malformed %s\n", >> - filename); >> - break; >> - } >> - >> - if (memcmp(bf, "Name:", 5) == 0) { >> - char *name = bf + 5; >> - while (*name && isspace(*name)) >> - ++name; >> - size = strlen(name) - 1; >> - if (size >= len) >> - size = len - 1; >> - memcpy(comm, name, size); >> - comm[size] = '\0'; >> - >> - } else if (memcmp(bf, "Tgid:", 5) == 0) { >> - char *tgids = bf + 5; >> - while (*tgids && isspace(*tgids)) >> - ++tgids; >> - tgid = atoi(tgids); >> - } >> + n = read(fd, bf, sizeof(bf) - 1); >> + close(fd); > > we have filename__read_str, which will read whole file into buffer, > and you dont need to worry about the file size.. not that there's > any worry wrt /proc/<pid>/status file, but who knows.. ;-) hmmm.... I did not know about that function. But I don't think we want to use it for this case: This function is called a LOT (think 50k or 100k threads) and we want to synthesize the threads as fast as possible. Using stack (like above) is much faster than dealing with malloc and free on each pass. David ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-03-30 20:00 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-03-29 22:30 [PATCH 1/2] perf tool: Refactor comm/tgid lookup David Ahern 2015-03-29 22:30 ` [PATCH 2/2] perf tool: Fix ppid for synthesized fork events David Ahern 2015-03-30 8:04 ` Jiri Olsa 2015-03-30 15:23 ` David Ahern 2015-03-30 15:29 ` Arnaldo Carvalho de Melo 2015-03-30 18:20 ` [PATCH v2 " David Ahern 2015-03-30 19:40 ` Arnaldo Carvalho de Melo 2015-03-30 19:53 ` David Ahern 2015-03-30 20:00 ` Arnaldo Carvalho de Melo 2015-03-30 8:01 ` [PATCH 1/2] perf tool: Refactor comm/tgid lookup Jiri Olsa 2015-03-30 15:34 ` David Ahern
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).