From: David Ahern <dsahern@gmail.com>
To: Namhyung Kim <namhyung@gmail.com>
Cc: acme@ghostprotocols.net, linux-kernel@vger.kernel.org,
mingo@elte.hu, peterz@infradead.org, fweisbec@gmail.com,
paulus@samba.org, tglx@linutronix.de
Subject: Re: [PATCH] perf tools: Allow multiple threads or processes in record, stat, top
Date: Wed, 08 Feb 2012 19:52:39 -0700 [thread overview]
Message-ID: <4F3334F7.70809@gmail.com> (raw)
In-Reply-To: <4F331F14.5070100@gmail.com>
On 02/08/2012 06:19 PM, Namhyung Kim wrote:
>> diff --git a/tools/perf/util/thread_map.c b/tools/perf/util/thread_map.c
>> index 3d4b6c5..e793c16 100644
>> --- a/tools/perf/util/thread_map.c
>> +++ b/tools/perf/util/thread_map.c
>> @@ -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
>
>
>> +#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));
>> +
>> + 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;
> }
>
The pid and tid functions are implemented differently. The commonality
is parsing a CSV string in a while loop.
>
>> +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]
>> diff --git a/tools/perf/util/usage.c b/tools/perf/util/usage.c
>> index d0c0139..1240bd6 100644
>> --- a/tools/perf/util/usage.c
>> +++ b/tools/perf/util/usage.c
>> @@ -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) {
>> + /* UUID and PID are mutually exclusive */
>
> s/UUID/UID/ ?
Yes. I was trying to fix the CPU typo when the function was created.
Will remove the extra 'U'.
David
next prev parent reply other threads:[~2012-02-09 2:52 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-02-08 16:32 [PATCH] perf tools: Allow multiple threads or processes in record, stat, top David Ahern
2012-02-09 1:19 ` Namhyung Kim
2012-02-09 2:52 ` David Ahern [this message]
2012-02-09 5:36 ` Namhyung Kim
2012-02-09 6:04 ` David Ahern
2012-02-09 7:37 ` Ingo Molnar
2012-02-09 14:34 ` Arnaldo Carvalho de Melo
2012-02-09 14:44 ` Arnaldo Carvalho de Melo
2012-02-10 5:42 ` Namhyung Kim
2012-02-10 14:28 ` Arnaldo Carvalho de Melo
2012-02-12 10:45 ` [PATCH] perf tools: Fix build dependency of perf python extension Namhyung Kim
2012-02-17 9:44 ` [tip:perf/core] " tip-bot for Namhyung Kim
2012-02-10 19:24 ` [PATCH] perf tools: Allow multiple threads or processes in record, stat, top Arnaldo Carvalho de Melo
2012-02-10 19:32 ` David Ahern
2012-02-10 19:34 ` Arnaldo Carvalho de Melo
2012-02-10 19:46 ` David Ahern
2012-02-10 19:58 ` Arnaldo Carvalho de Melo
2012-02-17 9:46 ` [tip:perf/core] " tip-bot for David Ahern
2012-03-02 10:56 ` Ingo Molnar
2012-03-02 14:21 ` David Ahern
2012-03-02 14:52 ` David Ahern
2012-03-03 7:42 ` Ingo Molnar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4F3334F7.70809@gmail.com \
--to=dsahern@gmail.com \
--cc=acme@ghostprotocols.net \
--cc=fweisbec@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@elte.hu \
--cc=namhyung@gmail.com \
--cc=paulus@samba.org \
--cc=peterz@infradead.org \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).