On 20.05.2020 15:38, Jiri Olsa wrote: > On Wed, May 13, 2020 at 11:04:25AM +0300, Alexey Budankov wrote: >> >> Implement handling of 'enable' and 'disable' control commands >> coming from control file descriptor. >> >> Signed-off-by: Alexey Budankov <[email protected]> >> --- >> tools/perf/builtin-record.c | 18 +++++++++++++++++- >> 1 file changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c >> index 50dc2fe626e5..72f388623364 100644 >> --- a/tools/perf/builtin-record.c >> +++ b/tools/perf/builtin-record.c >> @@ -1505,6 +1505,7 @@ static int __cmd_record(struct record *rec, int argc, >> const char **argv) >> bool disabled = false, draining = false; >> int fd; >> float ratio = 0; >> + enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED; >> >> atexit(record__sig_exit); >> signal(SIGCHLD, sig_handler); >> @@ -1802,8 +1803,23 @@ static int __cmd_record(struct record *rec, int argc, >> const char **argv) >> * Propagate error, only if there's any. Ignore positive >> * number of returned events and interrupt error. >> */ >> - if (err > 0 || (err < 0 && errno == EINTR)) >> + if (err > 0 || (err < 0 && errno == EINTR)) { >> err = 0; >> + if (evlist__ctlfd_process(rec->evlist, &cmd) > >> 0) { >> + switch (cmd) { >> + case EVLIST_CTL_CMD_ENABLE: >> + pr_info(EVLIST_ENABLED_MSG); >> + break; >> + case EVLIST_CTL_CMD_DISABLE: >> + pr_info(EVLIST_DISABLED_MSG); >> + break; >> + case EVLIST_CTL_CMD_ACK: >> + case EVLIST_CTL_CMD_UNSUPPORTED: >> + default: >> + break; >> + } >> + } >> + } > > so this will be processed only when: > > if (hits == rec->samples) { > > what if there's always somethign in the buffer? will this stall? > >> waking++; >> >> if (evlist__filter_pollfd(rec->evlist, POLLERR | >> POLLHUP) == 0) > > evlist__filter_pollfd will trigger draining if all the maps are closed, > but there's one more fd in pollfd now, will this prevent draining?
Good catch. This needs redesign to notice pending commands during active trace streaming. The same is regarding draining. > > I wonder this would fit better to the sideband thread (util/sideband_evlist.c) > so we don't disturb the main thread with another check ~Alexey
