vss in 30 microseconds or less

Author: iximeow

Cargo.lock

@@ -180,13 +180,16 @@ zerocopy = "0.7.34"
 # It's common during development to use a local copy of various complex
 # dependencies.  If you want to use those, uncomment one of these blocks.
 #
-# [patch."https://github.com/oxidecomputer/omicron"]
-# internal-dns = { path = "../omicron/internal-dns" }
-# nexus-client = { path = "../omicron/clients/nexus-client" }
-# omicron-common = { path = "../omicron/common" }
-# oximeter-instruments = { path = "../omicron/oximeter/instruments" }
-# oximeter-producer = { path = "../omicron/oximeter/producer" }
-# oximeter = { path = "../omicron/oximeter/oximeter" }
+[patch."https://github.com/oxidecomputer/omicron"]
+#internal-dns = { path = "../omicron/internal-dns" }
+nexus-client = { path = "../omicron/clients/nexus-client" }
+omicron-uuid-kinds = { path = "../omicron/uuid-kinds" }
+nexus-types = { path = "../omicron/nexus/types" }
+illumos-utils = { path = "../omicron/illumos-utils" }
+omicron-common = { path = "../omicron/common" }
+oximeter-instruments = { path = "../omicron/oximeter/instruments" }
+oximeter-producer = { path = "../omicron/oximeter/producer" }
+oximeter = { path = "../omicron/oximeter/oximeter" }
 # [patch."https://github.com/oxidecomputer/crucible"]
 # crucible = { path = "../crucible/upstairs" }
 # crucible-client-types = { path = "../crucible/crucible-client-types" }

Cargo.toml

@@ -79,34 +79,47 @@ struct ServerStatsInner {
     /// data.
     virtual_machine: VirtualMachine,
 
+    /// The total amount of virtual address space reserved in support of this
+    /// virtual machine. This is retained to correct the process-wide virtual
+    /// address space size down to the "non-VM" amount; mappings not derived
+    /// from a virtual machine explicitly asking for some fixed size of memory.
+    vm_va_size: usize,
+
     /// The reset count for the relevant instance.
     run_count: virtual_machine::Reset,
 
-    /// The resident set size for the Propolis managing this instance, excluding
+    /// Mapped virtual memory for the Propolis managing this instance, excluding
     /// VM memory.
-    vmm_rss: virtual_machine::VmmMaxRss,
+    vmm_vss: virtual_machine::VmmVss,
 
     /// Process RSS is sampled in a standalone task. The `tokio::sync::watch`
-    /// here is to observe that sampling and update the `vmm_rss` tracked
+    /// here is to observe that sampling and update the `vmm_vss` tracked
     /// here.
     process_stats_rx: tokio::sync::watch::Receiver<process::ProcessStats>,
 }
 
 impl ServerStatsInner {
-    pub fn new(virtual_machine: VirtualMachine) -> Self {
+    pub fn new(virtual_machine: VirtualMachine, vm_va_size: usize) -> Self {
         let rx = process::ProcessStats::new();
 
         ServerStatsInner {
             virtual_machine,
+            vm_va_size,
             run_count: virtual_machine::Reset { datum: Default::default() },
-            vmm_rss: virtual_machine::VmmMaxRss { datum: Default::default() },
+            vmm_vss: virtual_machine::VmmVss { datum: Default::default() },
             process_stats_rx: rx,
         }
     }
 
-    fn refresh_vmm_rss(&mut self) {
+    fn refresh_vmm_vss(&mut self) {
         let last_process_stats = self.process_stats_rx.borrow();
-        self.vmm_rss.datum = last_process_stats.unshared_rss;
+        if last_process_stats.vss < self.vm_va_size {
+            eprintln!("process VSS is smaller than expected; is the VMM being torn down or already stopped?");
+            return;
+        }
+
+        self.vmm_vss.datum = (last_process_stats.vss - self.vm_va_size) as u64;
+        eprintln!("reporting vmm_vss of {}", self.vmm_vss.datum);
     }
 }
 
@@ -122,8 +135,8 @@ pub struct ServerStats {
 
 impl ServerStats {
     /// Create new server stats, representing the provided instance.
-    pub fn new(vm: VirtualMachine) -> Self {
-        Self { inner: Arc::new(Mutex::new(ServerStatsInner::new(vm))) }
+    pub fn new(vm: VirtualMachine, vm_va_size: usize) -> Self {
+        Self { inner: Arc::new(Mutex::new(ServerStatsInner::new(vm, vm_va_size))) }
     }
 
     /// Increments the number of times the managed instance was reset.
@@ -138,16 +151,16 @@ impl Producer for ServerStats {
     ) -> Result<Box<dyn Iterator<Item = Sample> + 'static>, MetricsError> {
         let samples = {
             let mut inner = self.inner.lock().unwrap();
-            inner.refresh_vmm_rss();
+            inner.refresh_vmm_vss();
             let run_count = std::iter::once(Sample::new(
                 &inner.virtual_machine,
                 &inner.run_count,
             )?);
-            let rss = std::iter::once(Sample::new(
+            let vss = std::iter::once(Sample::new(
                 &inner.virtual_machine,
-                &inner.vmm_rss,
+                &inner.vmm_vss,
             )?);
-            run_count.chain(rss)
+            run_count.chain(vss)
         };
         Ok(Box::new(samples))
     }

bin/propolis-server/src/lib/stats/mod.rs

@@ -12,10 +12,8 @@ use zerocopy::{AsBytes, FromBytes, FromZeroes};
 #[derive(Debug, Default)]
 pub(crate) struct ProcessStats {
     pub(crate) measurement_time: Duration,
-    pub(crate) unshared_rss: u64,
-    pub(crate) unshared_vss: u64,
-    pub(crate) shared_rss: u64,
-    pub(crate) shared_vss: u64,
+    pub(crate) rss: usize,
+    pub(crate) vss: usize,
 }
 
 impl ProcessStats {
@@ -28,112 +26,88 @@ impl ProcessStats {
     }
 }
 
-/// There are some arrays with size `PRMAPSZ` holding names in `struct prmap`
-/// and `struct prxmap`.
-const PRMAPSZ: usize = 64;
-
-/// We're going to query our `/proc/<self>/xmap` here, which contains a series o
-/// `struct prxmap` records. So, define it here in the same way it's defined in
-/// illumos' `common/sys/procfs.h`
+const PRFNSZ: usize = 16;
+const PRARGSZ: usize = 80;
+
+/// From `sys/types.h`
+#[allow(non_camel_case_types)]
+type taskid_t = i32;
+/// From `sys/time_impl.h`
+#[allow(non_camel_case_types)]
+type timespec_t = [i64; 2];
+/// From `sys/time_impl.h`
+#[allow(non_camel_case_types)]
+type timestruc_t = timespec_t;
+/// From `sys/types.h`
+#[allow(non_camel_case_types)]
+type dev_t = u64;
+
+/// `psinfo`'s definition depends on the data model reported by illumos. This is
+/// in line with the 64-bit version of the struct, and ignores a few fields at
+/// the end of the struct.
 #[derive(Copy, Clone, Debug, FromZeroes, AsBytes, FromBytes)]
-// Kind of weird to need packed here, but the struct size is 0x
+#[cfg(target_arch = "x86_64")]
 #[repr(C)]
-struct prxmap {
-    /// virtual address of the mapping
-    pr_vaddr: usize,
-    /// size of the mapping in bytes
+struct psinfo {
+    pr_flag: i32,
+    pr_nlwp: i32,
+    pr_pid: u32,
+    pr_ppid: u32,
+    pr_pgid: u32,
+    pr_sid: u32,
+    pr_uid: u32,
+    pr_euid: u32,
+    pr_gid: u32,
+    pr_egid: u32,
+    pr_addr: usize,
     pr_size: usize,
-    /// name in /proc/<pid>/object
-    pr_mapname: [u8; PRMAPSZ],
-    /// offset into mapped object, if any
-    pr_offset: i64,
-    /// protection and attribute flags. See procfs.h' `MA_*` flags
-    pr_mflags: i32,
-    /// Pagesize (bytes) for this mapping
-    pr_pagesize: i32,
-    /// SysV shmid, -1 if not SysV shared memory
-    pr_shmid: i32,
-    /// illumos' `struct prxmap` on 64-bit operating systems has implicit
-    /// internal padding between `pr_shmid` and `pr_dev`. This should always be
-    /// zero.
-    // Rust rules are that padding bytes are uninitialized, so transforming
-    // `&prxmap` to a `&[u8]` is UB if there is internal padding.
-    // Consequently, `AsBytes` and `FromBytes` require no internal padding, so
-    // put a small field here to make the "padding" explicit.
-    _internal_pad: i32,
-    /// st_dev from stat64() of mapped object, or PRNODEV
-    pr_dev: i64,
-    /// st_ino from stat64() of mapped object, if any
-    pr_ino: u64,
-    /// pages of resident memory
-    pr_rss: isize,
-    /// pages of resident anonymous memory
-    pr_anon: isize,
-    /// pages of locked memory
-    pr_locked: isize,
-    /// currently unused
-    pr_pad: isize,
-    /// pagesize of the hat mapping
-    pr_hatpagesize: usize,
-    /// filler for future expansion
-    // illumos uses `ulong_t` here, which should be 32-bit, but
-    // sizes only line up if it's u64?
-    pr_filler: [u64; 7],
+    pr_rssize: usize,
+    // From `struct psinfo`. This seems to be present to ensure that `pr_ttydev`
+    // is 64-bit aligned even on 32-bit targets.
+    pr_pad1: usize,
+    pr_ttydev: dev_t,
+    pr_pctcpu: u16,
+    pr_pctmem: u16,
+    // This padding is not explicitly present in illumos' `struct procfs`, but
+    // is none the less there due to C struct layout rules.
+    _pad2: u32,
+    pr_start: timestruc_t,
+    pr_time: timestruc_t,
+    pr_ctime: timestruc_t,
+    pr_fname: [u8; PRFNSZ],
+    pr_psargs: [u8; PRARGSZ],
+    pr_wstat: u32,
+    pr_argc: u32,
+    pr_argv: usize,
+    pr_envp: usize,
+    pr_dmodel: u8,
+    // More padding from `struct psinfo`.
+    pr_pad2: [u8; 3],
+    pr_taskid: taskid_t,
 }
 
-const MA_SHARED: i32 = 0x08;
+pub fn process_stats() -> anyhow::Result<ProcessStats> {
+    let mut psinfo_file = std::fs::File::open("/proc/self/psinfo")?;
 
-// Computed here just because it's used in a few places in this file.
-const PRXMAPSZ: usize = std::mem::size_of::<prxmap>();
+    let mut stats = ProcessStats {
+        measurement_time: Duration::ZERO,
+        vss: 0,
+        rss: 0,
+    };
 
-impl prxmap {
-    fn is_shared(&self) -> bool {
-        self.pr_mflags & MA_SHARED == MA_SHARED
-    }
-}
+    let mut info: psinfo = FromZeroes::new_zeroed();
 
-pub fn process_stats() -> anyhow::Result<ProcessStats> {
     let stats_read_start = Instant::now();
 
-    // Safety: getpid() does not alter any state, or even read mutable state.
-    let mypid = unsafe { libc::getpid() };
-    let xmap_path = format!("/proc/{}/xmap", mypid);
-    let mut xmap_file = std::fs::File::open(&xmap_path)?;
+    psinfo_file.read(info.as_bytes_mut())
+        .context("reading struct psinfo from file")?;
 
-    let mut stats = ProcessStats {
-        measurement_time: Duration::ZERO,
-        unshared_rss: 0,
-        unshared_vss: 0,
-        shared_rss: 0,
-        shared_vss: 0,
-    };
+    stats.measurement_time = stats_read_start.elapsed();
 
-    let mut buf: [prxmap; 256] = [FromZeroes::new_zeroed(); 256];
-    loop {
-        let nread = xmap_file
-            .read(buf.as_bytes_mut())
-            .context("reading struct prxmap from file")?;
-        if nread == 0 {
-            // we've read all maps this go around.
-            stats.measurement_time = stats_read_start.elapsed();
-            return Ok(stats);
-        }
-
-        assert!(nread % PRXMAPSZ == 0);
-        let maps_read = nread / PRXMAPSZ;
-
-        for buf in buf[0..maps_read].iter() {
-            let map_rss = buf.pr_rss as u64 * buf.pr_pagesize as u64;
-            let map_vss = buf.pr_size as u64;
-            if buf.is_shared() {
-                stats.shared_rss += map_rss;
-                stats.shared_vss += map_vss;
-            } else {
-                stats.unshared_rss += map_rss;
-                stats.unshared_vss += map_vss;
-            }
-        }
-    }
+    stats.vss = info.pr_size;
+    stats.rss = info.pr_rssize;
+
+    Ok(stats)
 }
 
 async fn process_stats_task(tx: watch::Sender<ProcessStats>) {
@@ -155,16 +129,3 @@ async fn process_stats_task(tx: watch::Sender<ProcessStats>) {
         tokio::time::sleep(std::time::Duration::from_millis(15000)).await;
     }
 }
-
-#[cfg(test)]
-mod test {
-    use super::prxmap;
-    use std::mem::size_of;
-
-    #[test]
-    fn prxmap_size() {
-        // Double check `prxmap` size against that of structures in
-        // /proc/<pid>/xmap..
-        assert_eq!(size_of::<prxmap>(), 0xd8);
-    }
-}

bin/propolis-server/src/lib/stats/process.rs

@@ -31,7 +31,7 @@ oximeter::use_timeseries!("virtual-machine.toml");
 pub use self::virtual_machine::Reset;
 // [this won't exist in CI until the oximeter schema is updated. hacked on this
 // locally with Cargo.toml patching.]
-pub use self::virtual_machine::VmmMaxRss;
+pub use self::virtual_machine::VmmVss;
 use self::virtual_machine::{
     VcpuUsage, VirtualMachine as VirtualMachineTarget,
 };

bin/propolis-server/src/lib/stats/virtual_machine.rs

@@ -67,6 +67,7 @@ impl VmServices {
                 cfg,
                 registry,
                 vm_objects.instance_spec(),
+                vm_objects.machine().map_physmem.virtual_address_size(),
                 vm_properties,
             )
             .await
@@ -119,6 +120,7 @@ async fn register_oximeter_producer(
     cfg: &MetricsEndpointConfig,
     registry: &ProducerRegistry,
     spec: &Spec,
+    vm_va_size: usize,
     vm_properties: &InstanceProperties,
 ) -> OximeterState {
     let mut oximeter_state = OximeterState::default();
@@ -152,7 +154,7 @@ async fn register_oximeter_producer(
     // Assign our own metrics production for this VM instance to the
     // registry, letting the server actually return them to oximeter when
     // polled.
-    let stats = ServerStats::new(virtual_machine);
+    let stats = ServerStats::new(virtual_machine, vm_va_size);
     if let Err(e) = registry.register_producer(stats.clone()) {
         error!(
             log,

bin/propolis-server/src/lib/vm/services.rs

@@ -184,9 +184,9 @@ fn safe_end(start: usize, len: usize) -> Option<usize> {
 // Flatten the K/V nested tuple
 fn kv_flatten<'a, T>(i: (&'a usize, &'a (usize, T))) -> SpaceItem<'a, T> {
     let start = *i.0;
-    let end = (i.1).0;
+    let size = (i.1).0;
     let item = &(i.1).1;
-    (start, end, item)
+    (start, size, item)
 }
 
 /// Iterator for all items in an [ASpace], constructed by [ASpace::iter].

lib/propolis/src/util/aspace.rs

@@ -197,6 +197,19 @@ impl PhysMap {
         let mut guard = self.map.lock().unwrap();
         guard.clear();
     }
+
+    /// Sum the sizes of virtual address ranges supporting segments of memory
+    /// for this VM.
+    pub fn virtual_address_size(&self) -> usize {
+        let guard = self.map.lock().unwrap();
+        let mut va_size = 0usize;
+
+        for (_start, size, _item) in guard.iter() {
+            va_size += size;
+        }
+
+        va_size
+    }
 }
 
 #[cfg(test)]