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 7dac79c..f2eb8ba 100644
--- 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 @@
: 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 @@
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 @@
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 @@
} 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 @@
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 786f584..4652560 100644
--- 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 @@
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
