Skip to content

Commit

Permalink
Fix unaligned pointer access issues
Browse files Browse the repository at this point in the history 
8-byte alignment was blowing up. But this was showing up only in docker and not in local runs.

Bunch of changes here to make sure that tuple slots are aligned only on 8-byte boundaries, plus some changes made along the way while diagnosing & fixing.
  • Loading branch information
@rdaum
rdaum committed Jan 6, 2024
1 parent 555787e commit c62d660
Show file tree
Hide file tree
Showing 14 changed files with 316 additions and 128 deletions.
62 changes: 57 additions & 5 deletions crates/db/src/tb_worldstate.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use tracing::warn;
use uuid::Uuid;

use crate::tuplebox::TupleError;
use moor_values::model::defset::HasUuid;
use moor_values::model::defset::{HasUuid, Named};
use moor_values::model::objects::{ObjAttrs, ObjFlag};
use moor_values::model::objset::ObjSet;
use moor_values::model::propdef::{PropDef, PropDefs};
Expand Down Expand Up @@ -1099,7 +1099,7 @@ mod tests {

use strum::{EnumCount, IntoEnumIterator};

use moor_values::model::defset::HasUuid;
use moor_values::model::defset::{HasUuid, Named};
use moor_values::model::objects::ObjAttrs;
use moor_values::model::objset::ObjSet;
use moor_values::model::r#match::VerbArgsSpec;
Expand Down Expand Up @@ -1135,7 +1135,7 @@ mod tests {
#[tokio::test]
async fn test_create_object() {
let db = test_db().await;
let tx = TupleBoxTransaction::new(db);
let tx = TupleBoxTransaction::new(db.clone());
let oid = tx
.create_object(
None,
Expand All @@ -1156,6 +1156,11 @@ mod tests {
assert_eq!(tx.get_object_location(oid).await.unwrap(), NOTHING);
assert_eq!(tx.get_object_name(oid).await.unwrap(), "test");
assert_eq!(tx.commit().await, Ok(CommitResult::Success));

// Verify existence in a new transaction.
let tx = TupleBoxTransaction::new(db);
assert!(tx.object_valid(oid).await.unwrap());
assert_eq!(tx.get_object_owner(oid).await.unwrap(), NOTHING);
}

#[tokio::test]
Expand Down Expand Up @@ -1607,7 +1612,7 @@ mod tests {
#[tokio::test]
async fn test_verb_add_update() {
let db = test_db().await;
let tx = TupleBoxTransaction::new(db);
let tx = TupleBoxTransaction::new(db.clone());
let oid = tx
.create_object(
None,
Expand Down Expand Up @@ -1658,6 +1663,13 @@ mod tests {
// resolve with the new name.
let vh = tx.resolve_verb(oid, "test2".into(), None).await.unwrap();
assert_eq!(vh.names(), vec!["test2"]);

// Now commit, and try to resolve again.
assert_eq!(tx.commit().await, Ok(CommitResult::Success));
let tx = TupleBoxTransaction::new(db);
let vh = tx.resolve_verb(oid, "test2".into(), None).await.unwrap();
assert_eq!(vh.names(), vec!["test2"]);
assert_eq!(tx.commit().await, Ok(CommitResult::Success));
}

#[tokio::test]
Expand Down Expand Up @@ -1797,7 +1809,7 @@ mod tests {
#[tokio::test]
async fn test_verb_resolve() {
let db = test_db().await;
let tx = TupleBoxTransaction::new(db);
let tx = TupleBoxTransaction::new(db.clone());

let a = tx
.create_object(
Expand Down Expand Up @@ -1847,6 +1859,46 @@ mod tests {
.unwrap()
.uuid();
assert_eq!(tx.get_verb_binary(a, v_uuid).await.unwrap(), vec![]);

// Add a second verb with a different name
tx.add_object_verb(
a,
a,
vec!["test2".into()],
vec![],
BinaryType::LambdaMoo18X,
BitEnum::new(),
VerbArgsSpec::this_none_this(),
)
.await
.unwrap();

// Verify we can get it
assert_eq!(
tx.resolve_verb(a, "test2".into(), None)
.await
.unwrap()
.names(),
vec!["test2"]
);
assert_eq!(tx.commit().await, Ok(CommitResult::Success));

// Verify existence in a new transaction.
let tx = TupleBoxTransaction::new(db);
assert_eq!(
tx.resolve_verb(a, "test".into(), None)
.await
.unwrap()
.names(),
vec!["test"]
);
assert_eq!(
tx.resolve_verb(a, "test2".into(), None)
.await
.unwrap()
.names(),
vec!["test2"]
);
assert_eq!(tx.commit().await, Ok(CommitResult::Success));
}

Expand Down
6 changes: 3 additions & 3 deletions crates/db/src/tuplebox/tuples/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,13 @@
use thiserror::Error;

pub use slotbox::{PageId, SlotBox, SlotBoxError, SlotId};
pub use tuple::TupleRef;
pub use tuple_ref::TupleRef;
pub use tx_tuple::TxTuple;

mod slot_ptr;
mod slotbox;
mod slotted_page;
mod tuple;
mod tuple_ptr;
mod tuple_ref;
mod tx_tuple;

#[derive(Debug, Clone, Copy, Eq, PartialEq, Hash)]
Expand Down
127 changes: 73 additions & 54 deletions crates/db/src/tuplebox/tuples/slotbox.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,11 @@ use thiserror::Error;
use tracing::error;

use crate::tuplebox::pool::{Bid, BufferPool, PagerError};
use crate::tuplebox::tuples::slot_ptr::SlotPtr;
pub use crate::tuplebox::tuples::slotted_page::SlotId;
use crate::tuplebox::tuples::slotted_page::{
slot_index_overhead, slot_page_empty_size, SlottedPage,
};
use crate::tuplebox::tuples::tuple_ptr::TuplePtr;
use crate::tuplebox::tuples::{TupleId, TupleRef};
use crate::tuplebox::RelationId;

Expand Down Expand Up @@ -104,11 +104,11 @@ impl SlotBox {
let mut refs = vec![];
for (slot, buflen, addr) in slot_ids.into_iter() {
let tuple_id = TupleId { page: id, slot };
let swizref = Box::pin(SlotPtr::create(self.clone(), tuple_id, addr, buflen));
let swizref = Box::pin(TuplePtr::create(self.clone(), tuple_id, addr, buflen));
inner.swizrefs.insert(tuple_id, swizref);
let swizref = inner.swizrefs.get_mut(&tuple_id).unwrap();
let sp = unsafe { Pin::into_inner_unchecked(swizref.as_mut()) };
let ptr = sp as *mut SlotPtr;
let ptr = sp as *mut TuplePtr;
let tuple_ref = TupleRef::at_ptr(ptr);
refs.push(tuple_ref);
}
Expand Down Expand Up @@ -214,7 +214,7 @@ struct Inner {
/// pointers in the TupleRefs themselves.
// TODO: This needs to be broken down by page id, too, so that we can manage swap-in/swap-out at
// the page granularity.
swizrefs: HashMap<TupleId, Pin<Box<SlotPtr>>>,
swizrefs: HashMap<TupleId, Pin<Box<TuplePtr>>>,
}

impl Inner {
Expand All @@ -236,46 +236,42 @@ impl Inner {
let tuple_size = size + slot_index_overhead();
let page_size = max(32768, tuple_size.next_power_of_two());

// Check if we have a free spot for this relation that can fit the tuple.
let (page, offset) =
{ self.find_space(relation_id, tuple_size, slot_page_empty_size(page_size))? };

let mut page_handle = self.page_for(page)?;

let free_space = page_handle.available_content_bytes();
let mut page_write_lock = page_handle.write_lock();
if let Ok((slot, page_remaining, mut buf)) = page_write_lock.allocate(size, initial_value) {
self.finish_alloc(page, relation_id, offset, page_remaining);

// Make a swizzlable ptr reference and shove it in our set, and then return a tuple ref
// which has a ptr to it.
let buflen = buf.as_ref().len();
let bufaddr = buf.as_mut_ptr();
let tuple_id = TupleId { page, slot };

// Heap allocate the swizref, and and pin it, take the address of it, then stick the swizref
// in our set.
let mut swizref = Box::pin(SlotPtr::create(sb.clone(), tuple_id, bufaddr, buflen));
let swizaddr = unsafe { swizref.as_mut().get_unchecked_mut() } as *mut SlotPtr;
self.swizrefs.insert(tuple_id, swizref);

// Establish initial refcount using this existing lock.
page_write_lock.upcount(slot).unwrap();

return Ok(TupleRef::at_ptr(swizaddr));
// Our selected page should not in theory get taken while we're holding this allocation lock,
// but to be paranoid, we'll loop around and try again if it does.
let mut tries = 0;
loop {
// Check if we have a free spot for this relation that can fit the tuple.
let (page, offset) =
{ self.find_space(relation_id, tuple_size, slot_page_empty_size(page_size))? };
let mut page_handle = self.page_for(page)?;
let mut page_write_lock = page_handle.write_lock();
if let Ok((slot, page_remaining, mut buf)) =
page_write_lock.allocate(size, initial_value)
{
self.finish_alloc(page, relation_id, offset, page_remaining);

// Make a swizzlable ptr reference and shove it in our set, and then return a tuple ref
// which has a ptr to it.
let buflen = buf.as_ref().len();
let bufaddr = buf.as_mut_ptr();
let tuple_id = TupleId { page, slot };

// Heap allocate the swizref, and and pin it, take the address of it, then stick the swizref
// in our set.
let mut swizref = Box::pin(TuplePtr::create(sb.clone(), tuple_id, bufaddr, buflen));
let swizaddr = unsafe { swizref.as_mut().get_unchecked_mut() } as *mut TuplePtr;
self.swizrefs.insert(tuple_id, swizref);

// Establish initial refcount using this existing lock.
page_write_lock.upcount(slot).unwrap();

return Ok(TupleRef::at_ptr(swizaddr));
}
tries += 1;
if tries > 50 {
panic!("Could not allocate tuple after {tries} tries");
}
}

// If we get here, then we failed to allocate on the page we wanted to, which means there's
// data coherence issues between the pages last-reported free space and the actual free
panic!(
"Page {} failed to allocate, we wanted {} bytes, but it only has {},\
but our records show it has {}, and its pid in that offset is {:?}",
page,
size,
free_space,
self.available_page_space[relation_id.0].entries[offset] >> 64,
self.available_page_space[relation_id.0].entries[offset] & 0xFFFF_FFFF_FFFF
);
}

fn do_restore_page<'a>(&mut self, id: PageId) -> Result<SlottedPage<'a>, SlotBoxError> {
Expand Down Expand Up @@ -601,8 +597,18 @@ mod tests {
tuples.push((tuple_id, tuple));
}
for (tuple, expected_value) in tuples {
let retrieved = tuple.slot_buffer();
assert_eq!(expected_value, retrieved.as_slice());
let retrieved_domain = tuple.slot_buffer();
let retrieved_codomain = tuple.slot_buffer();
assert_eq!(
expected_value,
retrieved_domain.as_slice(),
"Tuple domain mismatch"
);
assert_eq!(
expected_value,
retrieved_codomain.as_slice(),
"Tuple codomain mismatch"
);
}
}

Expand Down Expand Up @@ -645,10 +651,17 @@ mod tests {
let mut sb = Arc::new(SlotBox::new(32768 * 32));
let mut tuples = fill_until_full(&mut sb);
for (i, (tuple, expected_value)) in tuples.iter().enumerate() {
let retrieved = tuple.domain();
let retrieved_domain = tuple.domain();
let retrieved_codomain = tuple.codomain();
assert_eq!(
*expected_value,
retrieved_domain.as_slice(),
"Mismatch at {}th tuple",
i
);
assert_eq!(
*expected_value,
retrieved.as_slice(),
retrieved_codomain.as_slice(),
"Mismatch at {}th tuple",
i
);
Expand Down Expand Up @@ -698,8 +711,10 @@ mod tests {

// What we expected to still be there is there.
for (tuple, expected_value) in &tuples {
let retrieved = tuple.domain();
assert_eq!(*expected_value, retrieved.as_slice());
let retrieved_domain = tuple.domain();
let retrieved_codomain = tuple.codomain();
assert_eq!(*expected_value, retrieved_domain.as_slice());
assert_eq!(*expected_value, retrieved_codomain.as_slice());
}
// What we expected to not be there is not there.
for (id, _) in freed_tuples {
Expand All @@ -709,17 +724,21 @@ mod tests {
let new_tuples = fill_until_full(&mut sb);
// Verify both the new tuples and the old tuples are there.
for (tuple, expected) in new_tuples {
let retrieved = tuple.domain();
assert_eq!(expected, retrieved.as_slice());
let retrieved_domain = tuple.domain();
let retrieved_codomain = tuple.codomain();
assert_eq!(expected, retrieved_domain.as_slice());
assert_eq!(expected, retrieved_codomain.as_slice());
}
for (tuple, expected) in tuples {
let retrieved = tuple.domain();
assert_eq!(expected, retrieved.as_slice());
let retrieved_domain = tuple.domain();
let retrieved_codomain = tuple.codomain();
assert_eq!(expected, retrieved_domain.as_slice());
assert_eq!(expected, retrieved_codomain.as_slice());
}
}

#[test]
fn encode_decode() {
fn alloc_encode_decode() {
let pid = 12345;
let available = 54321;
let encoded = super::encode(pid, available);
Expand Down
Loading

0 comments on commit c62d660

Please sign in to comment.