On 31.05.2020 21:19, Jiri Olsa wrote:
> 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 ;-)
Ok. Will try to reshape the patch that way.
~Alexey
