From: Jiri Olsa <jolsa@redhat.com>
To: Alexey Budankov <alexey.budankov@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>,
Namhyung Kim <namhyung@kernel.org>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>, Andi Kleen <ak@linux.intel.com>,
linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4 04/10] perf stat: factor out event handling loop into a function
Date: Sun, 31 May 2020 20:19:05 +0200 [thread overview]
Message-ID: <20200531181905.GB881900@krava> (raw)
In-Reply-To: <5f42c6c5-c301-accd-928e-4304fb1c15d0@linux.intel.com>
On Mon, May 25, 2020 at 05:19:45PM +0300, Alexey Budankov wrote:
SNIP
> @@ -544,12 +598,10 @@ static enum counter_recovery stat_handle_error(struct evsel *counter)
> static int __run_perf_stat(int argc, const char **argv, int run_idx)
> {
> int interval = stat_config.interval;
> - int times = stat_config.times;
> int timeout = stat_config.timeout;
> char msg[BUFSIZ];
> unsigned long long t0, t1;
> struct evsel *counter;
> - struct timespec ts;
> size_t l;
> int status = 0;
> const bool forks = (argc > 0);
> @@ -558,17 +610,6 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
> int i, cpu;
> bool second_pass = false;
>
> - if (interval) {
> - ts.tv_sec = interval / USEC_PER_MSEC;
> - ts.tv_nsec = (interval % USEC_PER_MSEC) * NSEC_PER_MSEC;
> - } else if (timeout) {
> - ts.tv_sec = timeout / USEC_PER_MSEC;
> - ts.tv_nsec = (timeout % USEC_PER_MSEC) * NSEC_PER_MSEC;
> - } else {
> - ts.tv_sec = 1;
> - ts.tv_nsec = 0;
> - }
> -
> if (forks) {
> if (perf_evlist__prepare_workload(evsel_list, &target, argv, is_pipe,
> workload_exec_failed_signal) < 0) {
> @@ -725,16 +766,9 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
> perf_evlist__start_workload(evsel_list);
> enable_counters();
>
> - if (interval || timeout) {
> - while (!waitpid(child_pid, &status, WNOHANG)) {
> - nanosleep(&ts, NULL);
> - if (timeout)
> - break;
> - process_interval();
> - if (interval_count && !(--times))
> - break;
> - }
> - }
> + if (interval || timeout)
> + dispatch_events(child_pid, &stat_config);
> +
> if (child_pid != -1) {
> if (timeout)
> kill(child_pid, SIGTERM);
> @@ -751,18 +785,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
> psignal(WTERMSIG(status), argv[0]);
> } else {
> enable_counters();
> - while (!done) {
> - nanosleep(&ts, NULL);
> - if (!is_target_alive(&target, evsel_list->core.threads))
> - break;
> - if (timeout)
> - break;
> - if (interval) {
> - process_interval();
> - if (interval_count && !(--times))
> - break;
> - }
> - }
> + dispatch_events(-1, &stat_config);
hum, from the discussion we had on v3 I expected more smaller patches
with easy changes, so the change is more transparent and easy to review
as I said before this part really makes me worried and needs to be as clear
as possible.. please introdce the new function first and replace the factored
places separately, also more verbose changelog would help ;-)
thanks,
jirka
> }
>
> disable_counters();
> --
> 2.24.1
>
next prev parent reply other threads:[~2020-05-31 18:19 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-25 14:11 [PATCH v4 00/10] perf: support enable and disable commands in stat and record modes Alexey Budankov
2020-05-25 14:17 ` [PATCH v4 01/10] tools/libperf: introduce static poll file descriptors Alexey Budankov
2020-05-31 18:19 ` Jiri Olsa
2020-06-01 7:34 ` Alexey Budankov
2020-05-25 14:18 ` [PATCH v4 02/10] perf evlist: introduce control " Alexey Budankov
2020-05-25 14:19 ` [PATCH v4 03/10] perf evlist: implement control command handling functions Alexey Budankov
2020-05-25 14:19 ` [PATCH v4 04/10] perf stat: factor out event handling loop into a function Alexey Budankov
2020-05-31 18:19 ` Jiri Olsa [this message]
2020-06-01 7:38 ` Alexey Budankov
2020-06-01 16:10 ` Alexey Budankov
2020-05-25 14:20 ` [PATCH v4 05/10] perf stat: extend -D,--delay option with -1 value Alexey Budankov
2020-05-25 14:21 ` [PATCH v4 06/10] perf stat: implement control commands handling Alexey Budankov
2020-05-25 14:21 ` [PATCH v4 07/10] perf stat: introduce --ctl-fd[-ack] options Alexey Budankov
2020-05-25 14:22 ` [PATCH v4 08/10] perf record: extend -D,--delay option with -1 value Alexey Budankov
2020-05-25 14:23 ` [PATCH v4 09/10] perf record: implement control commands handling Alexey Budankov
2020-05-25 14:23 ` [PATCH v4 10/10] perf record: introduce --ctl-fd[-ack] options Alexey Budankov
2020-06-03 12:05 ` Adrian Hunter
2020-06-03 12:52 ` Alexey Budankov
2020-06-03 15:44 ` Adrian Hunter
2020-06-03 15:52 ` Alexey Budankov
2020-05-27 9:27 ` [PATCH v4 00/10] perf: support enable and disable commands in stat and record modes Alexey Budankov
2020-05-27 10:29 ` Jiri Olsa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200531181905.GB881900@krava \
--to=jolsa@redhat.com \
--cc=acme@kernel.org \
--cc=ak@linux.intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=alexey.budankov@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox