From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753411AbbC3UAl (ORCPT ); Mon, 30 Mar 2015 16:00:41 -0400 Received: from casper.infradead.org ([85.118.1.10]:36772 "EHLO casper.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752886AbbC3UAh (ORCPT ); Mon, 30 Mar 2015 16:00:37 -0400 Date: Mon, 30 Mar 2015 17:00:45 -0300 From: Arnaldo Carvalho de Melo To: David Ahern Cc: linux-kernel@vger.kernel.org, Don Zickus , Joe Mario , Jiri Olsa , Ingo Molnar , Peter Zijlstra Subject: Re: [PATCH v2 2/2] perf tool: Fix ppid for synthesized fork events Message-ID: <20150330200045.GI32560@kernel.org> References: <20150330194055.GG32560@kernel.org> <5519A9A0.4030503@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5519A9A0.4030503@gmail.com> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > >#include > >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 > >#include > >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? > > > > > >>+ 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