avoid excessive Arc<Mapping> contention (#1012)

Conceptually it wasn't wrong for submappings to `Arc::clone` the
pointed-to base mapping, but in practice all the accessors off of
MemCtx return SubMapping with lifetimes constrained by that MemCtx.

The sheer number of submappings being created and destroyed during I/O
operations makes the refcount very hot under load. Lean into the de
facto lifetime constraint on SubMapping and just take a borrow of the
backing Mapping instead of Arc::clone (and implicit ref decrement in
later drop).

Author: iximeow

lib/propolis/src/common.rs

@@ -527,22 +527,18 @@ mod test {
         let mut buf = [0u8; 8];
         let mut ro8 = ReadOp::from_buf(0, &mut buf[0..1]);
         ro8.write_u8(1);
-        drop(ro8);
         assert_eq!(buf, [1, 0, 0, 0, 0, 0, 0, 0]);
 
         let mut ro16 = ReadOp::from_buf(0, &mut buf[0..2]);
         ro16.write_u16(0x2000);
-        drop(ro16);
         assert_eq!(buf, [0, 0x20, 0, 0, 0, 0, 0, 0]);
 
         let mut ro32 = ReadOp::from_buf(0, &mut buf[0..4]);
         ro32.write_u32(0x4000_0000);
-        drop(ro32);
         assert_eq!(buf, [0, 0, 0, 0x40, 0, 0, 0, 0]);
 
         let mut ro64 = ReadOp::from_buf(0, &mut buf);
         ro64.write_u64(0x8000_0000_0000_0000);
-        drop(ro64);
         assert_eq!(buf, [0, 0, 0, 0, 0, 0, 0, 0x80]);
     }
 
@@ -551,19 +547,15 @@ mod test {
         let buf = [0x10, 0x20, 0x30, 0x40, 0x50, 0x60, 0x70, 0x80];
         let mut wo8 = WriteOp::from_buf(0, &buf[0..1]);
         assert_eq!(wo8.read_u8(), 0x10);
-        drop(wo8);
 
         let mut wo16 = WriteOp::from_buf(0, &buf[0..2]);
         assert_eq!(wo16.read_u16(), 0x2010);
-        drop(wo16);
 
         let mut wo32 = WriteOp::from_buf(0, &buf[0..4]);
         assert_eq!(wo32.read_u32(), 0x40302010);
-        drop(wo32);
 
         let mut wo64 = WriteOp::from_buf(0, &buf);
         assert_eq!(wo64.read_u64(), 0x8070605040302010);
-        drop(wo64);
     }
 
     #[test]
@@ -588,7 +580,6 @@ mod test {
         let mut ro = ReadOp::from_buf(0, &mut buf);
         ro.write_u8(0x10);
         ro.write_u8(0x20);
-        drop(ro);
         assert_eq!(buf, [0x10, 0x20]);
     }
 

lib/propolis/src/hw/qemu/fwcfg.rs

@@ -898,7 +898,6 @@ mod test {
         let mut val = T::default();
         let mut ro = ReadOp::from_buf(0, val.as_mut_bytes());
         dev.pio_read(port, &mut ro);
-        drop(ro);
         val
     }
     fn pio_read_data<T: IntoBytes + FromBytes + Copy + Default>(

lib/propolis/src/util/regmap.rs

@@ -130,7 +130,6 @@ impl<ID> RegMap<ID> {
             let mut sro = ReadOp::from_buf(0, &mut scratch);
 
             f(&reg.id, RWOp::Read(&mut sro));
-            drop(sro);
             copy_op.write_bytes(
                 &scratch[copy_op.offset()..(copy_op.offset() + copy_op.len())],
             );

lib/propolis/src/vmm/mem.rs

@@ -378,7 +378,7 @@ unsafe impl Sync for Mapping {}
 // not reference them directly as a field.
 #[allow(dead_code)]
 enum Backing<'a> {
-    Base(Arc<Mapping>),
+    Base(&'a Mapping),
     Sub(&'a SubMapping<'a>),
 }
 
@@ -401,12 +401,9 @@ pub struct SubMapping<'a> {
 impl SubMapping<'_> {
     /// Create `SubMapping` using the entire region offered by an underlying
     /// `Mapping` object.
-    fn new_base<'a>(
-        _mem: &'a MemCtx,
-        base: &'_ Arc<Mapping>,
-    ) -> SubMapping<'a> {
+    fn new_base<'a>(base: &'a Mapping) -> SubMapping<'a> {
         SubMapping {
-            backing: Backing::Base(base.clone()),
+            backing: Backing::Base(base),
 
             ptr: base.ptr,
             len: base.len,
@@ -427,7 +424,7 @@ impl SubMapping<'_> {
     }
 
     #[cfg(test)]
-    fn new_base_test<'a>(base: Arc<Mapping>) -> SubMapping<'a> {
+    fn new_base_test(base: &Mapping) -> SubMapping<'_> {
         let ptr = base.ptr;
         let len = base.len;
         let prot = base.prot;
@@ -1121,7 +1118,7 @@ impl MemCtx {
                     format!("memory region {} not found", name),
                 )
             })?;
-        Ok(SubMapping::new_base(self, ent).constrain_access(Prot::WRITE))
+        Ok(SubMapping::new_base(ent).constrain_access(Prot::WRITE))
     }
 
     /// Like `writable_region`, but accesses the underlying memory segment
@@ -1167,12 +1164,12 @@ impl MemCtx {
                 MapKind::MmioReserve => None,
             }?;
 
-            let guest_map = SubMapping::new_base(self, &seg.map_guest)
+            let guest_map = SubMapping::new_base(&seg.map_guest)
                 .constrain_access(prot)
                 .constrain_region(req_offset, len)
                 .expect("mapping offset should be valid");
 
-            let seg_map = SubMapping::new_base(self, &seg.map_seg)
+            let seg_map = SubMapping::new_base(&seg.map_seg)
                 .constrain_region(req_offset, len)
                 .expect("mapping offset should be valid");
 
@@ -1342,15 +1339,15 @@ pub mod test {
     #[test]
     fn mapping_denies_read_beyond_end() {
         let (_hdl, base) = test_setup(Prot::READ);
-        let mapping = SubMapping::new_base_test(base);
+        let mapping = SubMapping::new_base_test(&base);
 
         assert!(mapping.read::<[u8; TEST_LEN + 1]>().is_err());
     }
 
     #[test]
     fn mapping_shortens_read_bytes_beyond_end() {
         let (_hdl, base) = test_setup(Prot::READ);
-        let mapping = SubMapping::new_base_test(base);
+        let mapping = SubMapping::new_base_test(&base);
 
         let mut buf: [u8; TEST_LEN + 1] = [0; TEST_LEN + 1];
         assert_eq!(TEST_LEN, mapping.read_bytes(&mut buf).unwrap());
@@ -1359,7 +1356,7 @@ pub mod test {
     #[test]
     fn mapping_shortens_write_bytes_beyond_end() {
         let (_hdl, base) = test_setup(Prot::RW);
-        let mapping = SubMapping::new_base_test(base);
+        let mapping = SubMapping::new_base_test(&base);
 
         let mut buf: [u8; TEST_LEN + 1] = [0; TEST_LEN + 1];
         assert_eq!(TEST_LEN, mapping.write_bytes(&mut buf).unwrap());
@@ -1369,7 +1366,7 @@ pub mod test {
     fn mapping_create_empty() {
         let (_hdl, base) = test_setup(Prot::READ);
         let mapping =
-            SubMapping::new_base_test(base).constrain_region(0, 0).unwrap();
+            SubMapping::new_base_test(&base).constrain_region(0, 0).unwrap();
 
         assert_eq!(0, mapping.len());
         assert!(mapping.is_empty());
@@ -1378,7 +1375,7 @@ pub mod test {
     #[test]
     fn mapping_valid_subregions() {
         let (_hdl, base) = test_setup(Prot::READ);
-        let mapping = SubMapping::new_base_test(base);
+        let mapping = SubMapping::new_base_test(&base);
 
         assert!(mapping.subregion(0, 0).is_some());
         assert!(mapping.subregion(0, TEST_LEN / 2).is_some());
@@ -1388,7 +1385,7 @@ pub mod test {
     #[test]
     fn mapping_invalid_subregions() {
         let (_hdl, base) = test_setup(Prot::READ);
-        let mapping = SubMapping::new_base_test(base);
+        let mapping = SubMapping::new_base_test(&base);
 
         // Beyond the end of the mapping.
         assert!(mapping.subregion(TEST_LEN + 1, 0).is_none());
@@ -1402,7 +1399,7 @@ pub mod test {
     #[test]
     fn subregion_protection() {
         let (_hdl, base) = test_setup(Prot::RW);
-        let mapping = SubMapping::new_base_test(base);
+        let mapping = SubMapping::new_base_test(&base);
 
         // Main region has full access
         let mut buf = [0u8];