| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4F335B69.8060701@gmail.com>
Date: Thu, 09 Feb 2012 14:36:41 +0900
From: Namhyung Kim <namhyung@...il.com>
To: linux-kernel@...r.kernel.org
Cc: acme@...stprotocols.net, linux-kernel@...r.kernel.org,
mingo@...e.hu, peterz@...radead.org, fweisbec@...il.com,
paulus@...ba.org, tglx@...utronix.de
Subject: Re: [PATCH] perf tools: Allow multiple threads or processes in record,
stat, top
2012-02-09 11:52 AM, David Ahern wrote:
> 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
>
OK, not a big deal.
>>
>>
>>> +#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.
>
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);
...
}
...
}
Thanks,
Namhyung
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists