[PATCH v2 0/3] perf diff: Add new filter options

[PATCH v2 0/3] perf diff: Add new filter options

[Jin Yao]

This patch series creates following new filter options for perf diff.

--time:
    It supports time percent with multipe time ranges. Time string is
    'a%/n,b%/m,...' or 'a%-b%,c%-%d,...'.

    For example:

    Select the second 10% time slice to diff:
    perf diff --time 10%/2

    Select from 0% to 10% time slice to diff:
    perf diff --time 0%-10%

    Select the first and the second 10% time slices to diff:
    perf diff --time 10%/1,10%/2

    Select from 0% to 10% and 30% to 40% slices to diff:
    perf diff --time 0%-10%,30%-40%

    It also supports to analyze samples within given time window as:
    <start>,<stop>. Times have the format seconds.microseconds. If start
    is not given (i.e., time string is ',x.y') then analysis starts at
    the beginning of the file. If stop time is not given (i.e, time
    string is 'x.y,') then analysis goes to end of file. Time string is
    'a1.b1,c1.d1:a2.b2,c2.d2'. Use ':' to separate timestamps for different
    perf.data files.

    For example, we get the timestamp information from perf script.

    perf script -i perf.data.old
      mgen 13940 [000]  3946.361400: ...

    perf script -i perf.data
      mgen 13940 [000]  3971.150589 ...

    perf diff --time 3946.361400,:3971.150589,

    It analyzes the perf.data.old from the timestamp 3946.361400 to
    the end of perf.data.old and analyzes the perf.data from the
    timestamp 3971.150589 to the end of perf.data.

--cpu:
    Only diff samples for the list of CPUs provided. Multiple CPUs can
    be provided as a comma-separated list with no space: 0,1. Ranges of
    CPUs are specified with -: 0-2. Default is to report samples on all
    CPUs.

    For example:
    perf diff --cpu 0,1

    It only diff the samples for CPU0 and CPU1.

--pid:
    Only diff samples for given process ID (comma separated list).

--tid:
    Only diff samples for given thread ID (comma separated list).

    For example,
    perf diff --tid 13965

    It only diff the samples for thread 13965.

 v2:
 ---
 Update patch "perf diff: Support --time filter option" according to
 Jiri's comments. Others are no functional changes.

Jin Yao (3):
  perf diff: Support --time filter option
  perf diff: Support --cpu filter option
  perf diff: Support --pid/--tid filter options

 tools/perf/Documentation/perf-diff.txt |  52 +++++++++
 tools/perf/builtin-diff.c              | 187 ++++++++++++++++++++++++++++++---
 2 files changed, 226 insertions(+), 13 deletions(-)

-- 
2.7.4

[PATCH v2 1/3] perf diff: Support --time filter option

[Jin Yao]

For better support for perf diff, it would be useful to add --time filter
option to diff the samples within given time window.

It supports time percent with multipe time ranges. Time string is
'a%/n,b%/m,...' or 'a%-b%,c%-%d,...'.

For example:

Select the second 10% time slice to diff:
perf diff --time 10%/2

Select from 0% to 10% time slice to diff:
perf diff --time 0%-10%

Select the first and the second 10% time slices to diff:
perf diff --time 10%/1,10%/2

Select from 0% to 10% and 30% to 40% slices to diff:
perf diff --time 0%-10%,30%-40%

It also supports to analyze samples within given time window as:
<start>,<stop>. Times have the format seconds.microseconds. If start
is not given (i.e., time string is ',x.y') then analysis starts at
the beginning of the file. If stop time is not given (i.e, time
string is 'x.y,') then analysis goes to end of file. Time string is
'a1.b1,c1.d1:a2.b2,c2.d2'. Use ':' to separate timestamps for different
perf.data files.

For example, we get the timestamp information from perf script.

perf script -i perf.data.old
  mgen 13940 [000]  3946.361400: ...

perf script -i perf.data
  mgen 13940 [000]  3971.150589 ...

perf diff --time 3946.361400,:3971.150589,

It analyzes the perf.data.old from the timestamp 3946.361400 to
the end of perf.data.old and analyzes the perf.data from the
timestamp 3971.150589 to the end of perf.data.

 v2:
 ---
 Update according to Jiri's comments. For example, put the time string
 processing code to separate functions and align the memebers in
 perf_diff.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/Documentation/perf-diff.txt |  41 ++++++++
 tools/perf/builtin-diff.c              | 167 ++++++++++++++++++++++++++++++---
 2 files changed, 195 insertions(+), 13 deletions(-)

diff --git a/tools/perf/Documentation/perf-diff.txt b/tools/perf/Documentation/perf-diff.txt
index a79c84a..5bfe876 100644
--- a/tools/perf/Documentation/perf-diff.txt
+++ b/tools/perf/Documentation/perf-diff.txt
@@ -118,6 +118,47 @@ OPTIONS
 	sum of shown entries will be always 100%.  "absolute" means it retains
 	the original value before and after the filter is applied.
 
+--time::
+	Analyze samples within given time window. It supports time
+	percent with multipe time ranges. Time string is 'a%/n,b%/m,...'
+	or 'a%-b%,c%-%d,...'.
+
+	For example:
+
+	Select the second 10% time slice to diff:
+	perf diff --time 10%/2
+
+	Select from 0% to 10% time slice to diff:
+	perf diff --time 0%-10%
+
+	Select the first and the second 10% time slices to diff:
+	perf diff --time 10%/1,10%/2
+
+	Select from 0% to 10% and 30% to 40% slices to diff:
+	perf diff --time 0%-10%,30%-40%
+
+	It also supports to analyze samples within given time window:
+	<start>,<stop>. Times have the format seconds.microseconds. If start
+	is not given (i.e., time string is ',x.y') then analysis starts at
+	the beginning of the file. If stop time is not given (i.e, time
+	string is 'x.y,') then analysis goes to end of file. Time string is
+	'a1.b1,c1.d1:a2.b2,c2.d2'. Use ':' to separate timestamps for different
+	perf.data files.
+
+	For example, we get the timestamp information from perf script.
+
+	perf script -i perf.data.old
+	  mgen 13940 [000]  3946.361400: ...
+
+	perf script -i perf.data
+	  mgen 13940 [000]  3971.150589 ...
+
+	perf diff --time 3946.361400,:3971.150589,
+
+	It analyzes the perf.data.old from the timestamp 3946.361400 to
+	the end of perf.data.old and analyzes the perf.data from the
+	timestamp 3971.150589 to the end of perf.data.
+
 COMPARISON
 ----------
 The comparison is governed by the baseline file. The baseline perf.data
diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index 58fe0e8..f841ff9 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -19,12 +19,21 @@
 #include "util/util.h"
 #include "util/data.h"
 #include "util/config.h"
+#include "util/time-utils.h"
 
 #include <errno.h>
 #include <inttypes.h>
 #include <stdlib.h>
 #include <math.h>
 
+struct perf_diff {
+	struct perf_tool		 tool;
+	const char			*time_str;
+	struct perf_time_interval	*ptime_range;
+	int				 range_size;
+	int				 range_num;
+};
+
 /* Diff command specific HPP columns. */
 enum {
 	PERF_HPP_DIFF__BASELINE,
@@ -323,16 +332,22 @@ static int formula_fprintf(struct hist_entry *he, struct hist_entry *pair,
 	return -1;
 }
 
-static int diff__process_sample_event(struct perf_tool *tool __maybe_unused,
+static int diff__process_sample_event(struct perf_tool *tool,
 				      union perf_event *event,
 				      struct perf_sample *sample,
 				      struct perf_evsel *evsel,
 				      struct machine *machine)
 {
+	struct perf_diff *pdiff = container_of(tool, struct perf_diff, tool);
 	struct addr_location al;
 	struct hists *hists = evsel__hists(evsel);
 	int ret = -1;
 
+	if (perf_time__ranges_skip_sample(pdiff->ptime_range, pdiff->range_num,
+					  sample->time)) {
+		return 0;
+	}
+
 	if (machine__resolve(machine, &al, sample) < 0) {
 		pr_warning("problem processing %d event, skipping it.\n",
 			   event->header.type);
@@ -359,17 +374,19 @@ static int diff__process_sample_event(struct perf_tool *tool __maybe_unused,
 	return ret;
 }
 
-static struct perf_tool tool = {
-	.sample	= diff__process_sample_event,
-	.mmap	= perf_event__process_mmap,
-	.mmap2	= perf_event__process_mmap2,
-	.comm	= perf_event__process_comm,
-	.exit	= perf_event__process_exit,
-	.fork	= perf_event__process_fork,
-	.lost	= perf_event__process_lost,
-	.namespaces = perf_event__process_namespaces,
-	.ordered_events = true,
-	.ordering_requires_timestamps = true,
+static struct perf_diff pdiff = {
+	.tool = {
+		.sample	= diff__process_sample_event,
+		.mmap	= perf_event__process_mmap,
+		.mmap2	= perf_event__process_mmap2,
+		.comm	= perf_event__process_comm,
+		.exit	= perf_event__process_exit,
+		.fork	= perf_event__process_fork,
+		.lost	= perf_event__process_lost,
+		.namespaces = perf_event__process_namespaces,
+		.ordered_events = true,
+		.ordering_requires_timestamps = true,
+	},
 };
 
 static struct perf_evsel *evsel_match(struct perf_evsel *evsel,
@@ -771,19 +788,136 @@ static void data__free(struct data__file *d)
 	}
 }
 
+static int parse_time_range(struct data__file *d,
+			    struct perf_time_interval *ptime_range,
+			    const char *time_str)
+{
+	if (perf_time__parse_str(ptime_range,
+				 time_str) != 0) {
+		if (d->session->evlist->first_sample_time == 0 &&
+		    d->session->evlist->last_sample_time == 0) {
+			pr_err("HINT: no first/last sample time found in perf data.\n"
+			       "Please use latest perf binary to execute 'perf record'\n"
+			       "(if '--buildid-all' is enabled, please set '--timestamp-boundary').\n");
+			return -EINVAL;
+		}
+
+		pdiff.range_num = perf_time__percent_parse_str(
+				ptime_range, pdiff.range_size, time_str,
+				d->session->evlist->first_sample_time,
+				d->session->evlist->last_sample_time);
+
+		if (pdiff.range_num < 0) {
+			pr_err("Invalid time string\n");
+			return -EINVAL;
+		}
+	} else {
+		pdiff.range_num = 1;
+	}
+
+	return 0;
+}
+
+static int parse_percent_time(struct data__file *d)
+{
+	return parse_time_range(d, pdiff.ptime_range, pdiff.time_str);
+}
+
+static int parse_absolute_time(struct data__file *d, char **pstr)
+{
+	char *p = *pstr;
+	int ret;
+
+	/*
+	 * Absolute timestamp for one file has the format: a.b,c.d
+	 * For multiple files, the format is: a.b,c.d:a.b,c.d
+	 */
+	p = strchr(*pstr, ':');
+	if (p) {
+		if (p == *pstr) {
+			pr_err("Invalid time string\n");
+			return -EINVAL;
+		}
+
+		*p = 0;
+		p++;
+		if (*p == 0) {
+			pr_err("Invalid time string\n");
+			return -EINVAL;
+		}
+	}
+
+	ret = parse_time_range(d, pdiff.ptime_range, *pstr);
+
+	if (!p || *p == 0)
+		*pstr = NULL;
+	else
+		*pstr = p;
+
+	return ret;
+}
+
+static int parse_time_str(char **postr)
+{
+	char *abstime_ostr = NULL;
+	bool abstime = false;
+
+	if (pdiff.time_str && strchr(pdiff.time_str, ':')) {
+		abstime = true;
+		pdiff.ptime_range = perf_time__range_alloc(NULL,
+							   &pdiff.range_size);
+	} else {
+		pdiff.ptime_range = perf_time__range_alloc(pdiff.time_str,
+							   &pdiff.range_size);
+	}
+
+	if (!pdiff.ptime_range)
+		return -ENOMEM;
+
+	if (abstime) {
+		abstime_ostr = strdup(pdiff.time_str);
+		if (!abstime_ostr) {
+			zfree(&pdiff.ptime_range);
+			return -ENOMEM;
+		}
+	}
+
+	*postr = abstime_ostr;
+	return 0;
+}
+
 static int __cmd_diff(void)
 {
 	struct data__file *d;
 	int ret = -EINVAL, i;
+	char *abstime_ostr, *abstime_tmp;
+
+	ret = parse_time_str(&abstime_ostr);
+	if (ret)
+		return ret;
+
+	abstime_tmp = abstime_ostr;
 
 	data__for_each_file(i, d) {
-		d->session = perf_session__new(&d->data, false, &tool);
+		d->session = perf_session__new(&d->data, false, &pdiff.tool);
 		if (!d->session) {
 			pr_err("Failed to open %s\n", d->data.path);
 			ret = -1;
 			goto out_delete;
 		}
 
+		if (abstime_ostr) {
+			ret = parse_absolute_time(d, &abstime_tmp);
+			if (ret < 0)
+				goto out_delete;
+		} else if (pdiff.time_str) {
+			ret = parse_percent_time(d);
+			if (ret < 0)
+				goto out_delete;
+		} else {
+			pdiff.range_num = 1;
+		}
+
 		ret = perf_session__process_events(d->session);
 		if (ret) {
 			pr_err("Failed to process %s\n", d->data.path);
@@ -802,6 +936,11 @@ static int __cmd_diff(void)
 	}
 
 	free(data__files);
+	zfree(&pdiff.ptime_range);
+
+	if (abstime_ostr)
+		free(abstime_ostr);
+
 	return ret;
 }
 
@@ -849,6 +988,8 @@ static const struct option options[] = {
 	OPT_UINTEGER('o', "order", &sort_compute, "Specify compute sorting."),
 	OPT_CALLBACK(0, "percentage", NULL, "relative|absolute",
 		     "How to display percentage of filtered entries", parse_filter_percentage),
+	OPT_STRING(0, "time", &pdiff.time_str, "str",
+		   "Time span (time percent or absolute timestamp)"),
 	OPT_END()
 };
 
-- 
2.7.4

[PATCH v2 2/3] perf diff: Support --cpu filter option

[Jin Yao]

For better support for perf diff, it would be useful to add --cpu filter
option.

Multiple CPUs can be provided as a comma-separated list with no space: 0,1.
Ranges of CPUs are specified with -: 0-2. Default is to report samples on
all CPUs.

For example,

perf diff --cpu 0,1

It only diff the samples for CPU0 and CPU1.

 v2:
 ---
 No functional change.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/Documentation/perf-diff.txt |  5 +++++
 tools/perf/builtin-diff.c              | 16 ++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/tools/perf/Documentation/perf-diff.txt b/tools/perf/Documentation/perf-diff.txt
index 5bfe876..ed13b3f 100644
--- a/tools/perf/Documentation/perf-diff.txt
+++ b/tools/perf/Documentation/perf-diff.txt
@@ -159,6 +159,11 @@ OPTIONS
 	the end of perf.data.old and analyzes the perf.data from the
 	timestamp 3971.150589 to the end of perf.data.
 
+--cpu:: Only diff samples for the list of CPUs provided. Multiple CPUs can
+	be provided as a comma-separated list with no space: 0,1. Ranges of
+	CPUs are specified with -: 0-2. Default is to report samples on all
+	CPUs.
+
 COMPARISON
 ----------
 The comparison is governed by the baseline file. The baseline perf.data
diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index f841ff9..78d0aa4 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -83,6 +83,9 @@ static unsigned int sort_compute = 1;
 static s64 compute_wdiff_w1;
 static s64 compute_wdiff_w2;
 
+static const char		*cpu_list;
+static DECLARE_BITMAP(cpu_bitmap, MAX_NR_CPUS);
+
 enum {
 	COMPUTE_DELTA,
 	COMPUTE_RATIO,
@@ -354,6 +357,11 @@ static int diff__process_sample_event(struct perf_tool *tool,
 		return -1;
 	}
 
+	if (cpu_list && !test_bit(sample->cpu, cpu_bitmap)) {
+		ret = 0;
+		goto out_put;
+	}
+
 	if (!hists__add_entry(hists, &al, NULL, NULL, NULL, sample, true)) {
 		pr_warning("problem incrementing symbol period, skipping event\n");
 		goto out_put;
@@ -918,6 +926,13 @@ static int __cmd_diff(void)
 			pdiff.range_num = 1;
 		}
 
+		if (cpu_list) {
+			ret = perf_session__cpu_bitmap(d->session, cpu_list,
+						       cpu_bitmap);
+			if (ret < 0)
+				goto out_delete;
+		}
+
 		ret = perf_session__process_events(d->session);
 		if (ret) {
 			pr_err("Failed to process %s\n", d->data.path);
@@ -990,6 +1005,7 @@ static const struct option options[] = {
 		     "How to display percentage of filtered entries", parse_filter_percentage),
 	OPT_STRING(0, "time", &pdiff.time_str, "str",
 		   "Time span (time percent or absolute timestamp)"),
+	OPT_STRING(0, "cpu", &cpu_list, "cpu", "list of cpus to profile"),
 	OPT_END()
 };
 
-- 
2.7.4

[PATCH v2 3/3] perf diff: Support --pid/--tid filter options

[Jin Yao]

For better filtering support for perf diff, it would be useful to
add --pid and --tid filter options.

For example,
perf diff --tid 13965

It only diff the samples for thread 13965.

 v2:
 ---
 No functional change.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/Documentation/perf-diff.txt | 6 ++++++
 tools/perf/builtin-diff.c              | 4 ++++
 2 files changed, 10 insertions(+)

diff --git a/tools/perf/Documentation/perf-diff.txt b/tools/perf/Documentation/perf-diff.txt
index ed13b3f..83c9496 100644
--- a/tools/perf/Documentation/perf-diff.txt
+++ b/tools/perf/Documentation/perf-diff.txt
@@ -164,6 +164,12 @@ OPTIONS
 	CPUs are specified with -: 0-2. Default is to report samples on all
 	CPUs.
 
+--pid=::
+	Only diff samples for given process ID (comma separated list).
+
+--tid=::
+	Only diff samples for given thread ID (comma separated list).
+
 COMPARISON
 ----------
 The comparison is governed by the baseline file. The baseline perf.data
diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index 78d0aa4..ce06b38 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -1006,6 +1006,10 @@ static const struct option options[] = {
 	OPT_STRING(0, "time", &pdiff.time_str, "str",
 		   "Time span (time percent or absolute timestamp)"),
 	OPT_STRING(0, "cpu", &cpu_list, "cpu", "list of cpus to profile"),
+	OPT_STRING(0, "pid", &symbol_conf.pid_list_str, "pid[,pid...]",
+		   "only consider symbols in these pids"),
+	OPT_STRING(0, "tid", &symbol_conf.tid_list_str, "tid[,tid...]",
+		   "only consider symbols in these tids"),
 	OPT_END()
 };
 
-- 
2.7.4

Re: [PATCH v2 1/3] perf diff: Support --time filter option

[Jiri Olsa]

On Tue, Feb 26, 2019 at 08:11:07PM +0800, Jin Yao wrote:

SNIP

> +	abstime_tmp = abstime_ostr;
>  
>  	data__for_each_file(i, d) {
> -		d->session = perf_session__new(&d->data, false, &tool);
> +		d->session = perf_session__new(&d->data, false, &pdiff.tool);
>  		if (!d->session) {
>  			pr_err("Failed to open %s\n", d->data.path);
>  			ret = -1;
>  			goto out_delete;
>  		}
>  
> +		if (abstime_ostr) {
> +			ret = parse_absolute_time(d, &abstime_tmp);
> +			if (ret < 0)
> +				goto out_delete;
> +		} else if (pdiff.time_str) {
> +			ret = parse_percent_time(d);
> +			if (ret < 0)
> +				goto out_delete;
> +		} else {
> +			pdiff.range_num = 1;

hum, why are we setting range_num to 1 again?

it's really hard to parse this code, maybe
it'd be better in separate loop/function
that would setup just timestamps..


thanks,
jirka

Re: [PATCH v2 1/3] perf diff: Support --time filter option

[Jiri Olsa]

On Tue, Feb 26, 2019 at 08:11:07PM +0800, Jin Yao wrote:

SNIP

> +		.ordered_events = true,
> +		.ordering_requires_timestamps = true,
> +	},
>  };
>  
>  static struct perf_evsel *evsel_match(struct perf_evsel *evsel,
> @@ -771,19 +788,136 @@ static void data__free(struct data__file *d)
>  	}
>  }
>  
> +static int parse_time_range(struct data__file *d,
> +			    struct perf_time_interval *ptime_range,
> +			    const char *time_str)
> +{
> +	if (perf_time__parse_str(ptime_range,
> +				 time_str) != 0) {
> +		if (d->session->evlist->first_sample_time == 0 &&
> +		    d->session->evlist->last_sample_time == 0) {
> +			pr_err("HINT: no first/last sample time found in perf data.\n"
> +			       "Please use latest perf binary to execute 'perf record'\n"
> +			       "(if '--buildid-all' is enabled, please set '--timestamp-boundary').\n");
> +			return -EINVAL;
> +		}
> +
> +		pdiff.range_num = perf_time__percent_parse_str(
> +				ptime_range, pdiff.range_size, time_str,
> +				d->session->evlist->first_sample_time,
> +				d->session->evlist->last_sample_time);
> +
> +		if (pdiff.range_num < 0) {
> +			pr_err("Invalid time string\n");
> +			return -EINVAL;
> +		}
> +	} else {
> +		pdiff.range_num = 1;

I dont understand why we set range_num to 1 if there's
not time option set.. it should be 0 and we should take
no action in diff__process_sample_event, right?

then I checked the report code and we do the same,
could we fix that? I'm assuming we don't need any
time check if the time option is not set.. please
correct me if I miss something

jirka

Re: [PATCH v2 1/3] perf diff: Support --time filter option

[Jin, Yao]

On 2/27/2019 5:28 PM, Jiri Olsa wrote:
> On Tue, Feb 26, 2019 at 08:11:07PM +0800, Jin Yao wrote:
> 
> SNIP
> 
>> +		.ordered_events = true,
>> +		.ordering_requires_timestamps = true,
>> +	},
>>   };
>>   
>>   static struct perf_evsel *evsel_match(struct perf_evsel *evsel,
>> @@ -771,19 +788,136 @@ static void data__free(struct data__file *d)
>>   	}
>>   }
>>   
>> +static int parse_time_range(struct data__file *d,
>> +			    struct perf_time_interval *ptime_range,
>> +			    const char *time_str)
>> +{
>> +	if (perf_time__parse_str(ptime_range,
>> +				 time_str) != 0) {
>> +		if (d->session->evlist->first_sample_time == 0 &&
>> +		    d->session->evlist->last_sample_time == 0) {
>> +			pr_err("HINT: no first/last sample time found in perf data.\n"
>> +			       "Please use latest perf binary to execute 'perf record'\n"
>> +			       "(if '--buildid-all' is enabled, please set '--timestamp-boundary').\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		pdiff.range_num = perf_time__percent_parse_str(
>> +				ptime_range, pdiff.range_size, time_str,
>> +				d->session->evlist->first_sample_time,
>> +				d->session->evlist->last_sample_time);
>> +
>> +		if (pdiff.range_num < 0) {
>> +			pr_err("Invalid time string\n");
>> +			return -EINVAL;
>> +		}
>> +	} else {
>> +		pdiff.range_num = 1;
> 
> I dont understand why we set range_num to 1 if there's
> not time option set.. it should be 0 and we should take
> no action in diff__process_sample_event, right?
> 
> then I checked the report code and we do the same,
> could we fix that? I'm assuming we don't need any
> time check if the time option is not set.. please
> correct me if I miss something
> 
> jirka
> 

We support multiple complicated time strings. :(

In parse_time_range(), perf_time__parse_str() returns 0 if the time 
string is a simple start/stop format. So next, we set the range_num to 
1. If the time string contains multiple time percent ranges (e.g. 
"10%/1,10%/2,..."), perf_time__parse_str() will return with error (<0), 
then we will continue checking with perf_time__percent_parse_str().

So when range_num is set to 1, it just means it's the simple time string.

Thanks
Jin Yao

Re: [PATCH v2 1/3] perf diff: Support --time filter option

[Jin, Yao]

On 2/27/2019 5:27 PM, Jiri Olsa wrote:
> On Tue, Feb 26, 2019 at 08:11:07PM +0800, Jin Yao wrote:
> 
> SNIP
> 
>> +	abstime_tmp = abstime_ostr;
>>   
>>   	data__for_each_file(i, d) {
>> -		d->session = perf_session__new(&d->data, false, &tool);
>> +		d->session = perf_session__new(&d->data, false, &pdiff.tool);
>>   		if (!d->session) {
>>   			pr_err("Failed to open %s\n", d->data.path);
>>   			ret = -1;
>>   			goto out_delete;
>>   		}
>>   
>> +		if (abstime_ostr) {
>> +			ret = parse_absolute_time(d, &abstime_tmp);
>> +			if (ret < 0)
>> +				goto out_delete;
>> +		} else if (pdiff.time_str) {
>> +			ret = parse_percent_time(d);
>> +			if (ret < 0)
>> +				goto out_delete;
>> +		} else {
>> +			pdiff.range_num = 1;
> 
> hum, why are we setting range_num to 1 again?

Yes, that may be not necessary. I will remove this line and test again.

> 
> it's really hard to parse this code, maybe
> it'd be better in separate loop/function
> that would setup just timestamps..
> 

Do you mean the above parsing code should be put in a separate function 
(e.g. parse_time_string in following example)?

data__for_each_file(i, d) {
	....
	d->session = perf_session__new(&d->data, false, &pdiff.tool);
	....
	parse_time_string(...);
	ret = perf_session__process_events(d->session);
	....
}

Thanks
Jin Yao

> 
> thanks,
> jirka
> 

Re: [PATCH v2 1/3] perf diff: Support --time filter option

[Jiri Olsa]

On Wed, Feb 27, 2019 at 08:51:44PM +0800, Jin, Yao wrote:
> 
> 
> On 2/27/2019 5:28 PM, Jiri Olsa wrote:
> > On Tue, Feb 26, 2019 at 08:11:07PM +0800, Jin Yao wrote:
> > 
> > SNIP
> > 
> > > +		.ordered_events = true,
> > > +		.ordering_requires_timestamps = true,
> > > +	},
> > >   };
> > >   static struct perf_evsel *evsel_match(struct perf_evsel *evsel,
> > > @@ -771,19 +788,136 @@ static void data__free(struct data__file *d)
> > >   	}
> > >   }
> > > +static int parse_time_range(struct data__file *d,
> > > +			    struct perf_time_interval *ptime_range,
> > > +			    const char *time_str)
> > > +{
> > > +	if (perf_time__parse_str(ptime_range,
> > > +				 time_str) != 0) {
> > > +		if (d->session->evlist->first_sample_time == 0 &&
> > > +		    d->session->evlist->last_sample_time == 0) {
> > > +			pr_err("HINT: no first/last sample time found in perf data.\n"
> > > +			       "Please use latest perf binary to execute 'perf record'\n"
> > > +			       "(if '--buildid-all' is enabled, please set '--timestamp-boundary').\n");
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		pdiff.range_num = perf_time__percent_parse_str(
> > > +				ptime_range, pdiff.range_size, time_str,
> > > +				d->session->evlist->first_sample_time,
> > > +				d->session->evlist->last_sample_time);
> > > +
> > > +		if (pdiff.range_num < 0) {
> > > +			pr_err("Invalid time string\n");
> > > +			return -EINVAL;
> > > +		}
> > > +	} else {
> > > +		pdiff.range_num = 1;
> > 
> > I dont understand why we set range_num to 1 if there's
> > not time option set.. it should be 0 and we should take
> > no action in diff__process_sample_event, right?
> > 
> > then I checked the report code and we do the same,
> > could we fix that? I'm assuming we don't need any
> > time check if the time option is not set.. please
> > correct me if I miss something
> > 
> > jirka
> > 
> 
> We support multiple complicated time strings. :(
> 
> In parse_time_range(), perf_time__parse_str() returns 0 if the time string
> is a simple start/stop format. So next, we set the range_num to 1. If the
> time string contains multiple time percent ranges (e.g. "10%/1,10%/2,..."),
> perf_time__parse_str() will return with error (<0), then we will continue
> checking with perf_time__percent_parse_str().
> 
> So when range_num is set to 1, it just means it's the simple time string.

why do we need to have time range set if there's no --time
option set by user?

jirka

Re: [PATCH v2 1/3] perf diff: Support --time filter option

[Jiri Olsa]

On Wed, Feb 27, 2019 at 09:04:21PM +0800, Jin, Yao wrote:
> 
> 
> On 2/27/2019 5:27 PM, Jiri Olsa wrote:
> > On Tue, Feb 26, 2019 at 08:11:07PM +0800, Jin Yao wrote:
> > 
> > SNIP
> > 
> > > +	abstime_tmp = abstime_ostr;
> > >   	data__for_each_file(i, d) {
> > > -		d->session = perf_session__new(&d->data, false, &tool);
> > > +		d->session = perf_session__new(&d->data, false, &pdiff.tool);
> > >   		if (!d->session) {
> > >   			pr_err("Failed to open %s\n", d->data.path);
> > >   			ret = -1;
> > >   			goto out_delete;
> > >   		}
> > > +		if (abstime_ostr) {
> > > +			ret = parse_absolute_time(d, &abstime_tmp);
> > > +			if (ret < 0)
> > > +				goto out_delete;
> > > +		} else if (pdiff.time_str) {
> > > +			ret = parse_percent_time(d);
> > > +			if (ret < 0)
> > > +				goto out_delete;
> > > +		} else {
> > > +			pdiff.range_num = 1;
> > 
> > hum, why are we setting range_num to 1 again?
> 
> Yes, that may be not necessary. I will remove this line and test again.
> 
> > 
> > it's really hard to parse this code, maybe
> > it'd be better in separate loop/function
> > that would setup just timestamps..
> > 
> 
> Do you mean the above parsing code should be put in a separate function
> (e.g. parse_time_string in following example)?
> 
> data__for_each_file(i, d) {
> 	....
> 	d->session = perf_session__new(&d->data, false, &pdiff.tool);
> 	....
> 	parse_time_string(...);
> 	ret = perf_session__process_events(d->session);
> 	....
> }

anything that would make this more clear/readable ;-)

thanks,
jirka

Re: [PATCH v2 1/3] perf diff: Support --time filter option

[Jin, Yao]

On 2/27/2019 9:10 PM, Jiri Olsa wrote:
> On Wed, Feb 27, 2019 at 08:51:44PM +0800, Jin, Yao wrote:
>>
>>
>> On 2/27/2019 5:28 PM, Jiri Olsa wrote:
>>> On Tue, Feb 26, 2019 at 08:11:07PM +0800, Jin Yao wrote:
>>>
>>> SNIP
>>>
>>>> +		.ordered_events = true,
>>>> +		.ordering_requires_timestamps = true,
>>>> +	},
>>>>    };
>>>>    static struct perf_evsel *evsel_match(struct perf_evsel *evsel,
>>>> @@ -771,19 +788,136 @@ static void data__free(struct data__file *d)
>>>>    	}
>>>>    }
>>>> +static int parse_time_range(struct data__file *d,
>>>> +			    struct perf_time_interval *ptime_range,
>>>> +			    const char *time_str)
>>>> +{
>>>> +	if (perf_time__parse_str(ptime_range,
>>>> +				 time_str) != 0) {
>>>> +		if (d->session->evlist->first_sample_time == 0 &&
>>>> +		    d->session->evlist->last_sample_time == 0) {
>>>> +			pr_err("HINT: no first/last sample time found in perf data.\n"
>>>> +			       "Please use latest perf binary to execute 'perf record'\n"
>>>> +			       "(if '--buildid-all' is enabled, please set '--timestamp-boundary').\n");
>>>> +			return -EINVAL;
>>>> +		}
>>>> +
>>>> +		pdiff.range_num = perf_time__percent_parse_str(
>>>> +				ptime_range, pdiff.range_size, time_str,
>>>> +				d->session->evlist->first_sample_time,
>>>> +				d->session->evlist->last_sample_time);
>>>> +
>>>> +		if (pdiff.range_num < 0) {
>>>> +			pr_err("Invalid time string\n");
>>>> +			return -EINVAL;
>>>> +		}
>>>> +	} else {
>>>> +		pdiff.range_num = 1;
>>>
>>> I dont understand why we set range_num to 1 if there's
>>> not time option set.. it should be 0 and we should take
>>> no action in diff__process_sample_event, right?
>>>
>>> then I checked the report code and we do the same,
>>> could we fix that? I'm assuming we don't need any
>>> time check if the time option is not set.. please
>>> correct me if I miss something
>>>
>>> jirka
>>>
>>
>> We support multiple complicated time strings. :(
>>
>> In parse_time_range(), perf_time__parse_str() returns 0 if the time string
>> is a simple start/stop format. So next, we set the range_num to 1. If the
>> time string contains multiple time percent ranges (e.g. "10%/1,10%/2,..."),
>> perf_time__parse_str() will return with error (<0), then we will continue
>> checking with perf_time__percent_parse_str().
>>
>> So when range_num is set to 1, it just means it's the simple time string.
> 
> why do we need to have time range set if there's no --time
> option set by user?
> 
> jirka
> 

Yes, that could be refined if no --time option set by user. I think I 
can add a new patch to fix these for perf report/script.

Thanks
Jin Yao