lkml.org 
[lkml]   [2013]   [Feb]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v2 2/4] Transform thread_map to linked list
On 2/26/13 2:41 AM, chenggang wrote:

---8<---

> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> index 5cd13d7..91d2848 100644
> --- a/tools/perf/util/event.c
> +++ b/tools/perf/util/event.c
> @@ -327,8 +327,8 @@ int perf_event__synthesize_thread_map(struct perf_tool *tool,
> err = 0;
> for (thread = 0; thread < threads->nr; ++thread) {
> if (__event__synthesize_thread(comm_event, mmap_event,
> - threads->map[thread], 0,
> - process, tool, machine)) {
> + thread_map__get_pid(threads,
> + thread), 0, process, tool,
> + machine)) {

ouch, that needs to be easier on the eyes. Use an intermediate variable
for the thread_map__get_pid(threads, thread).

> err = -1;
> break;
> }
> @@ -337,12 +337,14 @@ int perf_event__synthesize_thread_map(struct perf_tool *tool,
> * comm.pid is set to thread group id by
> * perf_event__synthesize_comm
> */
> - if ((int) comm_event->comm.pid != threads->map[thread]) {
> + if ((int) comm_event->comm.pid
> + != thread_map__get_pid(threads, thread)) {

ditto. intermediate variable will make that easier to read.

> bool need_leader = true;
>
> /* is thread group leader in thread_map? */
> for (j = 0; j < threads->nr; ++j) {
> - if ((int) comm_event->comm.pid == threads->map[j]) {
> + if ((int) comm_event->comm.pid
> + == thread_map__get_pid(threads, thread)) {

and again here. Now should that be j instead of thread? i.e,
thread_map__get_pid(threads, j)

> need_leader = false;
> break;
> }

---8<---

> diff --git a/tools/perf/util/thread_map.c b/tools/perf/util/thread_map.c
> index 9b5f856..5f96fdf 100644
> --- a/tools/perf/util/thread_map.c
> +++ b/tools/perf/util/thread_map.c
> @@ -19,9 +19,72 @@ static int filter(const struct dirent *dir)
> return 1;
> }
>
> -struct thread_map *thread_map__new_by_pid(pid_t pid)
> +struct thread_map *thread_map__init(void)
> {
> struct thread_map *threads;
> +
> + threads = malloc(sizeof(*threads));
> + if (threads == NULL)
> + return NULL;
> +
> + threads->nr = 0;
> + INIT_LIST_HEAD(&threads->head);
> + return threads;
> +}
> +
> +void thread_map__delete(struct thread_map *threads)
> +{
> + struct thread_pid *tp, *tmp;
> +
> + list_for_each_entry_safe(tp, tmp, &threads->head, next) {
> + list_del(&tp->next);
> + free(tp);
> + }
> +
> + free(threads);
> +}
> +
> +int thread_map__append(struct thread_map *threads, pid_t pid)
> +{
> + struct thread_pid *tp;
> +
> + if (threads == NULL)
> + return -1;
> +
> + list_for_each_entry(tp, &threads->head, next)
> + if (tp->pid == pid) /*The thread is exist*/
> + return 1;

braces around multi-line statements

> +
> + tp = malloc(sizeof(*tp));
> + if (tp == NULL)
> + return -1;
> +
> + tp->pid = pid;
> + list_add_tail(&tp->next, &threads->head);
> + threads->nr++;
> +
> + return 0; /*success*/
> +}
> +
> +int thread_map__remove(struct thread_map *threads, int idx)
> +{
> + struct thread_pid *tp;
> + int count = 0;
> +
> + list_for_each_entry(tp, &threads->head, next)
> + if (count++ == idx) {
> + list_del(&tp->next);
> + free(tp);
> + threads->nr--;
> + return 0;
> + }

braces

> +
> + return -1;
> +}
> +
> +struct thread_map *thread_map__new_by_pid(pid_t pid)
> +{
> + struct thread_map *threads = NULL;
> char name[256];
> int items;
> struct dirent **namelist = NULL;
> @@ -32,40 +95,49 @@ struct thread_map *thread_map__new_by_pid(pid_t pid)
> if (items <= 0)
> return NULL;
>
> - threads = malloc(sizeof(*threads) + sizeof(pid_t) * items);
> - if (threads != NULL) {
> + threads = thread_map__init();
> + if (threads != NULL)
> for (i = 0; i < items; i++)
> - threads->map[i] = atoi(namelist[i]->d_name);
> - threads->nr = items;
> - }
> + if (thread_map__append(threads,
> + atoi(namelist[i]->d_name)) == -1)
> + goto out_free_threads;

braces; check the indentation too. I think the above 3 lines go under
the 'if (threads != NULL)' check

>
> for (i=0; i<items; i++)
> free(namelist[i]);
> free(namelist);
>
> return threads;
> +
> +out_free_threads:
> + thread_map__delete(threads);
> + return NULL;
> }
>
> struct thread_map *thread_map__new_by_tid(pid_t tid)
> {
> - struct thread_map *threads = malloc(sizeof(*threads) + sizeof(pid_t));
> + struct thread_map *threads = NULL;
>
> - if (threads != NULL) {
> - threads->map[0] = tid;
> - threads->nr = 1;
> - }
> + threads = thread_map__init();
> + if (threads != NULL)
> + if (thread_map__append(threads, tid) == -1)
> + goto out_free_threads;

braces

>
> return threads;
> +
> +out_free_threads:
> + thread_map__delete(threads);
> + return NULL;
> }
>
> struct thread_map *thread_map__new_by_uid(uid_t uid)
> {
> DIR *proc;
> - int max_threads = 32, items, i;
> + int items, i;
> char path[256];
> struct dirent dirent, *next, **namelist = NULL;
> - struct thread_map *threads = malloc(sizeof(*threads) +
> - max_threads * sizeof(pid_t));
> + struct thread_map *threads = NULL;
> +
> + threads = thread_map__init();
> if (threads == NULL)
> goto out;
>
> @@ -73,11 +145,8 @@ struct thread_map *thread_map__new_by_uid(uid_t uid)
> if (proc == NULL)
> goto out_free_threads;
>
> - threads->nr = 0;
> -
> while (!readdir_r(proc, &dirent, &next) && next) {
> char *end;
> - bool grow = false;
> struct stat st;
> pid_t pid = strtol(dirent.d_name, &end, 10);
>
> @@ -97,30 +166,13 @@ struct thread_map *thread_map__new_by_uid(uid_t uid)
> if (items <= 0)
> goto out_free_closedir;
>
> - while (threads->nr + items >= max_threads) {
> - max_threads *= 2;
> - grow = true;
> - }
> -
> - if (grow) {
> - struct thread_map *tmp;
> -
> - tmp = realloc(threads, (sizeof(*threads) +
> - max_threads * sizeof(pid_t)));
> - if (tmp == NULL)
> - goto out_free_namelist;
> -
> - threads = tmp;
> - }
> -
> for (i = 0; i < items; i++)
> - threads->map[threads->nr + i] = atoi(namelist[i]->d_name);
> + if (thread_map__append(threads, atoi(namelist[i]->d_name) < 0))
> + goto out_free_namelist;
>
> for (i = 0; i < items; i++)
> free(namelist[i]);
> free(namelist);
> -
> - threads->nr += items;
> }
>
> out_closedir:
> @@ -129,7 +181,7 @@ out:
> return threads;
>
> out_free_threads:
> - free(threads);
> + thread_map__delete(threads);
> return NULL;
>
> out_free_namelist:
> @@ -138,7 +190,7 @@ out_free_namelist:
> free(namelist);
>
> out_free_closedir:
> - free(threads);
> + thread_map__delete(threads);
> threads = NULL;
> goto out_closedir;
> }
> @@ -156,11 +208,11 @@ struct thread_map *thread_map__new(pid_t pid, pid_t tid, uid_t uid)
>
> static struct thread_map *thread_map__new_by_pid_str(const char *pid_str)
> {
> - struct thread_map *threads = NULL, *nt;
> + struct thread_map *threads = NULL;
> char name[256];
> - int items, total_tasks = 0;
> + int items;
> struct dirent **namelist = NULL;
> - int i, j = 0;
> + int i;
> pid_t pid, prev_pid = INT_MAX;
> char *end_ptr;
> struct str_node *pos;
> @@ -169,6 +221,10 @@ static struct thread_map *thread_map__new_by_pid_str(const char *pid_str)
> if (!slist)
> return NULL;
>
> + threads = thread_map__init();
> + if (threads == NULL)
> + return NULL;
> +
> strlist__for_each(pos, slist) {
> pid = strtol(pos->s, &end_ptr, 10);
>
> @@ -184,19 +240,12 @@ static struct thread_map *thread_map__new_by_pid_str(const char *pid_str)
> if (items <= 0)
> goto out_free_threads;
>
> - total_tasks += items;
> - nt = realloc(threads, (sizeof(*threads) +
> - sizeof(pid_t) * total_tasks));
> - if (nt == NULL)
> - goto out_free_namelist;
> -
> - threads = nt;
> + for (i = 0; i < items; i++)
> + if (thread_map__append(threads, atoi(namelist[i]->d_name)) < 0)
> + goto out_free_namelist;

and more braces....

>
> - for (i = 0; i < items; i++) {
> - threads->map[j++] = atoi(namelist[i]->d_name);
> + for (i = 0; i < items; i++)
> free(namelist[i]);
> - }
> - threads->nr = total_tasks;
> free(namelist);
> }
>

---8<---

> diff --git a/tools/perf/util/xyarray.c b/tools/perf/util/xyarray.c
> index fc48bda..5777bc2 100644
> --- a/tools/perf/util/xyarray.c
> +++ b/tools/perf/util/xyarray.c
> @@ -78,13 +78,13 @@ int xyarray__remove(struct xyarray *xy, int y)
> void xyarray__delete(struct xyarray *xy)
> {
> unsigned int i;
> - struct xyentry *entry;
> + struct xyentry *entry, *tmp;
>
> if (!xy)
> return;
>
> for (i = 0; i < xy->row_count; i++)
> - list_for_each_entry(entry, &xy->rows[i].head, next) {
> + list_for_each_entry_safe(entry, tmp, &xy->rows[i].head, next) {
> list_del(&entry->next);
> free(entry);
> }
>

These xyarray changes should be in the first patch.

David


\
 
 \ /
  Last update: 2013-02-28 00:01    [W:0.059 / U:0.132 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site