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  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Fri, 1 Mar 2019 08:55:05 +0800
From:   "Jin, Yao" <yao.jin@...ux.intel.com>
To:     Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
Cc:     jolsa@...nel.org, peterz@...radead.org, mingo@...hat.com,
        alexander.shishkin@...ux.intel.com, Linux-kernel@...r.kernel.org,
        ak@...ux.intel.com, kan.liang@...el.com, yao.jin@...el.com
Subject: Re: [PATCH] perf util: Refactor time range parsing code



On 3/1/2019 1:57 AM, Arnaldo Carvalho de Melo wrote:
> Em Thu, Feb 28, 2019 at 10:30:09PM +0800, Jin Yao escreveu:
>> Jiri points out that we don't need any time checking and time string
>> parsing if the --time option is not set. That makes sense.
>>
>> This patch refactors the time range parsing code, move the duplicated
>> code from perf report and perf script to time_utils and check if --time
>> option is set before parsing the time string.
> 
> So, this should come with a "No logic change expected", but I'd say that
> for this to be more quickly/easily tested, please provide intructions
> about how to test that this indeed didn't change anything inadvertently.
> 
> Simply looking at the code should be enough, but having instructions on
> how to use this helps in testing and advertises further the feature.
> 
> I.e. try not to lose an opportunity to teach about these perf features
> :-)
> 
> - Arnaldo
>   

Oh, yes, you are right. I should provide some information in patch 
description about how to test that. I will add this instructions in v2.

Thanks
Jin Yao

>> Signed-off-by: Jin Yao <yao.jin@...ux.intel.com>
>> ---
>>   tools/perf/builtin-report.c  | 38 +++++++--------------------------
>>   tools/perf/builtin-script.c  | 39 +++++++--------------------------
>>   tools/perf/util/time-utils.c | 51 +++++++++++++++++++++++++++++++++++++++++++-
>>   tools/perf/util/time-utils.h |  6 ++++++
>>   4 files changed, 72 insertions(+), 62 deletions(-)
>>
>> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
>> index 1532ebd..ee93c18 100644
>> --- a/tools/perf/builtin-report.c
>> +++ b/tools/perf/builtin-report.c
>> @@ -1375,36 +1375,13 @@ int cmd_report(int argc, const char **argv)
>>   	if (symbol__init(&session->header.env) < 0)
>>   		goto error;
>>   
>> -	report.ptime_range = perf_time__range_alloc(report.time_str,
>> -						    &report.range_size);
>> -	if (!report.ptime_range) {
>> -		ret = -ENOMEM;
>> -		goto error;
>> -	}
>> -
>> -	if (perf_time__parse_str(report.ptime_range, report.time_str) != 0) {
>> -		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"
>> -			       "Please use latest perf binary to execute 'perf record'\n"
>> -			       "(if '--buildid-all' is enabled, please set '--timestamp-boundary').\n");
>> -			ret = -EINVAL;
>> -			goto error;
>> -		}
>> -
>> -		report.range_num = perf_time__percent_parse_str(
>> -					report.ptime_range, report.range_size,
>> -					report.time_str,
>> -					session->evlist->first_sample_time,
>> -					session->evlist->last_sample_time);
>> -
>> -		if (report.range_num < 0) {
>> -			pr_err("Invalid time string\n");
>> -			ret = -EINVAL;
>> +	if (report.time_str) {
>> +		ret = perf_time__parse_for_ranges(report.time_str, session,
>> +						  &report.ptime_range,
>> +						  &report.range_size,
>> +						  &report.range_num);
>> +		if (ret < 0)
>>   			goto error;
>> -		}
>> -	} else {
>> -		report.range_num = 1;
>>   	}
>>   
>>   	if (session->tevent.pevent &&
>> @@ -1426,7 +1403,8 @@ int cmd_report(int argc, const char **argv)
>>   		ret = 0;
>>   
>>   error:
>> -	zfree(&report.ptime_range);
>> +	if (report.ptime_range)
>> +		zfree(&report.ptime_range);
>>   
>>   	perf_session__delete(session);
>>   	return ret;
>> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
>> index 2d8cb1d..53f78cf 100644
>> --- a/tools/perf/builtin-script.c
>> +++ b/tools/perf/builtin-script.c
>> @@ -3699,37 +3699,13 @@ int cmd_script(int argc, const char **argv)
>>   	if (err < 0)
>>   		goto out_delete;
>>   
>> -	script.ptime_range = perf_time__range_alloc(script.time_str,
>> -						    &script.range_size);
>> -	if (!script.ptime_range) {
>> -		err = -ENOMEM;
>> -		goto out_delete;
>> -	}
>> -
>> -	/* needs to be parsed after looking up reference time */
>> -	if (perf_time__parse_str(script.ptime_range, script.time_str) != 0) {
>> -		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"
>> -			       "Please use latest perf binary to execute 'perf record'\n"
>> -			       "(if '--buildid-all' is enabled, please set '--timestamp-boundary').\n");
>> -			err = -EINVAL;
>> -			goto out_delete;
>> -		}
>> -
>> -		script.range_num = perf_time__percent_parse_str(
>> -					script.ptime_range, script.range_size,
>> -					script.time_str,
>> -					session->evlist->first_sample_time,
>> -					session->evlist->last_sample_time);
>> -
>> -		if (script.range_num < 0) {
>> -			pr_err("Invalid time string\n");
>> -			err = -EINVAL;
>> +	if (script.time_str) {
>> +		err = perf_time__parse_for_ranges(script.time_str, session,
>> +						  &script.ptime_range,
>> +						  &script.range_size,
>> +						  &script.range_num);
>> +		if (err < 0)
>>   			goto out_delete;
>> -		}
>> -	} else {
>> -		script.range_num = 1;
>>   	}
>>   
>>   	err = __cmd_script(&script);
>> @@ -3737,7 +3713,8 @@ int cmd_script(int argc, const char **argv)
>>   	flush_scripting();
>>   
>>   out_delete:
>> -	zfree(&script.ptime_range);
>> +	if (script.ptime_range)
>> +		zfree(&script.ptime_range);
>>   
>>   	perf_evlist__free_stats(session->evlist);
>>   	perf_session__delete(session);
>> diff --git a/tools/perf/util/time-utils.c b/tools/perf/util/time-utils.c
>> index 6193b46..0f53bae 100644
>> --- a/tools/perf/util/time-utils.c
>> +++ b/tools/perf/util/time-utils.c
>> @@ -11,6 +11,8 @@
>>   #include "perf.h"
>>   #include "debug.h"
>>   #include "time-utils.h"
>> +#include "session.h"
>> +#include "evlist.h"
>>   
>>   int parse_nsec_time(const char *str, u64 *ptime)
>>   {
>> @@ -374,7 +376,7 @@ bool perf_time__ranges_skip_sample(struct perf_time_interval *ptime_buf,
>>   	struct perf_time_interval *ptime;
>>   	int i;
>>   
>> -	if ((timestamp == 0) || (num == 0))
>> +	if ((!ptime_buf) || (timestamp == 0) || (num == 0))
>>   		return false;
>>   
>>   	if (num == 1)
>> @@ -396,6 +398,53 @@ bool perf_time__ranges_skip_sample(struct perf_time_interval *ptime_buf,
>>   	return (i == num) ? true : false;
>>   }
>>   
>> +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)
>> +{
>> +	struct perf_time_interval *ptime_range;
>> +	int size, num, ret;
>> +
>> +	ptime_range = perf_time__range_alloc(time_str, &size);
>> +	if (!ptime_range)
>> +		return -ENOMEM;
>> +
>> +	if (perf_time__parse_str(ptime_range, time_str) != 0) {
>> +		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"
>> +			       "Please use latest perf binary to execute 'perf record'\n"
>> +			       "(if '--buildid-all' is enabled, please set '--timestamp-boundary').\n");
>> +			ret = -EINVAL;
>> +			goto error;
>> +		}
>> +
>> +		num = perf_time__percent_parse_str(
>> +				ptime_range, size,
>> +				time_str,
>> +				session->evlist->first_sample_time,
>> +				session->evlist->last_sample_time);
>> +
>> +		if (num < 0) {
>> +			pr_err("Invalid time string\n");
>> +			ret = -EINVAL;
>> +			goto error;
>> +		}
>> +	} else {
>> +		num = 1;
>> +	}
>> +
>> +	*range_size = size;
>> +	*range_num = num;
>> +	*ranges = ptime_range;
>> +	return 0;
>> +
>> +error:
>> +	free(ptime_range);
>> +	return ret;
>> +}
>> +
>>   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 70b177d..b923de4 100644
>> --- a/tools/perf/util/time-utils.h
>> +++ b/tools/perf/util/time-utils.h
>> @@ -23,6 +23,12 @@ bool perf_time__skip_sample(struct perf_time_interval *ptime, u64 timestamp);
>>   bool perf_time__ranges_skip_sample(struct perf_time_interval *ptime_buf,
>>   				   int num, u64 timestamp);
>>   
>> +struct perf_session;
>> +
>> +int perf_time__parse_for_ranges(const char *str, struct perf_session *session,
>> +				struct perf_time_interval **ranges,
>> +				int *range_size, int *range_num);
>> +
>>   int timestamp__scnprintf_usec(u64 timestamp, char *buf, size_t sz);
>>   
>>   int fetch_current_timestamp(char *buf, size_t sz);
>> -- 
>> 2.7.4
> 

Powered by blists - more mailing lists