Skip to content

avoid excessive Arc<Mapping> contention #1012

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
iximeow merged 2 commits into from
Jan 9, 2026
Merged

avoid excessive Arc<Mapping> contention #1012

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f9afddb
avoid excessive Arc<Mapping> contention
iximeow Jan 9, 2026
cf53f14
more clippy stuff
iximeow Jan 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions lib/propolis/src/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
}

Expand All @@ -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]
Expand All @@ -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]);
}

Expand Down
1 change: 0 additions & 1 deletion lib/propolis/src/hw/qemu/fwcfg.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>(
Expand Down
1 change: 0 additions & 1 deletion lib/propolis/src/util/regmap.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 link
Member Author

@iximeow iximeow Jan 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clippy complains about this now, which was surprising. I think what happened is, Backing used to have an implicit Drop because of Arc, no longer does, and the implicit Drop had made this ineligible for the lint before. this used to be necessary because the lifetime on ReadOp would have scratch held for mutable access when we get to write_bytes below. the lint is, then, not surprising in its relevance:

call to std::mem::drpo witha value that doe not implement Drop. Dropping such a type only extends its contained lifetimes.

copy_op.write_bytes(
&scratch[copy_op.offset()..(copy_op.offset() + copy_op.len())],
);
Expand Down
31 changes: 14 additions & 17 deletions lib/propolis/src/vmm/mem.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>),
}

Expand All @@ -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>,
Comment on lines -405 to -406
Copy link
Member Author

@iximeow iximeow Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MemCtx was here for the lifetime constraint, but Mapping is the backing structure that a SubMapping's lifetime "should" be derived from. MemCtx is just a container for all the mappings of a guest. taking &Mapping directly means we're constraining callers of SubMapping::new_base a bit more than they theoretically could have been.

if we ever care about this, the caller can Arc::clone and call new_base freely, 'cause it's pretty close to free now.

) -> 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,
Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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");

Expand Down Expand Up @@ -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());
Expand All @@ -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());
Expand All @@ -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());
Expand All @@ -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());
Expand All @@ -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());
Expand All @@ -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];
Expand Down
Loading