perf tools: Allow multiple threads or processes in record, stat, top

Allow a user to collect events for multiple threads or processes using a comma separated list.

e.g., collect data on a VM and its vhost thread:

   perf top -p 21483,21485
   perf stat -p 21483,21485 -ddd
   perf record -p 21483,21485

or monitoring vcpu threads
perf top -t 21488,21489
perf stat -t 21488,21489 -ddd
perf record -t 21488,21489

Signed-off-by David Ahern <[email protected]>

---
  tools/perf/Documentation/perf-record.txt |    4 +-
  tools/perf/Documentation/perf-stat.txt   |    4 +-
  tools/perf/Documentation/perf-top.txt    |    4 +-
  tools/perf/builtin-record.c              |   10 +-
  tools/perf/builtin-stat.c                |   31 +++---
  tools/perf/builtin-test.c                |    2 -
  tools/perf/builtin-top.c                 |   12 +--
  tools/perf/perf.h                        |    4 +-
  tools/perf/util/evlist.c                 |   10 +-
  tools/perf/util/evlist.h                 |    4 +-
  tools/perf/util/evsel.c                  |    2 +-
  tools/perf/util/thread_map.c             |  152 ++++++++++++++++++++++++++++++
  tools/perf/util/thread_map.h             |    4 +
  tools/perf/util/top.c                    |   10 +-
  tools/perf/util/top.h                    |    2 +-
  tools/perf/util/usage.c                  |    6 +-
  tools/perf/util/util.h                   |    2 +-
  17 files changed, 207 insertions(+), 56 deletions(-)

tools/perf/util/thread_map.c +129 -0

+#include <string.h>
 #include "thread_map.h"
+#include "debug.h"
 
  /* Skip "." and ".." directories */
  static int filter(const struct dirent *dir)

@@ -152,6 +155,155 @@ struct thread_map *thread_map__new(pid_t pid, pid_t tid, uid_t uid)
  	return thread_map__new_by_tid(tid);
  }
 
+
+static pid_t get_next_task_id(char **str)
+{
+	char *p, *pend;
+	pid_t ret = -1;
+	bool adv_pend = false;
+
+	pend = p = *str;
+
+	/* empty strings */
+	if (*pend == '\0')
+		return -1;
+
+	/* only expecting digits separated by commas */
+	while (isdigit(*pend))
+		++pend;
+
+	if (*pend == ',') {
+		*pend = '\0';
+		adv_pend = true;
+		/* catch strings ending in a comma - like '1234,' */
+		if (*(pend+1) == '\0') {
+			ui__error("task id strings should not end in a comma\n");
+			goto out;
+		}
+	}
+
+	/* only convert if all characters are valid */
+	if (*pend == '\0') {
+		ret = atoi(p);
+		if (adv_pend)
+			pend++;
+		*str = pend;
+	}
+
+out:
+	return ret;
+}
+
+static struct thread_map *thread_map__new_by_pid_str(const char *pid_str)
+{
+	struct thread_map *threads = NULL;
+	char name[256];
+	int items, total_tasks = 0;
+	struct dirent **namelist = NULL;
+	int i, j = 0;
+	char *ostr, *str;
+	pid_t pid;
+
+	ostr = strdup(pid_str);
+	if (!ostr)
+		return NULL;
+	str = ostr;
+
+	while (*str) {
+		pid = get_next_task_id(&str);
+		if (pid<  0) {
+			free(threads);
+			threads = NULL;
+			break;
+		}
+
+		sprintf(name, "/proc/%d/task", pid);
+		items = scandir(name,&namelist, filter, NULL);
+		if (items<= 0)
+			break;
+
+		total_tasks += items;
+		if (threads)
+			threads = realloc(threads,
+					  sizeof(*threads) + sizeof(pid_t)*total_tasks);
+		else
+			threads = malloc(sizeof(*threads) + sizeof(pid_t)*items);
+
+		if (threads) {
+			for (i = 0; i<  items; i++)
+				threads->map[j++] = atoi(namelist[i]->d_name);
+			threads->nr = total_tasks;
+		}
+
+		for (i = 0; i<  items; i++)
+			free(namelist[i]);
+		free(namelist);
+
+		if (!threads)
+			break;
+	}
+
+	free(ostr);
+
+	return threads;
+}
+
+static struct thread_map *thread_map__new_by_tid_str(const char *tid_str)
+{
+	struct thread_map *threads = NULL;
+	char *str;
+	int ntasks = 0;
+	pid_t tid;
+
+	/* perf-stat expects threads to be generated even if tid not given */
+	if (!tid_str) {
+		threads = malloc(sizeof(*threads) + sizeof(pid_t));
+		threads->map[1] = -1;
+		threads->nr	= 1;
+		return threads;
+	}
+
+	str = strdup(tid_str);
+	if (!str)
+		return NULL;
+
+	while (*str) {
+		tid = get_next_task_id(&str);
+		if (tid < 0) {
+			free(threads);
+			threads = NULL;
+			break;
+		}
+
+		ntasks++;
+		if (threads)
+			threads = realloc(threads,
+					  sizeof(*threads) + sizeof(pid_t) * ntasks);
+		else
+			threads = malloc(sizeof(*threads) + sizeof(pid_t));
+

tools/perf/util/thread_map.c +12 -0

+struct thread_map *thread_map__new_str(const char *pid, const char *tid,
+				       uid_t uid)
+{
+	if (pid)
+		return thread_map__new_by_pid_str(pid);
+
+	if (!tid&&  uid != UINT_MAX)
+		return thread_map__new_by_uid(uid);
+
+	return thread_map__new_by_tid_str(tid);
+}
+
  void thread_map__delete(struct thread_map *threads)
  {
  	free(threads);

tools/perf/util/usage.c +1 -1

index d0c0139..1240bd6 100644
@@ -83,7 +83,7 @@ void warning(const char *warn, ...)
  	va_end(params);
  }
 
-uid_t parse_target_uid(const char *str, pid_t tid, pid_t pid)
+uid_t parse_target_uid(const char *str, const char *tid, const char *pid)
  {
  	struct passwd pwd, *result;
  	char buf[1024];

tools/perf/util/usage.c +1 -0

+	if (tid || pid) {
  		ui__warning("PID/TID switch overriding UID\n");
  		sleep(1);
  		return UINT_MAX;

tools/perf/util/util.h +1 -1

index 232d17e..7917b09 100644
@@ -245,7 +245,7 @@ struct perf_event_attr;
 
  void event_attr_init(struct perf_event_attr *attr);
 
-uid_t parse_target_uid(const char *str, pid_t tid, pid_t pid);
+uid_t parse_target_uid(const char *str, const char *tid, const char *pid);
 
  #define _STR(x) #x
  #define STR(x) _STR(x)

tools/perf/util/thread_map.c line 9

Quoted 9 lines

tools/perf/util/thread_map.c +1 -0

index 3d4b6c5..e793c16 100644
@@ -6,7 +6,10 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <unistd.h>
+#include <ctype.h>

I was trying to remove ctype.h, you might use util.h here.

Quoted 139 lines

tools/perf/util/thread_map.c +129 -0

+#include <string.h>
 #include "thread_map.h"
+#include "debug.h"
 
  /* Skip "." and ".." directories */
  static int filter(const struct dirent *dir)

@@ -152,6 +155,155 @@ struct thread_map *thread_map__new(pid_t pid, pid_t tid, uid_t uid)
  	return thread_map__new_by_tid(tid);
  }
 
+
+static pid_t get_next_task_id(char **str)
+{
+	char *p, *pend;
+	pid_t ret = -1;
+	bool adv_pend = false;
+
+	pend = p = *str;
+
+	/* empty strings */
+	if (*pend == '\0')
+		return -1;
+
+	/* only expecting digits separated by commas */
+	while (isdigit(*pend))
+		++pend;
+
+	if (*pend == ',') {
+		*pend = '\0';
+		adv_pend = true;
+		/* catch strings ending in a comma - like '1234,' */
+		if (*(pend+1) == '\0') {
+			ui__error("task id strings should not end in a comma\n");
+			goto out;
+		}
+	}
+
+	/* only convert if all characters are valid */
+	if (*pend == '\0') {
+		ret = atoi(p);
+		if (adv_pend)
+			pend++;
+		*str = pend;
+	}
+
+out:
+	return ret;
+}
+
+static struct thread_map *thread_map__new_by_pid_str(const char *pid_str)
+{
+	struct thread_map *threads = NULL;
+	char name[256];
+	int items, total_tasks = 0;
+	struct dirent **namelist = NULL;
+	int i, j = 0;
+	char *ostr, *str;
+	pid_t pid;
+
+	ostr = strdup(pid_str);
+	if (!ostr)
+		return NULL;
+	str = ostr;
+
+	while (*str) {
+		pid = get_next_task_id(&str);
+		if (pid<  0) {
+			free(threads);
+			threads = NULL;
+			break;
+		}
+
+		sprintf(name, "/proc/%d/task", pid);
+		items = scandir(name,&namelist, filter, NULL);
+		if (items<= 0)
+			break;
+
+		total_tasks += items;
+		if (threads)
+			threads = realloc(threads,
+					  sizeof(*threads) + sizeof(pid_t)*total_tasks);
+		else
+			threads = malloc(sizeof(*threads) + sizeof(pid_t)*items);
+
+		if (threads) {
+			for (i = 0; i<  items; i++)
+				threads->map[j++] = atoi(namelist[i]->d_name);
+			threads->nr = total_tasks;
+		}
+
+		for (i = 0; i<  items; i++)
+			free(namelist[i]);
+		free(namelist);
+
+		if (!threads)
+			break;
+	}
+
+	free(ostr);
+
+	return threads;
+}
+
+static struct thread_map *thread_map__new_by_tid_str(const char *tid_str)
+{
+	struct thread_map *threads = NULL;
+	char *str;
+	int ntasks = 0;
+	pid_t tid;
+
+	/* perf-stat expects threads to be generated even if tid not given */
+	if (!tid_str) {
+		threads = malloc(sizeof(*threads) + sizeof(pid_t));
+		threads->map[1] = -1;
+		threads->nr	= 1;
+		return threads;
+	}
+
+	str = strdup(tid_str);
+	if (!str)
+		return NULL;
+
+	while (*str) {
+		tid = get_next_task_id(&str);
+		if (tid < 0) {
+			free(threads);
+			threads = NULL;
+			break;
+		}
+
+		ntasks++;
+		if (threads)
+			threads = realloc(threads,
+					  sizeof(*threads) + sizeof(pid_t) * ntasks);
+		else
+			threads = malloc(sizeof(*threads) + sizeof(pid_t));
+

tools/perf/util/thread_map.c struct thread_map *thread_map__new(pid_t pid, pid_t tid, uid_t uid) · line 294

Quoted 10 lines

tools/perf/util/thread_map.c +10 -0

+		if (threads == NULL)
+			break;
+
+		threads->map[ntasks-1] = tid;
+		threads->nr	= ntasks;
+	}
+
+	return threads;
+}
+

This will ends up being a code duplication. How about something like below?

	while (*str) {
		tid = get_next_task_id(&str);
		if (tid < 0)
			goto out_err;

		tmp = thread_map__new_by_tid(tid); /* and for pid too */
		if (tmp == NULL)
			goto out_err;
			
		threads = thread_map__merge(threads, tmp); /* should be implemented */
		if (threads == NULL)
			break;
	}

Quoted 16 lines

tools/perf/util/thread_map.c +12 -0

+struct thread_map *thread_map__new_str(const char *pid, const char *tid,
+				       uid_t uid)
+{
+	if (pid)
+		return thread_map__new_by_pid_str(pid);
+
+	if (!tid&&  uid != UINT_MAX)
+		return thread_map__new_by_uid(uid);
+
+	return thread_map__new_by_tid_str(tid);
+}
+
  void thread_map__delete(struct thread_map *threads)
  {
  	free(threads);

[snip]

Quoted 17 lines

tools/perf/util/usage.c +1 -1

index d0c0139..1240bd6 100644
@@ -83,7 +83,7 @@ void warning(const char *warn, ...)
  	va_end(params);
  }
 
-uid_t parse_target_uid(const char *str, pid_t tid, pid_t pid)
+uid_t parse_target_uid(const char *str, const char *tid, const char *pid)
  {
  	struct passwd pwd, *result;
  	char buf[1024];

@@ -91,8 +91,8 @@ uid_t parse_target_uid(const char *str, pid_t tid, pid_t pid)
  	if (str == NULL)
  		return UINT_MAX;
 

tools/perf/util/usage.c uid_t parse_target_uid(const char *str, pid_t tid, pid_t pid) · line 94

Quoted 3 lines

tools/perf/util/usage.c +1 -2

-	/* CPU and PID are mutually exclusive */
-	if (tid > 0 || pid > 0) {
+	/* UUID and PID are mutually exclusive */

s/UUID/UID/ ?

Quoted 11 lines · Namhyung Kim

tools/perf/util/thread_map.c line 9

Quoted 9 lines

tools/perf/util/thread_map.c +1 -0

index 3d4b6c5..e793c16 100644
@@ -6,7 +6,10 @@
  #include<sys/types.h>
  #include<sys/stat.h>
  #include<unistd.h>
+#include<ctype.h>

I was trying to remove ctype.h, you might use util.h here.

Right, knew that. But, in this case I am adding a call to isdigit which means a direct dependency on ctype.h. I would prefer a direct relationship versus an indirect via util.h

Quoted 161 lines

Quoted 139 lines

tools/perf/util/thread_map.c +129 -0

+#include<string.h>
  #include "thread_map.h"
+#include "debug.h"
 
   /* Skip "." and ".." directories */
   static int filter(const struct dirent *dir)

@@ -152,6 +155,155 @@ struct thread_map *thread_map__new(pid_t pid, pid_t tid, uid_t uid)
   	return thread_map__new_by_tid(tid);
   }
 
+
+static pid_t get_next_task_id(char **str)
+{
+	char *p, *pend;
+	pid_t ret = -1;
+	bool adv_pend = false;
+
+	pend = p = *str;
+
+	/* empty strings */
+	if (*pend == '\0')
+		return -1;
+
+	/* only expecting digits separated by commas */
+	while (isdigit(*pend))
+		++pend;
+
+	if (*pend == ',') {
+		*pend = '\0';
+		adv_pend = true;
+		/* catch strings ending in a comma - like '1234,' */
+		if (*(pend+1) == '\0') {
+			ui__error("task id strings should not end in a comma\n");
+			goto out;
+		}
+	}
+
+	/* only convert if all characters are valid */
+	if (*pend == '\0') {
+		ret = atoi(p);
+		if (adv_pend)
+			pend++;
+		*str = pend;
+	}
+
+out:
+	return ret;
+}
+
+static struct thread_map *thread_map__new_by_pid_str(const char *pid_str)
+{
+	struct thread_map *threads = NULL;
+	char name[256];
+	int items, total_tasks = 0;
+	struct dirent **namelist = NULL;
+	int i, j = 0;
+	char *ostr, *str;
+	pid_t pid;
+
+	ostr = strdup(pid_str);
+	if (!ostr)
+		return NULL;
+	str = ostr;
+
+	while (*str) {
+		pid = get_next_task_id(&str);
+		if (pid<   0) {
+			free(threads);
+			threads = NULL;
+			break;
+		}
+
+		sprintf(name, "/proc/%d/task", pid);
+		items = scandir(name,&namelist, filter, NULL);
+		if (items<= 0)
+			break;
+
+		total_tasks += items;
+		if (threads)
+			threads = realloc(threads,
+					  sizeof(*threads) + sizeof(pid_t)*total_tasks);
+		else
+			threads = malloc(sizeof(*threads) + sizeof(pid_t)*items);
+
+		if (threads) {
+			for (i = 0; i<   items; i++)
+				threads->map[j++] = atoi(namelist[i]->d_name);
+			threads->nr = total_tasks;
+		}
+
+		for (i = 0; i<   items; i++)
+			free(namelist[i]);
+		free(namelist);
+
+		if (!threads)
+			break;
+	}
+
+	free(ostr);
+
+	return threads;
+}
+
+static struct thread_map *thread_map__new_by_tid_str(const char *tid_str)
+{
+	struct thread_map *threads = NULL;
+	char *str;
+	int ntasks = 0;
+	pid_t tid;
+
+	/* perf-stat expects threads to be generated even if tid not given */
+	if (!tid_str) {
+		threads = malloc(sizeof(*threads) + sizeof(pid_t));
+		threads->map[1] = -1;
+		threads->nr	= 1;
+		return threads;
+	}
+
+	str = strdup(tid_str);
+	if (!str)
+		return NULL;
+
+	while (*str) {
+		tid = get_next_task_id(&str);
+		if (tid<  0) {
+			free(threads);
+			threads = NULL;
+			break;
+		}
+
+		ntasks++;
+		if (threads)
+			threads = realloc(threads,
+					  sizeof(*threads) + sizeof(pid_t) * ntasks);
+		else
+			threads = malloc(sizeof(*threads) + sizeof(pid_t));
+

tools/perf/util/thread_map.c struct thread_map *thread_map__new(pid_t pid, pid_t tid, uid_t uid) · line 294

Quoted 10 lines

tools/perf/util/thread_map.c +10 -0

+		if (threads == NULL)
+			break;
+
+		threads->map[ntasks-1] = tid;
+		threads->nr	= ntasks;
+	}
+
+	return threads;
+}
+

This will ends up being a code duplication. How about something like below?

	while (*str) {
		tid = get_next_task_id(&str);
		if (tid<  0)
			goto out_err;

		tmp = thread_map__new_by_tid(tid); /* and for pid too */
		if (tmp == NULL)
			goto out_err;

Quoted 5 lines

		threads = thread_map__merge(threads, tmp); /* should be implemented */
		if (threads == NULL)
			break;
	}

The pid and tid functions are implemented differently. The commonality is parsing a CSV string in a while loop.

Oh, I meant this:

static struct thread_map *thread_map__new_by_pid_str(const char *pid_str)

{
	...
	while (*str) {
		...
		tmp = thread_map__new_by_pid(pid);
		...
	}
	...
}

static struct thread_map *thread_map__new_by_tid_str(const char *tid_str)

{
	...
	while (*str) {
		...
		tmp = thread_map__new_by_tid(tid);
		...
	}
	...
}

Quoted 1 line

I was trying to remove ctype.h, you might use util.h here.

Right, knew that. But, in this case I am adding a call to isdigit which means a direct dependency on ctype.h. I would prefer a direct relationship versus an indirect via util.h

Quoted 6 lines

Quoted 2 lines

I was trying to remove ctype.h, you might use util.h here.

Right, knew that. But, in this case I am adding a call to isdigit which means a direct dependency on ctype.h. I would prefer a direct relationship versus an indirect via util.h

Please just remove ctype.h *altogether* from perf, it's just an insane header.

Have a look at how Git solves these types of problems, it defines sane string functions in git-compat-util.h:

/* Sane ctype - no locale, and works with signed chars */ #undef isascii
#undef isspace
#undef isdigit
#undef isalpha
#undef isalnum
#undef tolower
#undef toupper
extern unsigned char sane_ctype[256];
#define GIT_SPACE 0x01
#define GIT_DIGIT 0x02
#define GIT_ALPHA 0x04
#define GIT_GLOB_SPECIAL 0x08
#define GIT_REGEX_SPECIAL 0x10

Thanks,

Ingo

Em Thu, Feb 09, 2012 at 08:37:27AM +0100, Ingo Molnar escreveu:

Quoted 12 lines · David Ahern

Quoted 6 lines

Quoted 2 lines

I was trying to remove ctype.h, you might use util.h here.

Right, knew that. But, in this case I am adding a call to isdigit which means a direct dependency on ctype.h. I would prefer a direct relationship versus an indirect via util.h

Please just remove ctype.h *altogether* from perf, it's just an insane header.

Quoted 2 lines

Have a look at how Git solves these types of problems, it defines sane string functions in git-compat-util.h:

Yeah, these are in util.h, that doesn't includes ctype.h

I'm fixing this up and also that s/UUID/UID/g Kim pointed out, then testing if the python binding still is ok with these changes.

Em Thu, Feb 09, 2012 at 12:34:49PM -0200, Arnaldo Carvalho de Melo escreveu:

Quoted 13 lines

Em Thu, Feb 09, 2012 at 08:37:27AM +0100, Ingo Molnar escreveu:

Quoted 12 lines · David Ahern

Quoted 6 lines

Quoted 2 lines

I was trying to remove ctype.h, you might use util.h here.

Right, knew that. But, in this case I am adding a call to isdigit which means a direct dependency on ctype.h. I would prefer a direct relationship versus an indirect via util.h

Please just remove ctype.h *altogether* from perf, it's just an insane header.

Quoted 7 lines

Quoted 2 lines

Have a look at how Git solves these types of problems, it defines sane string functions in git-compat-util.h:

Yeah, these are in util.h, that doesn't includes ctype.h

I'm fixing this up and also that s/UUID/UID/g Kim pointed out, then testing if the python binding still is ok with these changes.

[root@aninha linux]# tools/perf/python/twatch.py
Traceback (most recent call last):
File "tools/perf/python/twatch.py", line 16, in<module>
import perf
ImportError: /home/acme/git/build/perf/python/perf.so: undefined symbol:
ui__error
[root@aninha linux]#

it breaks, I'll check an alternative way to report problems without calling ui__ methods from thread_map.

- Arnaldo

Quoted 8 lines · Arnaldo Carvalho de Melo

[root@aninha linux]# tools/perf/python/twatch.py
Traceback (most recent call last):
File "tools/perf/python/twatch.py", line 16, in<module>
import perf
ImportError: /home/acme/git/build/perf/python/perf.so: undefined symbol: ui__error

it breaks, I'll check an alternative way to report problems without calling ui__ methods from thread_map.

I have a different result:

 $ git checkout tip/perf/core
 HEAD is now at c98fdeaa9273... x86/sched/perf/AMD: Set sched_clock_stable
 $ patch -p1 < perf-allow-multiple-threads.patch
 patching file tools/perf/Documentation/perf-record.txt

 $ cd tools/perf
 $ make -j8

 $ sudo python/twatch.py
 Traceback (most recent call last):
   File "python/twatch.py", line 41, in <module>
     main()
   File "python/twatch.py", line 25, in main
     evsel.open(cpus = cpus, threads = threads);
 OSError: [Errno 22] Invalid argument

Allow a user to collect events for multiple threads or processes using a comma separated list.

e.g., collect data on a VM and its vhost thread:
perf top -p 21483,21485
perf stat -p 21483,21485 -ddd
perf record -p 21483,21485

or monitoring vcpu threads
perf top -t 21488,21489
perf stat -t 21488,21489 -ddd
perf record -t 21488,21489

tools/perf/util/thread_map.c +3 -0

index 3d4b6c5..e793c16 100644
+static struct thread_map *thread_map__new_by_pid_str(const char *pid_str)
+{
+	struct thread_map *threads = NULL;

Em Wed, Feb 08, 2012 at 09:32:52AM -0700, David Ahern escreveu:

Quoted 8 lines

Allow a user to collect events for multiple threads or processes using a comma separated list.

e.g., collect data on a VM and its vhost thread:
perf top -p 21483,21485
perf stat -p 21483,21485 -ddd
perf record -p 21483,21485

Quoted 4 lines

or monitoring vcpu threads
perf top -t 21488,21489
perf stat -t 21488,21489 -ddd
perf record -t 21488,21489

I found some problems below:

Quoted 7 lines

tools/perf/util/thread_map.c +3 -0

index 3d4b6c5..e793c16 100644
+static struct thread_map *thread_map__new_by_pid_str(const char *pid_str)
+{
+	struct thread_map *threads = NULL;

<SNIP>

tools/perf/util/thread_map.c

Quoted 5 lines

tools/perf/util/thread_map.c +5 -0

+		if (threads)
+			threads = realloc(threads,
+					  sizeof(*threads) + sizeof(pid_t)*total_tasks);
+		else
+			threads = malloc(sizeof(*threads) + sizeof(pid_t)*items);

If realloc fails and threads was allocated before... we leak memory.

We don't need to use this if clause, realloc handles threads == NULL just fines and then works as malloc.

Also I didn't use ctype altogether + that CSV parsing routine, we have strlist for that, it even will allow us to trow away duplicate pids/tids and also supports passing a file with a list of threads to monitor, for free.

Take a look at the modified patch below, if you're ok with it I can keep your autorship and put a [committer note: use strlist] or take autorship and state that it was based on a patch made by you, definetely your call. I'd go for the committer note.

- Arnaldo

tools/perf/Documentation/perf-record.txt +2 -2

index ff9a66e..a5766b4 100644
@@ -52,11 +52,11 @@ OPTIONS
 
 -p::
 --pid=::
-	Record events on existing process ID.
+	Record events on existing process ID (comma separated list).
 
 -t::
 --tid=::
-        Record events on existing thread ID.
+        Record events on existing thread ID (comma separated list).
 
 -u::
 --uid=::

tools/perf/Documentation/perf-stat.txt +2 -2

index 8966b9a..2fa173b 100644
@@ -35,11 +35,11 @@ OPTIONS
         child tasks do not inherit counters
 -p::
 --pid=<pid>::
-        stat events on existing process id
+        stat events on existing process id (comma separated list)
 
 -t::
 --tid=<tid>::
-        stat events on existing thread id
+        stat events on existing thread id (comma separated list)
 
 
 -a::

tools/perf/Documentation/perf-top.txt +2 -2

index ab1454e..4a5680c 100644
@@ -72,11 +72,11 @@ Default is to monitor all CPUS.
 
 -p <pid>::
 --pid=<pid>::
-	Profile events on existing Process ID.
+	Profile events on existing Process ID (comma separated list).
 
 -t <tid>::
 --tid=<tid>::
-        Profile events on existing thread ID.
+        Profile events on existing thread ID (comma separated list).
 
 -u::
 --uid=::

tools/perf/builtin-record.c +4 -6

index d6d1c6c..08ed24b 100644
@@ -645,8 +645,6 @@ static const char * const record_usage[] = {
  */
 static struct perf_record record = {
 	.opts = {
-		.target_pid	     = -1,
-		.target_tid	     = -1,
 		.mmap_pages	     = UINT_MAX,
 		.user_freq	     = UINT_MAX,
 		.user_interval	     = ULLONG_MAX,

@@ -670,9 +668,9 @@ const struct option record_options[] = {
 		     parse_events_option),
 	OPT_CALLBACK(0, "filter", &record.evlist, "filter",
 		     "event filter", parse_filter),
-	OPT_INTEGER('p', "pid", &record.opts.target_pid,
+	OPT_STRING('p', "pid", &record.opts.target_pid, "pid",
 		    "record events on existing process id"),
-	OPT_INTEGER('t', "tid", &record.opts.target_tid,
+	OPT_STRING('t', "tid", &record.opts.target_tid, "tid",
 		    "record events on existing thread id"),
 	OPT_INTEGER('r', "realtime", &record.realtime_prio,
 		    "collect data with this RT SCHED_FIFO priority"),

@@ -739,7 +737,7 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
 
 	argc = parse_options(argc, argv, record_options, record_usage,
 			    PARSE_OPT_STOP_AT_NON_OPTION);
-	if (!argc && rec->opts.target_pid == -1 && rec->opts.target_tid == -1 &&
+	if (!argc && !rec->opts.target_pid && !rec->opts.target_tid &&
 		!rec->opts.system_wide && !rec->opts.cpu_list && !rec->uid_str)
 		usage_with_options(record_usage, record_options);
 

@@ -785,7 +783,7 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
 	if (rec->uid_str != NULL && rec->opts.uid == UINT_MAX - 1)
 		goto out_free_fd;
 
-	if (rec->opts.target_pid != -1)
+	if (rec->opts.target_pid)
 		rec->opts.target_tid = rec->opts.target_pid;
 
 	if (perf_evlist__create_maps(evsel_list, rec->opts.target_pid,

tools/perf/builtin-stat.c +16 -15

index d14b37a..ea40e4e 100644
@@ -182,8 +182,8 @@ static int			run_count			=  1;
 static bool			no_inherit			= false;
 static bool			scale				=  true;
 static bool			no_aggr				= false;
-static pid_t			target_pid			= -1;
-static pid_t			target_tid			= -1;
+static const char		*target_pid;
+static const char		*target_tid;
 static pid_t			child_pid			= -1;
 static bool			null_run			=  false;
 static int			detailed_run			=  0;

@@ -296,7 +296,7 @@ static int create_perf_stat_counter(struct perf_evsel *evsel,
 	if (system_wide)
 		return perf_evsel__open_per_cpu(evsel, evsel_list->cpus,
 						group, group_fd);
-	if (target_pid == -1 && target_tid == -1) {
+	if (!target_pid && !target_tid) {
 		attr->disabled = 1;
 		attr->enable_on_exec = 1;
 	}

@@ -446,7 +446,7 @@ static int run_perf_stat(int argc __used, const char **argv)
 			exit(-1);
 		}
 
-		if (target_tid == -1 && target_pid == -1 && !system_wide)
+		if (!target_tid && !target_pid && !system_wide)
 			evsel_list->threads->map[0] = child_pid;
 
 		/*

@@ -968,14 +968,14 @@ static void print_stat(int argc, const char **argv)
 	if (!csv_output) {
 		fprintf(output, "\n");
 		fprintf(output, " Performance counter stats for ");
-		if(target_pid == -1 && target_tid == -1) {
+		if (!target_pid && !target_tid) {
 			fprintf(output, "\'%s", argv[0]);
 			for (i = 1; i < argc; i++)
 				fprintf(output, " %s", argv[i]);
-		} else if (target_pid != -1)
-			fprintf(output, "process id \'%d", target_pid);
+		} else if (target_pid)
+			fprintf(output, "process id \'%s", target_pid);
 		else
-			fprintf(output, "thread id \'%d", target_tid);
+			fprintf(output, "thread id \'%s", target_tid);
 
 		fprintf(output, "\'");
 		if (run_count > 1)

@@ -1049,10 +1049,10 @@ static const struct option options[] = {
 		     "event filter", parse_filter),
 	OPT_BOOLEAN('i', "no-inherit", &no_inherit,
 		    "child tasks do not inherit counters"),
-	OPT_INTEGER('p', "pid", &target_pid,
-		    "stat events on existing process id"),
-	OPT_INTEGER('t', "tid", &target_tid,
-		    "stat events on existing thread id"),
+	OPT_STRING('p', "pid", &target_pid, "pid",
+		   "stat events on existing process id"),
+	OPT_STRING('t', "tid", &target_tid, "tid",
+		   "stat events on existing thread id"),
 	OPT_BOOLEAN('a', "all-cpus", &system_wide,
 		    "system-wide collection from all CPUs"),
 	OPT_BOOLEAN('g', "group", &group,

@@ -1190,7 +1190,7 @@ int cmd_stat(int argc, const char **argv, const char *prefix __used)
 	} else if (big_num_opt == 0) /* User passed --no-big-num */
 		big_num = false;
 
-	if (!argc && target_pid == -1 && target_tid == -1)
+	if (!argc && !target_pid && !target_tid)
 		usage_with_options(stat_usage, options);
 	if (run_count <= 0)
 		usage_with_options(stat_usage, options);

@@ -1206,10 +1206,11 @@ int cmd_stat(int argc, const char **argv, const char *prefix __used)
 	if (add_default_attributes())
 		goto out;
 
-	if (target_pid != -1)
+	if (target_pid)
 		target_tid = target_pid;
 
-	evsel_list->threads = thread_map__new(target_pid, target_tid, UINT_MAX);
+	evsel_list->threads = thread_map__new_str(target_pid,
+						  target_tid, UINT_MAX);
 	if (evsel_list->threads == NULL) {
 		pr_err("Problems finding threads of monitor\n");
 		usage_with_options(stat_usage, options);

tools/perf/builtin-test.c +0 -2

index 70c4eb2..0f15195 100644
@@ -1010,8 +1010,6 @@ realloc:
 static int test__PERF_RECORD(void)
 {
 	struct perf_record_opts opts = {
-		.target_pid = -1,
-		.target_tid = -1,
 		.no_delay   = true,
 		.freq	    = 10,
 		.mmap_pages = 256,

tools/perf/builtin-top.c +6 -8

index d869b21..5ed62a8 100644
@@ -965,7 +965,7 @@ static int __cmd_top(struct perf_top *top)
 	if (ret)
 		goto out_delete;
 
-	if (top->target_tid != -1 || top->uid != UINT_MAX)
+	if (top->target_tid || top->uid != UINT_MAX)
 		perf_event__synthesize_thread_map(&top->tool, top->evlist->threads,
 						  perf_event__process,
 						  &top->session->host_machine);

@@ -1103,8 +1103,6 @@ int cmd_top(int argc, const char **argv, const char *prefix __used)
 	struct perf_top top = {
 		.count_filter	     = 5,
 		.delay_secs	     = 2,
-		.target_pid	     = -1,
-		.target_tid	     = -1,
 		.uid		     = UINT_MAX,
 		.freq		     = 1000, /* 1 KHz */
 		.sample_id_all_avail = true,

@@ -1118,9 +1116,9 @@ int cmd_top(int argc, const char **argv, const char *prefix __used)
 		     parse_events_option),
 	OPT_INTEGER('c', "count", &top.default_interval,
 		    "event period to sample"),
-	OPT_INTEGER('p', "pid", &top.target_pid,
+	OPT_STRING('p', "pid", &top.target_pid, "pid",
 		    "profile events on existing process id"),
-	OPT_INTEGER('t', "tid", &top.target_tid,
+	OPT_STRING('t', "tid", &top.target_tid, "tid",
 		    "profile events on existing thread id"),
 	OPT_BOOLEAN('a', "all-cpus", &top.system_wide,
 			    "system-wide collection from all CPUs"),

@@ -1210,19 +1208,19 @@ int cmd_top(int argc, const char **argv, const char *prefix __used)
 		goto out_delete_evlist;
 
 	/* CPU and PID are mutually exclusive */
-	if (top.target_tid > 0 && top.cpu_list) {
+	if (top.target_tid && top.cpu_list) {
 		printf("WARNING: PID switch overriding CPU\n");
 		sleep(1);
 		top.cpu_list = NULL;
 	}
 
-	if (top.target_pid != -1)
+	if (top.target_pid)
 		top.target_tid = top.target_pid;
 
 	if (perf_evlist__create_maps(top.evlist, top.target_pid,
 				     top.target_tid, top.uid, top.cpu_list) < 0)
 		usage_with_options(top_usage, options);
-
+	
 	if (!top.evlist->nr_entries &&
 	    perf_evlist__add_default(top.evlist) < 0) {
 		pr_err("Not enough memory for event selector list\n");

tools/perf/perf.h +2 -2

index 92af168..deb17db 100644
@@ -186,8 +186,8 @@ extern const char perf_version_string[];
 void pthread__unblock_sigwinch(void);
 
 struct perf_record_opts {
-	pid_t	     target_pid;
-	pid_t	     target_tid;
+	const char   *target_pid;
+	const char   *target_tid;
 	uid_t	     uid;
 	bool	     call_graph;
 	bool	     group;

tools/perf/util/evlist.c +5 -5

index a57a8cf..5c61dc5 100644
@@ -593,15 +593,15 @@ int perf_evlist__mmap(struct perf_evlist *evlist, unsigned int pages,
 	return perf_evlist__mmap_per_cpu(evlist, prot, mask);
 }
 
-int perf_evlist__create_maps(struct perf_evlist *evlist, pid_t target_pid,
-			     pid_t target_tid, uid_t uid, const char *cpu_list)
+int perf_evlist__create_maps(struct perf_evlist *evlist, const char *target_pid,
+			     const char *target_tid, uid_t uid, const char *cpu_list)
 {
-	evlist->threads = thread_map__new(target_pid, target_tid, uid);
+	evlist->threads = thread_map__new_str(target_pid, target_tid, uid);
 
 	if (evlist->threads == NULL)
 		return -1;
 
-	if (uid != UINT_MAX || (cpu_list == NULL && target_tid != -1))
+	if (uid != UINT_MAX || (cpu_list == NULL && target_tid))
 		evlist->cpus = cpu_map__dummy_new();
 	else
 		evlist->cpus = cpu_map__new(cpu_list);

@@ -820,7 +820,7 @@ int perf_evlist__prepare_workload(struct perf_evlist *evlist,
 		exit(-1);
 	}
 
-	if (!opts->system_wide && opts->target_tid == -1 && opts->target_pid == -1)
+	if (!opts->system_wide && !opts->target_tid && !opts->target_pid)
 		evlist->threads->map[0] = evlist->workload.pid;
 
 	close(child_ready_pipe[1]);

tools/perf/util/evlist.h +2 -2

index 1b4282b..21f1c9e 100644
@@ -106,8 +106,8 @@ static inline void perf_evlist__set_maps(struct perf_evlist *evlist,
 	evlist->threads	= threads;
 }
 
-int perf_evlist__create_maps(struct perf_evlist *evlist, pid_t target_pid,
-			     pid_t tid, uid_t uid, const char *cpu_list);
+int perf_evlist__create_maps(struct perf_evlist *evlist, const char *target_pid,
+			     const char *tid, uid_t uid, const char *cpu_list);
 void perf_evlist__delete_maps(struct perf_evlist *evlist);
 int perf_evlist__set_filters(struct perf_evlist *evlist);
 

tools/perf/util/evsel.c +1 -1

index 9a11f9e..f910f50 100644
@@ -130,7 +130,7 @@ void perf_evsel__config(struct perf_evsel *evsel, struct perf_record_opts *opts)
 	attr->mmap = track;
 	attr->comm = track;
 
-	if (opts->target_pid == -1 && opts->target_tid == -1 && !opts->system_wide) {
+	if (!opts->target_pid && !opts->target_tid && !opts->system_wide) {
 		attr->disabled = 1;
 		attr->enable_on_exec = 1;
 	}

tools/perf/util/setup.py +2 -1

index 36d4c56..4f85b8f 100644
@@ -28,7 +28,8 @@ perf = Extension('perf',
 		  sources = ['util/python.c', 'util/ctype.c', 'util/evlist.c',
 			     'util/evsel.c', 'util/cpumap.c', 'util/thread_map.c',
 			     'util/util.c', 'util/xyarray.c', 'util/cgroup.c',
-			     'util/debugfs.c'],
+			     'util/debugfs.c', '../../lib/rbtree.c',
+			     'util/strlist.c'],
 		  include_dirs = ['util/include'],
 		  extra_compile_args = cflags,
                  )

tools/perf/util/thread_map.c +128 -0

index 3d4b6c5..884f7e2 100644
@@ -6,6 +6,8 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <unistd.h>
+#include "strlist.h"
+#include <string.h>
 #include "thread_map.h"
 
 /* Skip "." and ".." directories */

@@ -152,6 +154,132 @@ struct thread_map *thread_map__new(pid_t pid, pid_t tid, uid_t uid)
 	return thread_map__new_by_tid(tid);
 }
 
+static struct thread_map *thread_map__new_by_pid_str(const char *pid_str)
+{
+	struct thread_map *threads = NULL, *nt;
+	char name[256];
+	int items, total_tasks = 0;
+	struct dirent **namelist = NULL;
+	int i, j = 0;
+	pid_t pid, prev_pid = LONG_MAX;
+	char *end_ptr;
+	struct str_node *pos;
+	struct strlist *slist = strlist__new(false, pid_str);
+
+	if (!slist)
+		return NULL;
+
+	strlist__for_each(pos, slist) {
+		pid = strtol(pos->s, &end_ptr, 10);
+
+		if (pid == LONG_MIN || pid == LONG_MAX ||
+		    (*end_ptr != '\0' && *end_ptr != ','))
+			goto out_free_threads;
+
+		if (pid == prev_pid)
+			continue;
+
+		sprintf(name, "/proc/%d/task", pid);
+		items = scandir(name, &namelist, filter, NULL);
+		if (items <= 0)
+			break;
+
+		total_tasks += items;
+		nt = realloc(threads, (sizeof(*threads) +
+				       sizeof(pid_t) * total_tasks));
+		if (nt == NULL)
+			goto out_free_threads;
+
+		threads = nt;
+
+		if (threads) {
+			for (i = 0; i < items; i++)
+				threads->map[j++] = atoi(namelist[i]->d_name);
+			threads->nr = total_tasks;
+		}
+
+		for (i = 0; i < items; i++)
+			free(namelist[i]);
+		free(namelist);
+
+		if (!threads)
+			break;
+	}
+
+out:
+	strlist__delete(slist);
+	return threads;
+
+out_free_threads:
+	free(threads);
+	threads = NULL;
+	goto out;
+}
+
+static struct thread_map *thread_map__new_by_tid_str(const char *tid_str)
+{
+	struct thread_map *threads = NULL, *nt;
+	int ntasks = 0;
+	pid_t tid, prev_tid = LONG_MAX;
+	char *end_ptr;
+	struct str_node *pos;
+	struct strlist *slist;
+
+	/* perf-stat expects threads to be generated even if tid not given */
+	if (!tid_str) {
+		threads = malloc(sizeof(*threads) + sizeof(pid_t));
+		if (threads != NULL) {
+			threads->map[1] = -1;
+			threads->nr	= 1;
+		}
+		return threads;
+	}
+
+	slist = strlist__new(false, tid_str);
+	if (!slist)
+		return NULL;
+
+	strlist__for_each(pos, slist) {
+		tid = strtol(pos->s, &end_ptr, 10);
+
+		if (tid == LONG_MIN || tid == LONG_MAX ||
+		    (*end_ptr != '\0' && *end_ptr != ','))
+			goto out_free_threads;
+
+		if (tid == prev_tid)
+			continue;
+
+		ntasks++;
+		nt = realloc(threads, sizeof(*threads) + sizeof(pid_t) * ntasks);
+
+		if (nt == NULL)
+			goto out_free_threads;
+
+		threads = nt;
+		threads->map[ntasks - 1] = tid;
+		threads->nr		 = ntasks;
+	}
+out:
+	return threads;
+
+out_free_threads:
+	free(threads);
+	threads = NULL;
+	goto out;
+}
+
+struct thread_map *thread_map__new_str(const char *pid, const char *tid,
+				       uid_t uid)
+{
+	if (pid)
+		return thread_map__new_by_pid_str(pid);
+
+	if (!tid && uid != UINT_MAX)
+		return thread_map__new_by_uid(uid);
+
+	return thread_map__new_by_tid_str(tid);
+}
+
 void thread_map__delete(struct thread_map *threads)
 {
 	free(threads);

tools/perf/util/thread_map.h +4 -0

index c75ddba..7da80f1 100644
@@ -13,6 +13,10 @@ struct thread_map *thread_map__new_by_pid(pid_t pid);
 struct thread_map *thread_map__new_by_tid(pid_t tid);
 struct thread_map *thread_map__new_by_uid(uid_t uid);
 struct thread_map *thread_map__new(pid_t pid, pid_t tid, uid_t uid);
+
+struct thread_map *thread_map__new_str(const char *pid,
+		const char *tid, uid_t uid);
+
 void thread_map__delete(struct thread_map *threads);
 
 size_t thread_map__fprintf(struct thread_map *threads, FILE *fp);

tools/perf/util/top.c +5 -5

index e4370ca..09fe579 100644
@@ -69,11 +69,11 @@ size_t perf_top__header_snprintf(struct perf_top *top, char *bf, size_t size)
 
 	ret += SNPRINTF(bf + ret, size - ret, "], ");
 
-	if (top->target_pid != -1)
-		ret += SNPRINTF(bf + ret, size - ret, " (target_pid: %d",
+	if (top->target_pid)
+		ret += SNPRINTF(bf + ret, size - ret, " (target_pid: %s",
 				top->target_pid);
-	else if (top->target_tid != -1)
-		ret += SNPRINTF(bf + ret, size - ret, " (target_tid: %d",
+	else if (top->target_tid)
+		ret += SNPRINTF(bf + ret, size - ret, " (target_tid: %s",
 				top->target_tid);
 	else if (top->uid_str != NULL)
 		ret += SNPRINTF(bf + ret, size - ret, " (uid: %s",

@@ -85,7 +85,7 @@ size_t perf_top__header_snprintf(struct perf_top *top, char *bf, size_t size)
 		ret += SNPRINTF(bf + ret, size - ret, ", CPU%s: %s)",
 				top->evlist->cpus->nr > 1 ? "s" : "", top->cpu_list);
 	else {
-		if (top->target_tid != -1)
+		if (top->target_tid)
 			ret += SNPRINTF(bf + ret, size - ret, ")");
 		else
 			ret += SNPRINTF(bf + ret, size - ret, ", %d CPU%s)",

tools/perf/util/top.h +1 -1

index def3e53..49eb848 100644
@@ -23,7 +23,7 @@ struct perf_top {
 	u64		   guest_us_samples, guest_kernel_samples;
 	int		   print_entries, count_filter, delay_secs;
 	int		   freq;
-	pid_t		   target_pid, target_tid;
+	const char	   *target_pid, *target_tid;
 	uid_t		   uid;
 	bool		   hide_kernel_symbols, hide_user_symbols, zero;
 	bool		   system_wide;

tools/perf/util/usage.c +3 -3

index d0c0139..52bb07c 100644
@@ -83,7 +83,7 @@ void warning(const char *warn, ...)
 	va_end(params);
 }
 
-uid_t parse_target_uid(const char *str, pid_t tid, pid_t pid)
+uid_t parse_target_uid(const char *str, const char *tid, const char *pid)
 {
 	struct passwd pwd, *result;
 	char buf[1024];

@@ -91,8 +91,8 @@ uid_t parse_target_uid(const char *str, pid_t tid, pid_t pid)
 	if (str == NULL)
 		return UINT_MAX;
 
-	/* CPU and PID are mutually exclusive */
-	if (tid > 0 || pid > 0) {
+	/* UID and PID are mutually exclusive */
+	if (tid || pid) {
 		ui__warning("PID/TID switch overriding UID\n");
 		sleep(1);
 		return UINT_MAX;

tools/perf/util/util.h +1 -1

index 232d17e..7917b09 100644
@@ -245,7 +245,7 @@ struct perf_event_attr;
 
 void event_attr_init(struct perf_event_attr *attr);
 
-uid_t parse_target_uid(const char *str, pid_t tid, pid_t pid);
+uid_t parse_target_uid(const char *str, const char *tid, const char *pid);
 
 #define _STR(x) #x
 #define STR(x) _STR(x)

Quoted 24 lines

Em Wed, Feb 08, 2012 at 09:32:52AM -0700, David Ahern escreveu:

Quoted 8 lines

Allow a user to collect events for multiple threads or processes using a comma separated list.

e.g., collect data on a VM and its vhost thread:
perf top -p 21483,21485
perf stat -p 21483,21485 -ddd
perf record -p 21483,21485

Quoted 4 lines

or monitoring vcpu threads
perf top -t 21488,21489
perf stat -t 21488,21489 -ddd
perf record -t 21488,21489

I found some problems below:

Quoted 7 lines

tools/perf/util/thread_map.c +3 -0

index 3d4b6c5..e793c16 100644
+static struct thread_map *thread_map__new_by_pid_str(const char *pid_str)
+{
+	struct thread_map *threads = NULL;

<SNIP>

Quoted 7 lines

tools/perf/util/thread_map.c

Quoted 5 lines

tools/perf/util/thread_map.c +5 -0

+		if (threads)
+			threads = realloc(threads,
+					  sizeof(*threads) + sizeof(pid_t)*total_tasks);
+		else
+			threads = malloc(sizeof(*threads) + sizeof(pid_t)*items);

If realloc fails and threads was allocated before... we leak memory.

ok.

Quoted 3 lines

We don't need to use this if clause, realloc handles threads == NULL just fines and then works as malloc.

ok.

Quoted 5 lines

Also I didn't use ctype altogether + that CSV parsing routine, we have strlist for that, it even will allow us to trow away duplicate pids/tids and also supports passing a file with a list of threads to monitor, for free.

Interesting. I did look at strlist before going with the string parsing. It was not obvious to me how to use it for this case.

Quoted 1 line

Quoted 4 lines

Take a look at the modified patch below, if you're ok with it I can keep your autorship and put a [committer note: use strlist] or take autorship and state that it was based on a patch made by you, definetely your call. I'd go for the committer note.

I'm fine with either one. Patch below looks fine. If needed:

Acked-by David Ahern <[email protected]>

Em Fri, Feb 10, 2012 at 12:32:09PM -0700, David Ahern escreveu:

Quoted 60 lines · Arnaldo Carvalho de Melo

Quoted 24 lines

Em Wed, Feb 08, 2012 at 09:32:52AM -0700, David Ahern escreveu:

Quoted 8 lines

Allow a user to collect events for multiple threads or processes using a comma separated list.

e.g., collect data on a VM and its vhost thread:
perf top -p 21483,21485
perf stat -p 21483,21485 -ddd
perf record -p 21483,21485

Quoted 4 lines

or monitoring vcpu threads
perf top -t 21488,21489
perf stat -t 21488,21489 -ddd
perf record -t 21488,21489

I found some problems below:

Quoted 7 lines

tools/perf/util/thread_map.c +3 -0

index 3d4b6c5..e793c16 100644
+static struct thread_map *thread_map__new_by_pid_str(const char *pid_str)
+{
+	struct thread_map *threads = NULL;

<SNIP>

Quoted 7 lines

tools/perf/util/thread_map.c

Quoted 5 lines

tools/perf/util/thread_map.c +5 -0

+		if (threads)
+			threads = realloc(threads,
+					  sizeof(*threads) + sizeof(pid_t)*total_tasks);
+		else
+			threads = malloc(sizeof(*threads) + sizeof(pid_t)*items);

If realloc fails and threads was allocated before... we leak memory.

ok.

Quoted 3 lines

We don't need to use this if clause, realloc handles threads == NULL just fines and then works as malloc.

ok.

Quoted 5 lines

Also I didn't use ctype altogether + that CSV parsing routine, we have strlist for that, it even will allow us to trow away duplicate pids/tids and also supports passing a file with a list of threads to monitor, for free.

Interesting. I did look at strlist before going with the string parsing. It was not obvious to me how to use it for this case.

Quoted 5 lines

Take a look at the modified patch below, if you're ok with it I can keep your autorship and put a [committer note: use strlist] or take autorship and state that it was based on a patch made by you, definetely your call. I'd go for the committer note.

I'm fine with either one. Patch below looks fine. If needed:

Quoted 1 line

Acked-by David Ahern <[email protected]>

I assume you tested it in a few scenarios (I know I did, but hey, more testing is always good) and that I can add another stamp, a Tested-by:
ya, right?

- Arnaldo

Quoted 5 lines

I assume you tested it in a few scenarios (I know I did, but hey, more testing is always good) and that I can add another stamp, a Tested-by:
ya, right?

- Arnaldo

Hmmm... that was a radical idea. Build breaks on Fedora 16, x86_64:

util/thread_map.c: In function ‘thread_map__new_by_pid_str’:
util/thread_map.c:164:2: error: overflow in implicit constant conversion [-Werror=overflow]
util/thread_map.c:175:3: error: comparison is always false due to limited range of data type [-Werror=type-limits] util/thread_map.c:175:3: error: comparison is always false due to limited range of data type [-Werror=type-limits] util/thread_map.c: In function ‘thread_map__new_by_tid_str’:
util/thread_map.c:223:2: error: overflow in implicit constant conversion [-Werror=overflow]
util/thread_map.c:245:3: error: comparison is always false due to limited range of data type [-Werror=type-limits] util/thread_map.c:245:3: error: comparison is always false due to limited range of data type [-Werror=type-limits]
cc1: all warnings being treated as errors

I think you want INT_MAX not LONG_MAX.

Commit-ID:  b52956c961be3a04182ae7b776623531601e0fb7
Gitweb:     http://git.kernel.org/tip/b52956c961be3a04182ae7b776623531601e0fb7
Author:     David Ahern <[email protected]>
AuthorDate: Wed, 8 Feb 2012 09:32:52 -0700
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Mon, 13 Feb 2012 22:54:11 -0200

perf tools: Allow multiple threads or processes in record, stat, top

Allow a user to collect events for multiple threads or processes using a comma separated list.

e.g., collect data on a VM and its vhost thread:
perf top -p 21483,21485
perf stat -p 21483,21485 -ddd
perf record -p 21483,21485

or monitoring vcpu threads
perf top -t 21488,21489
perf stat -t 21488,21489 -ddd
perf record -t 21488,21489

Cc Frederic Weisbecker <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Paul Mackerras <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Thomas Gleixner <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by David Ahern <[email protected]>

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/Documentation/perf-record.txt |    4 +-
 tools/perf/Documentation/perf-stat.txt   |    4 +-
 tools/perf/Documentation/perf-top.txt    |    4 +-
 tools/perf/builtin-record.c              |   10 +--
 tools/perf/builtin-stat.c                |   31 ++++----
 tools/perf/builtin-test.c                |    2 -
 tools/perf/builtin-top.c                 |   12 +--
 tools/perf/perf.h                        |    4 +-
 tools/perf/util/evlist.c                 |   10 +-
 tools/perf/util/evlist.h                 |    4 +-
 tools/perf/util/evsel.c                  |    2 +-

 tools/perf/util/python-ext-sources       |    2 +
 tools/perf/util/thread_map.c             |  128 ++++++++++++++++++++++++++++++
 tools/perf/util/thread_map.h             |    4 +
 tools/perf/util/top.c                    |   10 +-
 tools/perf/util/top.h                    |    2 +-
 tools/perf/util/usage.c                  |    6 +-
 tools/perf/util/util.h                   |    2 +-
 18 files changed, 185 insertions(+), 56 deletions(-)

I have a bigger NUMA testbox where perf sampling (perf top, perf record, etc.) stopped working (perf stat is fine), and I've bisected it back to the above commit.

b52956c961be3a04182ae7b776623531601e0fb7 is the first bad commit

Checking out eca1c3e3f937 works fine, b52956c961b is broken, repeatedly - so the bisection is reliable.

One symptom is no sampling records in the perf.data:

phoenix:~> perf record -a sleep 1
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.023 MB perf.data (~988 samples) ] phoenix:~> perf report --stdio
Warning:
The perf.data file has no samples!
# ========

# captured on: Fri Mar  2 12:01:41 2012
# hostname : phoenix
# os release : 3.3.0-rc5+
# perf version : 3.3.rc5.1698.g68a63a.dirty
# arch : x86_64
# nrcpus online : 16
# nrcpus avail : 16
# cpudesc : Quad-Core AMD Opteron(tm) Processor 8356
# cpuid : AuthenticAMD,16,2,3
# total memory : 33012240 kB
# cmdline : /home/mingo/bin/perf record -a sleep 1
# event : name = cycles, type = 0, config = 0x0, config1 = 0x0, config2 = 0x0, excl_usr = 0, excl_k
# HEADER_CPU_TOPOLOGY info available, use -I to display
# HEADER_NUMA_TOPOLOGY info available, use -I to display
# ========
phoenix:~>  perf --version
perf version 3.3.rc5.1698.g68a63a.dirty
phoenix:~>

The later fixes to b52956c9, such as:

6b1bee9035d4: perf tools: fix broken perf record -a mode

did not fix this system - it's still broken as of today's -tip.

Another system running the same kernel does not exhibit this problem, so it's somehow specific to this system.

I have not looked deep into this, let me know if you need more data.

Thanks,

Ingo

phoenix:~> perf record -a sleep 1
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.023 MB perf.data (~988 samples) ] phoenix:~> perf report --stdio
Warning:
The perf.data file has no samples!
# ========

# captured on: Fri Mar  2 12:01:41 2012
# hostname : phoenix
# os release : 3.3.0-rc5+
# perf version : 3.3.rc5.1698.g68a63a.dirty
# arch : x86_64
# nrcpus online : 16
# nrcpus avail : 16
# cpudesc : Quad-Core AMD Opteron(tm) Processor 8356
# cpuid : AuthenticAMD,16,2,3

# total memory : 33012240 kB
# cmdline : /home/mingo/bin/perf record -a sleep 1
# event : name = cycles, type = 0, config = 0x0, config1 = 0x0, config2 = 0x0, excl_usr = 0, excl_k
# HEADER_CPU_TOPOLOGY info available, use -I to display
# HEADER_NUMA_TOPOLOGY info available, use -I to display
# ========
phoenix:~>  perf --version
perf version 3.3.rc5.1698.g68a63a.dirty
phoenix:~>

The later fixes to b52956c9, such as:

6b1bee9035d4: perf tools: fix broken perf record -a mode

did not fix this system - it's still broken as of today's -tip.

Another system running the same kernel does not exhibit this problem, so it's somehow specific to this system.

That first cup of coffee is so important.

Quoted 7 lines · Ingo Molnar

phoenix:~> perf record -a sleep 1
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.023 MB perf.data (~988 samples) ] phoenix:~> perf report --stdio
Warning:
The perf.data file has no samples!
# ========

Quoted 9 lines

# captured on: Fri Mar  2 12:01:41 2012
# hostname : phoenix
# os release : 3.3.0-rc5+
# perf version : 3.3.rc5.1698.g68a63a.dirty
# arch : x86_64
# nrcpus online : 16
# nrcpus avail : 16
# cpudesc : Quad-Core AMD Opteron(tm) Processor 8356
# cpuid : AuthenticAMD,16,2,3

Would you mind adding this patch:
https://lkml.org/lkml/2012/2/29/217
and see if it fixes your problem?