linux-kernel - Re: [PATCH] Put common histogram functions for perf in their own file

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20090928140158.GA6178@nowhere>
Date:	Mon, 28 Sep 2009 16:02:01 +0200
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	John Kacur <jkacur@...hat.com>
Cc:	mingo@...e.hu, Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org
Subject: Re: [PATCH] Put common histogram functions for perf in their own
	file

On Mon, Sep 28, 2009 at 03:05:29PM +0200, John Kacur wrote:
> From d187ab3a62526ffbdff91911f4b2da813ccf037f Mon Sep 17 00:00:00 2001
> From: John Kacur <jkacur@...hat.com>
> Date: Mon, 28 Sep 2009 14:53:08 +0200
> Subject: [PATCH] Move histogram related functions into their own files (hist.c and hist.h)
>  and make use of them in builtin-annotate.c and builtin-report.c



I guess a part of the title should be in the changelog, and the title
should be a compressed version :)


 
> Signed-off-by: John Kacur <jkacur@...hat.com>
>  static int
>  process_sample_event(event_t *event, unsigned long offset, unsigned long head)
>  {
> @@ -861,7 +713,7 @@ more:
>  		dsos__fprintf(stdout);
>  
>  	collapse__resort();
> -	output__resort();
> +	output__resort(total);



Now I wonder if we actually ever need to sort the entries
in perf annotate. The only thing we need is to count the
hits per ip.


>  
>  	find_annotations();
>  
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 7b43504..ff6de9e 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -28,6 +28,7 @@
>  
>  #include "util/thread.h"
>  #include "util/sort.h"
> +#include "util/hist.h"
>  
>  static char		const *input_name = "perf.data";
>  
> @@ -55,8 +56,6 @@ static int		exclude_other = 1;
>  
>  static char		callchain_default_opt[] = "fractal,0.5";
>  
> -static int		callchain;
> -
>  static char		__cwd[PATH_MAX];
>  static char		*cwd = __cwd;
>  static int		cwdlen;
> @@ -66,50 +65,8 @@ static struct thread	*last_match;
>  
>  static struct perf_header *header;
>  
> -static
> -struct callchain_param	callchain_param = {
> -	.mode	= CHAIN_GRAPH_REL,
> -	.min_percent = 0.5
> -};



At least for now, this part is only used by report. May be better
keep it in report?

Concerning the whole, that looks nice. Especially annotate doesn't
even seem to need any sort.

So we could eventually keep the hist sorting only in report.
But that said, it's nice to make all these sorting and histogram
features in a separate file for general purpose. We can bet that will
be reused soon or later. If not this is still good to split it, to clear
a bit builtin-report.c

But the callchain bits should perhaps stay in report. If that needs
to be used elsewhere, it would need a kind of cleanup before.

--
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/