From: David Ahern <dsahern@gmail.com>
To: chenggang <chenggang.qin@gmail.com>
Cc: linux-kernel@vger.kernel.org,
chenggang <chenggang.qcg@taobao.com>,
Peter Zijlstra <a.p.zijlstra@chello.nl>,
Paul Mackerras <paulus@samba.org>, Ingo Molnar <mingo@redhat.com>,
Arnaldo Carvalho de Melo <acme@ghostprotocols.net>,
Arjan van de Ven <arjan@linux.intel.com>,
Namhyung Kim <namhyung@gmail.com>,
Yanmin Zhang <yanmin.zhang@intel.com>,
Wu Fengguang <fengguang.wu@intel.com>,
Mike Galbraith <efault@gmx.de>,
Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH v2 2/4] Transform thread_map to linked list
Date: Wed, 27 Feb 2013 15:30:10 -0700 [thread overview]
Message-ID: <512E88F2.4030501@gmail.com> (raw)
In-Reply-To: <1361871710-6251-3-git-send-email-chenggang.qin@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
next prev parent reply other threads:[~2013-02-27 22:30 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-02-26 9:41 [PATCH v2 4/4] Add fork and exit callback functions into top->perf_tool chenggang
2013-02-26 9:41 ` [PATCH v2 3/4] Transform mmap and other related structures to list with new xyarray chenggang
2013-02-28 16:34 ` David Ahern
2013-02-26 9:41 ` [PATCH v2 2/4] Transform thread_map to linked list chenggang
2013-02-27 22:30 ` David Ahern [this message]
2013-02-26 9:41 ` [PATCH v2 1/4] Transform xyarray " chenggang
2013-02-26 9:41 ` [PATCH v2 0/4] perf: Make the 'perf top -p $pid' can perceive the new forked threads chenggang
-- strict thread matches above, loose matches on Subject: below --
2013-02-26 9:20 chenggang
2013-02-26 9:20 ` [PATCH v2 2/4] Transform thread_map to linked list chenggang
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=512E88F2.4030501@gmail.com \
--to=dsahern@gmail.com \
--cc=a.p.zijlstra@chello.nl \
--cc=acme@ghostprotocols.net \
--cc=akpm@linux-foundation.org \
--cc=arjan@linux.intel.com \
--cc=chenggang.qcg@taobao.com \
--cc=chenggang.qin@gmail.com \
--cc=efault@gmx.de \
--cc=fengguang.wu@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=namhyung@gmail.com \
--cc=paulus@samba.org \
--cc=yanmin.zhang@intel.com \
/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