* [PATCH 1/2] perf daemon: Force waipid for all session on SIGCHLD delivery
@ 2021-03-20 22:10 Jiri Olsa
2021-03-20 22:10 ` [PATCH 2/2] perf daemon: Return from kill functions Jiri Olsa
2021-03-24 13:24 ` [PATCH 1/2] perf daemon: Force waipid for all session on SIGCHLD delivery Arnaldo Carvalho de Melo
0 siblings, 2 replies; 3+ messages in thread
From: Jiri Olsa @ 2021-03-20 22:10 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Ian Rogers, lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Namhyung Kim, Alexander Shishkin, Michael Petlan
If we don't process SIGCHLD before another comes, we will
see just one SIGCHLD as a result. In this case current code
will miss exit notification for a session and wait forever.
Adding extra waitpid check for all sessions when SIGCHLD
is received, to make sure we don't miss any session exit.
Also fix close condition for signal_fd.
Reported-by: Ian Rogers <irogers@google.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/builtin-daemon.c | 50 +++++++++++++++++++++----------------
1 file changed, 28 insertions(+), 22 deletions(-)
diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
index ace8772a4f03..4697493842f5 100644
--- a/tools/perf/builtin-daemon.c
+++ b/tools/perf/builtin-daemon.c
@@ -402,35 +402,42 @@ static pid_t handle_signalfd(struct daemon *daemon)
int status;
pid_t pid;
+ /*
+ * Take signal fd data as pure signal notification and check all
+ * the sessions state. The reason is that multiple signals can get
+ * coalesced in kernel and we can receive only single signal even
+ * if multiple SIGCHLD were generated.
+ */
err = read(daemon->signal_fd, &si, sizeof(struct signalfd_siginfo));
- if (err != sizeof(struct signalfd_siginfo))
+ if (err != sizeof(struct signalfd_siginfo)) {
+ pr_err("failed to read signal fd\n");
return -1;
+ }
list_for_each_entry(session, &daemon->sessions, list) {
+ if (session->pid == -1)
+ continue;
- if (session->pid != (int) si.ssi_pid)
+ pid = waitpid(session->pid, &status, WNOHANG);
+ if (pid <= 0)
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);
- }
+ 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 = KILL;
session->pid = -1;
- return pid;
}
return 0;
@@ -443,7 +450,6 @@ static int daemon_session__wait(struct daemon_session *session, struct daemon *d
.fd = daemon->signal_fd,
.events = POLLIN,
};
- pid_t wpid = 0, pid = session->pid;
time_t start;
start = time(NULL);
@@ -452,7 +458,7 @@ static int daemon_session__wait(struct daemon_session *session, struct daemon *d
int err = poll(&pollfd, 1, 1000);
if (err > 0) {
- wpid = handle_signalfd(daemon);
+ handle_signalfd(daemon);
} else if (err < 0) {
perror("failed: poll\n");
return -1;
@@ -460,7 +466,7 @@ static int daemon_session__wait(struct daemon_session *session, struct daemon *d
if (start + secs < time(NULL))
return -1;
- } while (wpid != pid);
+ } while (session->pid != -1);
return 0;
}
@@ -1344,7 +1350,7 @@ 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)
+ if (signal_fd != -1)
close(signal_fd);
pr_info("daemon exited\n");
--
2.30.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH 2/2] perf daemon: Return from kill functions
2021-03-20 22:10 [PATCH 1/2] perf daemon: Force waipid for all session on SIGCHLD delivery Jiri Olsa
@ 2021-03-20 22:10 ` Jiri Olsa
2021-03-24 13:24 ` [PATCH 1/2] perf daemon: Force waipid for all session on SIGCHLD delivery Arnaldo Carvalho de Melo
1 sibling, 0 replies; 3+ messages in thread
From: Jiri Olsa @ 2021-03-20 22:10 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland, Namhyung Kim,
Alexander Shishkin, Michael Petlan, Ian Rogers
We should return correctly and warn in both daemon_session__kill
and daemon__kill functions after we tried everything to kill
sessions. Current code will keep on looping and wait.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/builtin-daemon.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
index 4697493842f5..7c4a9d424a64 100644
--- a/tools/perf/builtin-daemon.c
+++ b/tools/perf/builtin-daemon.c
@@ -908,7 +908,9 @@ static void daemon_session__kill(struct daemon_session *session,
daemon_session__signal(session, SIGKILL);
break;
default:
- break;
+ pr_err("failed to wait for session %s\n",
+ session->name);
+ return;
}
how++;
@@ -961,7 +963,8 @@ static void daemon__kill(struct daemon *daemon)
daemon__signal(daemon, SIGKILL);
break;
default:
- break;
+ pr_err("failed to wait for sessions\n");
+ return;
}
how++;
--
2.30.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 1/2] perf daemon: Force waipid for all session on SIGCHLD delivery
2021-03-20 22:10 [PATCH 1/2] perf daemon: Force waipid for all session on SIGCHLD delivery Jiri Olsa
2021-03-20 22:10 ` [PATCH 2/2] perf daemon: Return from kill functions Jiri Olsa
@ 2021-03-24 13:24 ` Arnaldo Carvalho de Melo
1 sibling, 0 replies; 3+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-03-24 13:24 UTC (permalink / raw)
To: Jiri Olsa
Cc: Ian Rogers, lkml, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Namhyung Kim, Alexander Shishkin, Michael Petlan
Em Sat, Mar 20, 2021 at 11:10:12PM +0100, Jiri Olsa escreveu:
> If we don't process SIGCHLD before another comes, we will
> see just one SIGCHLD as a result. In this case current code
> will miss exit notification for a session and wait forever.
>
> Adding extra waitpid check for all sessions when SIGCHLD
> is received, to make sure we don't miss any session exit.
>
> Also fix close condition for signal_fd.
Thanks, applied.
- Arnaldo
> Reported-by: Ian Rogers <irogers@google.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> tools/perf/builtin-daemon.c | 50 +++++++++++++++++++++----------------
> 1 file changed, 28 insertions(+), 22 deletions(-)
>
> diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
> index ace8772a4f03..4697493842f5 100644
> --- a/tools/perf/builtin-daemon.c
> +++ b/tools/perf/builtin-daemon.c
> @@ -402,35 +402,42 @@ static pid_t handle_signalfd(struct daemon *daemon)
> int status;
> pid_t pid;
>
> + /*
> + * Take signal fd data as pure signal notification and check all
> + * the sessions state. The reason is that multiple signals can get
> + * coalesced in kernel and we can receive only single signal even
> + * if multiple SIGCHLD were generated.
> + */
> err = read(daemon->signal_fd, &si, sizeof(struct signalfd_siginfo));
> - if (err != sizeof(struct signalfd_siginfo))
> + if (err != sizeof(struct signalfd_siginfo)) {
> + pr_err("failed to read signal fd\n");
> return -1;
> + }
>
> list_for_each_entry(session, &daemon->sessions, list) {
> + if (session->pid == -1)
> + continue;
>
> - if (session->pid != (int) si.ssi_pid)
> + pid = waitpid(session->pid, &status, WNOHANG);
> + if (pid <= 0)
> 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);
> - }
> + 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 = KILL;
> session->pid = -1;
> - return pid;
> }
>
> return 0;
> @@ -443,7 +450,6 @@ static int daemon_session__wait(struct daemon_session *session, struct daemon *d
> .fd = daemon->signal_fd,
> .events = POLLIN,
> };
> - pid_t wpid = 0, pid = session->pid;
> time_t start;
>
> start = time(NULL);
> @@ -452,7 +458,7 @@ static int daemon_session__wait(struct daemon_session *session, struct daemon *d
> int err = poll(&pollfd, 1, 1000);
>
> if (err > 0) {
> - wpid = handle_signalfd(daemon);
> + handle_signalfd(daemon);
> } else if (err < 0) {
> perror("failed: poll\n");
> return -1;
> @@ -460,7 +466,7 @@ static int daemon_session__wait(struct daemon_session *session, struct daemon *d
>
> if (start + secs < time(NULL))
> return -1;
> - } while (wpid != pid);
> + } while (session->pid != -1);
>
> return 0;
> }
> @@ -1344,7 +1350,7 @@ 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)
> + if (signal_fd != -1)
> close(signal_fd);
>
> pr_info("daemon exited\n");
> --
> 2.30.2
>
--
- Arnaldo
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-03-24 13:25 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-20 22:10 [PATCH 1/2] perf daemon: Force waipid for all session on SIGCHLD delivery Jiri Olsa
2021-03-20 22:10 ` [PATCH 2/2] perf daemon: Return from kill functions Jiri Olsa
2021-03-24 13:24 ` [PATCH 1/2] perf daemon: Force waipid for all session on SIGCHLD delivery Arnaldo Carvalho de Melo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).