RwLock::read for UFT hits, kstats use AtomicU64 (#636)

This PR replaces the per-`Port` `KMutex` with a `KRwLock`. This allows
ioctls to read port state without preventing packets from being served.
This also prevents successful UFT hits from blocking one another on the
port for any length of time.

A requirement is that we move to atomic accesses on kstat values, which
allows us to update kstats from outside a write-lock on the port.

Contention according to lockstat is solved, and
apparently the time spent in `xde_rx`/`xde_mc_tx` is reduced -- we're no
longer:

 * S2C -- the 3rd and 10th most spin-happy locks,
 * C2S -- the 2rd and 3rd most spin-happy locks.

After some refactoring, this has moved from 2.8->2.9 Gbps (pre-LSO) on
`glasgow`. Nor are we egregiously blocking on the port lock.

Closes #435.

Author: FelixMcFelix

crates/illumos-sys-hdrs/src/lib.rs

@@ -2,7 +2,7 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
-// Copyright 2024 Oxide Computer Company
+// Copyright 2025 Oxide Computer Company
 #![cfg_attr(feature = "kernel", feature(extern_types))]
 #![allow(non_camel_case_types)]
 #![no_std]
@@ -13,6 +13,10 @@ pub mod kernel;
 pub use kernel::*;
 
 use core::ptr;
+use core::sync::atomic::AtomicI32;
+use core::sync::atomic::AtomicI64;
+use core::sync::atomic::AtomicU32;
+use core::sync::atomic::AtomicU64;
 
 // The following are "C type" aliases for native Rust types so that
 // the native illumos structures may be defined almost verbatim to the
@@ -117,8 +121,27 @@ impl kstat_named_t {
         Self::default()
     }
 
-    pub fn val_u64(&self) -> u64 {
-        unsafe { self.value._u64 }
+    /// Validates at compile time whether `self._u64` can be safely used as
+    /// an AtomicU64.
+    const KSTAT_ATOMIC_U64_SAFE: () = if align_of::<KStatNamedValue>() % 8 == 0
+    {
+    } else {
+        panic!("Platform does not meet u64 8B alignment for AtomicU64");
+    };
+
+    /// Interprets the value of this named kstat as a `u64`.
+    ///
+    /// # SAFETY
+    /// Callers must ensure that `self` is interpreted as only *one* class of
+    /// atomic integer. Mixed-width atomic read/writes on the same kstat are
+    /// undefined behaviour.
+    #[inline(always)]
+    #[allow(clippy::let_unit_value)]
+    pub unsafe fn as_u64(&self) -> &AtomicU64 {
+        _ = Self::KSTAT_ATOMIC_U64_SAFE;
+        // SAFETY: KStatNamedValue must have 8B alignment on target platform.
+        //         Validated by compile time check in `KSTAT_ATOMIC_U64_SAFE`.
+        unsafe { &self.value._u64 }
     }
 }
 
@@ -132,33 +155,20 @@ impl Default for kstat_named_t {
     }
 }
 
+use core::mem::ManuallyDrop;
+
 #[repr(C)]
 pub union KStatNamedValue {
     _c: [c_char; 16],
-    _i32: i32,
-    _u32: u32,
-    _i64: i64,
-    _u64: u64,
-}
-
-impl core::ops::AddAssign<u64> for KStatNamedValue {
-    #[inline]
-    fn add_assign(&mut self, other: u64) {
-        unsafe { self._u64 += other };
-    }
-}
 
-impl core::ops::SubAssign<u64> for KStatNamedValue {
-    #[inline]
-    fn sub_assign(&mut self, other: u64) {
-        unsafe { self._u64 -= other };
-    }
-}
-
-impl KStatNamedValue {
-    pub fn set_u64(&mut self, val: u64) {
-        self._u64 = val;
-    }
+    // ManuallyDrop or Copy are required in a union (to guarantee
+    // no drop code runs).
+    // We know that atomic integer types have no `Drop` impl, so
+    // these are equivalent to the `Atomic` types.
+    _i32: ManuallyDrop<AtomicI32>,
+    _u32: ManuallyDrop<AtomicU32>,
+    _i64: ManuallyDrop<AtomicI64>,
+    _u64: ManuallyDrop<AtomicU64>,
 }
 
 pub const KSTAT_FLAG_VIRTUAL: c_int = 0x1;

lib/opte/src/ddi/kstat.rs

@@ -2,7 +2,7 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
-// Copyright 2023 Oxide Computer Company
+// Copyright 2025 Oxide Computer Company
 
 //! Export Rust structs as illumos kstats.
 //!
@@ -11,6 +11,8 @@ use alloc::boxed::Box;
 use alloc::string::String;
 use core::fmt;
 use core::fmt::Display;
+use core::sync::atomic::AtomicU64;
+use core::sync::atomic::Ordering;
 
 pub use kstat_macro::KStatProvider;
 
@@ -29,7 +31,7 @@ cfg_if! {
 ///
 /// An implementation of this trait acts as a provider of "named"
 /// kstats (i.e. `KSTAT_TYPE_NAMED` in `kstat_create(9F)`). The kstats
-/// are always virtual (`KSTAT_FLAG_VRITUAL`), meaning that the
+/// are always virtual (`KSTAT_FLAG_VIRTUAL`), meaning that the
 /// allocation of the kstat data is performed by the caller (Rust).
 ///
 /// Rather than implementing this trait manually, the kstat-macro
@@ -217,39 +219,18 @@ impl KStatU64 {
         Ok(())
     }
 
-    pub fn new() -> Self {
-        Self { inner: kstat_named_t::new() }
-    }
-
-    pub fn set(&mut self, val: u64) {
-        self.inner.value.set_u64(val);
-    }
-
-    pub fn val(&self) -> u64 {
-        self.inner.val_u64()
-    }
-}
-
-#[cfg(all(not(feature = "std"), not(test)))]
-impl core::ops::AddAssign<u64> for KStatU64 {
-    #[inline]
-    fn add_assign(&mut self, other: u64) {
-        self.inner.value += other;
-    }
-}
-
-#[cfg(all(not(feature = "std"), not(test)))]
-impl core::ops::SubAssign<u64> for KStatU64 {
-    #[inline]
-    fn sub_assign(&mut self, other: u64) {
-        self.inner.value -= other;
+    #[inline(always)]
+    fn inner(&self) -> &AtomicU64 {
+        // SAFETY: only `inner.as_u64` is called on this type, so mixed-width
+        // reads/writes are disallowed.
+        unsafe { self.inner.as_u64() }
     }
 }
 
 #[cfg(any(feature = "std", test))]
 #[derive(Default)]
 pub struct KStatU64 {
-    value: u64,
+    value: AtomicU64,
 }
 
 #[cfg(any(feature = "std", test))]
@@ -258,30 +239,45 @@ impl KStatU64 {
         Ok(())
     }
 
+    #[inline(always)]
+    fn inner(&self) -> &AtomicU64 {
+        &self.value
+    }
+}
+
+impl KStatU64 {
     pub fn new() -> Self {
         Self::default()
     }
 
-    pub fn set(&mut self, val: u64) {
-        self.value = val;
+    pub fn set(&self, val: u64) {
+        self.inner().store(val, Ordering::Relaxed)
     }
 
     pub fn val(&self) -> u64 {
-        self.value
+        self.inner().load(Ordering::Relaxed)
+    }
+
+    pub fn incr(&self, val: u64) {
+        self.inner().fetch_add(val, Ordering::Relaxed);
+    }
+
+    pub fn decr(&self, val: u64) {
+        self.inner().fetch_sub(val, Ordering::Relaxed);
     }
 }
 
-#[cfg(any(feature = "std", test))]
 impl core::ops::AddAssign<u64> for KStatU64 {
+    #[inline]
     fn add_assign(&mut self, other: u64) {
-        self.value += other;
+        self.incr(other);
     }
 }
 
-#[cfg(any(feature = "std", test))]
 impl core::ops::SubAssign<u64> for KStatU64 {
+    #[inline]
     fn sub_assign(&mut self, other: u64) {
-        self.value -= other;
+        self.decr(other);
     }
 }
 

lib/opte/src/engine/layer.rs

@@ -745,7 +745,7 @@ impl Layer {
 
         // Unwrap: We know this is fine because the stat names are
         // generated from the LayerStats structure.
-        let mut stats = KStatNamed::new(
+        let stats = KStatNamed::new(
             "xde",
             &format!("{}_{}", port, name),
             LayerStats::new(),