linux-kernel - Re: [PATCH 12/38] perf tools: Introduce thread__comm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Tue, 3 Mar 2015 17:28:40 +0100
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Namhyung Kim <namhyung@...nel.org>
Cc:	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Ingo Molnar <mingo@...nel.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Jiri Olsa <jolsa@...hat.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Adrian Hunter <adrian.hunter@...el.com>,
	Stephane Eranian <eranian@...gle.com>,
	Andi Kleen <andi@...stfloor.org>,
	David Ahern <dsahern@...il.com>
Subject: Re: [PATCH 12/38] perf tools: Introduce thread__comm_time() helpers

On Tue, Mar 03, 2015 at 12:07:24PM +0900, Namhyung Kim wrote:
> When data file indexing is enabled, it processes all task, comm and mmap
> events first and then goes to the sample events.  So all it sees is the
> last comm of a thread although it has information at the time of sample.
> 
> Sort thread's comm by time so that it can find appropriate comm at the
> sample time.  The thread__comm_time() will mostly work even if
> PERF_SAMPLE_TIME bit is off since in that case, sample->time will be
> -1 so it'll take the last comm anyway.
> 
> Cc: Frederic Weisbecker <fweisbec@...il.com>
> Signed-off-by: Namhyung Kim <namhyung@...nel.org>
> ---
>  tools/perf/util/thread.c | 33 ++++++++++++++++++++++++++++++++-
>  tools/perf/util/thread.h |  2 ++
>  2 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
> index 9ebc8b1f9be5..ad96725105c2 100644
> --- a/tools/perf/util/thread.c
> +++ b/tools/perf/util/thread.c
> @@ -103,6 +103,21 @@ struct comm *thread__exec_comm(const struct thread *thread)
>  	return last;
>  }
>  
> +struct comm *thread__comm_time(const struct thread *thread, u64 timestamp)

Usually thread__comm_foo() would suggest that we return the "foo" from a thread comm.
For example thread__comm_len() returns the len of the last thread comm.
thread__comm_str() returns the string of the last thread comm.

So thread__comm_time() suggests that we return the timestamp of the default thread comm.
Ideally all thread__comm_foo() accessors should now take a timestamp as an argument. Now there
are quite some callers of such APIs, I'm not sure they will all turn into passing a precise timestamp,
but the current callers are interested in the last comm so perhaps those can be turned into thread__last_comm[_str]().
The call would gain some clarity.

> +{
> +	struct comm *comm;
> +
> +	list_for_each_entry(comm, &thread->comm_list, list) {
> +		if (timestamp >= comm->start)
> +			return comm;
> +	}
> +
> +	if (list_empty(&thread->comm_list))
> +		return NULL;
> +
> +	return list_last_entry(&thread->comm_list, struct comm, list);
> +}

Yes, handling the thread's comm lifecycle correctly with fetching the right comm at a given time is
the evolution I had in mind. I haven't looked at the rest of your patchset but this
change alone seem to go to the right direction.

Thanks!


> +
>  int __thread__set_comm(struct thread *thread, const char *str, u64 timestamp,
>  		       bool exec)
>  {
> @@ -118,7 +133,13 @@ int __thread__set_comm(struct thread *thread, const char *str, u64 timestamp,
>  		new = comm__new(str, timestamp, exec);
>  		if (!new)
>  			return -ENOMEM;
> -		list_add(&new->list, &thread->comm_list);
> +
> +		/* sort by time */
> +		list_for_each_entry(curr, &thread->comm_list, list) {
> +			if (timestamp >= curr->start)
> +				break;
> +		}
> +		list_add_tail(&new->list, &curr->list);
>  
>  		if (exec)
>  			unwind__flush_access(thread);
> @@ -139,6 +160,16 @@ const char *thread__comm_str(const struct thread *thread)
>  	return comm__str(comm);
>  }
>  
> +const char *thread__comm_str_time(const struct thread *thread, u64 timestamp)
> +{
> +	const struct comm *comm = thread__comm_time(thread, timestamp);
> +
> +	if (!comm)
> +		return NULL;
> +
> +	return comm__str(comm);
> +}
> +
>  /* CHECKME: it should probably better return the max comm len from its comm list */
>  int thread__comm_len(struct thread *thread)
>  {
> diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
> index 160fd066a7d1..be67c3bad5e7 100644
> --- a/tools/perf/util/thread.h
> +++ b/tools/perf/util/thread.h
> @@ -53,7 +53,9 @@ static inline int thread__set_comm(struct thread *thread, const char *comm,
>  int thread__comm_len(struct thread *thread);
>  struct comm *thread__comm(const struct thread *thread);
>  struct comm *thread__exec_comm(const struct thread *thread);
> +struct comm *thread__comm_time(const struct thread *thread, u64 timestamp);
>  const char *thread__comm_str(const struct thread *thread);
> +const char *thread__comm_str_time(const struct thread *thread, u64 timestamp);
>  void thread__insert_map(struct thread *thread, struct map *map);
>  int thread__fork(struct thread *thread, struct thread *parent, u64 timestamp);
>  size_t thread__fprintf(struct thread *thread, FILE *fp);
> -- 
> 2.2.2
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/