2019-10-11 18:22:29

by Andi Kleen

[permalink] [raw]
Subject: [PATCH 1/2] perf script: Fix --reltime with --time

From: Andi Kleen <[email protected]>

My earlier patch to just enable --reltime with --time was a little too optimistic.
The --time parsing would accept absolute time, which is very confusing to the user.

Support relative time in --time parsing too. This only works with recent perf
record that records the first sample time. Otherwise we error out.

Fixes: 3714437d3fcc ("perf script: Allow --time with --reltime")
Signed-off-by: Andi Kleen <[email protected]>
---
tools/perf/builtin-script.c | 5 +++--
tools/perf/util/time-utils.c | 27 ++++++++++++++++++++++++---
tools/perf/util/time-utils.h | 5 +++++
3 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 1c797a948ada..f86c5cce5b2c 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -3864,10 +3864,11 @@ int cmd_script(int argc, const char **argv)
goto out_delete;

if (script.time_str) {
- err = perf_time__parse_for_ranges(script.time_str, session,
+ err = perf_time__parse_for_ranges_reltime(script.time_str, session,
&script.ptime_range,
&script.range_size,
- &script.range_num);
+ &script.range_num,
+ reltime);
if (err < 0)
goto out_delete;

diff --git a/tools/perf/util/time-utils.c b/tools/perf/util/time-utils.c
index 9796a2e43f67..302443921681 100644
--- a/tools/perf/util/time-utils.c
+++ b/tools/perf/util/time-utils.c
@@ -458,10 +458,11 @@ bool perf_time__ranges_skip_sample(struct perf_time_interval *ptime_buf,
return true;
}

-int perf_time__parse_for_ranges(const char *time_str,
+int perf_time__parse_for_ranges_reltime(const char *time_str,
struct perf_session *session,
struct perf_time_interval **ranges,
- int *range_size, int *range_num)
+ int *range_size, int *range_num,
+ bool reltime)
{
bool has_percent = strchr(time_str, '%');
struct perf_time_interval *ptime_range;
@@ -471,7 +472,7 @@ int perf_time__parse_for_ranges(const char *time_str,
if (!ptime_range)
return -ENOMEM;

- if (has_percent) {
+ if (has_percent || reltime) {
if (session->evlist->first_sample_time == 0 &&
session->evlist->last_sample_time == 0) {
pr_err("HINT: no first/last sample time found in perf data.\n"
@@ -479,7 +480,9 @@ int perf_time__parse_for_ranges(const char *time_str,
"(if '--buildid-all' is enabled, please set '--timestamp-boundary').\n");
goto error;
}
+ }

+ if (has_percent) {
num = perf_time__percent_parse_str(
ptime_range, size,
time_str,
@@ -492,6 +495,15 @@ int perf_time__parse_for_ranges(const char *time_str,
if (num < 0)
goto error_invalid;

+ if (reltime) {
+ int i;
+
+ for (i = 0; i < num; i++) {
+ ptime_range[i].start += session->evlist->first_sample_time;
+ ptime_range[i].end += session->evlist->first_sample_time;
+ }
+ }
+
*range_size = size;
*range_num = num;
*ranges = ptime_range;
@@ -504,6 +516,15 @@ int perf_time__parse_for_ranges(const char *time_str,
return ret;
}

+int perf_time__parse_for_ranges(const char *time_str,
+ struct perf_session *session,
+ struct perf_time_interval **ranges,
+ int *range_size, int *range_num)
+{
+ return perf_time__parse_for_ranges_reltime(time_str, session, ranges,
+ range_size, range_num, false);
+}
+
int timestamp__scnprintf_usec(u64 timestamp, char *buf, size_t sz)
{
u64 sec = timestamp / NSEC_PER_SEC;
diff --git a/tools/perf/util/time-utils.h b/tools/perf/util/time-utils.h
index 4f42988eb2f7..1142b0bddd5e 100644
--- a/tools/perf/util/time-utils.h
+++ b/tools/perf/util/time-utils.h
@@ -26,6 +26,11 @@ bool perf_time__ranges_skip_sample(struct perf_time_interval *ptime_buf,

struct perf_session;

+int perf_time__parse_for_ranges_reltime(const char *str, struct perf_session *session,
+ struct perf_time_interval **ranges,
+ int *range_size, int *range_num,
+ bool reltime);
+
int perf_time__parse_for_ranges(const char *str, struct perf_session *session,
struct perf_time_interval **ranges,
int *range_size, int *range_num);
--
2.21.0


2019-10-11 18:22:36

by Andi Kleen

[permalink] [raw]
Subject: [PATCH 2/2] perf evlist: Fix fix for freed id arrays

From: Andi Kleen <[email protected]>

In the earlier fix for the memory overrun of id arrays I managed
to typo the wrong event in the fix.

Of course we need to close the current event in the loop, not the
original failing event.

The same test case as in the original patch still passes.

Fixes: 7834fa948beb ("perf evlist: Fix access of freed id arrays")
Signed-off-by: Andi Kleen <[email protected]>
---
tools/perf/util/evlist.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index e33b46aca5cb..2f8ac60af76b 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1728,7 +1728,7 @@ struct evsel *perf_evlist__reset_weak_group(struct evlist *evsel_list,
is_open = false;
if (c2->leader == leader) {
if (is_open)
- perf_evsel__close(&evsel->core);
+ perf_evsel__close(&c2->core);
c2->leader = c2;
c2->core.nr_members = 0;
}
--
2.21.0

2019-10-14 14:10:35

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH 1/2] perf script: Fix --reltime with --time

Em Fri, Oct 11, 2019 at 11:21:39AM -0700, Andi Kleen escreveu:
> From: Andi Kleen <[email protected]>
>
> My earlier patch to just enable --reltime with --time was a little too optimistic.
> The --time parsing would accept absolute time, which is very confusing to the user.
>
> Support relative time in --time parsing too. This only works with recent perf
> record that records the first sample time. Otherwise we error out.

Thanks, applied both patches.

- Arnaldo

> Fixes: 3714437d3fcc ("perf script: Allow --time with --reltime")
> Signed-off-by: Andi Kleen <[email protected]>
> ---
> tools/perf/builtin-script.c | 5 +++--
> tools/perf/util/time-utils.c | 27 ++++++++++++++++++++++++---
> tools/perf/util/time-utils.h | 5 +++++
> 3 files changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 1c797a948ada..f86c5cce5b2c 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -3864,10 +3864,11 @@ int cmd_script(int argc, const char **argv)
> goto out_delete;
>
> if (script.time_str) {
> - err = perf_time__parse_for_ranges(script.time_str, session,
> + err = perf_time__parse_for_ranges_reltime(script.time_str, session,
> &script.ptime_range,
> &script.range_size,
> - &script.range_num);
> + &script.range_num,
> + reltime);
> if (err < 0)
> goto out_delete;
>
> diff --git a/tools/perf/util/time-utils.c b/tools/perf/util/time-utils.c
> index 9796a2e43f67..302443921681 100644
> --- a/tools/perf/util/time-utils.c
> +++ b/tools/perf/util/time-utils.c
> @@ -458,10 +458,11 @@ bool perf_time__ranges_skip_sample(struct perf_time_interval *ptime_buf,
> return true;
> }
>
> -int perf_time__parse_for_ranges(const char *time_str,
> +int perf_time__parse_for_ranges_reltime(const char *time_str,
> struct perf_session *session,
> struct perf_time_interval **ranges,
> - int *range_size, int *range_num)
> + int *range_size, int *range_num,
> + bool reltime)
> {
> bool has_percent = strchr(time_str, '%');
> struct perf_time_interval *ptime_range;
> @@ -471,7 +472,7 @@ int perf_time__parse_for_ranges(const char *time_str,
> if (!ptime_range)
> return -ENOMEM;
>
> - if (has_percent) {
> + if (has_percent || reltime) {
> if (session->evlist->first_sample_time == 0 &&
> session->evlist->last_sample_time == 0) {
> pr_err("HINT: no first/last sample time found in perf data.\n"
> @@ -479,7 +480,9 @@ int perf_time__parse_for_ranges(const char *time_str,
> "(if '--buildid-all' is enabled, please set '--timestamp-boundary').\n");
> goto error;
> }
> + }
>
> + if (has_percent) {
> num = perf_time__percent_parse_str(
> ptime_range, size,
> time_str,
> @@ -492,6 +495,15 @@ int perf_time__parse_for_ranges(const char *time_str,
> if (num < 0)
> goto error_invalid;
>
> + if (reltime) {
> + int i;
> +
> + for (i = 0; i < num; i++) {
> + ptime_range[i].start += session->evlist->first_sample_time;
> + ptime_range[i].end += session->evlist->first_sample_time;
> + }
> + }
> +
> *range_size = size;
> *range_num = num;
> *ranges = ptime_range;
> @@ -504,6 +516,15 @@ int perf_time__parse_for_ranges(const char *time_str,
> return ret;
> }
>
> +int perf_time__parse_for_ranges(const char *time_str,
> + struct perf_session *session,
> + struct perf_time_interval **ranges,
> + int *range_size, int *range_num)
> +{
> + return perf_time__parse_for_ranges_reltime(time_str, session, ranges,
> + range_size, range_num, false);
> +}
> +
> int timestamp__scnprintf_usec(u64 timestamp, char *buf, size_t sz)
> {
> u64 sec = timestamp / NSEC_PER_SEC;
> diff --git a/tools/perf/util/time-utils.h b/tools/perf/util/time-utils.h
> index 4f42988eb2f7..1142b0bddd5e 100644
> --- a/tools/perf/util/time-utils.h
> +++ b/tools/perf/util/time-utils.h
> @@ -26,6 +26,11 @@ bool perf_time__ranges_skip_sample(struct perf_time_interval *ptime_buf,
>
> struct perf_session;
>
> +int perf_time__parse_for_ranges_reltime(const char *str, struct perf_session *session,
> + struct perf_time_interval **ranges,
> + int *range_size, int *range_num,
> + bool reltime);
> +
> int perf_time__parse_for_ranges(const char *str, struct perf_session *session,
> struct perf_time_interval **ranges,
> int *range_size, int *range_num);
> --
> 2.21.0

--

- Arnaldo

Subject: [tip: perf/urgent] perf evlist: Fix fix for freed id arrays

The following commit has been merged into the perf/urgent branch of tip:

Commit-ID: 98a8b2e60c69927b1f405c3b001a1de3f4e53901
Gitweb: https://git.kernel.org/tip/98a8b2e60c69927b1f405c3b001a1de3f4e53901
Author: Andi Kleen <[email protected]>
AuthorDate: Fri, 11 Oct 2019 11:21:40 -07:00
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitterDate: Tue, 15 Oct 2019 11:51:33 -03:00

perf evlist: Fix fix for freed id arrays

In the earlier fix for the memory overrun of id arrays I managed to typo
the wrong event in the fix.

Of course we need to close the current event in the loop, not the
original failing event.

The same test case as in the original patch still passes.

Fixes: 7834fa948beb ("perf evlist: Fix access of freed id arrays")
Signed-off-by: Andi Kleen <[email protected]>
Cc: Jiri Olsa <[email protected]>
Link: http://lore.kernel.org/lkml/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/util/evlist.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index d277a98..de79c73 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1659,7 +1659,7 @@ struct evsel *perf_evlist__reset_weak_group(struct evlist *evsel_list,
is_open = false;
if (c2->leader == leader) {
if (is_open)
- perf_evsel__close(&evsel->core);
+ perf_evsel__close(&c2->core);
c2->leader = c2;
c2->core.nr_members = 0;
}

Subject: [tip: perf/core] perf evlist: Fix fix for freed id arrays

The following commit has been merged into the perf/core branch of tip:

Commit-ID: 5a40e1994815ab09c59614c6a13d94eef55d1a7f
Gitweb: https://git.kernel.org/tip/5a40e1994815ab09c59614c6a13d94eef55d1a7f
Author: Andi Kleen <[email protected]>
AuthorDate: Fri, 11 Oct 2019 11:21:40 -07:00
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitterDate: Tue, 15 Oct 2019 08:36:22 -03:00

perf evlist: Fix fix for freed id arrays

In the earlier fix for the memory overrun of id arrays I managed to typo
the wrong event in the fix.

Of course we need to close the current event in the loop, not the
original failing event.

The same test case as in the original patch still passes.

Fixes: 7834fa948beb ("perf evlist: Fix access of freed id arrays")
Signed-off-by: Andi Kleen <[email protected]>
Cc: Jiri Olsa <[email protected]>
Link: http://lore.kernel.org/lkml/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/util/evlist.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 21b77ef..8793b4e 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1599,7 +1599,7 @@ struct evsel *perf_evlist__reset_weak_group(struct evlist *evsel_list,
is_open = false;
if (c2->leader == leader) {
if (is_open)
- perf_evsel__close(&evsel->core);
+ perf_evsel__close(&c2->core);
c2->leader = c2;
c2->core.nr_members = 0;
}

Subject: [tip: perf/core] perf script: Fix --reltime with --time

The following commit has been merged into the perf/core branch of tip:

Commit-ID: b3509b6ed7a79ec49f6b64e4f3b780f259a2a468
Gitweb: https://git.kernel.org/tip/b3509b6ed7a79ec49f6b64e4f3b780f259a2a468
Author: Andi Kleen <[email protected]>
AuthorDate: Fri, 11 Oct 2019 11:21:39 -07:00
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitterDate: Tue, 15 Oct 2019 08:36:22 -03:00

perf script: Fix --reltime with --time

My earlier patch to just enable --reltime with --time was a little too
optimistic. The --time parsing would accept absolute time, which is
very confusing to the user.

Support relative time in --time parsing too. This only works with recent
perf record that records the first sample time. Otherwise we error out.

Fixes: 3714437d3fcc ("perf script: Allow --time with --reltime")
Signed-off-by: Andi Kleen <[email protected]>
Cc: Jiri Olsa <[email protected]>
Link: http://lore.kernel.org/lkml/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/builtin-script.c | 5 +++--
tools/perf/util/time-utils.c | 27 ++++++++++++++++++++++++---
tools/perf/util/time-utils.h | 5 +++++
3 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 1c797a9..f86c5cc 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -3864,10 +3864,11 @@ int cmd_script(int argc, const char **argv)
goto out_delete;

if (script.time_str) {
- err = perf_time__parse_for_ranges(script.time_str, session,
+ err = perf_time__parse_for_ranges_reltime(script.time_str, session,
&script.ptime_range,
&script.range_size,
- &script.range_num);
+ &script.range_num,
+ reltime);
if (err < 0)
goto out_delete;

diff --git a/tools/perf/util/time-utils.c b/tools/perf/util/time-utils.c
index 9796a2e..3024439 100644
--- a/tools/perf/util/time-utils.c
+++ b/tools/perf/util/time-utils.c
@@ -458,10 +458,11 @@ bool perf_time__ranges_skip_sample(struct perf_time_interval *ptime_buf,
return true;
}

-int perf_time__parse_for_ranges(const char *time_str,
+int perf_time__parse_for_ranges_reltime(const char *time_str,
struct perf_session *session,
struct perf_time_interval **ranges,
- int *range_size, int *range_num)
+ int *range_size, int *range_num,
+ bool reltime)
{
bool has_percent = strchr(time_str, '%');
struct perf_time_interval *ptime_range;
@@ -471,7 +472,7 @@ int perf_time__parse_for_ranges(const char *time_str,
if (!ptime_range)
return -ENOMEM;

- if (has_percent) {
+ if (has_percent || reltime) {
if (session->evlist->first_sample_time == 0 &&
session->evlist->last_sample_time == 0) {
pr_err("HINT: no first/last sample time found in perf data.\n"
@@ -479,7 +480,9 @@ int perf_time__parse_for_ranges(const char *time_str,
"(if '--buildid-all' is enabled, please set '--timestamp-boundary').\n");
goto error;
}
+ }

+ if (has_percent) {
num = perf_time__percent_parse_str(
ptime_range, size,
time_str,
@@ -492,6 +495,15 @@ int perf_time__parse_for_ranges(const char *time_str,
if (num < 0)
goto error_invalid;

+ if (reltime) {
+ int i;
+
+ for (i = 0; i < num; i++) {
+ ptime_range[i].start += session->evlist->first_sample_time;
+ ptime_range[i].end += session->evlist->first_sample_time;
+ }
+ }
+
*range_size = size;
*range_num = num;
*ranges = ptime_range;
@@ -504,6 +516,15 @@ error:
return ret;
}

+int perf_time__parse_for_ranges(const char *time_str,
+ struct perf_session *session,
+ struct perf_time_interval **ranges,
+ int *range_size, int *range_num)
+{
+ return perf_time__parse_for_ranges_reltime(time_str, session, ranges,
+ range_size, range_num, false);
+}
+
int timestamp__scnprintf_usec(u64 timestamp, char *buf, size_t sz)
{
u64 sec = timestamp / NSEC_PER_SEC;
diff --git a/tools/perf/util/time-utils.h b/tools/perf/util/time-utils.h
index 4f42988..1142b0b 100644
--- a/tools/perf/util/time-utils.h
+++ b/tools/perf/util/time-utils.h
@@ -26,6 +26,11 @@ bool perf_time__ranges_skip_sample(struct perf_time_interval *ptime_buf,

struct perf_session;

+int perf_time__parse_for_ranges_reltime(const char *str, struct perf_session *session,
+ struct perf_time_interval **ranges,
+ int *range_size, int *range_num,
+ bool reltime);
+
int perf_time__parse_for_ranges(const char *str, struct perf_session *session,
struct perf_time_interval **ranges,
int *range_size, int *range_num);