Messages in this thread | | | Date | Wed, 3 Feb 2021 18:24:09 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH 09/24] perf daemon: Add signalfd support |
| |
Em Sun, Jan 31, 2021 at 12:48:41AM +0100, Jiri Olsa escreveu: > Using signalfd fd for tracking SIGCHLD signals as > notification for perf session termination. > > This way we don't need to actively check for child > status, but we are notified if there's change. > > Suggested-by: Alexei Budankov <abudankov@huawei.com> > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > tools/perf/builtin-daemon.c | 147 ++++++++++++++++++++++++++++++++++-- > 1 file changed, 141 insertions(+), 6 deletions(-) > > diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c > index 324666058842..69d3af70b642 100644 > --- a/tools/perf/builtin-daemon.c > +++ b/tools/perf/builtin-daemon.c > @@ -7,6 +7,7 @@ > #include <string.h> > #include <signal.h> > #include <stdlib.h> > +#include <time.h> > #include <stdio.h> > #include <unistd.h> > #include <errno.h> > @@ -16,6 +17,8 @@ > #include <sys/socket.h> > #include <sys/un.h> > #include <sys/stat.h> > +#include <sys/signalfd.h> > +#include <sys/wait.h> > #include <poll.h> > #include <sys/stat.h> > #include "builtin.h" > @@ -81,6 +84,7 @@ struct daemon { > struct list_head sessions; > FILE *out; > char perf[PATH_MAX]; > + int signal_fd; > }; > > static struct daemon __daemon = { > @@ -351,6 +355,103 @@ static int session__run(struct session *session, struct daemon *daemon) > return -1; > } > > +static pid_t handle_signalfd(struct daemon *daemon) > +{ > + struct signalfd_siginfo si; > + struct session *session; > + ssize_t err; > + int status; > + pid_t pid; > + > + err = read(daemon->signal_fd, &si, sizeof(struct signalfd_siginfo));
I saw these and recalled we have a readn() function, shouldn't we be usint it in this series?
Its even in tools/lib/perf/lib.c
> + if (err != sizeof(struct signalfd_siginfo)) > + return -1; > + > + list_for_each_entry(session, &daemon->sessions, list) { > + > + if (session->pid != (int) si.ssi_pid) > + continue; > + > + pid = waitpid(session->pid, &status, 0); > + if (pid == session->pid) { > + if (WIFEXITED(status)) { > + pr_info("session '%s' exited, status=%d\n", > + session->name, WEXITSTATUS(status)); > + } else if (WIFSIGNALED(status)) { > + pr_info("session '%s' killed (signal %d)\n", > + session->name, WTERMSIG(status)); > + } else if (WIFSTOPPED(status)) { > + pr_info("session '%s' stopped (signal %d)\n", > + session->name, WSTOPSIG(status)); > + } else { > + pr_info("session '%s' Unexpected status (0x%x)\n", > + session->name, status); > + } > + } > + > + session->state = SESSION_STATE__KILL; > + session->pid = -1; > + return pid; > + } > + > + return 0; > +} > + > +static int session__wait(struct session *session, struct daemon *daemon, int secs) > +{ > + struct pollfd pollfd = { > + .fd = daemon->signal_fd, > + .events = POLLIN, > + }; > + pid_t wpid = 0, pid = session->pid; > + time_t start; > + > + start = time(NULL);
time_t start = time(NULL); > + > + do { > + if (poll(&pollfd, 1, 1000)) > + wpid = handle_signalfd(daemon);
Shouldn't we check for -1 and handle that differently?
> + > + if (start + secs < time(NULL)) > + return -1; > + } while (wpid != pid); > + > + return 0; > +} > + > +static bool daemon__has_alive_session(struct daemon *daemon) > +{ > + struct session *session; > + > + list_for_each_entry(session, &daemon->sessions, list) { > + if (session->pid != -1) > + return true; > + } > + > + return false; > +} > + > +static int daemon__wait(struct daemon *daemon, int secs) > +{ > + struct pollfd pollfd = { > + .fd = daemon->signal_fd, > + .events = POLLIN, > + }; > + time_t start; > + > + start = time(NULL); > + > + do { > + if (poll(&pollfd, 1, 1000)) > + handle_signalfd(daemon);
ditto
> + > + if (start + secs < time(NULL)) > + return -1; > + } while (daemon__has_alive_session(daemon)); > + > + return 0; > +} > + > static int setup_server_socket(struct daemon *daemon) > { > struct sockaddr_un addr; > @@ -456,9 +557,13 @@ static int session__signal(struct session *session, int sig) > return kill(session->pid, sig); > } > > -static void session__kill(struct session *session) > +static void session__kill(struct session *session, struct daemon *daemon) > { > session__signal(session, SIGTERM); > + if (session__wait(session, daemon, 10)) { > + session__signal(session, SIGKILL); > + session__wait(session, daemon, 10); > + } > } > > static void daemon__signal(struct daemon *daemon, int sig) > @@ -486,6 +591,10 @@ static void session__remove(struct session *session) > static void daemon__kill(struct daemon *daemon) > { > daemon__signal(daemon, SIGTERM); > + if (daemon__wait(daemon, 10)) { > + daemon__signal(daemon, SIGKILL); > + daemon__wait(daemon, 10); > + } > } > > static void __daemon__free(struct daemon *daemon) > @@ -523,7 +632,7 @@ static int daemon__reconfig(struct daemon *daemon) > /* Remove session. */ > if (session->state == SESSION_STATE__KILL) { > if (session->pid > 0) { > - session__kill(session); > + session__kill(session, daemon); > pr_info("reconfig: session '%s' killed\n", session->name); > } > session__remove(session); > @@ -532,7 +641,7 @@ static int daemon__reconfig(struct daemon *daemon) > > /* Reconfig session. */ > if (session->pid > 0) { > - session__kill(session); > + session__kill(session, daemon); > pr_info("reconfig: session '%s' killed\n", session->name); > } > if (session__run(session, daemon)) > @@ -690,6 +799,20 @@ static int go_background(struct daemon *daemon) > return 0; > } > > +static int setup_signalfd(struct daemon *daemon) > +{ > + sigset_t mask; > + > + sigemptyset(&mask); > + sigaddset(&mask, SIGCHLD); > + > + if (sigprocmask(SIG_BLOCK, &mask, NULL) == -1) > + return -1; > + > + daemon->signal_fd = signalfd(-1, &mask, SFD_NONBLOCK|SFD_CLOEXEC); > + return daemon->signal_fd; > +} > + > static int __cmd_start(struct daemon *daemon, struct option parent_options[], > int argc, const char **argv) > { > @@ -699,8 +822,8 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[], > OPT_PARENT(parent_options), > OPT_END() > }; > - int sock_fd = -1, conf_fd = -1; > - int sock_pos, file_pos; > + int sock_fd = -1, conf_fd = -1, signal_fd = -1; > + int sock_pos, file_pos, signal_pos; > struct fdarray fda; > int err = 0; > > @@ -732,7 +855,7 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[], > > pr_info("daemon started (pid %d)\n", getpid()); > > - fdarray__init(&fda, 2); > + fdarray__init(&fda, 3); > > sock_fd = setup_server_socket(daemon); > if (sock_fd < 0) > @@ -742,6 +865,10 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[], > if (conf_fd < 0) > goto out; > > + signal_fd = setup_signalfd(daemon); > + if (signal_fd < 0) > + goto out; > + > sock_pos = fdarray__add(&fda, sock_fd, POLLIN|POLLERR|POLLHUP, 0); > if (sock_pos < 0) > goto out; > @@ -750,6 +877,10 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[], > if (file_pos < 0) > goto out; > > + signal_pos = fdarray__add(&fda, signal_fd, POLLIN|POLLERR|POLLHUP, 0); > + if (signal_pos < 0) > + goto out; > + > signal(SIGINT, sig_handler); > signal(SIGTERM, sig_handler); > > @@ -763,6 +894,8 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[], > err = handle_server_socket(daemon, sock_fd); > if (fda.entries[file_pos].revents & POLLIN) > err = handle_config_changes(daemon, conf_fd, &reconfig); > + if (fda.entries[signal_pos].revents & POLLIN) > + err = handle_signalfd(daemon) < 0; > > if (reconfig) > err = setup_server_config(daemon); > @@ -778,6 +911,8 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[], > close(sock_fd); > if (conf_fd != -1) > close(conf_fd); > + if (conf_fd != -1) > + close(signal_fd); > > pr_info("daemon exited\n"); > fclose(daemon->out); > -- > 2.29.2 >
--
- Arnaldo
| |