Messages in this thread | | | Date | Mon, 30 Mar 2015 13:53:04 -0600 | From | David Ahern <> | Subject | Re: [PATCH v2 2/2] perf tool: Fix ppid for synthesized fork events |
| |
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
| |