Add no console option #40

alexis-langlet · 2023-03-21T12:56:59Z

When a running VM stops, the VMM is blocked and never returns.
This is due to the epoll's loop (in lib.rs) that is used to listen to event and prompt stdin, the epoll::wait() being a blocking function.

In order to use the lib in another context (lambdo project), we need for the VMM to finish.
Moreover, we don't want to have output from VM being printed on our console.

To do that, we will allow to stop outputting to ttyS0 (if no console is specified)
And in case the VM stop running, we will stop the VMM

alexis-langlet · 2023-03-21T13:03:20Z

@sameo we have a problem.

Everything is working with only one vCPU.
When we use 2 vCPUs, one thread stop but the other is hanging, so the VM never stops.
We've reduced the bug cause to the run function (src/vmm/src/cpu/mod.rs, line 223) it is probably what's blocking the thread and never return.

Is this a wanted behavior of ioctl ? what are we supposed to do ?

alexis-langlet · 2023-03-27T07:47:40Z

@sameo we have a problem.

Everything is working with only one vCPU. When we use 2 vCPUs, one thread stop but the other is hanging, so the VM never stops. We've reduced the bug cause to the run function (src/vmm/src/cpu/mod.rs, line 223) it is probably what's blocking the thread and never return.

Is this a wanted behavior of ioctl ? what are we supposed to do ?

We've discussed the matter a bit further...
At first we thought that killing all thread at the end of the first one to finish could be an option, be because of a a potential deadlock we probably better not.
Because the blocking problem was already there with the console, we, in fact, have no regression... Is it possible to merge it as it is or do you see another option ?

alexis-langlet · 2023-03-27T11:04:43Z

If it can help, here is the init file we use for testing

#! /bin/sh
mount -t devtmpfs dev /dev
mount -t proc proc /proc
mount -t sysfs sysfs /sys
ip link set up dev lo

echo "hello" > /dev/ttyS0
exit
poweroff -f

sameo

I can not merge this PR for 3 reasons:

It carries a bunch of unrelated commits.
On the single commit that seems to be related to this PR, I have a few comments.
You give no explanations about why is no_console needed.

src/main.rs

sameo · 2023-04-04T07:18:51Z

src/vmm/src/cpu/mod.rs

        // Call into KVM to launch (VMLAUNCH) or resume (VMRESUME) the virtual CPU.
        // This is a blocking function, it only returns for either an error or a
        // VM-Exit. In the latter case, we can inspect the exit reason.
-        match self.vcpu_fd.run() {
+        println!("Before running vCPU {}...", self.index);


Please clean the debug logs up.

src/vmm/src/cpu/mod.rs

src/vmm/src/lib.rs

It is now possible to run a VM without a console with --no-console option Co-authored-by: Esteban Baron <[email protected]> Signed-off-by: Alexis Langlet <[email protected]> (cherry picked from commit 9990b84)

Signed-off-by: Simon LUCIDO <[email protected]>

Delete the no console attribute where not longer used Co-authored-by: Esteban Baron <[email protected]> Signed-off-by: Alexis Langlet <[email protected]> (cherry picked from commit f144cbe)

alexis-langlet · 2023-04-11T18:46:22Z

Thanks for your review! Everything should have been resolved and cleaned-up.

sameo

This PR brings a major change on how vCPU threads are managed (and potentially brutally killed). None of the three commits in it mention this. And none of the added code seems to come with any comments or explanation.
https://groups.google.com/g/comp.lang.c++/c/rYCO5yn4lXw/m/oITtSkZOtoUJ

sameo · 2023-04-16T14:09:14Z

src/vmm/src/cpu/mod.rs

-
-                    unsafe { libc::exit(0) };
-                }
+        loop {


Did we move the loop here to avoid passing the socket_name after each vm exit?

sameo · 2023-04-16T14:14:36Z

src/vmm/src/cpu/mod.rs

+                    // The VM stopped (Shutdown ot HLT).
+                    VcpuExit::Shutdown | VcpuExit::Hlt => {
+                        println!("Guest shutdown: {:?}. Bye!", exit_reason);
+                        unix_socket.write_all(b"1").unwrap();


We notify the main thread and then continue running?

sameo · 2023-04-16T14:14:45Z

src/vmm/src/lib.rs

+        let mut unix_socket_name = String::from("/tmp/vmm.sock");
+        while Path::new(&unix_socket_name).exists() {
+            let rng = rand::rand_alphanumerics(8);
+            unix_socket_name = format!("/tmp/vmm-{}.sock", rng.to_str().unwrap());
+        }


iiuc you're passing a named unix socket to the vcpu for it to check in when it get a shutdown/halt?
A simpler, more idiomatic approach is mpsc. And unless I'm missing why it would not work, I'd prefer that you use it. And yes, you may need to run a monitoring thread.

sameo · 2023-04-16T14:22:16Z

src/vmm/src/lib.rs

+
+                if connections.iter().any(|c| c.as_raw_fd() == event_data) {
+                    use vmm_sys_util::signal::Killable;
+                    handlers.iter().for_each(|handler| {


So we're killing each thread whenever ones is shutdown/halted, right?

sameo · 2023-04-16T14:25:26Z

src/vmm/src/lib.rs

+    /// Access thread handler error
+    AccessThreadHandlerError,
+    /// Join thread error
+    JoinThreadError(Box<dyn Any + Send>),


This is unused.

sameo · 2023-04-16T14:26:30Z

src/vmm/src/lib.rs

@@ -86,6 +90,10 @@ pub enum Error {
    VirtioNet(devices::net::VirtioNetError),
    /// Error related to IOManager.
    IoManager(vm_device::device_manager::Error),
+    /// Access thread handler error


What does that mean?

sameo · 2023-04-16T14:32:01Z

src/vmm/src/lib.rs

@@ -383,6 +427,14 @@ impl VMM {
                        .process_tap()
                        .map_err(Error::VirtioNet)?;
                }
+


Why are we still listening to stdin when the VM has no console?

sameo · 2023-04-16T14:34:17Z

src/vmm/src/lib.rs

+        while connections.len() < total_cpus {
+            let connection = listener.accept().unwrap().0;
+            self.epoll.add_fd(connection.as_raw_fd()).unwrap();
+            connections.push(connection);


This would be a good place to explain what you're trying to do here: You're going to listen on a socket for any vcpu to halt and then do something about that information.

sameo · 2023-04-16T14:34:52Z

src/vmm/src/lib.rs

+        let mut connections: Vec<_> = Vec::new();
+
+        while connections.len() < total_cpus {
+            let connection = listener.accept().unwrap().0;


Please handle errors properly, by propagating them.

sameo · 2023-04-16T14:34:59Z

src/vmm/src/lib.rs

+
+        while connections.len() < total_cpus {
+            let connection = listener.accept().unwrap().0;
+            self.epoll.add_fd(connection.as_raw_fd()).unwrap();


EstebanBAR0N force-pushed the add-no-console branch from 4a1ed13 to f749c64 Compare March 22, 2023 09:53

alexis-langlet force-pushed the add-no-console branch 2 times, most recently from 09d53d1 to 04ec86d Compare March 27, 2023 07:39

alexis-langlet marked this pull request as ready for review March 27, 2023 07:40

alexis-langlet force-pushed the add-no-console branch from 04ec86d to 6daafab Compare March 30, 2023 15:49

sameo requested changes Apr 4, 2023

View reviewed changes

alexis-langlet force-pushed the add-no-console branch from 6daafab to 9990b84 Compare April 11, 2023 17:23

EstebanBAR0N and others added 3 commits April 11, 2023 20:27

feat: add --no-console option

31e702c

It is now possible to run a VM without a console with --no-console option Co-authored-by: Esteban Baron <[email protected]> Signed-off-by: Alexis Langlet <[email protected]> (cherry picked from commit 9990b84)

fix: don't block epoll when in no console

faf2274

Signed-off-by: Simon LUCIDO <[email protected]>

fix: clean-up no-console

4e051ce

Delete the no console attribute where not longer used Co-authored-by: Esteban Baron <[email protected]> Signed-off-by: Alexis Langlet <[email protected]> (cherry picked from commit f144cbe)

alexis-langlet force-pushed the add-no-console branch from f144cbe to 4e051ce Compare April 11, 2023 18:38

alexis-langlet requested a review from sameo April 11, 2023 18:46

sameo requested changes Apr 16, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add no console option #40

Add no console option #40

alexis-langlet commented Mar 21, 2023 •

edited

Loading

alexis-langlet commented Mar 21, 2023

alexis-langlet commented Mar 27, 2023

alexis-langlet commented Mar 27, 2023

sameo left a comment

sameo Apr 4, 2023

alexis-langlet commented Apr 11, 2023

sameo left a comment

sameo Apr 16, 2023

sameo Apr 16, 2023

sameo Apr 16, 2023

sameo Apr 16, 2023

sameo Apr 16, 2023

sameo Apr 16, 2023

sameo Apr 16, 2023

sameo Apr 16, 2023

sameo Apr 16, 2023

sameo Apr 16, 2023

Add no console option #40

Are you sure you want to change the base?

Add no console option #40

Conversation

alexis-langlet commented Mar 21, 2023 • edited Loading

alexis-langlet commented Mar 21, 2023

alexis-langlet commented Mar 27, 2023

alexis-langlet commented Mar 27, 2023

sameo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexis-langlet commented Apr 11, 2023

sameo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexis-langlet commented Mar 21, 2023 •

edited

Loading