Reworked locking around fork & fork/exec.

rubinius · Jan 25, 2015 · 96f8eda · 96f8eda
1 parent af7eb1b
commit 96f8eda
Show file tree

Hide file tree

Showing 5 changed files with 57 additions and 89 deletions.
diff --git a/vm/builtin/system.cpp b/vm/builtin/system.cpp
@@ -382,6 +382,23 @@ namespace rubinius {
     return cNil;
   }
 
+  static int fork_exec(STATE, int errors_fd) {
+    utilities::thread::Mutex::LockGuard guard(state->shared().fork_exec_lock());
+
+    state->shared().auxiliary_threads()->before_fork_exec(state);
+
+    // If execvp() succeeds, we'll read EOF and know.
+    fcntl(errors_fd, F_SETFD, FD_CLOEXEC);
+
+    int pid = ::fork();
+
+    if(pid > 0) {
+      state->shared().auxiliary_threads()->after_fork_exec_parent(state);
+    }
+
+    return pid;
+  }
+
   Object* System::vm_spawn(STATE, GCToken gct, Object* spawn_state, String* path,
                            Array* args, CallFrame* calling_environment)
   {
@@ -392,45 +409,20 @@ namespace rubinius {
      */
     ExecCommand exe(state, path, args);
 
-    {
-      // TODO: Make this guard unnecessary
-      GCIndependent guard(state, calling_environment);
-      state->shared().auxiliary_threads()->before_fork_exec(state);
-    }
-
-    int pid = -1, errors[2];
+    int errors[2];
 
     if(pipe(errors) != 0) {
-      {
-        // TODO: Make this guard unnecessary
-        GCIndependent guard(state, calling_environment);
-        state->shared().auxiliary_threads()->after_fork_exec_parent(state);
-      }
-
       Exception::errno_error(state, "error setting up pipes", errno, "pipe(2)");
       return NULL;
     }
 
-    // If execvp() succeeds, we'll read EOF and know.
-    fcntl(errors[1], F_SETFD, FD_CLOEXEC);
-
-    {
-      StopTheWorld stw(state, gct, calling_environment);
-
-      pid = ::fork();
-    }
+    int pid = fork_exec(state, errors[1]);
 
     // error
     if(pid == -1) {
       close(errors[0]);
       close(errors[1]);
 
-      {
-        // TODO: Make this guard unnecessary
-        GCIndependent guard(state, calling_environment);
-        state->shared().auxiliary_threads()->after_fork_exec_parent(state);
-      }
-
       Exception::errno_error(state, "error forking", errno, "fork(2)");
       return NULL;
     }
@@ -485,8 +477,6 @@ namespace rubinius {
 
     close(errors[1]);
 
-    state->shared().auxiliary_threads()->after_fork_exec_parent(state);
-
     int error_no;
     ssize_t size;
 
@@ -522,44 +512,19 @@ namespace rubinius {
      */
     ExecCommand exe(state, str);
 
-    {
-      // TODO: Make this guard unnecessary
-      GCIndependent guard(state, calling_environment);
-      state->shared().auxiliary_threads()->before_fork_exec(state);
-    }
-
-    int pid, errors[2], output[2];
+    int errors[2], output[2];
 
     if(pipe(errors) != 0) {
-      {
-        // TODO: Make this guard unnecessary
-        GCIndependent guard(state, calling_environment);
-        state->shared().auxiliary_threads()->after_fork_exec_parent(state);
-      }
-
       Exception::errno_error(state, "error setting up pipes", errno, "pipe(2)");
       return NULL;
     }
 
     if(pipe(output) != 0) {
-      {
-        // TODO: Make this guard unnecessary
-        GCIndependent guard(state, calling_environment);
-        state->shared().auxiliary_threads()->after_fork_exec_parent(state);
-      }
-
       Exception::errno_error(state, "error setting up pipes", errno, "pipe(2)");
       return NULL;
     }
 
-    // If execvp() succeeds, we'll read EOF and know.
-    fcntl(errors[1], F_SETFD, FD_CLOEXEC);
-
-    {
-      StopTheWorld stw(state, gct, calling_environment);
-
-      pid = ::fork();
-    }
+    int pid = fork_exec(state, errors[1]);
 
     // error
     if(pid == -1) {
@@ -568,12 +533,6 @@ namespace rubinius {
       close(output[0]);
       close(output[1]);
 
-      {
-        // TODO: Make this guard unnecessary
-        GCIndependent guard(state, calling_environment);
-        state->shared().auxiliary_threads()->after_fork_exec_parent(state);
-      }
-
       Exception::errno_error(state, "error forking", errno, "fork(2)");
       return NULL;
     }
@@ -621,8 +580,6 @@ namespace rubinius {
     close(errors[1]);
     close(output[1]);
 
-    state->shared().auxiliary_threads()->after_fork_exec_parent(state);
-
     int error_no;
     ssize_t size;
 
@@ -685,16 +642,14 @@ namespace rubinius {
   Object* System::vm_exec(STATE, String* path, Array* args,
                           CallFrame* calling_environment)
   {
+    utilities::thread::Mutex::LockGuard guard(state->shared().fork_exec_lock());
+
     /* Setting up the command and arguments may raise an exception so do it
      * before everything else.
      */
     ExecCommand exe(state, path, args);
 
-    {
-      // TODO: Make this guard unnecessary
-      GCIndependent guard(state, calling_environment);
-      state->shared().auxiliary_threads()->before_exec(state);
-    }
+    state->shared().auxiliary_threads()->before_exec(state);
 
     void* old_handlers[NSIG];
 
@@ -823,22 +778,16 @@ namespace rubinius {
 #else
     int pid = -1;
 
-    /*
-     * We have to bring all the threads to a safe point before we can
-     * fork the process so any internal locks are unlocked before we fork
-     */
-
     {
-      // TODO: Make this guard unnecessary
-      GCIndependent guard(state, calling_environment);
-      state->shared().auxiliary_threads()->before_fork(state);
-    }
+      utilities::thread::Mutex::LockGuard guard(state->shared().fork_exec_lock());
 
-    {
-      StopTheWorld stw(state, gct, calling_environment);
+      state->shared().auxiliary_threads()->before_fork(state);
 
-      // ok, now fork!
       pid = ::fork();
+
+      if(pid > 0) {
+        state->shared().auxiliary_threads()->after_fork_parent(state);
+      }
     }
 
     // We're in the child...
@@ -851,12 +800,6 @@ namespace rubinius {
 
       // In the child, the PID is nil in Ruby.
       return nil<Fixnum>();
-    } else {
-      {
-        // TODO: Make this guard unnecessary
-        GCIndependent guard(state, calling_environment);
-        state->shared().auxiliary_threads()->after_fork_parent(state);
-      }
     }
 
     if(pid == -1) {

diff --git a/vm/metrics.cpp b/vm/metrics.cpp
@@ -192,6 +192,7 @@ namespace rubinius {
       : AuxiliaryThread()
       , shared_(state->shared())
       , vm_(NULL)
+      , enabled_(true)
       , thread_exit_(false)
       , thread_running_(false)
       , thread_(state)
@@ -419,6 +420,7 @@ namespace rubinius {
 
     void Metrics::start(STATE) {
       vm_ = NULL;
+      enabled_ = true;
       thread_exit_ = false;
       thread_running_ = false;
 
@@ -484,9 +486,13 @@ namespace rubinius {
     }
 
     void Metrics::add_historical_metrics(MetricsData* metrics) {
-      utilities::thread::Mutex::LockGuard guard(metrics_lock_);
+      if(!enabled_) return;
 
-      metrics_history_.add(metrics);
+      {
+        utilities::thread::Mutex::LockGuard guard(metrics_lock_);
+
+        metrics_history_.add(metrics);
+      }
     }
 
     void Metrics::process_metrics(STATE) {

diff --git a/vm/metrics.hpp b/vm/metrics.hpp
@@ -379,6 +379,7 @@ namespace rubinius {
     class Metrics : public AuxiliaryThread, public Lockable {
       SharedState& shared_;
       VM* vm_;
+      bool enabled_;
       bool thread_exit_;
       bool thread_running_;
 
@@ -409,6 +410,10 @@ namespace rubinius {
       void init_ruby_metrics(STATE);
       void update_ruby_values(STATE);
 
+      void disable(STATE) {
+        enabled_ = false;
+      }
+
       void start(STATE);
 
       void start_thread(STATE);

diff --git a/vm/shared_state.cpp b/vm/shared_state.cpp
@@ -177,6 +177,10 @@ namespace rubinius {
     return metrics_;
   }
 
+  void SharedState::disable_metrics(STATE) {
+    if(metrics_) metrics_->disable(state);
+  }
+
   void SharedState::reset_threads(STATE, GCToken gct, CallFrame* call_frame) {
     VM* current = state->vm();
 
@@ -211,12 +215,15 @@ namespace rubinius {
 
     env_->set_root_vm(state->vm());
 
+    disable_metrics(state);
+
     reset_threads(state, gct, call_frame);
 
     // Reinit the locks for this object
     lock_init(state->vm());
     global_cache->reset();
     ruby_critical_lock_.init();
+    fork_exec_lock_.init();
     capi_ds_lock_.init();
     capi_locks_lock_.init();
     capi_constant_lock_.init();

diff --git a/vm/shared_state.hpp b/vm/shared_state.hpp
@@ -116,6 +116,8 @@ namespace rubinius {
     utilities::thread::Mutex ruby_critical_lock_;
     pthread_t ruby_critical_thread_;
 
+    utilities::thread::Mutex fork_exec_lock_;
+
     utilities::thread::SpinLock capi_ds_lock_;
     utilities::thread::SpinLock capi_locks_lock_;
     utilities::thread::SpinLock capi_constant_lock_;
@@ -227,6 +229,7 @@ namespace rubinius {
     }
 
     metrics::Metrics* start_metrics(STATE);
+    void disable_metrics(STATE);
 
     Environment* env() const {
       return env_;
@@ -275,6 +278,10 @@ namespace rubinius {
 
     const unsigned int* object_memory_mark_address() const;
 
+    utilities::thread::Mutex& fork_exec_lock() {
+      return fork_exec_lock_;
+    }
+
     void set_use_capi_lock(bool s) {
       use_capi_lock_ = s;
     }