Some bug fixes to the res sample / script code

Some bug fixes to the res sample / script code

[Andi Kleen]

Some minor bug fixes to the recently merged res sample / scripting
improvelement in perf report.

-Andi

[PATCH 1/3] perf, tools, report: Handle samples without time

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

When a sample doesn't have a time stamp (e.g. from --no-time),
show the beginning of the trace for res samples instead of generating
an impossible time range that errors out.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 tools/perf/ui/browsers/res_sample.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/perf/ui/browsers/res_sample.c b/tools/perf/ui/browsers/res_sample.c
index c0dd73176d42..c450a3536f10 100644
--- a/tools/perf/ui/browsers/res_sample.c
+++ b/tools/perf/ui/browsers/res_sample.c
@@ -60,7 +60,9 @@ int res_sample_browse(struct res_sample *res_samples, int num_res,
 		return -1;
 	r = &res_samples[choice];
 
-	n = timestamp__scnprintf_nsec(r->time - context_len, trange, sizeof trange);
+	n = timestamp__scnprintf_nsec(r->time > context_len ?
+				      r->time - context_len : r->time,
+				      trange, sizeof trange);
 	trange[n++] = ',';
 	timestamp__scnprintf_nsec(r->time + context_len, trange + n, sizeof trange - n);
 
-- 
2.20.1

Re: [PATCH 1/3] perf, tools, report: Handle samples without time

[Jiri Olsa]

On Mon, Mar 11, 2019 at 08:52:22PM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> When a sample doesn't have a time stamp (e.g. from --no-time),
> show the beginning of the trace for res samples instead of generating
> an impossible time range that errors out.
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

> ---
>  tools/perf/ui/browsers/res_sample.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/ui/browsers/res_sample.c b/tools/perf/ui/browsers/res_sample.c
> index c0dd73176d42..c450a3536f10 100644
> --- a/tools/perf/ui/browsers/res_sample.c
> +++ b/tools/perf/ui/browsers/res_sample.c
> @@ -60,7 +60,9 @@ int res_sample_browse(struct res_sample *res_samples, int num_res,
>  		return -1;
>  	r = &res_samples[choice];
>  
> -	n = timestamp__scnprintf_nsec(r->time - context_len, trange, sizeof trange);
> +	n = timestamp__scnprintf_nsec(r->time > context_len ?
> +				      r->time - context_len : r->time,
> +				      trange, sizeof trange);
>  	trange[n++] = ',';
>  	timestamp__scnprintf_nsec(r->time + context_len, trange + n, sizeof trange - n);
>  
> -- 
> 2.20.1
> 

[PATCH 2/3] perf, tools, report: Pass on -f to child perf script

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Pass on -f to the child perf script, so that it can read the perf.data
file if it's owned by a different user.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 tools/perf/ui/browsers/res_sample.c | 3 ++-
 tools/perf/ui/browsers/scripts.c    | 5 +++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/perf/ui/browsers/res_sample.c b/tools/perf/ui/browsers/res_sample.c
index c450a3536f10..6e17e11a2ab9 100644
--- a/tools/perf/ui/browsers/res_sample.c
+++ b/tools/perf/ui/browsers/res_sample.c
@@ -70,7 +70,7 @@ int res_sample_browse(struct res_sample *res_samples, int num_res,
 
 	attr_to_script(extra_format, &evsel->attr);
 
-	if (asprintf(&cmd, "%s script %s%s --time %s %s%s %s%s --ns %s %s %s %s %s | less +/%s",
+	if (asprintf(&cmd, "%s script %s%s --time %s %s%s %s%s --ns %s %s %s %s %s %s | less +/%s",
 		     perf,
 		     input_name ? "-i " : "",
 		     input_name ? input_name : "",
@@ -85,6 +85,7 @@ int res_sample_browse(struct res_sample *res_samples, int num_res,
 		     symbol_conf.inline_name ? "--inline" : "",
 		     "--show-lost-events ",
 		     r->tid ? "--show-switch-events --show-task-events " : "",
+		     symbol_conf.force ? "-f" : "",
 		     tsample) < 0)
 		return -1;
 	run_script(cmd);
diff --git a/tools/perf/ui/browsers/scripts.c b/tools/perf/ui/browsers/scripts.c
index 27cf3ab88d13..f4c438dead66 100644
--- a/tools/perf/ui/browsers/scripts.c
+++ b/tools/perf/ui/browsers/scripts.c
@@ -171,12 +171,13 @@ int script_browse(const char *script_opt, struct perf_evsel *evsel)
 	if (list_scripts(script_name, &custom, evsel))
 		return -1;
 
-	if (asprintf(&cmd, "%s%s %s %s%s 2>&1 | less",
+	if (asprintf(&cmd, "%s%s %s %s%s %s 2>&1 | less",
 			custom ? "perf script -s " : "",
 			script_name,
 			script_opt ? script_opt : "",
 			input_name ? "-i " : "",
-			input_name ? input_name : "") < 0)
+			input_name ? input_name : "",
+			symbol_conf.force ? "-f " : "") < 0)
 		return -1;
 
 	run_script(cmd);
-- 
2.20.1

Re: [PATCH 2/3] perf, tools, report: Pass on -f to child perf script

[Jiri Olsa]

On Mon, Mar 11, 2019 at 08:52:23PM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> Pass on -f to the child perf script, so that it can read the perf.data
> file if it's owned by a different user.
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

> ---
>  tools/perf/ui/browsers/res_sample.c | 3 ++-
>  tools/perf/ui/browsers/scripts.c    | 5 +++--
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/ui/browsers/res_sample.c b/tools/perf/ui/browsers/res_sample.c
> index c450a3536f10..6e17e11a2ab9 100644
> --- a/tools/perf/ui/browsers/res_sample.c
> +++ b/tools/perf/ui/browsers/res_sample.c
> @@ -70,7 +70,7 @@ int res_sample_browse(struct res_sample *res_samples, int num_res,
>  
>  	attr_to_script(extra_format, &evsel->attr);
>  
> -	if (asprintf(&cmd, "%s script %s%s --time %s %s%s %s%s --ns %s %s %s %s %s | less +/%s",
> +	if (asprintf(&cmd, "%s script %s%s --time %s %s%s %s%s --ns %s %s %s %s %s %s | less +/%s",
>  		     perf,
>  		     input_name ? "-i " : "",
>  		     input_name ? input_name : "",
> @@ -85,6 +85,7 @@ int res_sample_browse(struct res_sample *res_samples, int num_res,
>  		     symbol_conf.inline_name ? "--inline" : "",
>  		     "--show-lost-events ",
>  		     r->tid ? "--show-switch-events --show-task-events " : "",
> +		     symbol_conf.force ? "-f" : "",
>  		     tsample) < 0)
>  		return -1;
>  	run_script(cmd);
> diff --git a/tools/perf/ui/browsers/scripts.c b/tools/perf/ui/browsers/scripts.c
> index 27cf3ab88d13..f4c438dead66 100644
> --- a/tools/perf/ui/browsers/scripts.c
> +++ b/tools/perf/ui/browsers/scripts.c
> @@ -171,12 +171,13 @@ int script_browse(const char *script_opt, struct perf_evsel *evsel)
>  	if (list_scripts(script_name, &custom, evsel))
>  		return -1;
>  
> -	if (asprintf(&cmd, "%s%s %s %s%s 2>&1 | less",
> +	if (asprintf(&cmd, "%s%s %s %s%s %s 2>&1 | less",
>  			custom ? "perf script -s " : "",
>  			script_name,
>  			script_opt ? script_opt : "",
>  			input_name ? "-i " : "",
> -			input_name ? input_name : "") < 0)
> +			input_name ? input_name : "",
> +			symbol_conf.force ? "-f " : "") < 0)
>  		return -1;
>  
>  	run_script(cmd);
> -- 
> 2.20.1
> 

[PATCH 3/3] perf, tools, report: Set up samples correctly in hierarchy mode

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

In hierarchy mode the res samples need to be cloned from the parent
entry. Copy them in this case. This fixes res sample browsing
with --hierarchy.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 tools/perf/util/hist.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 1f230285d78a..dcf24581dfbd 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -437,8 +437,15 @@ static int hist_entry__init(struct hist_entry *he,
 	}
 
 	if (symbol_conf.res_sample) {
-		he->res_samples = calloc(sizeof(struct res_sample),
-					symbol_conf.res_sample);
+		if (he->res_samples) {
+			he->res_samples = memdup(he->res_samples,
+						sizeof(struct res_sample) *
+						symbol_conf.res_sample);
+		} else {
+			he->res_samples = calloc(sizeof(struct res_sample),
+						 symbol_conf.res_sample);
+			he->num_res = 0;
+		}
 		if (!he->res_samples)
 			goto err_srcline;
 	}
-- 
2.20.1

Re: [PATCH 3/3] perf, tools, report: Set up samples correctly in hierarchy mode

[Jiri Olsa]

On Mon, Mar 11, 2019 at 08:52:24PM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> In hierarchy mode the res samples need to be cloned from the parent
> entry. Copy them in this case. This fixes res sample browsing
> with --hierarchy.
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  tools/perf/util/hist.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 1f230285d78a..dcf24581dfbd 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -437,8 +437,15 @@ static int hist_entry__init(struct hist_entry *he,
>  	}
>  
>  	if (symbol_conf.res_sample) {
> -		he->res_samples = calloc(sizeof(struct res_sample),
> -					symbol_conf.res_sample);
> +		if (he->res_samples) {

I dont think this leg will ever execute because we don't set
res_samples in template

jirka

> +			he->res_samples = memdup(he->res_samples,
> +						sizeof(struct res_sample) *
> +						symbol_conf.res_sample);
> +		} else {
> +			he->res_samples = calloc(sizeof(struct res_sample),
> +						 symbol_conf.res_sample);
> +			he->num_res = 0;
> +		}
>  		if (!he->res_samples)
>  			goto err_srcline;
>  	}
> -- 
> 2.20.1
> 

Re: [PATCH 3/3] perf, tools, report: Set up samples correctly in hierarchy mode

[Andi Kleen]

On Tue, Mar 12, 2019 at 12:12:59PM +0100, Jiri Olsa wrote:
> On Mon, Mar 11, 2019 at 08:52:24PM -0700, Andi Kleen wrote:
> > From: Andi Kleen <ak@linux.intel.com>
> > 
> > In hierarchy mode the res samples need to be cloned from the parent
> > entry. Copy them in this case. This fixes res sample browsing
> > with --hierarchy.
> > 
> > Signed-off-by: Andi Kleen <ak@linux.intel.com>
> > ---
> >  tools/perf/util/hist.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > index 1f230285d78a..dcf24581dfbd 100644
> > --- a/tools/perf/util/hist.c
> > +++ b/tools/perf/util/hist.c
> > @@ -437,8 +437,15 @@ static int hist_entry__init(struct hist_entry *he,
> >  	}
> >  
> >  	if (symbol_conf.res_sample) {
> > -		he->res_samples = calloc(sizeof(struct res_sample),
> > -					symbol_conf.res_sample);
> > +		if (he->res_samples) {
> 
> I dont think this leg will ever execute because we don't set
> res_samples in template

I originally thought this too, but ...

The hierarchy mode calls it with a hist entry that is not
a template.

-Andi

Re: [PATCH 3/3] perf, tools, report: Set up samples correctly in hierarchy mode

[Jiri Olsa]

On Tue, Mar 12, 2019 at 06:52:30AM -0700, Andi Kleen wrote:
> On Tue, Mar 12, 2019 at 12:12:59PM +0100, Jiri Olsa wrote:
> > On Mon, Mar 11, 2019 at 08:52:24PM -0700, Andi Kleen wrote:
> > > From: Andi Kleen <ak@linux.intel.com>
> > > 
> > > In hierarchy mode the res samples need to be cloned from the parent
> > > entry. Copy them in this case. This fixes res sample browsing
> > > with --hierarchy.
> > > 
> > > Signed-off-by: Andi Kleen <ak@linux.intel.com>
> > > ---
> > >  tools/perf/util/hist.c | 11 +++++++++--
> > >  1 file changed, 9 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > > index 1f230285d78a..dcf24581dfbd 100644
> > > --- a/tools/perf/util/hist.c
> > > +++ b/tools/perf/util/hist.c
> > > @@ -437,8 +437,15 @@ static int hist_entry__init(struct hist_entry *he,
> > >  	}
> > >  
> > >  	if (symbol_conf.res_sample) {
> > > -		he->res_samples = calloc(sizeof(struct res_sample),
> > > -					symbol_conf.res_sample);
> > > +		if (he->res_samples) {
> > 
> > I dont think this leg will ever execute because we don't set
> > res_samples in template
> 
> I originally thought this too, but ...
> 
> The hierarchy mode calls it with a hist entry that is not
> a template.

right, forgot about this

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka