| 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 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <9537c5a0-66a2-1135-1bb1-a33b251ca666@linux.intel.com>
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