Shutdown VCPU threads so they can thread::join

This continues the work from the prior commit toward the goal of
releasing all resources with a clean shutdown.  The VCPU threads in
non-test configurations were running with an unbreakable wait, so
they only way to get past them was with exit():

    // Wait indefinitely
    // The VMM thread will kill the entire process
    let barrier = Barrier::new(2);
    barrier.wait();

Here that is changed, along with several other entangled issues.
One was that Drop methods were used to try and clean up the Vmm
and the VcpuHandles, but these were not being called.  The reason
is that there was a cyclic dependency: threads held onto the Vmm,
so the Vmm could not exit VCPU threads in its Drop method (as it
wouldn't be dropped so long as the VCPU threads were around).

Another complexity surrounds defining the difference between an
exited thread (from which an exit event can be read out of the
channel) and a finished thread (which tears down the channel).
Trying to send a message to a thread which has finished will
panic, and some CPUs might be in the exit state while others may
not...and reading the "exit state" by taking the message out of
the channel removes it.  It's a bit complicated.

This commit tries to move toward simplification by being explicit
about the Vmm.stop() call being responsible for exiting the VCPUs,
and making the VcpuHandle's Drop method only do the simple task
of joining an exited thread.

Author: AE1020

src/vmm/src/lib.rs

@@ -36,7 +36,7 @@ use std::collections::HashMap;
 use std::fmt::{Display, Formatter};
 use std::io;
 use std::os::unix::io::AsRawFd;
-use std::sync::mpsc::RecvTimeoutError;
+use std::sync::mpsc::{RecvTimeoutError, TryRecvError};
 use std::sync::Mutex;
 use std::time::Duration;
 
@@ -317,12 +317,26 @@ impl Vmm {
 
     /// Sends an exit command to the vCPUs.
     pub fn exit_vcpus(&mut self) -> Result<()> {
-        self.broadcast_vcpu_event(
-            VcpuEvent::Exit,
-            VcpuResponse::Exited(FC_EXIT_CODE_GENERIC_ERROR),
-        )
-        .map_err(|_| Error::VcpuExit)
+        //
+        // We actually send a "Finish" event.  If a VCPU has already exited, this is
+        // the only message it will accept...but runinng and paused will take it as well.
+        // It breaks out of the state machine loop so that the thread can be joined.
+        //
+        for handle in &self.vcpus_handles {
+            handle
+                .send_event(VcpuEvent::Finish)
+                .map_err(|_| Error::VcpuMessage)?;
+        }
+
+        // The actual thread::join() that runs to release the thread's resource is done in
+        // the VcpuHandle's Drop trait.  We can trigger that to happen now by clearing the
+        // list of handles (Vmm's Drop will also asserts this list is empty).
+        //
+        self.vcpus_handles.clear();
+
+        Ok(())
     }
+
     /// Returns a reference to the inner `GuestMemoryMmap` object if present, or `None` otherwise.
     pub fn guest_memory(&self) -> &GuestMemoryMmap {
         &self.guest_memory
@@ -344,6 +358,8 @@ impl Vmm {
     pub fn stop(&mut self) {
         info!("Vmm is stopping.");
 
+        self.exit_vcpus().unwrap();  // exit all not-already-exited VCPUs, join their threads
+
         if let Some(observer) = self.events_observer.as_mut() {
             if let Err(e) = observer.on_vmm_stop() {
                 warn!("{}", Error::VmmObserverTeardown(e));
@@ -706,7 +722,24 @@ fn construct_kvm_mpidrs(vcpu_states: &[VcpuState]) -> Vec<u64> {
 
 impl Drop for Vmm {
     fn drop(&mut self) {
-        let _ = self.exit_vcpus();
+        //
+        // !!! This used to think it exited the cpus, which would do a thread join and tie up
+        // the vcpu threads:
+        //
+        //     let _ = self.exit_vcpus();
+        //
+        // But since the main firecracker process used exit() to terminate in mid-stack, the
+        // destructors for Vmm and other classes would not run.  Even after mitigating that by
+        // running the exit from a top-level main() wrapper, the Vmm's Drop still was not
+        // happening because of a cycle: VCPU threads held references to the Vmm, so you couldn't
+        // have the Vmm drop be how the threads were exiting.  Further, the VCPU threads would
+        // not exit cleanly--they'd just block on a barrier--and could not be thread::join'd.
+        //
+        // To be more explicit and untangle those problems, exit_vcpus() is moved into Vmm::stop().
+        // So now, this Drop method just asserts that happened.  It won't trigger if there's
+        // another situation where there's no Vmm Drop...but a Valgrind run will pick up the leak.
+        //
+        assert!(self.vcpus_handles.is_empty());
     }
 }
 
@@ -718,20 +751,49 @@ impl Subscriber for Vmm {
 
         if source == self.exit_evt.as_raw_fd() && event_set == EventSet::IN {
             let _ = self.exit_evt.read();
+
+            let mut opt_exit_code: Option<ExitCode> = None;
+
             // Query each vcpu for the exit_code.
+            //
+            for handle in &self.vcpus_handles {
+                match handle.response_receiver().try_recv() {
+                    Ok(VcpuResponse::Exited(status)) => {
+                        if opt_exit_code.is_none() {
+                            opt_exit_code = Some(status);
+                        } else {
+                            warn!("Multiple VCPU exit states detected, using first exit code");
+                        }
+                    }
+                    Err(TryRecvError::Empty) => {}  // Nothing pending in channel
+                    Ok(_response) => {
+                        //
+                        // !!! This removes a response from the VCPU channel.  Is there
+                        // anything to lose from that, given that we are exiting?
+                    }
+                    Err(e) => {
+                        panic!("Error while looking for VCPU exit status: {}", e);
+                    }
+                }
+            }
+
+            // !!! The caller of this routine is receiving the exit code to bubble back up
+            // to the main() function to return cleanly.  However, it does not have clean
+            // access to the Vmm to shut it down (here we have it, since it is `self`).  It
+            // might seem tempting to put the stop() code in the Drop trait...but since
+            // stopping the Vmm involves shutting down VCPU threads which hold references
+            // on the Vmm, that presents a cycle.  The solution of the moment is to do
+            // everything here...which means this is the only process_exitable() handler
+            // that will actually work with an exit code (all other Subscriber trait
+            // implementers must use process())
+            //
+            self.stop();
+
             // If the exit_code can't be found on any vcpu, it means that the exit signal
             // has been issued by the i8042 controller in which case we exit with
             // FC_EXIT_CODE_OK.
-            let exit_code = self
-                .vcpus_handles
-                .iter()
-                .find_map(|handle| match handle.response_receiver().try_recv() {
-                    Ok(VcpuResponse::Exited(exit_code)) => Some(exit_code),
-                    _ => None,
-                })
-                .unwrap_or(FC_EXIT_CODE_OK);
-            self.stop();
-            Some(exit_code)
+            //
+            Some(opt_exit_code.unwrap_or(FC_EXIT_CODE_OK))
         } else {
             error!("Spurious EventManager event for handler: Vmm");
             None

src/vmm/src/vstate/vcpu/mod.rs

@@ -6,8 +6,6 @@
 // found in the THIRD-PARTY file.
 
 use libc::{c_int, c_void, siginfo_t};
-#[cfg(not(test))]
-use std::sync::Barrier;
 #[cfg(test)]
 use std::sync::Mutex;
 use std::{
@@ -332,6 +330,7 @@ impl Vcpu {
                     .expect("failed to send save not allowed status");
             }
             Ok(VcpuEvent::Exit) => return self.exit(FC_EXIT_CODE_GENERIC_ERROR),
+            Ok(VcpuEvent::Finish) => return StateMachine::finish(),
             // Unhandled exit of the other end.
             Err(TryRecvError::Disconnected) => {
                 // Move to 'exited' state.
@@ -396,6 +395,7 @@ impl Vcpu {
                 StateMachine::next(Self::paused)
             }
             Ok(VcpuEvent::Exit) => self.exit(FC_EXIT_CODE_GENERIC_ERROR),
+            Ok(VcpuEvent::Finish) => StateMachine::finish(),
             // Unhandled exit of the other end.
             Err(_) => {
                 // Move to 'exited' state.
@@ -404,41 +404,38 @@ impl Vcpu {
         }
     }
 
-    #[cfg(not(test))]
     // Transition to the exited state.
     fn exit(&mut self, exit_code: u8) -> StateMachine<Self> {
         self.response_sender
             .send(VcpuResponse::Exited(exit_code))
             .expect("vcpu channel unexpectedly closed");
 
-        if let Err(e) = self.exit_evt.write(1) {
-            METRICS.vcpu.failures.inc();
-            error!("Failed signaling vcpu exit event: {}", e);
-        }
-
-        // State machine reached its end.
         StateMachine::next(Self::exited)
     }
 
-    #[cfg(not(test))]
     // This is the main loop of the `Exited` state.
     fn exited(&mut self) -> StateMachine<Self> {
-        // Wait indefinitely.
-        // The VMM thread will kill the entire process.
-        let barrier = Barrier::new(2);
-        barrier.wait();
-
-        StateMachine::finish()
-    }
+        //
+        // !!! This signaled `EventFd` is not used by the main protocol.  Is it specific to test?
+        // What does the test actually want to know by seeing it set?
+        //
+        if let Err(e) = self.exit_evt.write(1) {
+            METRICS.vcpu.failures.inc();
+            error!("Failed signaling vcpu exit event: {}", e);
+        }
 
-    #[cfg(test)]
-    // In tests the main/vmm thread exits without 'exit()'ing the whole process.
-    // All channels get closed on the other side while this Vcpu thread is still running.
-    // This Vcpu thread should just do a clean finish without reporting back to the main thread.
-    fn exit(&mut self, _: u8) -> StateMachine<Self> {
-        self.exit_evt.write(1).unwrap();
-        // State machine reached its end.
-        StateMachine::finish()
+        // !!! Stylistically we might like to force a transition to an exit state for all VCPUs
+        // before allowing them to accept a Finish and terminate the state loop.  But you run into
+        // trouble with sending Exit to already exited VCPUs.  This is because right now, reading
+        // (and removing) an exited event from the channel is how the main thread discovers exits
+        // arising from the VCPU itself.  So by the time clean shutdown is performed, one can't
+        // tell if a VCPU is in the exit state or not.  For the moment, the main benefit is that
+        // the exit state is able to refuse to process any other events besides Finish.
+        //
+        match self.event_receiver.recv() {
+            Ok(VcpuEvent::Finish) => StateMachine::finish(),
+            _ => panic!("exited() state of VCPU can only respond to Finish events (for thread join)")
+        }
     }
 
     #[cfg(not(test))]
@@ -558,6 +555,8 @@ impl Drop for Vcpu {
 pub enum VcpuEvent {
     /// The vCPU will go to exited state when receiving this message.
     Exit,
+    /// Actual thread (channel) ends on finish--hence no response.
+    Finish,
     /// Pause the Vcpu.
     Pause,
     /// Event to resume the Vcpu.
@@ -825,15 +824,20 @@ mod tests {
         }
     }
 
-    // In tests we need to close any pending Vcpu threads on test completion.
+    // Wait for the Vcpu thread to finish execution
     impl Drop for VcpuHandle {
         fn drop(&mut self) {
-            // Make sure the Vcpu is out of KVM_RUN.
-            self.send_event(VcpuEvent::Pause).unwrap();
-            // Close the original channel so that the Vcpu thread errors and goes to exit state.
-            let (event_sender, _event_receiver) = channel();
-            self.event_sender = event_sender;
-            // Wait for the Vcpu thread to finish execution
+            //
+            // !!! Previously, this Drop code would attempt to send messages to the Vcpu
+            // to put it in an exit state.  We now assume that by the time a VcpuHandle is
+            // dropped, other code has run to get the state machine loop to finish so the
+            // thread is ready to join.  The strategy of avoiding more complex messaging
+            // protocols during the Drop helps avoid cycles which were preventing a truly
+            // clean shutdown.
+            //
+            // If the code hangs at this point, that means that a Finish event was not
+            // sent by Vmm.stop().
+            //
             self.vcpu_thread.take().unwrap().join().unwrap();
         }
     }