Break the spawnd kill API into kill + cleanup.

Kill just revokes the DCB, thus removing it from the run queue. Cleanup then
revokes the victim dispatcher's root cnode.

The change is meant to fix the problem where killing the main dispatcher of a
domain or having it naturally exit main() would cause the other dispatchers to
pagefault. This used to happen because dispatchers of the same domain share
their vspace, hence when kill & cleanup were performed together the vspace
of all dispatchers would be revoked as soon as the first dispatcher exited or
was killed.

The process manager attempts to fix the issue by first sending a kill message
to each spawnd running dispatchers for a domain, in order to dequeue the DCBs.
Then, only when all DCBs have been dequeued, a cleanup message is sent to each
spawnd to further clean up the victim's cspace.

Signed-off-by: Razvan Damachi <razvan.damachi@gmail.com>

diff --git a/if/spawn.if b/if/spawn.if
index 1d0e0aa..eedfa28 100644 (file)
--- a/if/spawn.if
+++ b/if/spawn.if
@@ -38,6 +38,7 @@ interface spawn "Interface to spawn domains" {
     message span_request(cap procmng_cap, cap domain_cap, cap vroot,
                          cap dispframe);
     message kill_request(cap procmng_cap, cap domain_cap);
+    message cleanup_request(cap procmng_cap, cap domain_cap);
     message spawn_reply(cap domain_cap, errval err);
 
     rpc spawn_proc_mgmt_domain(in cap domain_cap,

diff --git a/usr/proc_mgmt/domain.c b/usr/proc_mgmt/domain.c
index 9a0d89d..1d71ca4 100644 (file)
--- a/usr/proc_mgmt/domain.c
+++ b/usr/proc_mgmt/domain.c
@@ -33,6 +33,7 @@ errval_t domain_new(struct capref domain_cap, struct domain_entry **ret_entry)
     entry->status = DOMAIN_STATUS_NIL;
     memset(entry->spawnds, 0, sizeof(entry->spawnds));
     entry->num_spawnds_running = 0;
+    entry->num_spawnds_resources = 0;
     entry->waiters = NULL;
 
     if (domain_table == NULL) {
@@ -87,6 +88,7 @@ void domain_run_on_core(struct domain_entry *entry, coreid_t core_id)
 
     entry->spawnds[core_id] = spawnd_state_get(core_id);
     ++entry->num_spawnds_running;
+    ++entry->num_spawnds_resources;
 }
 
 errval_t domain_spawn(struct capref domain_cap, coreid_t core_id)

diff --git a/usr/proc_mgmt/domain.h b/usr/proc_mgmt/domain.h
index 017b588..2383fff 100644 (file)
--- a/usr/proc_mgmt/domain.h
+++ b/usr/proc_mgmt/domain.h
@@ -23,8 +23,8 @@ enum domain_status {
     DOMAIN_STATUS_NIL,
     DOMAIN_STATUS_RUNNING,
     DOMAIN_STATUS_STOP_PEND,
-    DOMAIN_STATUS_STOPPED
-    // TODO(razvan): Add the others, as per the state machine.
+    DOMAIN_STATUS_STOPPED,
+    DOMAIN_STATUS_CLEANED
 };
 
 struct domain_waiter {
@@ -38,6 +38,7 @@ struct domain_entry {
 
     struct spawnd_state *spawnds[MAX_COREID];  // Spawnds running this domain.
     coreid_t num_spawnds_running;
+    coreid_t num_spawnds_resources;
 
     struct domain_waiter *waiters;  // Clients waiting after this domain.
 

diff --git a/usr/proc_mgmt/service.c b/usr/proc_mgmt/service.c
index 563658e..66fc779 100644 (file)
--- a/usr/proc_mgmt/service.c
+++ b/usr/proc_mgmt/service.c
@@ -53,8 +53,8 @@ static void add_spawnd_handler(struct proc_mgmt_binding *b, coreid_t core_id,
 static void add_spawnd_handler_non_monitor(struct proc_mgmt_binding *b,
                                            coreid_t core_id, iref_t iref)
 {
-    debug_printf("Ignoring add_spawnd call: %s\n",
-                 err_getstring(PROC_MGMT_ERR_NOT_MONITOR));
+    // debug_printf("Ignoring add_spawnd call: %s\n",
+    //              err_getstring(PROC_MGMT_ERR_NOT_MONITOR));
 }
 
 static void spawn_reply_handler(struct spawn_binding *b,
@@ -118,44 +118,81 @@ static void spawn_reply_handler(struct spawn_binding *b,
                 break;
             }
 
-            assert(entry->num_spawnds_running > 0);
-            assert(entry->status != DOMAIN_STATUS_STOPPED);
+            assert(entry->num_spawnds_resources > 0 ||
+                   entry->num_spawnds_running > 0);
+            assert(entry->status != DOMAIN_STATUS_CLEANED);
 
-            --entry->num_spawnds_running;
-            if (entry->num_spawnds_running == 0) {
-                entry->status = DOMAIN_STATUS_STOPPED;
-                entry->exit_status = EXIT_STATUS_KILLED;  // TODO(razvan): Is this desirable?
-
-                resp_err = cl->b->tx_vtbl.kill_response(cl->b, NOP_CONT,
-                                                        err);
+            if (entry->num_spawnds_running > 0) {
+                --entry->num_spawnds_running;
+                    
+                err = pending_clients_add(domain_cap, cl->b,
+                                          ClientType_Kill, MAX_COREID);
+                if (err_is_fail(err)) {
+                    DEBUG_ERR(err, "pending_clients_add in reply handler");
+                }
 
-                // At this point, the domain exists in state STOPPED for
-                // history reasons. For instance, if some other domain
-                // issues a wait call for this one, the process manager can
-                // return the exit status directly.
-                // At some point, however, we might want to just clean up
-                // the domain entry and recycle the domain cap.
-                struct domain_waiter *waiter = entry->waiters;
-                while (waiter != NULL) {
-                    waiter->b->tx_vtbl.wait_response(waiter->b,
-                                                     NOP_CONT,
-                                                     SYS_ERR_OK,
-                                                     entry->exit_status);
-                    struct domain_waiter *aux = waiter;
-                    waiter = waiter->next;
-                    free(aux);
+                if (entry->num_spawnds_running == 0) {
+                    entry->status = DOMAIN_STATUS_STOPPED;
+                    entry->exit_status = EXIT_STATUS_KILLED;
+
+                    // TODO(razvan): Might it be more sane if we respond back
+                    // to the client after the domain has been cleaned up (i.e.
+                    // the cspace root has been revoked for all dispatchers)?
+                    resp_err = cl->b->tx_vtbl.kill_response(cl->b, NOP_CONT,
+                                                           err);
+                    
+                    // TODO(razvan): Same problem applies to the waiters: would
+                    // it be better if we sent them wait_responses after the
+                    // cspace root has been revoked, too? (here and in the exit
+                    // case).
+                    struct domain_waiter *waiter = entry->waiters;
+                    while (waiter != NULL) {
+                        waiter->b->tx_vtbl.wait_response(waiter->b, NOP_CONT,
+                                                         SYS_ERR_OK,
+                                                         entry->exit_status);
+                        struct domain_waiter *aux = waiter;
+                        waiter = waiter->next;
+                        free(aux);
+                    }
+
+                    for (coreid_t i = 0; i < MAX_COREID; ++i) {
+                        if (entry->spawnds[i] == NULL) {
+                            continue;
+                        }
+
+                        struct spawn_binding *spb = entry->spawnds[i]->b;
+                        spb->rx_vtbl.spawn_reply = spawn_reply_handler;
+                        errval_t req_err = spb->tx_vtbl.cleanup_request(spb,
+                                NOP_CONT, cap_procmng, domain_cap);
+                        if (err_is_fail(req_err)) {
+                            DEBUG_ERR(req_err, "failed to send cleanup_request "
+                                      "to spawnd %u\n", i);
+                        }
+                    }
                 }
-                break;
             } else {
-                // Expecting to receive further kill replies from other spawnds
-                // for the same domain cap, hence re-add the pending client.
-                err = pending_clients_add(domain_cap, cl->b, ClientType_Kill,
-                                          MAX_COREID);
-                if (err_is_fail(err)) {
-                DEBUG_ERR(err, "pending_clients_add in kill reply handler");
+                --entry->num_spawnds_resources;
+
+                if (entry->num_spawnds_resources == 0) {
+                    entry->status = DOMAIN_STATUS_CLEANED;
+
+                    // At this point, the domain exists in state CLEANED for
+                    // history reasons. For instance, if some other domain
+                    // issues a wait call for this one, the process manager can
+                    // return the exit status directly.
+                    // At some point, however, we might want to just clean up
+                    // the domain entry and recycle the domain cap.
+                } else {
+                    // Expecting to receive further cleanup replies from other
+                    // spawnds for the same domain cap, hence re-add the
+                    // pending client.
+                    err = pending_clients_add(domain_cap, cl->b,
+                                              ClientType_Exit, MAX_COREID);
+                    if (err_is_fail(err)) {
+                        DEBUG_ERR(err, "pending_clients_add in reply handler");
+                    }
                 }
             }
-
             break;
 
         case ClientType_Exit:
@@ -174,41 +211,71 @@ static void spawn_reply_handler(struct spawn_binding *b,
                 break;
             }
 
-            assert(entry->num_spawnds_running > 0);
-            assert(entry->status != DOMAIN_STATUS_STOPPED);
-
-            --entry->num_spawnds_running;
-            if (entry->num_spawnds_running == 0) {
-                entry->status = DOMAIN_STATUS_STOPPED;
-
-                // At this point, the domain exists in state STOPPED for
-                // history reasons. For instance, if some other domain
-                // issues a wait call for this one, the process manager can
-                // return the exit status directly.
-                // At some point, however, we might want to just clean up
-                // the domain entry and recycle the domain cap.
-                struct domain_waiter *waiter = entry->waiters;
-                while (waiter != NULL) {
-                    waiter->b->tx_vtbl.wait_response(waiter->b,
-                                                     NOP_CONT,
-                                                     SYS_ERR_OK,
-                                                     entry->exit_status);
-                    struct domain_waiter *aux = waiter;
-                    waiter = waiter->next;
-                    free(aux);
+            assert(entry->num_spawnds_resources > 0 ||
+                   entry->num_spawnds_running > 0);
+            assert(entry->status != DOMAIN_STATUS_CLEANED);
+
+            if (entry->num_spawnds_running > 0) {
+                --entry->num_spawnds_running;
+
+                err = pending_clients_add(domain_cap, cl->b,
+                                          ClientType_Exit, MAX_COREID);
+                if (err_is_fail(err)) {
+                    DEBUG_ERR(err, "pending_clients_add in reply handler");
+                }
+
+                if (entry->num_spawnds_running == 0) {
+                    entry->status = DOMAIN_STATUS_STOPPED;
+
+                    struct domain_waiter *waiter = entry->waiters;
+                    while (waiter != NULL) {
+                        waiter->b->tx_vtbl.wait_response(waiter->b, NOP_CONT,
+                                                         SYS_ERR_OK,
+                                                         entry->exit_status);
+                        struct domain_waiter *aux = waiter;
+                        waiter = waiter->next;
+                        free(aux);
+                    }
+
+                    for (coreid_t i = 0; i < MAX_COREID; ++i) {
+                        if (entry->spawnds[i] == NULL) {
+                            continue;
+                        }
+
+                        struct spawn_binding *spb = entry->spawnds[i]->b;
+                        spb->rx_vtbl.spawn_reply = spawn_reply_handler;
+                        errval_t req_err = spb->tx_vtbl.cleanup_request(spb,
+                                NOP_CONT, cap_procmng, domain_cap);
+                        if (err_is_fail(req_err)) {
+                            DEBUG_ERR(req_err, "failed to send cleanup_request "
+                                      "to spawnd %u\n", i);
+                        }
+                    }
                 }
-                break;
             } else {
-                // Expecting to receive further kill replies from other spawnds
-                // for the same domain cap, hence re-add the pending client.
-                err = pending_clients_add(domain_cap, cl->b, ClientType_Exit,
-                                          MAX_COREID);
-                if (err_is_fail(err)) {
-                DEBUG_ERR(err, "pending_clients_add in exit reply handler");
+                --entry->num_spawnds_resources;
+
+                if (entry->num_spawnds_resources == 0) {
+                    entry->status = DOMAIN_STATUS_CLEANED;
+
+                    // At this point, the domain exists in state CLEANED for
+                    // history reasons. For instance, if some other domain
+                    // issues a wait call for this one, the process manager can
+                    // return the exit status directly.
+                    // At some point, however, we might want to just clean up
+                    // the domain entry and recycle the domain cap.
+                } else {
+                    // Expecting to receive further cleanup replies from other
+                    // spawnds for the same domain cap, hence re-add the
+                    // pending client.
+                    err = pending_clients_add(domain_cap, cl->b,
+                                              ClientType_Exit, MAX_COREID);
+                    if (err_is_fail(err)) {
+                        DEBUG_ERR(err, "pending_clients_add in reply handler");
+                    }
                 }
             }
-
-        break;
+            break;
 
         default:
             // TODO(razvan): Handle the other cases, e.g. wait.
@@ -236,6 +303,9 @@ static void spawn_reply_handler(struct spawn_binding *b,
                 // assertive than logging an error message.
                 DEBUG_ERR(err, "failed to send kill request for dangling "
                           "dispatcher");
+            } else {
+                pending_clients_add(domain_cap, cl->b, ClientType_Kill,
+                                    MAX_COREID);
             }
         }
     }

diff --git a/usr/spawnd/ps.c b/usr/spawnd/ps.c
index 500fd6d..46e66a3 100644 (file)
--- a/usr/spawnd/ps.c
+++ b/usr/spawnd/ps.c
@@ -80,8 +80,8 @@ errval_t ps_hash_domain(struct ps_entry *entry, struct capref domain_cap)
     return SYS_ERR_OK;
 }
 
-errval_t ps_release_domain(struct capref domain_cap,
-                           struct ps_entry **ret_entry)
+errval_t ps_get_domain(struct capref domain_cap, struct ps_entry **ret_entry,
+                       uint64_t *ret_hash_key)
 {
     assert(ret_entry != NULL);
 
@@ -97,6 +97,24 @@ errval_t ps_release_domain(struct capref domain_cap,
     }
     *ret_entry = (struct ps_entry*) table_entry;
 
+    if (ret_hash_key != NULL) {
+        *ret_hash_key = key;
+    }
+
+    return SYS_ERR_OK;
+}
+
+errval_t ps_release_domain(struct capref domain_cap,
+                           struct ps_entry **ret_entry)
+{
+    assert(ret_entry != NULL);
+
+    uint64_t key;
+    errval_t err = ps_get_domain(domain_cap, ret_entry, &key);
+    if (err_is_fail(err)) {
+        return err;
+    }
+
     collections_hash_delete(ps_table, key);
 
     return SYS_ERR_OK;

diff --git a/usr/spawnd/ps.h b/usr/spawnd/ps.h
index 5c22100..cf88fde 100644 (file)
--- a/usr/spawnd/ps.h
+++ b/usr/spawnd/ps.h
@@ -51,6 +51,8 @@ bool ps_exists(domainid_t domain_id);
 struct ps_entry *ps_get(domainid_t domain_id);
 
 errval_t ps_hash_domain(struct ps_entry *entry, struct capref domain_cap);
+errval_t ps_get_domain(struct capref domain_cap, struct ps_entry **ret_entry,
+                       uint64_t *ret_hash_key);
 errval_t ps_release_domain(struct capref domain_cap,
                            struct ps_entry **ret_entry);
 

diff --git a/usr/spawnd/service.c b/usr/spawnd/service.c
index 36232fb..a1c9b7e 100644 (file)
--- a/usr/spawnd/service.c
+++ b/usr/spawnd/service.c
@@ -602,14 +602,45 @@ static void kill_request_handler(struct spawn_binding *b,
     }
 
     struct ps_entry *pe;
+    err = ps_get_domain(domain_cap, &pe, NULL);
+    if (err_is_fail(err)) {
+        err = err_push(err, SPAWN_ERR_DOMAIN_NOTFOUND);
+        goto reply;
+    }
+
+    cleanup_cap(pe->dcb);
+
+reply:
+    reply_err = b->tx_vtbl.spawn_reply(b, NOP_CONT, domain_cap, err);
+    if (err_is_fail(reply_err)) {
+        DEBUG_ERR(err, "failed to send spawn_reply");
+    }
+}
+
+static void cleanup_request_handler(struct spawn_binding *b,
+                                    struct capref procmng_cap,
+                                    struct capref domain_cap)
+{
+    errval_t err, reply_err;
+    struct capability ret;
+    err = monitor_cap_identify_remote(procmng_cap, &ret);
+    if (err_is_fail(err)) {
+        err = err_push(err, SPAWN_ERR_IDENTIFY_PROC_MNGR_CAP);
+        goto reply;
+    }
+
+    if (ret.type != ObjType_ProcessManager) {
+        err = SPAWN_ERR_NOT_PROC_MNGR;
+        goto reply;
+    }
+
+    struct ps_entry *pe;
     err = ps_release_domain(domain_cap, &pe);
     if (err_is_fail(err)) {
         err = err_push(err, SPAWN_ERR_DOMAIN_NOTFOUND);
         goto reply;
     }
 
-    // Garbage collect victim's capabilities
-    cleanup_cap(pe->dcb);       // Deschedule dispatcher (do this first!)
     cleanup_cap(pe->rootcn_cap);
 
     // Cleanup struct ps_entry. Note that waiters will be handled by the process
@@ -820,6 +851,7 @@ static struct spawn_rx_vtbl rx_vtbl = {
     .spawn_with_caps_request = spawn_with_caps_request_handler,
     .span_request            = span_request_handler,
     .kill_request            = kill_request_handler,
+    .cleanup_request         = cleanup_request_handler,
 
     .use_local_memserv_call = use_local_memserv_handler,
     .kill_call = kill_handler,