From 3480bc667878dc254d8ff31f098d11a0d85a894f Mon Sep 17 00:00:00 2001 From: Kenny Levinsen Date: Tue, 11 Feb 2025 12:41:59 +0100 Subject: [PATCH] Rework fork/exec strategy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cmd_exec_process is used whenever sway is meant to execute a child process on behalf of the user, and had a lot of complexity. In order to avoid having to wait on the user's process, a double-fork was used, which in turn required us to wait on the outer process. In order to track the child PID for launcher purposes, a pipe was used to transmit the PID back to sway. This resulted in sway blocking for 5-6 ms per exec on my system, which is quite significant. The error handling was also quite lacking - the read loop did not handle errors at all for example. Instead, teach sway to handle SIGCHLD and do away with the double-fork. This in turn allows us to get rid of the pipe as we can record the child's PID directly. This reduces the time we block to just 1.5 ms on my system. We'd be able to get down to just 150 µs if we could use posix_spawn(3), but posix_spawn(3) cannot reset NOFILE. clone(2) or vfork(2) would be alternatives, but that presents portability issues. This change is replicated for swaybar, swaybg and swaynag handling, which had similar albeit less complicated implementations. --- sway/commands/exec_always.c | 75 ++++++++++--------------------------- sway/config/bar.c | 39 +++++++------------ sway/config/output.c | 40 +++++++------------- sway/main.c | 8 ++++ sway/swaynag.c | 51 ++++++++++--------------- 5 files changed, 73 insertions(+), 140 deletions(-) diff --git a/sway/commands/exec_always.c b/sway/commands/exec_always.c index 8bc1048cd..14566fc43 100644 --- a/sway/commands/exec_always.c +++ b/sway/commands/exec_always.c @@ -25,16 +25,6 @@ struct cmd_results *cmd_exec_validate(int argc, char **argv) { return error; } -static void export_xdga_token(struct launcher_ctx *ctx) { - const char *token = launcher_ctx_get_token_name(ctx); - setenv("XDG_ACTIVATION_TOKEN", token, 1); -} - -static void export_startup_id(struct launcher_ctx *ctx) { - const char *token = launcher_ctx_get_token_name(ctx); - setenv("DESKTOP_STARTUP_ID", token, 1); -} - struct cmd_results *cmd_exec_process(int argc, char **argv) { struct cmd_results *error = NULL; char *cmd = NULL; @@ -56,67 +46,42 @@ struct cmd_results *cmd_exec_process(int argc, char **argv) { sway_log(SWAY_DEBUG, "Executing %s", cmd); - int fd[2]; - if (pipe(fd) != 0) { - sway_log(SWAY_ERROR, "Unable to create pipe for fork"); - } - - pid_t pid, child; struct launcher_ctx *ctx = launcher_ctx_create_internal(); + // Fork process - if ((pid = fork()) == 0) { - // Fork child process again + pid_t child = fork(); + if (child == 0) { restore_nofile_limit(); setsid(); sigset_t set; sigemptyset(&set); sigprocmask(SIG_SETMASK, &set, NULL); signal(SIGPIPE, SIG_DFL); - close(fd[0]); - if ((child = fork()) == 0) { - close(fd[1]); - if (ctx) { - export_xdga_token(ctx); + + if (ctx) { + const char *token = launcher_ctx_get_token_name(ctx); + setenv("XDG_ACTIVATION_TOKEN", token, 1); + if (!no_startup_id) { + setenv("DESKTOP_STARTUP_ID", token, 1); } - if (ctx && !no_startup_id) { - export_startup_id(ctx); - } - execlp("sh", "sh", "-c", cmd, (void *)NULL); - sway_log_errno(SWAY_ERROR, "execlp failed"); - _exit(1); } - ssize_t s = 0; - while ((size_t)s < sizeof(pid_t)) { - s += write(fd[1], ((uint8_t *)&child) + s, sizeof(pid_t) - s); - } - close(fd[1]); + + execlp("sh", "sh", "-c", cmd, (void*)NULL); + sway_log_errno(SWAY_ERROR, "execve failed"); _exit(0); // Close child process - } else if (pid < 0) { + } else if (child < 0) { + launcher_ctx_destroy(ctx); free(cmd); - close(fd[0]); - close(fd[1]); return cmd_results_new(CMD_FAILURE, "fork() failed"); } - free(cmd); - close(fd[1]); // close write - ssize_t s = 0; - while ((size_t)s < sizeof(pid_t)) { - s += read(fd[0], ((uint8_t *)&child) + s, sizeof(pid_t) - s); - } - close(fd[0]); - // cleanup child process - waitpid(pid, NULL, 0); - if (child > 0) { - sway_log(SWAY_DEBUG, "Child process created with pid %d", child); - if (ctx != NULL) { - sway_log(SWAY_DEBUG, "Recording workspace for process %d", child); - ctx->pid = child; - } - } else { - launcher_ctx_destroy(ctx); - return cmd_results_new(CMD_FAILURE, "Second fork() failed"); + + sway_log(SWAY_DEBUG, "Child process created with pid %d", child); + if (ctx != NULL) { + sway_log(SWAY_DEBUG, "Recording workspace for process %d", child); + ctx->pid = child; } + free(cmd); return cmd_results_new(CMD_SUCCESS, NULL); } diff --git a/sway/config/bar.c b/sway/config/bar.c index ecefb61af..f7eddfbb1 100644 --- a/sway/config/bar.c +++ b/sway/config/bar.c @@ -220,29 +220,21 @@ static void invoke_swaybar(struct bar_config *bar) { signal(SIGPIPE, SIG_DFL); restore_nofile_limit(); - - pid = fork(); - if (pid < 0) { - sway_log_errno(SWAY_ERROR, "fork failed"); - _exit(EXIT_FAILURE); - } else if (pid == 0) { - if (!sway_set_cloexec(sockets[1], false)) { - _exit(EXIT_FAILURE); - } - - char wayland_socket_str[16]; - snprintf(wayland_socket_str, sizeof(wayland_socket_str), - "%d", sockets[1]); - setenv("WAYLAND_SOCKET", wayland_socket_str, true); - - // run custom swaybar - char *const cmd[] = { - bar->swaybar_command ? bar->swaybar_command : "swaybar", - "-b", bar->id, NULL}; - execvp(cmd[0], cmd); + if (!sway_set_cloexec(sockets[1], false)) { _exit(EXIT_FAILURE); } - _exit(EXIT_SUCCESS); + + char wayland_socket_str[16]; + snprintf(wayland_socket_str, sizeof(wayland_socket_str), + "%d", sockets[1]); + setenv("WAYLAND_SOCKET", wayland_socket_str, true); + + // run custom swaybar + char *const cmd[] = { + bar->swaybar_command ? bar->swaybar_command : "swaybar", + "-b", bar->id, NULL}; + execvp(cmd[0], cmd); + _exit(EXIT_FAILURE); } if (close(sockets[1]) != 0) { @@ -250,11 +242,6 @@ static void invoke_swaybar(struct bar_config *bar) { return; } - if (waitpid(pid, NULL, 0) < 0) { - sway_log_errno(SWAY_ERROR, "waitpid failed"); - return; - } - sway_log(SWAY_DEBUG, "Spawned swaybar %s", bar->id); } diff --git a/sway/config/output.c b/sway/config/output.c index 9fb3a12ac..4d4a47b46 100644 --- a/sway/config/output.c +++ b/sway/config/output.c @@ -1061,41 +1061,27 @@ static bool _spawn_swaybg(char **command) { return false; } else if (pid == 0) { restore_nofile_limit(); - - pid = fork(); - if (pid < 0) { - sway_log_errno(SWAY_ERROR, "fork failed"); - _exit(EXIT_FAILURE); - } else if (pid == 0) { - if (!sway_set_cloexec(sockets[1], false)) { - _exit(EXIT_FAILURE); - } - - char wayland_socket_str[16]; - snprintf(wayland_socket_str, sizeof(wayland_socket_str), - "%d", sockets[1]); - setenv("WAYLAND_SOCKET", wayland_socket_str, true); - - execvp(command[0], command); - sway_log_errno(SWAY_ERROR, "failed to execute '%s' " - "(background configuration probably not applied)", - command[0]); + if (!sway_set_cloexec(sockets[1], false)) { _exit(EXIT_FAILURE); } - _exit(EXIT_SUCCESS); + + char wayland_socket_str[16]; + snprintf(wayland_socket_str, sizeof(wayland_socket_str), + "%d", sockets[1]); + setenv("WAYLAND_SOCKET", wayland_socket_str, true); + + execvp(command[0], command); + sway_log_errno(SWAY_ERROR, "failed to execute '%s' " + "(background configuration probably not applied)", + command[0]); + _exit(EXIT_FAILURE); } if (close(sockets[1]) != 0) { sway_log_errno(SWAY_ERROR, "close failed"); return false; } - int fork_status = 0; - if (waitpid(pid, &fork_status, 0) < 0) { - sway_log_errno(SWAY_ERROR, "waitpid failed"); - return false; - } - - return WIFEXITED(fork_status) && WEXITSTATUS(fork_status) == EXIT_SUCCESS; + return true; } bool spawn_swaybg(void) { diff --git a/sway/main.c b/sway/main.c index accffc71f..0cc7623d8 100644 --- a/sway/main.c +++ b/sway/main.c @@ -48,6 +48,13 @@ void sig_handler(int signal) { sway_terminate(EXIT_SUCCESS); } +void sigchld_handler(int signal) { + pid_t pid; + do { + pid = waitpid(-1, NULL, WNOHANG); + } while (pid > 0); +} + void run_as_ipc_client(char *command, char *socket_path) { int socketfd = ipc_open_socket(socket_path); uint32_t len = strlen(command); @@ -325,6 +332,7 @@ int main(int argc, char **argv) { // handle SIGTERM signals signal(SIGTERM, sig_handler); signal(SIGINT, sig_handler); + signal(SIGCHLD, sigchld_handler); // prevent ipc from crashing sway signal(SIGPIPE, SIG_IGN); diff --git a/sway/swaynag.c b/sway/swaynag.c index bc5e23ea4..f0a31218a 100644 --- a/sway/swaynag.c +++ b/sway/swaynag.c @@ -64,35 +64,27 @@ bool swaynag_spawn(const char *swaynag_command, goto failed; } else if (pid == 0) { restore_nofile_limit(); - - pid = fork(); - if (pid < 0) { - sway_log_errno(SWAY_ERROR, "fork failed"); - _exit(EXIT_FAILURE); - } else if (pid == 0) { - if (!sway_set_cloexec(sockets[1], false)) { - _exit(EXIT_FAILURE); - } - - if (swaynag->detailed) { - close(swaynag->fd[1]); - dup2(swaynag->fd[0], STDIN_FILENO); - close(swaynag->fd[0]); - } - - char wayland_socket_str[16]; - snprintf(wayland_socket_str, sizeof(wayland_socket_str), - "%d", sockets[1]); - setenv("WAYLAND_SOCKET", wayland_socket_str, true); - - size_t length = strlen(swaynag_command) + strlen(swaynag->args) + 2; - char *cmd = malloc(length); - snprintf(cmd, length, "%s %s", swaynag_command, swaynag->args); - execlp("sh", "sh", "-c", cmd, NULL); - sway_log_errno(SWAY_ERROR, "execlp failed"); + if (!sway_set_cloexec(sockets[1], false)) { _exit(EXIT_FAILURE); } - _exit(EXIT_SUCCESS); + + if (swaynag->detailed) { + close(swaynag->fd[1]); + dup2(swaynag->fd[0], STDIN_FILENO); + close(swaynag->fd[0]); + } + + char wayland_socket_str[16]; + snprintf(wayland_socket_str, sizeof(wayland_socket_str), + "%d", sockets[1]); + setenv("WAYLAND_SOCKET", wayland_socket_str, true); + + size_t length = strlen(swaynag_command) + strlen(swaynag->args) + 2; + char *cmd = malloc(length); + snprintf(cmd, length, "%s %s", swaynag_command, swaynag->args); + execlp("sh", "sh", "-c", cmd, NULL); + sway_log_errno(SWAY_ERROR, "execlp failed"); + _exit(EXIT_FAILURE); } if (swaynag->detailed) { @@ -107,11 +99,6 @@ bool swaynag_spawn(const char *swaynag_command, return false; } - if (waitpid(pid, NULL, 0) < 0) { - sway_log_errno(SWAY_ERROR, "waitpid failed"); - return false; - } - return true; failed: