From 303c285b7f8a5aef85fce6db9fb45f8031f276c4 Mon Sep 17 00:00:00 2001 From: Takaya Saeki Date: Thu, 27 Oct 2022 06:55:41 +0000 Subject: [PATCH] cros_async: Move --async-executor to run/device/devices subcommands Currently, --async-executor option to switch the backend of cros_async::Executor is a global option of crosvm main command. This is because --async-executor is an option to switch the async runtime backend engine of cros_async crate for a whole crosvm. However, in practice, only run, device, and devices subcommands have the motivation to switch the backends for the performance. Other subcommands may or may not use async functions, but it's unlikely they want to switch the backends. So, it makes sense to make --async-executor an option of those three subcommands, not of the global option. Also, it allows us to switch --async-executor in arvm_dev.conf, which overrides the options for `crosvm run` command, and to control the async executor by the coming configuration file feature. Thus, this commit moves the --async-executor option from the global command to the run, device, and devices subcommands. This is a breaking change, but it is unlikely to have many users using this relatively new option. Takayas also confirmed there's no usage on codesearch. BUG=b:251289312 TEST=confirmed `crosvm --log-level debug run --async-executor ...` switches the executor with additional debug log in the coming CL. TEST=confirmed `crosvm --log-level debug device --async-executor ...` switches the executor with additional debug log in the coming CL. TEST=confirmed `crosvm --log-level debug devices --async-executor ...` switches the executor with additional debug log in the coming CL. TEST=./tools/dev_container ./tools/presubmit Change-Id: Ia25f2d0b296b8d73f31d71c362e5f90678166d96 Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/4005473 Auto-Submit: Takaya Saeki Commit-Queue: Takaya Saeki Reviewed-by: Keiichi Watanabe Commit-Queue: Keiichi Watanabe --- cros_async/src/sys/unix/executor.rs | 5 ++++- cros_async/src/sys/windows/executor.rs | 5 ++++- src/crosvm/cmdline.rs | 16 ++++++++++++---- src/crosvm/config.rs | 3 +++ src/crosvm/sys/unix.rs | 10 ++++++++++ src/crosvm/sys/unix/cmdline.rs | 5 +++++ src/main.rs | 10 +++++----- 7 files changed, 43 insertions(+), 11 deletions(-) diff --git a/cros_async/src/sys/unix/executor.rs b/cros_async/src/sys/unix/executor.rs index aa4ae9ddae..7d13616fd2 100644 --- a/cros_async/src/sys/unix/executor.rs +++ b/cros_async/src/sys/unix/executor.rs @@ -10,6 +10,7 @@ use base::AsRawDescriptors; use base::RawDescriptor; use once_cell::sync::OnceCell; use serde::Deserialize; +use serde::Serialize; use thiserror::Error as ThisError; use super::poll_source::Error as PollError; @@ -161,7 +162,9 @@ pub enum Executor { } /// An enum to express the kind of the backend of `Executor` -#[derive(Clone, Copy, Debug, PartialEq, Eq, Deserialize, serde_keyvalue::FromKeyValues)] +#[derive( + Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize, serde_keyvalue::FromKeyValues, +)] #[serde(deny_unknown_fields, rename_all = "kebab-case")] pub enum ExecutorKind { Uring, diff --git a/cros_async/src/sys/windows/executor.rs b/cros_async/src/sys/windows/executor.rs index aee0d595b1..623e59d1a5 100644 --- a/cros_async/src/sys/windows/executor.rs +++ b/cros_async/src/sys/windows/executor.rs @@ -7,6 +7,7 @@ use std::future::Future; use async_task::Task; use once_cell::sync::OnceCell; use serde::Deserialize; +use serde::Serialize; use thiserror::Error as ThisError; use super::HandleExecutor; @@ -126,7 +127,9 @@ pub enum Executor { } /// An enum to express the kind of the backend of `Executor` -#[derive(Clone, Copy, Debug, PartialEq, Eq, Deserialize, serde_keyvalue::FromKeyValues)] +#[derive( + Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize, serde_keyvalue::FromKeyValues, +)] #[serde(deny_unknown_fields, rename_all = "kebab-case")] pub enum ExecutorKind { Handle, diff --git a/src/crosvm/cmdline.rs b/src/crosvm/cmdline.rs index 37600e5095..7bce8d90f1 100644 --- a/src/crosvm/cmdline.rs +++ b/src/crosvm/cmdline.rs @@ -113,10 +113,6 @@ pub struct CrosvmCmdlineArgs { #[argh(switch)] /// disable output to syslog pub no_syslog: bool, - /// configure async executor backend; "uring" or "epoll" on Linux, "handle" on Windows. - /// If this option is omitted on Linux, "epoll" is used by default. - #[argh(option, arg_name = "EXECUTOR")] - pub async_executor: Option, #[argh(subcommand)] pub command: Command, } @@ -389,6 +385,11 @@ pub struct VfioCrosvmCommand { #[argh(subcommand, name = "device")] /// Start a device process pub struct DeviceCommand { + /// configure async executor backend; "uring" or "epoll" on Linux, "handle" on Windows. + /// If this option is omitted on Linux, "epoll" is used by default. + #[argh(option, arg_name = "EXECUTOR")] + pub async_executor: Option, + #[argh(subcommand)] pub command: DeviceSubcommand, } @@ -590,6 +591,11 @@ pub struct RunCommand { /// path to Android fstab pub android_fstab: Option, + /// configure async executor backend; "uring" or "epoll" on Linux, "handle" on Windows. + /// If this option is omitted on Linux, "epoll" is used by default. + #[argh(option, arg_name = "EXECUTOR")] + pub async_executor: Option, + #[argh(option, arg_name = "N")] #[serde(skip)] // TODO(b/255223604) /// amount to bias balance of memory between host and guest as the balloon inflates, in mib. @@ -1725,6 +1731,8 @@ impl TryFrom for super::config::Config { cfg.android_fstab = cmd.android_fstab; + cfg.async_executor = cmd.async_executor; + cfg.params.extend(cmd.params); cfg.per_vm_core_scheduling = cmd.per_vm_core_scheduling; diff --git a/src/crosvm/config.rs b/src/crosvm/config.rs index 5ec766e9c5..6f40bc0b6f 100644 --- a/src/crosvm/config.rs +++ b/src/crosvm/config.rs @@ -18,6 +18,7 @@ use arch::Pstore; use arch::VcpuAffinity; use base::debug; use base::pagesize; +use cros_async::ExecutorKind; use devices::serial_device::SerialHardware; use devices::serial_device::SerialParameters; use devices::virtio::block::block::DiskOption; @@ -1095,6 +1096,7 @@ pub struct Config { pub ac97_parameters: Vec, pub acpi_tables: Vec, pub android_fstab: Option, + pub async_executor: Option, pub balloon: bool, pub balloon_bias: i64, pub balloon_control: Option, @@ -1295,6 +1297,7 @@ impl Default for Config { ac97_parameters: Vec::new(), acpi_tables: Vec::new(), android_fstab: None, + async_executor: None, balloon: true, balloon_bias: 0, balloon_control: None, diff --git a/src/crosvm/sys/unix.rs b/src/crosvm/sys/unix.rs index a5788cc372..ecef2719d7 100644 --- a/src/crosvm/sys/unix.rs +++ b/src/crosvm/sys/unix.rs @@ -1391,6 +1391,11 @@ fn get_default_hypervisor() -> Result { } pub fn run_config(cfg: Config) -> Result { + if let Some(async_executor) = cfg.async_executor { + Executor::set_default_executor_kind(async_executor) + .context("Failed to set the default async executor")?; + } + let components = setup_vm_components(&cfg)?; let guest_mem_layout = @@ -3014,6 +3019,11 @@ fn start_vhost_user_control_server( } pub fn start_devices(opts: DevicesCommand) -> anyhow::Result<()> { + if let Some(async_executor) = opts.async_executor { + Executor::set_default_executor_kind(async_executor) + .context("Failed to set the default async executor")?; + } + struct DeviceJailInfo { // Unique name for the device, in the form `foomatic-0`. name: String, diff --git a/src/crosvm/sys/unix/cmdline.rs b/src/crosvm/sys/unix/cmdline.rs index 31dcebb3c0..ce02e6c707 100644 --- a/src/crosvm/sys/unix/cmdline.rs +++ b/src/crosvm/sys/unix/cmdline.rs @@ -5,6 +5,7 @@ use std::path::PathBuf; use argh::FromArgs; +use cros_async::ExecutorKind; use devices::virtio::block::block::DiskOption; use devices::virtio::vhost::user::device; use devices::virtio::vhost::user::VhostUserParams; @@ -41,6 +42,10 @@ fn parse_vu_serial_options(s: &str) -> Result, #[argh(subcommand, name = "devices")] /// Start one or several jailed device processes. pub struct DevicesCommand { + /// configure async executor backend to "uring" or "epoll" (default). + #[argh(option, arg_name = "EXECUTOR")] + pub async_executor: Option, + #[argh(switch)] /// disable sandboxing. Will nullify the --jail option if it was present. pub disable_sandbox: bool, diff --git a/src/main.rs b/src/main.rs index 48f44b5028..da3c8be4e2 100644 --- a/src/main.rs +++ b/src/main.rs @@ -398,6 +398,11 @@ fn create_qcow2(cmd: cmdline::CreateQcow2Command) -> std::result::Result<(), ()> } fn start_device(opts: cmdline::DeviceCommand) -> std::result::Result<(), ()> { + if let Some(async_executor) = opts.async_executor { + cros_async::Executor::set_default_executor_kind(async_executor) + .map_err(|e| error!("Failed to set the default async executor: {:#}", e))?; + } + let result = match opts.command { cmdline::DeviceSubcommand::CrossPlatform(command) => match command { CrossPlatformDevicesCommands::Block(cfg) => run_block_device(cfg), @@ -588,11 +593,6 @@ fn crosvm_main>(args: I) -> Result ..Default::default() }; - if let Some(async_executor) = args.async_executor { - cros_async::Executor::set_default_executor_kind(async_executor) - .context("Failed to set the default async executor")?; - } - let ret = match args.command { Command::CrossPlatform(command) => { // Past this point, usage of exit is in danger of leaking zombie processes.