Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933785Ab3IMN75 (ORCPT ); Fri, 13 Sep 2013 09:59:57 -0400 Received: from mail-wg0-f54.google.com ([74.125.82.54]:55027 "EHLO mail-wg0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932780Ab3IMN7y (ORCPT ); Fri, 13 Sep 2013 09:59:54 -0400 Date: Fri, 13 Sep 2013 15:59:49 +0200 From: Frederic Weisbecker To: Namhyung Kim Cc: LKML , Jiri Olsa , David Ahern , Ingo Molnar , Peter Zijlstra , Arnaldo Carvalho de Melo , Stephane Eranian Subject: Re: [PATCH 4/4] perf tools: Compare hists comm by addresses Message-ID: <20130913135934.GD4844@somewhere> References: <1379017783-27032-1-git-send-email-fweisbec@gmail.com> <1379017783-27032-5-git-send-email-fweisbec@gmail.com> <87txhpqh5h.fsf@sejong.aot.lge.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87txhpqh5h.fsf@sejong.aot.lge.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7432 Lines: 204 On Fri, Sep 13, 2013 at 05:07:06PM +0900, Namhyung Kim wrote: > Hi, > > On Thu, 12 Sep 2013 22:29:43 +0200, Frederic Weisbecker wrote: > > Now that comm strings are allocated only once and refcounted to be shared > > among threads, these can now be safely compared by addresses. This > > should remove most hists collapses on post processing. > > [SNIP] > > > diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c > > index 26d8f11..3b307e7 100644 > > --- a/tools/perf/util/sort.c > > +++ b/tools/perf/util/sort.c > > @@ -79,7 +79,8 @@ struct sort_entry sort_thread = { > > static int64_t > > sort__comm_cmp(struct hist_entry *left, struct hist_entry *right) > > { > > - return right->thread->tid - left->thread->tid; > > + /* Compare the addr that should be unique among comm */ > > + return thread__comm_curr(right->thread) - thread__comm_curr(left->thread); > > I don't think this is enough. A hist entry should reference the comm > itself at that time. Saving thread can lead to referencing different > comm between insert and collapse time if thread changed the comm in the > meanwhile, right? Exactly! That's indeed what is missing in this patchset. The comm supports timed comm but not yet the hists. > > What about something like below: > > > From 431fd9bfa844ddfe28ab1390959bc7de28804a9c Mon Sep 17 00:00:00 2001 > From: Namhyung Kim > Date: Fri, 13 Sep 2013 16:28:57 +0900 > Subject: [PATCH] perf tools: Get current comm instead of last one > > At insert time, a hist entry should reference comm at the time > otherwise it can get a different comm later. > > Cc: Frederic Weisbecker > Signed-off-by: Namhyung Kim > --- > tools/perf/util/hist.c | 3 +++ > tools/perf/util/sort.c | 9 +++++---- > tools/perf/util/sort.h | 1 + > tools/perf/util/thread.c | 10 ++-------- > tools/perf/util/thread.h | 2 ++ > 5 files changed, 13 insertions(+), 12 deletions(-) > > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c > index 1f5767f97dce..1fe90bd9dcb7 100644 > --- a/tools/perf/util/hist.c > +++ b/tools/perf/util/hist.c > @@ -412,6 +412,7 @@ struct hist_entry *__hists__add_mem_entry(struct hists *self, > { > struct hist_entry entry = { > .thread = al->thread, > + .comm = curr_comm(al->thread), > .ms = { > .map = al->map, > .sym = al->sym, > @@ -442,6 +443,7 @@ struct hist_entry *__hists__add_branch_entry(struct hists *self, > { > struct hist_entry entry = { > .thread = al->thread, > + .comm = curr_comm(al->thread), > .ms = { > .map = bi->to.map, > .sym = bi->to.sym, > @@ -471,6 +473,7 @@ struct hist_entry *__hists__add_entry(struct hists *self, > { > struct hist_entry entry = { > .thread = al->thread, > + .comm = curr_comm(al->thread), > .ms = { > .map = al->map, > .sym = al->sym, > diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c > index 3b307e740d6e..840481859e4b 100644 > --- a/tools/perf/util/sort.c > +++ b/tools/perf/util/sort.c > @@ -1,5 +1,6 @@ > #include "sort.h" > #include "hist.h" > +#include "comm.h" > #include "symbol.h" > > regex_t parent_regex; > @@ -80,14 +81,14 @@ static int64_t > sort__comm_cmp(struct hist_entry *left, struct hist_entry *right) > { > /* Compare the addr that should be unique among comm */ > - return thread__comm_curr(right->thread) - thread__comm_curr(left->thread); > + return comm__str(right->comm) - comm__str(left->comm); If comm and fork events don't have a timestamp, or they aren't time ordered, we should override the comm entry of a thread and forget the previous one. So perhaps what we should do instead is to compare "struct comm" addresses directly. But it means that on thread__set_comm(), if we override the old entry due to missing or unordered timestamps (in which case we need to force them to be 0), we shouldn't reallocate a new struct comm, nor should we keep the old one and queue a new. Instead we need to override list_first_entry(thread->comm)->comm_str pointer only to make it point to a new struct comm_str. > } > > static int64_t > sort__comm_collapse(struct hist_entry *left, struct hist_entry *right) > { > - const char *comm_l = thread__comm_curr(left->thread); > - const char *comm_r = thread__comm_curr(right->thread); > + const char *comm_l = comm__str(left->comm); > + const char *comm_r = comm__str(right->comm); > > if (!comm_l || !comm_r) > return cmp_null(comm_l, comm_r); > @@ -98,7 +99,7 @@ sort__comm_collapse(struct hist_entry *left, struct hist_entry *right) > static int hist_entry__comm_snprintf(struct hist_entry *self, char *bf, > size_t size, unsigned int width) > { > - return repsep_snprintf(bf, size, "%*s", width, thread__comm_curr(self->thread)); > + return repsep_snprintf(bf, size, "%*s", width, comm__str(self->comm)); > } > > struct sort_entry sort_comm = { > diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h > index 4e80dbd271e7..4d0153f83e3c 100644 > --- a/tools/perf/util/sort.h > +++ b/tools/perf/util/sort.h > @@ -84,6 +84,7 @@ struct hist_entry { > struct he_stat stat; > struct map_symbol ms; > struct thread *thread; > + struct comm *comm; > u64 ip; > s32 cpu; > > diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c > index d3ca133329b2..e7c7fe6694e8 100644 > --- a/tools/perf/util/thread.c > +++ b/tools/perf/util/thread.c > @@ -54,7 +54,7 @@ void thread__delete(struct thread *self) > free(self); > } > > -static struct comm *curr_comm(const struct thread *self) > +struct comm *curr_comm(const struct thread *self) > { > if (list_empty(&self->comm_list)) > return NULL; > @@ -65,13 +65,7 @@ static struct comm *curr_comm(const struct thread *self) > /* CHECKME: time should always be 0 if event aren't ordered */ > int thread__set_comm(struct thread *self, const char *str, u64 timestamp) > { > - struct comm *new, *curr = curr_comm(self); > - > - /* Override latest entry if it had no specific time coverage */ > - if (!curr->start) { > - list_del(&curr->list); > - comm__free(curr); > - } We must still remove the old entry if timestamps aren't there or aren't ordered. Just we need to keep the old "struct comm" and replace the comm_str. Thanks. > + struct comm *new; > > new = comm__new(str, timestamp); > if (!new) > diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h > index 1d9378abb515..cf8e31d0028b 100644 > --- a/tools/perf/util/thread.h > +++ b/tools/perf/util/thread.h > @@ -26,6 +26,7 @@ struct thread { > }; > > struct machine; > +struct comm; > > struct thread *thread__new(pid_t pid, pid_t tid); > void thread__delete(struct thread *self); > @@ -36,6 +37,7 @@ static inline void thread__exited(struct thread *thread) > > int thread__set_comm(struct thread *self, const char *comm, u64 timestamp); > int thread__comm_len(struct thread *self); > +struct comm *curr_comm(const struct thread *self); > const char *thread__comm_curr(const struct thread *self); > void thread__insert_map(struct thread *self, struct map *map); > int thread__fork(struct thread *self, struct thread *parent, u64 timestamp); > -- > 1.7.11.7 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/