From 4c727ae935c22dc80ec7c02a54f10498a48dba39 Mon Sep 17 00:00:00 2001 From: Chia-I Wu Date: Tue, 21 Dec 2021 15:16:41 -0800 Subject: [PATCH] server: rework worker apis and internals Now that render_worker_jail tracks workers, the apis are simplified such that - render_worker_create forks a subprocess - render_worker_destroy kills a subprocess - render_worker_jail_reap_workers reaps all exited subprocesses Also add render_worker_jail_detach_workers that can be called by a forked subprocess to "detach" (free without killing/reaping) workers. v2: replace some checks for worker->{destroyed,reaped} by asserts (Ryan) Signed-off-by: Chia-I Wu Reviewed-by: Yiwei Zhang (v1) Reviewed-by: Ryan Neph (v1) --- server/render_client.c | 67 +++++++----------------- server/render_client.h | 1 - server/render_server.c | 4 +- server/render_worker.c | 116 ++++++++++++++++++++++++++++------------- server/render_worker.h | 16 +++--- 5 files changed, 110 insertions(+), 94 deletions(-) diff --git a/server/render_client.c b/server/render_client.c index 3b12d79..cc85e3d 100644 --- a/server/render_client.c +++ b/server/render_client.c @@ -20,9 +20,6 @@ * RENDER_CLIENT_OP_DESTROY_CONTEXT to us to remove the record. Because we * are responsible for cleaning up the worker, we don't care if the worker has * terminated or not. We always kill, reap, and remove the record. - * - * TODO We reap with WNOHANG in render_client_dispatch currently. We should - * use SIGCHLD instead. */ struct render_context_record { uint32_t ctx_id; @@ -47,52 +44,33 @@ render_client_detach_all_records(struct render_client *client) { struct render_server *srv = client->server; - /* destroy all records without killing nor reaping */ - list_splicetail(&client->context_records, &client->reap_records); - list_for_each_entry_safe (struct render_context_record, rec, &client->reap_records, - head) { - render_worker_destroy(srv->worker_jail, rec->worker); - free(rec); - } + /* free all render_workers without killing nor reaping */ + render_worker_jail_detach_workers(srv->worker_jail); + list_for_each_entry_safe (struct render_context_record, rec, &client->context_records, + head) + free(rec); list_inithead(&client->context_records); - list_inithead(&client->reap_records); } static void -render_client_kill_one_record(struct render_client *client, - struct render_context_record *rec) +render_client_remove_record(struct render_client *client, + struct render_context_record *rec) { - render_worker_kill(rec->worker); - - list_del(&rec->head); - list_addtail(&rec->head, &client->reap_records); -} + struct render_server *srv = client->server; -static void -render_client_kill_all_records(struct render_client *client) -{ - list_for_each_entry (struct render_context_record, rec, &client->context_records, head) - render_worker_kill(rec->worker); + render_worker_destroy(srv->worker_jail, rec->worker); - list_splicetail(&client->context_records, &client->reap_records); - list_inithead(&client->context_records); + list_del(&rec->head); + free(rec); } static void -render_client_reap_all_records(struct render_client *client, bool wait) +render_client_clear_records(struct render_client *client) { - struct render_server *srv = client->server; - - list_for_each_entry_safe (struct render_context_record, rec, &client->reap_records, - head) { - if (!render_worker_reap(rec->worker, wait)) - continue; - - render_worker_destroy(srv->worker_jail, rec->worker); - list_del(&rec->head); - free(rec); - } + list_for_each_entry_safe (struct render_context_record, rec, &client->context_records, + head) + render_client_remove_record(client, rec); } static void @@ -195,7 +173,7 @@ render_client_dispatch_destroy_context(struct render_client *client, const uint32_t ctx_id = req->destroy_context.ctx_id; struct render_context_record *rec = render_client_find_record(client, ctx_id); if (rec) - render_client_kill_one_record(client, rec); + render_client_remove_record(client, rec); return true; } @@ -233,8 +211,7 @@ static bool render_client_dispatch_reset(struct render_client *client, UNUSED const union render_client_op_request *req) { - render_client_kill_all_records(client); - render_client_reap_all_records(client, true /* wait */); + render_client_clear_records(client); return true; } @@ -308,9 +285,6 @@ render_client_dispatch(struct render_client *client) if (!entry->dispatch(client, &req)) render_log("failed to dispatch client op %d", req.header.op); - /* TODO this should be triggered by SIGCHLD */ - render_client_reap_all_records(client, false /* wait */); - return true; } @@ -320,11 +294,9 @@ render_client_destroy(struct render_client *client) struct render_server *srv = client->server; if (srv->state == RENDER_SERVER_STATE_SUBPROCESS) { - assert(list_is_empty(&client->context_records) && - list_is_empty(&client->reap_records)); + assert(list_is_empty(&client->context_records)); } else { - render_client_kill_all_records(client); - render_client_reap_all_records(client, true /* wait */); + render_client_clear_records(client); /* see render_client_dispatch_init */ #ifndef ENABLE_TRACING @@ -348,7 +320,6 @@ render_client_create(struct render_server *srv, int client_fd) render_socket_init(&client->socket, client_fd); list_inithead(&client->context_records); - list_inithead(&client->reap_records); return client; } diff --git a/server/render_client.h b/server/render_client.h index 9d9fb4e..214ba20 100644 --- a/server/render_client.h +++ b/server/render_client.h @@ -15,7 +15,6 @@ struct render_client { uint32_t init_flags; struct list_head context_records; - struct list_head reap_records; }; struct render_client * diff --git a/server/render_server.c b/server/render_server.c index f4e1d7c..bc55388 100644 --- a/server/render_server.c +++ b/server/render_server.c @@ -17,10 +17,12 @@ static bool render_server_run(struct render_server *srv) { while (srv->state == RENDER_SERVER_STATE_RUN) { - /* TODO handle SIGCHLD */ struct render_client *client = srv->client; if (!render_client_dispatch(client)) return false; + + /* TODO this should be triggered by SIGCHLD */ + render_worker_jail_reap_workers(srv->worker_jail); } return true; diff --git a/server/render_worker.c b/server/render_worker.c index 165cf98..b7343dd 100644 --- a/server/render_worker.c +++ b/server/render_worker.c @@ -45,6 +45,7 @@ struct render_worker { #else pid_t pid; #endif + bool destroyed; bool reaped; struct list_head head; @@ -173,6 +174,33 @@ render_worker_jail_remove_worker(struct render_worker_jail *jail, free(worker); } +static struct render_worker * +render_worker_jail_reap_any_worker(struct render_worker_jail *jail, bool block) +{ +#ifdef ENABLE_RENDER_SERVER_WORKER_THREAD + (void)jail; + (void)block; + return NULL; +#else + const int options = WEXITED | (block ? 0 : WNOHANG); + siginfo_t siginfo = { 0 }; + const int ret = waitid(P_ALL, 0, &siginfo, options); + const pid_t pid = ret ? 0 : siginfo.si_pid; + if (!pid) + return NULL; + + list_for_each_entry (struct render_worker, worker, &jail->workers, head) { + if (worker->pid == pid) { + worker->reaped = true; + return worker; + } + } + + render_log("unknown child process %d", pid); + return NULL; +#endif +} + struct render_worker_jail * render_worker_jail_create(int max_worker_count, enum render_worker_jail_seccomp_filter seccomp_filter, @@ -203,10 +231,23 @@ fail: return NULL; } +static void +render_worker_jail_wait_workers(struct render_worker_jail *jail) +{ + while (jail->worker_count) { + struct render_worker *worker = + render_worker_jail_reap_any_worker(jail, true /* block */); + if (worker) { + assert(worker->destroyed && worker->reaped); + render_worker_jail_remove_worker(jail, worker); + } + } +} + void render_worker_jail_destroy(struct render_worker_jail *jail) { - assert(!jail->worker_count); + render_worker_jail_wait_workers(jail); #if defined(ENABLE_RENDER_SERVER_WORKER_MINIJAIL) minijail_destroy(jail->minijail); @@ -215,6 +256,29 @@ render_worker_jail_destroy(struct render_worker_jail *jail) free(jail); } +void +render_worker_jail_reap_workers(struct render_worker_jail *jail) +{ + do { + struct render_worker *worker = + render_worker_jail_reap_any_worker(jail, false /* block */); + if (!worker) + break; + + assert(worker->reaped); + if (worker->destroyed) + render_worker_jail_remove_worker(jail, worker); + } while (true); +} + +void +render_worker_jail_detach_workers(struct render_worker_jail *jail) +{ + /* free workers without killing nor reaping */ + list_for_each_entry_safe (struct render_worker, worker, &jail->workers, head) + render_worker_jail_remove_worker(jail, worker); +} + struct render_worker * render_worker_create(struct render_worker_jail *jail, int (*thread_func)(void *thread_data), @@ -254,54 +318,34 @@ render_worker_create(struct render_worker_jail *jail, return worker; } -bool -render_worker_is_record(const struct render_worker *worker) -{ - /* return false if called from the worker itself */ -#ifdef ENABLE_RENDER_SERVER_WORKER_THREAD - return !thrd_equal(worker->thread, thrd_current()); -#else - return worker->pid > 0; -#endif -} - void -render_worker_kill(struct render_worker *worker) +render_worker_destroy(struct render_worker_jail *jail, struct render_worker *worker) { assert(render_worker_is_record(worker)); #ifdef ENABLE_RENDER_SERVER_WORKER_THREAD /* we trust the thread to clean up and exit in finite time */ + thrd_join(worker->thread, NULL); + worker->reaped = true; #else - kill(worker->pid, SIGKILL); + /* kill to make sure the worker exits in finite time */ + if (!worker->reaped) + kill(worker->pid, SIGKILL); #endif -} -bool -render_worker_reap(struct render_worker *worker, bool wait) -{ - assert(render_worker_is_record(worker)); + worker->destroyed = true; if (worker->reaped) - return true; + render_worker_jail_remove_worker(jail, worker); +} - bool ok; +bool +render_worker_is_record(const struct render_worker *worker) +{ + /* return false if called from the worker itself */ #ifdef ENABLE_RENDER_SERVER_WORKER_THREAD - (void)wait; - ok = thrd_join(worker->thread, NULL) == thrd_success; + return !thrd_equal(worker->thread, thrd_current()); #else - const int options = WEXITED | (wait ? 0 : WNOHANG); - siginfo_t siginfo = { 0 }; - const int ret = waitid(P_PID, worker->pid, &siginfo, options); - ok = !ret && siginfo.si_pid == worker->pid; + return worker->pid > 0; #endif - - worker->reaped = ok; - return ok; -} - -void -render_worker_destroy(struct render_worker_jail *jail, struct render_worker *worker) -{ - render_worker_jail_remove_worker(jail, worker); } diff --git a/server/render_worker.h b/server/render_worker.h index 4d2d0fb..15ae9c0 100644 --- a/server/render_worker.h +++ b/server/render_worker.h @@ -26,22 +26,22 @@ render_worker_jail_create(int max_worker_count, void render_worker_jail_destroy(struct render_worker_jail *jail); +void +render_worker_jail_reap_workers(struct render_worker_jail *jail); + +void +render_worker_jail_detach_workers(struct render_worker_jail *jail); + struct render_worker * render_worker_create(struct render_worker_jail *jail, int (*thread_func)(void *thread_data), void *thread_data, size_t thread_data_size); -bool -render_worker_is_record(const struct render_worker *worker); - void -render_worker_kill(struct render_worker *worker); +render_worker_destroy(struct render_worker_jail *jail, struct render_worker *worker); bool -render_worker_reap(struct render_worker *worker, bool wait); - -void -render_worker_destroy(struct render_worker_jail *jail, struct render_worker *worker); +render_worker_is_record(const struct render_worker *worker); #endif /* RENDER_WORKER_H */