Fix #450 - compute numa nodes and distances properly

Author: Lars T Hansen

Cargo.lock

@@ -1,6 +1,6 @@
 [package]
 name = "sonar"
-version = "0.16.0"
+version = "0.16.1-devel"
 edition = "2021"
 repository = "https://github.com/NordicHPC/sonar"
 

Cargo.toml

@@ -14,6 +14,11 @@ relied upon.  Consumers should expect to see the "new" JSON format for new data,
 should ask for that format.
 
 
+## Changes in v0.16.1 (on `release_0_16`)
+
+* Bug 450 - compute distances correctly to avoid erroring out when NUMA nodes are fewer than sockets;
+  expose `numa_nodes` field on sysinfo for good measure.
+
 ## Changes in v0.16.0 (on `release_0_16`)
 
 * Bug 335 / Bug 415 - **BREAKING CHANGE.** The `cpu_util` field was not emitted properly, it should

doc/CHANGELOG.md

@@ -314,6 +314,11 @@ Operating system version (the `release` field of `struct utsname`)
 
 Architecture name (the `machine` field of `struct utsname`)
 
+#### **`numa_nodes`** uint64
+
+Number of NUMA nodes (frequently smaller than the number of sockets), zero if we can't
+compute it
+
 #### **`sockets`** NonzeroUint
 
 Number of CPU sockets
@@ -337,20 +342,21 @@ Primary memory in kilobytes
 #### **`topo_svg`** string
 
 Base64-encoded text output (SVG source text) of `lstopo` or similar, using the base64
-alphabet from RFC4648.
+alphabet from RFC4648
 
 #### **`topo_text`** string
 
 Base64-encoded text output (human-readable) of `hwloc-ls` or similar, using the base64
-alphabet from RFC4648.
+alphabet from RFC4648
 
 #### **`cards`** []SysinfoGpuCard
 
 Per-card information
 
 #### **`distances`** [][]uint64
 
-Square matrix of standard NUMA node-to-node distances (normalized to 10 for self distance)
+Square matrix of standard NUMA node-to-node distances (normalized to 10 for self distance),
+empty if we can't compute it
 
 ### Type: `SysinfoGpuCard`
 

doc/NEW-FORMAT.md

@@ -101,6 +101,10 @@ SysinfoAttributes:
       type: 'NonemptyString'
       doc: >
         Architecture name (the `machine` field of `struct utsname`)
+    numa_nodes:
+      type: 'uint64'
+      doc: >
+        Number of NUMA nodes (frequently smaller than the number of sockets), zero if we can't compute it
     sockets:
       type: 'NonzeroUint'
       doc: >
@@ -124,19 +128,19 @@ SysinfoAttributes:
     topo_svg:
       type: 'string'
       doc: >
-        Base64-encoded text output (SVG source text) of `lstopo` or similar, using the base64 alphabet from RFC4648.
+        Base64-encoded text output (SVG source text) of `lstopo` or similar, using the base64 alphabet from RFC4648
     topo_text:
       type: 'string'
       doc: >
-        Base64-encoded text output (human-readable) of `hwloc-ls` or similar, using the base64 alphabet from RFC4648.
+        Base64-encoded text output (human-readable) of `hwloc-ls` or similar, using the base64 alphabet from RFC4648
     cards:
       type: '[]SysinfoGpuCard'
       doc: >
         Per-card information
     distances:
       type: '[][]uint64'
       doc: >
-        Square matrix of standard NUMA node-to-node distances (normalized to 10 for self distance)
+        Square matrix of standard NUMA node-to-node distances (normalized to 10 for self distance), empty if we can't compute it
 SysinfoGpuCard:
   fields:
     index:

doc/types.spec.yaml

@@ -31,6 +31,7 @@ pub const SYSINFO_ATTRIBUTES_NODE: &str = "node"; // Hostname
 pub const SYSINFO_ATTRIBUTES_OS_NAME: &str = "os_name"; // NonemptyString
 pub const SYSINFO_ATTRIBUTES_OS_RELEASE: &str = "os_release"; // NonemptyString
 pub const SYSINFO_ATTRIBUTES_ARCHITECTURE: &str = "architecture"; // NonemptyString
+pub const SYSINFO_ATTRIBUTES_NUMA_NODES: &str = "numa_nodes"; // uint64
 pub const SYSINFO_ATTRIBUTES_SOCKETS: &str = "sockets"; // NonzeroUint
 pub const SYSINFO_ATTRIBUTES_CORES_PER_SOCKET: &str = "cores_per_socket"; // NonzeroUint
 pub const SYSINFO_ATTRIBUTES_THREADS_PER_CORE: &str = "threads_per_core"; // NonzeroUint

src/json_tags.rs

@@ -2,7 +2,7 @@ use crate::gpu;
 use crate::gpu::mockgpu;
 use crate::jobsapi;
 use crate::json_tags;
-use crate::linux::procfs;
+use crate::linux::{procfs, system};
 use crate::systemapi;
 use crate::time;
 
@@ -15,6 +15,7 @@ use std::path;
 #[derive(Default)]
 pub struct Builder {
     proc_files: Option<HashMap<String, String>>,
+    node_files: Option<HashMap<String, String>>,
     pids: Option<Vec<(usize, u32)>>,
     threads: Option<HashMap<usize, Vec<(usize, u32)>>>,
     users: Option<HashMap<u32, String>>,
@@ -39,13 +40,22 @@ impl Builder {
         }
     }
 
+    // Files below /proc
     pub fn with_proc_files(self, files: HashMap<String, String>) -> Builder {
         Builder {
             proc_files: Some(files),
             ..self
         }
     }
 
+    // Files below /sys/devices/system/node
+    pub fn with_node_files(self, files: HashMap<String, String>) -> Builder {
+        Builder {
+            node_files: Some(files),
+            ..self
+        }
+    }
+
     pub fn with_pids(self, pids: Vec<(usize, u32)>) -> Builder {
         Builder {
             pids: Some(pids),
@@ -188,6 +198,7 @@ impl Builder {
                     threads,
                 }
             },
+            node_files: self.node_files.unwrap_or_default(),
             now: if let Some(x) = self.now {
                 x
             } else {
@@ -216,6 +227,7 @@ pub struct MockSystem {
     os_release: String,
     architecture: String,
     fs: MockFS,
+    node_files: HashMap<String, String>,
     gpus: mockgpu::MockGpuAPI,
     users: HashMap<u32, String>,
     pid: u32,
@@ -298,7 +310,7 @@ impl systemapi::SystemAPI for MockSystem {
     }
 
     fn get_numa_distances(&self) -> Result<Vec<Vec<u32>>, String> {
-        Ok(vec![vec![10u32]])
+        system::get_numa_distances(self)
     }
 
     fn compute_node_information(&self) -> Result<(u64, Vec<u64>), String> {
@@ -337,6 +349,13 @@ impl systemapi::SystemAPI for MockSystem {
         panic!("Not in use yet");
     }
 
+    fn read_node_file_to_string(&self, filename: &str) -> io::Result<String> {
+        match self.node_files.get(filename) {
+            Some(contents) => Ok(contents.clone()),
+            None => Err(io::Error::from(io::ErrorKind::NotFound)),
+        }
+    }
+
     fn run_sacct(
         &self,
         _job_states: &[&str],

src/linux/mocksystem.rs

@@ -207,28 +207,7 @@ impl systemapi::SystemAPI for System {
     }
 
     fn get_numa_distances(&self) -> Result<Vec<Vec<u32>>, String> {
-        let mut m = vec![];
-        for n in 0..self.get_cpu_info()?.sockets {
-            let filename = format!("/sys/devices/system/node/node{n}/distance");
-            match fs::read_to_string(path::Path::new(&filename)) {
-                Ok(s) => {
-                    let mut r = vec![];
-                    for v in s.trim().split(" ") {
-                        match v.parse::<u32>() {
-                            Ok(n) => r.push(n),
-                            Err(_) => {
-                                return Err(format!("Unable to parse {s}"));
-                            }
-                        }
-                    }
-                    m.push(r);
-                }
-                Err(_) => {
-                    return Err(format!("Unable to read {filename}"));
-                }
-            }
-        }
-        Ok(m)
+        get_numa_distances(self)
     }
 
     fn compute_node_information(&self) -> Result<(u64, Vec<u64>), String> {
@@ -267,6 +246,11 @@ impl systemapi::SystemAPI for System {
         fs::remove_file(p)
     }
 
+    fn read_node_file_to_string(&self, f: &str) -> io::Result<String> {
+        let filename = format!("/sys/devices/system/node/{f}");
+        fs::read_to_string(path::Path::new(&filename))
+    }
+
     fn run_sacct(
         &self,
         job_states: &[&str],
@@ -497,3 +481,44 @@ impl procfs::ProcfsAPI for RealProcFS {
         Ok(pids)
     }
 }
+
+pub fn get_numa_distances(system: &dyn systemapi::SystemAPI) -> Result<Vec<Vec<u32>>, String> {
+    let mut m = vec![];
+    // The number of NUMA nodes is no greater than the number of sockets but it is frequently
+    // smaller.  The rule here is that we enumerate the files and we will expect a dense range
+    // (even though numactl currently handles a sparse range), but if there's any kind of hiccup
+    // we fall back to returning an empty matrix, it is only if we can't parse a file that we
+    // could read that we return an error.
+    for n in 0..system.get_cpu_info()?.sockets {
+        match system.read_node_file_to_string(&format!("node{n}/distance")) {
+            Ok(s) => {
+                let mut r = vec![];
+                for v in s.trim().split(" ") {
+                    match v.parse::<u32>() {
+                        Ok(n) => r.push(n),
+                        Err(_) => {
+                            return Err(format!("Unable to parse {s}"));
+                        }
+                    }
+                }
+                m.push(r);
+            }
+            Err(_) => {
+                break
+            }
+        }
+    }
+    // Check that we have a square matrix.
+    if m.len() > 0 {
+        let n = m[0].len();
+        if m.len() != n {
+            return Ok(vec![]);
+        }
+        for i in 1..m.len() {
+            if m[i].len() != n {
+                return Ok(vec![]);
+            }
+        }
+    }
+    Ok(m)
+}

src/linux/system.rs

@@ -122,6 +122,7 @@ fn layout_sysinfo_newfmt(
     attrs.push_s(SYSINFO_ATTRIBUTES_NODE, node_info.node.clone());
     attrs.push_s(SYSINFO_ATTRIBUTES_OS_NAME, system.get_os_name());
     attrs.push_s(SYSINFO_ATTRIBUTES_OS_RELEASE, system.get_os_release());
+    attrs.push_u(SYSINFO_ATTRIBUTES_NUMA_NODES, node_info.numa_nodes);
     attrs.push_u(SYSINFO_ATTRIBUTES_SOCKETS, node_info.sockets);
     attrs.push_u(
         SYSINFO_ATTRIBUTES_CORES_PER_SOCKET,
@@ -326,6 +327,7 @@ struct NodeInfo {
     node: String,
     description: String,
     logical_cores: u64,
+    numa_nodes: u64,
     sockets: u64,
     cores_per_socket: u64,
     threads_per_core: u64,
@@ -409,6 +411,7 @@ fn compute_nodeinfo(system: &dyn systemapi::SystemAPI) -> Result<NodeInfo, Strin
             "{sockets}x{cores_per_socket}{ht} {model_name}, {mem_gb} GiB{gpu_desc}"
         ),
         logical_cores: (sockets * cores_per_socket * threads_per_core) as u64,
+        numa_nodes: distances.len() as u64,
         sockets: sockets as u64,
         cores_per_socket: cores_per_socket as u64,
         threads_per_core: threads_per_core as u64,

src/sysinfo.rs

@@ -14,23 +14,36 @@ use std::collections::HashMap;
 #[test]
 pub fn sysinfo_output_test() {
     // FIXME: Information leakage!!
-    let mut files = HashMap::new();
-    files.insert(
+    let mut proc_files = HashMap::new();
+    proc_files.insert(
         "cpuinfo".to_string(),
         std::include_str!("linux/testdata/cpuinfo-x86_64.txt").to_string(),
     );
-    files.insert(
+    proc_files.insert(
         "meminfo".to_string(),
         "MemTotal:       16093776 kB".to_string(),
     );
+    // Really we want something here where we have multiple nodes, but we *also* want sockets>nodes
+    // to test that bit.  However I don't want to change the test data on release_0_16 too much so
+    // for now let's be happy with sockets==nodes.
+    let mut node_files = HashMap::new();
+    node_files.insert(
+        "node0/distance".to_string(),
+        "10 21".to_string(),
+    );
+    node_files.insert(
+        "node1/distance".to_string(),
+        "21 10".to_string(),
+    );
     let system = mocksystem::Builder::new()
         .with_version("0.13.100")
         .with_timestamp("2025-02-11T08:47+01:00")
         .with_hostname("yes.no")
         .with_cluster("kain.uio.no")
         .with_os("CP/M", "2.2")
         .with_architecture("Z80")
-        .with_proc_files(files)
+        .with_proc_files(proc_files)
+        .with_node_files(node_files)
         .with_card(gpu::Card {
             bus_addr: "12:14:16".to_string(),
             device: gpu::Name {
@@ -97,13 +110,14 @@ pub fn sysinfo_output_test() {
 "node":"yes.no",
 "os_name":"CP/M",
 "os_release":"2.2",
+"numa_nodes":2,
 "sockets":2,
 "cores_per_socket":4,
 "threads_per_core":2,
 "cpu_model":"Intel(R) Xeon(R) CPU E5-2637 v4 @ 3.50GHz",
 "architecture":"Z80",
 "memory":16093776,
-"distances":[[10]],
+"distances":[[10,21],[21,10]],
 "cards":[
 {
 "index":0,