All of lore.kernel.org
 help / color / mirror / Atom feed
* [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 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 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

* 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

* [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

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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.