Messages in this thread Patch in this message | | | Date | Wed, 27 Feb 2013 15:30:10 -0700 | From | David Ahern <> | Subject | Re: [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
| |