Messages in this thread | | | Date | Thu, 21 May 2020 13:48:59 +0200 | From | Jiri Olsa <> | Subject | Re: [PATCH v3 3/9] perf stat: factor out event handling loop into a function |
| |
On Wed, May 20, 2020 at 06:17:40PM +0300, Alexey Budankov wrote:
SNIP
> >> @@ -675,16 +708,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) > >> + handle_events(child_pid, &stat_config); > >> + > >> if (child_pid != -1) { > >> if (timeout) > >> kill(child_pid, SIGTERM); > >> @@ -701,18 +727,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; > >> - } > >> - } > >> + handle_events(-1, &stat_config); > > > > this makes me worried.. I'm not sure if it's good idea > > to squash these 2 looops into one, because they are already > > complex as they are.. and one of you following patches is > > making it even more complex > > Loops bodies are mostly identical. The only difference is in events > they wait for and API used for that. Adding of more events will > complicate further. The code is duplicated, thus needs refactoring. > If the following patch complicates lets organize the patch it into > several smaller functions.
yea, that might help
jirka
> > > > > wouldn't it be better if you just add single call into > > each of them.. that would poll on your fd and process the > > commands if needed? > > That's of course possible, but doesn't manage existing complexity > at the first place - __run_perf_stat(). > > Let's still have handle_events() as a general dispatcher and implement > handlers for different events as separate functions? > > ~Alexey >
| |