Re: [PATCH v2 2/4] Transform thread_map to linked list

[David Ahern <dsahern@gmail.com>]

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