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 <olvaffe@gmail.com>
Reviewed-by: Yiwei Zhang <zzyiwei@chromium.org> (v1)
Reviewed-by: Ryan Neph <ryanneph@google.com> (v1)

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 */
+   /* free all render_workers without killing nor reaping */
-   list_splicetail(&client->context_records, &client->reap_records);
+   render_worker_jail_detach_workers(srv->worker_jail);
-   list_for_each_entry_safe (struct render_context_record, rec, &client->reap_records,
-                             head) {
-      render_worker_destroy(srv->worker_jail, rec->worker);
-      free(rec);
-   }
 
+   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,
+render_client_remove_record(struct render_client *client,
-                              struct render_context_record *rec)
+                            struct render_context_record *rec)
 {
-   render_worker_kill(rec->worker);
+   struct render_server *srv = client->server;
-
-   list_del(&rec->head);
-   list_addtail(&rec->head, &client->reap_records);
-}
 
-static void
+   render_worker_destroy(srv->worker_jail, rec->worker);
-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);
 
-   list_splicetail(&client->context_records, &client->reap_records);
+   list_del(&rec->head);
-   list_inithead(&client->context_records);
+   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->context_records,
-
+                             head)
-   list_for_each_entry_safe (struct render_context_record, rec, &client->reap_records,
+      render_client_remove_record(client, rec);
-                             head) {
-      if (!render_worker_reap(rec->worker, wait))
-         continue;
-
-      render_worker_destroy(srv->worker_jail, rec->worker);
-      list_del(&rec->head);
-      free(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_clear_records(client);
-   render_client_reap_all_records(client, true /* wait */);
    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) &&
+      assert(list_is_empty(&client->context_records));
-             list_is_empty(&client->reap_records));
    } else {
-      render_client_kill_all_records(client);
+      render_client_clear_records(client);
-      render_client_reap_all_records(client, true /* wait */);
 
       /* 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;
 }

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 *

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;

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
+   worker->destroyed = true;
-render_worker_reap(struct render_worker *worker, bool wait)
-{
-   assert(render_worker_is_record(worker));
 
    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;
+   return !thrd_equal(worker->thread, thrd_current());
-   ok = thrd_join(worker->thread, NULL) == thrd_success;
 #else
-   const int options = WEXITED | (wait ? 0 : WNOHANG);
+   return worker->pid > 0;
-   siginfo_t siginfo = { 0 };
-   const int ret = waitid(P_PID, worker->pid, &siginfo, options);
-   ok = !ret && siginfo.si_pid == worker->pid;
 #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);
 }

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);
+render_worker_is_record(const struct render_worker *worker);
-
-void
-render_worker_destroy(struct render_worker_jail *jail, struct render_worker *worker);
 
 #endif /* RENDER_WORKER_H */