From 567cdb43b3cbc58f45fed900b139ccc41d6332ee Mon Sep 17 00:00:00 2001 From: Tim Zhang Date: Thu, 5 Nov 2020 11:18:39 +0800 Subject: [PATCH] Use Option as resource fields, remove the update switch: update_values Use idiomatic Option::None to represent optional fields. This enables updates where not all fields need to be specified. Signed-off-by: Tim Zhang --- src/blkio.rs | 51 +++++++++++++++++------------------- src/cgroup_builder.rs | 26 +++++------------- src/cpu.rs | 27 +++++-------------- src/cpuset.rs | 8 ++---- src/devices.rs | 12 ++++----- src/hugetlb.rs | 11 ++++---- src/lib.rs | 61 +++++++++++++++++++++++-------------------- src/memory.rs | 14 +++++----- src/net_cls.rs | 8 ++---- src/net_prio.rs | 6 ++--- src/pid.rs | 18 +++++-------- 11 files changed, 99 insertions(+), 143 deletions(-) diff --git a/src/blkio.rs b/src/blkio.rs index f648343..b0f78a2 100644 --- a/src/blkio.rs +++ b/src/blkio.rs @@ -344,39 +344,36 @@ impl ControllerInternal for BlkIoController { // get the resources that apply to this controller let res: &BlkIoResources = &res.blkio; - if res.update_values { - if let Some(weight) = res.weight { - let _ = self.set_weight(weight as u64); - } - if let Some(leaf_weight) = res.leaf_weight { - let _ = self.set_leaf_weight(leaf_weight as u64); - } + if let Some(weight) = res.weight { + let _ = self.set_weight(weight as u64); + } + if let Some(leaf_weight) = res.leaf_weight { + let _ = self.set_leaf_weight(leaf_weight as u64); + } - for dev in &res.weight_device { - if let Some(weight) = dev.weight { - let _ = self.set_weight_for_device(dev.major, dev.minor, weight as u64); - } - if let Some(leaf_weight) = dev.leaf_weight { - let _ = - self.set_leaf_weight_for_device(dev.major, dev.minor, leaf_weight as u64); - } + for dev in &res.weight_device { + if let Some(weight) = dev.weight { + let _ = self.set_weight_for_device(dev.major, dev.minor, weight as u64); } + if let Some(leaf_weight) = dev.leaf_weight { + let _ = self.set_leaf_weight_for_device(dev.major, dev.minor, leaf_weight as u64); + } + } - for dev in &res.throttle_read_bps_device { - let _ = self.throttle_read_bps_for_device(dev.major, dev.minor, dev.rate); - } + for dev in &res.throttle_read_bps_device { + let _ = self.throttle_read_bps_for_device(dev.major, dev.minor, dev.rate); + } - for dev in &res.throttle_write_bps_device { - let _ = self.throttle_write_bps_for_device(dev.major, dev.minor, dev.rate); - } + for dev in &res.throttle_write_bps_device { + let _ = self.throttle_write_bps_for_device(dev.major, dev.minor, dev.rate); + } - for dev in &res.throttle_read_iops_device { - let _ = self.throttle_read_iops_for_device(dev.major, dev.minor, dev.rate); - } + for dev in &res.throttle_read_iops_device { + let _ = self.throttle_read_iops_for_device(dev.major, dev.minor, dev.rate); + } - for dev in &res.throttle_write_iops_device { - let _ = self.throttle_write_iops_for_device(dev.major, dev.minor, dev.rate); - } + for dev in &res.throttle_write_iops_device { + let _ = self.throttle_write_iops_for_device(dev.major, dev.minor, dev.rate); } Ok(()) diff --git a/src/cgroup_builder.rs b/src/cgroup_builder.rs index 0432fa5..174b8ec 100644 --- a/src/cgroup_builder.rs +++ b/src/cgroup_builder.rs @@ -47,8 +47,8 @@ //! .limit("2G".to_string(), 2 * 1024 * 1024 * 1024) //! .done() //! .blkio() -//! .weight(Some(123)) -//! .leaf_weight(Some(99)) +//! .weight(123) +//! .leaf_weight(99) //! .weight_device(6, 1, Some(100), Some(55)) //! .weight_device(6, 1, Some(100), Some(55)) //! .throttle_iops() @@ -70,8 +70,7 @@ macro_rules! gen_setter { ($res:ident, $cont:ident, $func:ident, $name:ident, $ty:ty) => { /// See the similarly named function in the respective controller. pub fn $name(mut self, $name: $ty) -> Self { - self.cgroup.resources.$res.update_values = true; - self.cgroup.resources.$res.$name = $name; + self.cgroup.resources.$res.$name = Some($name); self } }; @@ -214,8 +213,7 @@ pub struct CpuResourceBuilder<'a> { } impl<'a> CpuResourceBuilder<'a> { - // FIXME this should all changed to options. - gen_setter!(cpu, CpuSetController, set_cpus, cpus, Option); + gen_setter!(cpu, CpuSetController, set_cpus, cpus, String); gen_setter!(cpu, CpuSetController, set_mems, mems, String); gen_setter!(cpu, CpuController, set_shares, shares, u64); gen_setter!(cpu, CpuController, set_cfs_quota, quota, i64); @@ -244,7 +242,6 @@ impl<'a> DeviceResourceBuilder<'a> { allow: bool, access: Vec, ) -> DeviceResourceBuilder<'a> { - self.cgroup.resources.devices.update_values = true; self.cgroup.resources.devices.devices.push(DeviceResource { major, minor, @@ -272,7 +269,6 @@ impl<'a> NetworkResourceBuilder<'a> { /// Set the priority of the tasks when operating on a networking device defined by `name` to be /// `priority`. pub fn priority(mut self, name: String, priority: u64) -> NetworkResourceBuilder<'a> { - self.cgroup.resources.network.update_values = true; self.cgroup .resources .network @@ -295,7 +291,6 @@ pub struct HugepagesResourceBuilder<'a> { impl<'a> HugepagesResourceBuilder<'a> { /// Limit the usage of certain hugepages (determined by `size`) to be at most `limit` bytes. pub fn limit(mut self, size: String, limit: u64) -> HugepagesResourceBuilder<'a> { - self.cgroup.resources.hugepages.update_values = true; self.cgroup .resources .hugepages @@ -317,14 +312,8 @@ pub struct BlkIoResourcesBuilder<'a> { } impl<'a> BlkIoResourcesBuilder<'a> { - gen_setter!(blkio, BlkIoController, set_weight, weight, Option); - gen_setter!( - blkio, - BlkIoController, - set_leaf_weight, - leaf_weight, - Option - ); + gen_setter!(blkio, BlkIoController, set_weight, weight, u16); + gen_setter!(blkio, BlkIoController, set_leaf_weight, leaf_weight, u16); /// Set the weight of a certain device. pub fn weight_device( @@ -334,7 +323,6 @@ impl<'a> BlkIoResourcesBuilder<'a> { weight: Option, leaf_weight: Option, ) -> BlkIoResourcesBuilder<'a> { - self.cgroup.resources.blkio.update_values = true; self.cgroup .resources .blkio @@ -362,7 +350,6 @@ impl<'a> BlkIoResourcesBuilder<'a> { /// Limit the read rate of the current metric for a certain device. pub fn read(mut self, major: u64, minor: u64, rate: u64) -> BlkIoResourcesBuilder<'a> { - self.cgroup.resources.blkio.update_values = true; let throttle = BlkIoDeviceThrottleResource { major, minor, rate }; if self.throttling_iops { self.cgroup @@ -382,7 +369,6 @@ impl<'a> BlkIoResourcesBuilder<'a> { /// Limit the write rate of the current metric for a certain device. pub fn write(mut self, major: u64, minor: u64, rate: u64) -> BlkIoResourcesBuilder<'a> { - self.cgroup.resources.blkio.update_values = true; let throttle = BlkIoDeviceThrottleResource { major, minor, rate }; if self.throttling_iops { self.cgroup diff --git a/src/cpu.rs b/src/cpu.rs index 9f34911..105a438 100644 --- a/src/cpu.rs +++ b/src/cpu.rs @@ -75,28 +75,15 @@ impl ControllerInternal for CpuController { // get the resources that apply to this controller let res: &CpuResources = &res.cpu; - if res.update_values { - let _ = self.set_shares(res.shares); - if self.shares()? != res.shares as u64 { - return Err(Error::new(ErrorKind::Other)); - } + update_and_test!(self, set_shares, res.shares, shares); + update_and_test!(self, set_cfs_period, res.period, cfs_period); + update_and_test!(self, set_cfs_quota, res.quota, cfs_quota); - let _ = self.set_cfs_period(res.period); - if self.cfs_period()? != res.period as u64 { - return Err(Error::new(ErrorKind::Other)); - } + res.attrs.iter().for_each(|(k, v)| { + let _ = self.set(k, v); + }); - let _ = self.set_cfs_quota(res.quota); - if self.cfs_quota()? != res.quota { - return Err(Error::new(ErrorKind::Other)); - } - - res.attrs.iter().for_each(|(k, v)| { - let _ = self.set(k, v); - }) - - // TODO: rt properties (CONFIG_RT_GROUP_SCHED) are not yet supported - } + // TODO: rt properties (CONFIG_RT_GROUP_SCHED) are not yet supported Ok(()) } diff --git a/src/cpuset.rs b/src/cpuset.rs index 28de1eb..4d4427d 100644 --- a/src/cpuset.rs +++ b/src/cpuset.rs @@ -110,12 +110,8 @@ impl ControllerInternal for CpuSetController { // get the resources that apply to this controller let res: &CpuResources = &res.cpu; - if res.update_values { - if res.cpus.is_some() { - let _ = self.set_cpus(res.cpus.as_ref().unwrap().as_str()); - } - let _ = self.set_mems(&res.mems); - } + update!(self, set_cpus, res.cpus.as_ref()); + update!(self, set_mems, res.mems.as_ref()); Ok(()) } diff --git a/src/devices.rs b/src/devices.rs index 605e735..5a78a2e 100644 --- a/src/devices.rs +++ b/src/devices.rs @@ -155,13 +155,11 @@ impl ControllerInternal for DevicesController { // get the resources that apply to this controller let res: &DeviceResources = &res.devices; - if res.update_values { - for i in &res.devices { - if i.allow { - let _ = self.allow_device(i.devtype, i.major, i.minor, &i.access); - } else { - let _ = self.deny_device(i.devtype, i.major, i.minor, &i.access); - } + for i in &res.devices { + if i.allow { + let _ = self.allow_device(i.devtype, i.major, i.minor, &i.access); + } else { + let _ = self.deny_device(i.devtype, i.major, i.minor, &i.access); } } diff --git a/src/hugetlb.rs b/src/hugetlb.rs index c65b4f4..d9e8d3c 100644 --- a/src/hugetlb.rs +++ b/src/hugetlb.rs @@ -54,14 +54,13 @@ impl ControllerInternal for HugeTlbController { // get the resources that apply to this controller let res: &HugePageResources = &res.hugepages; - if res.update_values { - for i in &res.limits { - let _ = self.set_limit_in_bytes(&i.size, i.limit); - if self.limit_in_bytes(&i.size)? != i.limit { - return Err(Error::new(Other)); - } + for i in &res.limits { + let _ = self.set_limit_in_bytes(&i.size, i.limit); + if self.limit_in_bytes(&i.size)? != i.limit { + return Err(Error::new(Other)); } } + Ok(()) } } diff --git a/src/lib.rs b/src/lib.rs index d1aa71f..9fe2bef 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -11,6 +11,25 @@ use std::fs::File; use std::io::{BufRead, BufReader, Read, Write}; use std::path::{Path, PathBuf}; +macro_rules! update_and_test { + ($self: ident, $set_func:ident, $value:expr, $get_func:ident) => { + if let Some(v) = $value { + $self.$set_func(v)?; + if $self.$get_func()? != v { + return Err(Error::new(Other)); + } + } + }; +} + +macro_rules! update { + ($self: ident, $set_func:ident, $value:expr) => { + if let Some(v) = $value { + let _ = $self.$set_func(v); + } + }; +} + pub mod blkio; pub mod cgroup; pub mod cgroup_builder; @@ -357,25 +376,23 @@ pub trait Hierarchy { /// Resource limits for the memory subsystem. #[derive(Debug, Clone, Eq, PartialEq, Default)] pub struct MemoryResources { - /// Whether values should be applied to the controller. - pub update_values: bool, /// How much memory (in bytes) can the kernel consume. - pub kernel_memory_limit: i64, + pub kernel_memory_limit: Option, /// Upper limit of memory usage of the control group's tasks. - pub memory_hard_limit: i64, + pub memory_hard_limit: Option, /// How much memory the tasks in the control group can use when the system is under memory /// pressure. - pub memory_soft_limit: i64, + pub memory_soft_limit: Option, /// How much of the kernel's memory (in bytes) can be used for TCP-related buffers. - pub kernel_tcp_memory_limit: i64, + pub kernel_tcp_memory_limit: Option, /// How much memory and swap together can the tasks in the control group use. - pub memory_swap_limit: i64, + pub memory_swap_limit: Option, /// Controls the tendency of the kernel to swap out parts of the address space of the tasks to /// disk. Lower value implies less likely. /// /// Note, however, that a value of zero does not mean the process is never swapped out. Use the /// traditional `mlock(2)` system call for that purpose. - pub swappiness: u64, + pub swappiness: Option, /// Customized key-value attributes /// /// # Usage: @@ -389,40 +406,36 @@ pub struct MemoryResources { /// Resources limits on the number of processes. #[derive(Debug, Clone, Eq, PartialEq, Default)] pub struct PidResources { - /// Whether values should be applied to the controller. - pub update_values: bool, /// The maximum number of processes that can exist in the control group. /// /// Note that attaching processes to the control group will still succeed _even_ if the limit /// would be violated, however forks/clones inside the control group will have with `EAGAIN` if /// they would violate the limit set here. - pub maximum_number_of_processes: MaxValue, + pub maximum_number_of_processes: Option, } /// Resources limits about how the tasks can use the CPU. #[derive(Debug, Clone, Eq, PartialEq, Default)] pub struct CpuResources { - /// Whether values should be applied to the controller. - pub update_values: bool, // cpuset /// A comma-separated list of CPU IDs where the task in the control group can run. Dashes /// between numbers indicate ranges. pub cpus: Option, /// Same syntax as the `cpus` field of this structure, but applies to memory nodes instead of /// processors. - pub mems: String, + pub mems: Option, // cpu /// Weight of how much of the total CPU time should this control group get. Note that this is /// hierarchical, so this is weighted against the siblings of this control group. - pub shares: u64, + pub shares: Option, /// In one `period`, how much can the tasks run in nanoseconds. - pub quota: i64, + pub quota: Option, /// Period of time in nanoseconds. - pub period: u64, + pub period: Option, /// This is currently a no-operation. - pub realtime_runtime: i64, + pub realtime_runtime: Option, /// This is currently a no-operation. - pub realtime_period: u64, + pub realtime_period: Option, /// Customized key-value attributes /// # Usage: /// ``` @@ -451,8 +464,6 @@ pub struct DeviceResource { /// Limit the usage of devices for the control group's tasks. #[derive(Debug, Clone, Eq, PartialEq, Default)] pub struct DeviceResources { - /// Whether values should be applied to the controller. - pub update_values: bool, /// For each device in the list, the limits in the structure are applied. pub devices: Vec, } @@ -470,12 +481,10 @@ pub struct NetworkPriority { /// control group. #[derive(Debug, Clone, Eq, PartialEq, Default)] pub struct NetworkResources { - /// Whether values should be applied to the controller. - pub update_values: bool, /// The networking class identifier to attach to the packets. /// /// This can then later be used in iptables and such to have special rules. - pub class_id: u64, + pub class_id: Option, /// Priority of the egress traffic for each interface. pub priorities: Vec, } @@ -493,8 +502,6 @@ pub struct HugePageResource { /// Provides the ability to set consumption limit on each type of hugepages. #[derive(Debug, Clone, Eq, PartialEq, Default)] pub struct HugePageResources { - /// Whether values should be applied to the controller. - pub update_values: bool, /// Set a limit of consumption for each hugepages type. pub limits: Vec, } @@ -526,8 +533,6 @@ pub struct BlkIoDeviceThrottleResource { /// General block I/O resource limits. #[derive(Debug, Clone, Eq, PartialEq, Default)] pub struct BlkIoResources { - /// Whether values should be applied to the controller. - pub update_values: bool, /// The weight of the control group against descendant nodes. pub weight: Option, /// The weight of the control group against sibling nodes. diff --git a/src/memory.rs b/src/memory.rs index 0e3ea8c..3b48289 100644 --- a/src/memory.rs +++ b/src/memory.rs @@ -453,14 +453,12 @@ impl ControllerInternal for MemController { // get the resources that apply to this controller let memres: &MemoryResources = &res.memory; - if memres.update_values { - let _ = self.set_limit(memres.memory_hard_limit); - let _ = self.set_soft_limit(memres.memory_soft_limit); - let _ = self.set_kmem_limit(memres.kernel_memory_limit); - let _ = self.set_memswap_limit(memres.memory_swap_limit); - let _ = self.set_tcp_limit(memres.kernel_tcp_memory_limit); - let _ = self.set_swappiness(memres.swappiness); - } + update!(self, set_limit, memres.memory_hard_limit); + update!(self, set_soft_limit, memres.memory_soft_limit); + update!(self, set_kmem_limit, memres.kernel_memory_limit); + update!(self, set_memswap_limit, memres.memory_swap_limit); + update!(self, set_tcp_limit, memres.kernel_tcp_memory_limit); + update!(self, set_swappiness, memres.swappiness); Ok(()) } diff --git a/src/net_cls.rs b/src/net_cls.rs index 860ae8d..ad2af39 100644 --- a/src/net_cls.rs +++ b/src/net_cls.rs @@ -47,12 +47,8 @@ impl ControllerInternal for NetClsController { // get the resources that apply to this controller let res: &NetworkResources = &res.network; - if res.update_values { - let _ = self.set_class(res.class_id); - if self.get_class()? != res.class_id { - return Err(Error::new(Other)); - } - } + update_and_test!(self, set_class, res.class_id, get_class); + return Ok(()); } } diff --git a/src/net_prio.rs b/src/net_prio.rs index ed16ed9..3728796 100644 --- a/src/net_prio.rs +++ b/src/net_prio.rs @@ -48,10 +48,8 @@ impl ControllerInternal for NetPrioController { // get the resources that apply to this controller let res: &NetworkResources = &res.network; - if res.update_values { - for i in &res.priorities { - let _ = self.set_if_prio(&i.name, i.priority); - } + for i in &res.priorities { + let _ = self.set_if_prio(&i.name, i.priority); } Ok(()) diff --git a/src/pid.rs b/src/pid.rs index d08ba30..e9c4869 100644 --- a/src/pid.rs +++ b/src/pid.rs @@ -50,17 +50,13 @@ impl ControllerInternal for PidController { // get the resources that apply to this controller let pidres: &PidResources = &res.pid; - if pidres.update_values { - // apply pid_max - let _ = self.set_pid_max(pidres.maximum_number_of_processes); - - // now, verify - if self.get_pid_max()? == pidres.maximum_number_of_processes { - return Ok(()); - } else { - return Err(Error::new(Other)); - } - } + // apply pid_max + update_and_test!( + self, + set_pid_max, + pidres.maximum_number_of_processes, + get_pid_max + ); Ok(()) }