dev, virtio: Improvements to diod process handling

* When dispatching multiple gem5 simulations at once, they race
for the socket id, resulting in a panic when calling 'bind'. To
avoid this problem, the socket id is now created before the diod
process is created.  In case of a race, a panic is called in the
gem5 process, whereas before the panic was called in the diod
process where it didn't have any effect.

* In some cases killing the diod process in terminateDiod() using
only SIGTERM failed, so a call using SIGKILL is added.

Change-Id: Ie10741e10af52c8d255210cd4bfe0e5d761485d3
Reviewed-by: Andreas Sandberg <andreas.sandberg@arm.com>
Reviewed-by: Sascha Bischoff <sascha.bischoff@arm.com>
Reviewed-on: https://gem5-review.googlesource.com/2821
Maintainer: Andreas Sandberg <andreas.sandberg@arm.com>

diff --git a/src/dev/virtio/fs9p.cc b/src/dev/virtio/fs9p.cc
index 7dac79ca09a98d58e7ede5b2de25182d124177d5..f2eb8ba52aa2c127268e3f69fd3a974f34e00dbb 100644 (file)
--- a/src/dev/virtio/fs9p.cc
+++ b/src/dev/virtio/fs9p.cc
@@ -45,10 +45,13 @@
 #include <sys/socket.h>
 #include <sys/types.h>
 #include <sys/un.h>
+#include <sys/wait.h>
 #include <unistd.h>
 
+#include <csignal>
 #include <fstream>
 
+#include "base/callback.hh"
 #include "base/output.hh"
 #include "debug/VIO9P.hh"
 #include "debug/VIO9PData.hh"
@@ -313,6 +316,10 @@ VirtIO9PDiod::VirtIO9PDiod(Params *params)
     : VirtIO9PProxy(params),
       fd_to_diod(-1), fd_from_diod(-1), diod_pid(-1)
 {
+    // Register an exit callback so we can kill the diod process
+    Callback* cb = new MakeCallback<VirtIO9PDiod,
+                                    &VirtIO9PDiod::terminateDiod>(this);
+    registerExitCallback(cb);
 }
 
 VirtIO9PDiod::~VirtIO9PDiod()
@@ -346,12 +353,38 @@ VirtIO9PDiod::startDiod()
     fd_to_diod = pipe_rfd[1];
     fd_from_diod = pipe_wfd[0];
 
+    // Create Unix domain socket
+    int socket_id = socket(AF_UNIX, SOCK_STREAM, 0);
+    if (socket_id == -1) {
+        panic("Socket creation failed %i \n", errno);
+    }
+    // Bind the socket to a path which will not be read
+    struct sockaddr_un socket_address;
+    memset(&socket_address, 0, sizeof(struct sockaddr_un));
+    socket_address.sun_family = AF_UNIX;
+
+    const std::string socket_path = simout.resolve(p->socketPath);
+    fatal_if(!OutputDirectory::isAbsolute(socket_path), "Please make the" \
+             " output directory an absolute path, else diod will fail!\n");
+
+    // Prevent overflow in strcpy
+    fatal_if(sizeof(socket_address.sun_path) <= socket_path.length(),
+             "Incorrect length of socket path");
+    strncpy(socket_address.sun_path, socket_path.c_str(),
+            sizeof(socket_address.sun_path));
+    if (bind(socket_id, (struct sockaddr*) &socket_address,
+             sizeof(struct sockaddr_un)) == -1){
+        perror("Socket binding");
+        panic("Socket binding to %i failed - most likely the output dir" \
+              " and hence unused socket already exists \n", socket_id);
+    }
+
     diod_pid = fork();
     if (diod_pid == -1) {
         panic("Fork failed: %i\n", errno);
     } else if (diod_pid == 0) {
+        // Create the socket which will later by used by the diod process
         close(STDIN_FILENO);
-
         if (dup2(pipe_rfd[0], DIOD_RFD) == -1 ||
             dup2(pipe_wfd[1], DIOD_WFD) == -1) {
 
@@ -359,33 +392,7 @@ VirtIO9PDiod::startDiod()
                   errno);
         }
 
-        // Create Unix domain socket
-        int socket_id = socket(AF_UNIX, SOCK_STREAM, 0);
-        if (socket_id == -1) {
-            panic("Socket creation failed %i \n", errno);
-        }
-        // Bind the socket to a path which will not be read
-        struct sockaddr_un socket_address;
-        memset(&socket_address, 0, sizeof(struct sockaddr_un));
-        socket_address.sun_family = AF_UNIX;
-
-        const std::string socket_path = simout.resolve(p->socketPath);
-        fatal_if(!OutputDirectory::isAbsolute(socket_path), "Please make the" \
-                 " output directory an absolute path, else diod will fail!\n");
-
-        // Prevent overflow in strcpy
-        fatal_if(sizeof(socket_address.sun_path) <= socket_path.length(),
-                 "Incorrect length of socket path");
-        strncpy(socket_address.sun_path, socket_path.c_str(),
-                sizeof(socket_address.sun_path));
-
-        if (bind(socket_id, (struct sockaddr*) &socket_address,
-                 sizeof(struct sockaddr_un)) == -1){
-            perror("Socket binding");
-            panic("Socket binding to %i failed - most likely the output dir" \
-                  " and hence unused socket already exists \n", socket_id);
-        }
-
+        // Start diod
         execlp(diod, diod,
                "-f", // start in foreground
                "-r", "3", // setup read FD
@@ -400,6 +407,8 @@ VirtIO9PDiod::startDiod()
     } else {
         close(pipe_rfd[0]);
         close(pipe_wfd[1]);
+        inform("Started diod with PID %u, you might need to manually kill " \
+                " diod if gem5 crashes \n", diod_pid);
     }
 
 #undef DIOD_RFD
@@ -428,6 +437,46 @@ VirtIO9PDiod::DiodDataEvent::process(int revent)
     parent.serverDataReady();
 }
 
+void
+VirtIO9PDiod::terminateDiod()
+{
+    assert(diod_pid != -1);
+
+    DPRINTF(VIO9P, "Trying to kill diod at pid %u \n", diod_pid);
+
+    if (kill(diod_pid, SIGTERM) != 0) {
+        perror("Killing diod process");
+        warn("Failed to kill diod using SIGTERM");
+        return;
+    }
+
+    // Check if kill worked
+    for (unsigned i = 0; i < 5; i++) {
+        int wait_return = waitpid(diod_pid, NULL, WNOHANG);
+        if (wait_return == diod_pid) {
+            // Managed to kill diod
+            return;
+        } else if (wait_return == 0) {
+            // Diod is not killed so sleep and try again
+            usleep(500);
+        } else {
+            // Failed in waitpid
+            perror("Waitpid");
+            warn("Failed in waitpid");
+        }
+    }
+
+    // Try again to kill diod with sigkill
+    inform("Trying to kill diod with SIGKILL as SIGTERM failed \n");
+    if (kill(diod_pid, SIGKILL) != 0) {
+        perror("Killing diod process");
+        warn("Failed to kill diod using SIGKILL");
+    } else {
+        // Managed to kill diod
+        return;
+    }
+
+}
 VirtIO9PDiod *
 VirtIO9PDiodParams::create()
 {

diff --git a/src/dev/virtio/fs9p.hh b/src/dev/virtio/fs9p.hh
index 786f584643522f5410a26d6f4b8897b919da3824..46525603d0fa6b298f6620b58227567588728edb 100644 (file)
--- a/src/dev/virtio/fs9p.hh
+++ b/src/dev/virtio/fs9p.hh
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2014-2015 ARM Limited
+ * Copyright (c) 2014-2017 ARM Limited
  * All rights reserved
  *
  * The license below extends only to copyright in the software and shall
@@ -305,6 +305,8 @@ class VirtIO9PDiod : public VirtIO9PProxy
 
     ssize_t read(uint8_t *data, size_t len);
     ssize_t write(const uint8_t *data, size_t len);
+    /** Kill the diod child process at the end of the simulation */
+    void terminateDiod();
 
   private:
     class DiodDataEvent : public PollEvent