Some bug fixes to the res sample / script code

PATCH In review Andi Kleen 2 participants 9 messages Mar 12, 2019 – Mar 13, 2019 perf events

lore

Share & tools

lore

Andi Kleen

author

2019-03-12 03:52 UTC #0 lore raw

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

-Andi

Andi Kleen

author

2019-03-12 03:52 UTC #1 lore raw

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

From: Andi Kleen <[email protected]>

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 <[email protected]>

---
 tools/perf/ui/browsers/res_sample.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

tools/perf/ui/browsers/res_sample.c +3 -1

index c0dd73176d42..c450a3536f10 100644
@@ -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);

Signature · 1 line

2.20.1

Jiri Olsa

Acked-by on v1 1/3

2019-03-12 11:14 UTC #2 lore raw

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

Quoted 7 lines · Andi Kleen

From: Andi Kleen <[email protected]>
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 <[email protected]>

Acked-by Jiri Olsa <[email protected]>

thanks,
jirka

Quoted 22 lines

---
 tools/perf/ui/browsers/res_sample.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

tools/perf/ui/browsers/res_sample.c +3 -1

index c0dd73176d42..c450a3536f10 100644
@@ -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);

Signature · 1 line

2.20.1

Andi Kleen

author

2019-03-12 03:52 UTC #3 lore raw

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

In reply to Andi Kleen

From: Andi Kleen <[email protected]>

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 <[email protected]>

---
 tools/perf/ui/browsers/res_sample.c | 3 ++-
 tools/perf/ui/browsers/scripts.c    | 5 +++--
 2 files changed, 5 insertions(+), 3 deletions(-)

tools/perf/ui/browsers/res_sample.c +2 -1

index c450a3536f10..6e17e11a2ab9 100644
@@ -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);

tools/perf/ui/browsers/scripts.c +3 -2

index 27cf3ab88d13..f4c438dead66 100644
@@ -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);

Signature · 1 line

2.20.1

Jiri Olsa

Acked-by on v1 2/3

2019-03-12 11:13 UTC #4 lore raw

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

Quoted 6 lines · Andi Kleen

From: Andi Kleen <[email protected]>
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 <[email protected]>

Acked-by Jiri Olsa <[email protected]>

thanks,
jirka

Quoted 49 lines

---
 tools/perf/ui/browsers/res_sample.c | 3 ++-
 tools/perf/ui/browsers/scripts.c    | 5 +++--
 2 files changed, 5 insertions(+), 3 deletions(-)

tools/perf/ui/browsers/res_sample.c +2 -1

index c450a3536f10..6e17e11a2ab9 100644
@@ -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);

tools/perf/ui/browsers/scripts.c +3 -2

index 27cf3ab88d13..f4c438dead66 100644
@@ -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);

Signature · 1 line

2.20.1

Andi Kleen

author

2019-03-12 03:52 UTC #5 lore raw

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

In reply to Andi Kleen

From: Andi Kleen <[email protected]>

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 <[email protected]>

---
 tools/perf/util/hist.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

tools/perf/util/hist.c +9 -2

index 1f230285d78a..dcf24581dfbd 100644
@@ -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;
 	}

Signature · 1 line

2.20.1

Jiri Olsa

2019-03-12 11:12 UTC #6 lore raw

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

Quoted 16 lines · Andi Kleen

From: Andi Kleen <[email protected]>
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 <[email protected]>
---
 tools/perf/util/hist.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
tools/perf/util/hist.c +0 -0
index 1f230285d78a..dcf24581dfbd 100644

tools/perf/util/hist.c static int hist_entry__init(struct hist_entry *he, · line 440

Quoted 6 lines

tools/perf/util/hist.c +1 -2

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

Quoted 14 lines

tools/perf/util/hist.c +8 -0

+			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;
 	}

Signature · 1 line

2.20.1

Andi Kleen

author

2019-03-12 13:52 UTC #7 lore raw

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

Quoted 19 lines · Jiri Olsa

Quoted 18 lines · Andi Kleen
From: Andi Kleen <[email protected]>
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 <[email protected]>
---
 tools/perf/util/hist.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
tools/perf/util/hist.c +0 -0
index 1f230285d78a..dcf24581dfbd 100644
@@ -437,8 +437,15 @@ static int hist_entry__init(struct hist_entry *he,
 	}
 

Quoted 7 lines

tools/perf/util/hist.c static int hist_entry__init(struct hist_entry *he, · line 440
Quoted 4 lines
tools/perf/util/hist.c +1 -2
 	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

Jiri Olsa

Acked-by

2019-03-13 09:13 UTC #8 lore raw

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

Quoted 28 lines · Andi Kleen

Quoted 19 lines · Jiri Olsa
Quoted 18 lines · Andi Kleen
From: Andi Kleen <[email protected]>
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 <[email protected]>
---
 tools/perf/util/hist.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
tools/perf/util/hist.c +0 -0
index 1f230285d78a..dcf24581dfbd 100644
@@ -437,8 +437,15 @@ static int hist_entry__init(struct hist_entry *he,
 	}
 
Quoted 7 lines
tools/perf/util/hist.c static int hist_entry__init(struct hist_entry *he, · line 440
Quoted 4 lines
tools/perf/util/hist.c +1 -2
 	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

Quoted 4 lines

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 <[email protected]>

thanks,
jirka