Skip to content

Commit 20a965d

Browse files
committed
rust_binder: avoid destructors in insert_or_update_handle()
The insert_or_update_handle() function currently has two places where it drops objects under the node_refs lock. In preparation for changing node_refs into a spinlock, update the code to either entirely remove the codepath or drop the node_refs lock first before running the destructor. This also has the side-benefit that we avoid traversing the by_node rbtree twice. Currently it's first traversed to see if the new node is present, and then traversed again to insert it. By saving the VacantEntry from the first lookup, we can perform the insertion without traversing the tree again. Reviewed-by: Matthew Maurer <mmaurer@google.com> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
1 parent 03e071a commit 20a965d

1 file changed

Lines changed: 14 additions & 8 deletions

File tree

drivers/android/binder/process.rs

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -861,14 +861,17 @@ impl Process {
861861
let handle = unused_id.as_u32();
862862

863863
// Do a lookup again as node may have been inserted before the lock was reacquired.
864-
if let Some(handle_ref) = refs.by_node.get(&node_ref.node.global_id()) {
865-
let handle = *handle_ref;
866-
let info = refs.by_handle.get_mut(&handle).unwrap();
867-
info.node_ref().absorb(node_ref);
868-
return Ok(handle);
869-
}
864+
let by_node_slot = match refs.by_node.entry(node_ref.node.global_id()) {
865+
rbtree::Entry::Vacant(by_node_slot) => by_node_slot,
866+
rbtree::Entry::Occupied(handle_ref) => {
867+
// The node was inserted by another thread while we didn't hold the lock.
868+
let handle = handle_ref.get();
869+
let info = refs.by_handle.get_mut(handle).unwrap();
870+
info.node_ref().absorb(node_ref);
871+
return Ok(*handle);
872+
}
873+
};
870874

871-
let gid = node_ref.node.global_id();
872875
let (info_proc, info_node) = {
873876
let info_init = NodeRefInfo::new(node_ref, handle, self.into());
874877
match info.pin_init_with(info_init) {
@@ -884,14 +887,17 @@ impl Process {
884887
// first thing in `deferred_release`, process cleanup will not miss the items inserted into
885888
// `refs` below.
886889
if self.inner.lock().is_dead {
890+
// Explicitly drop the lock so that `info_proc` and `info_node` are dropped outside of
891+
// the lock.
892+
drop(refs_lock);
887893
return Err(ESRCH);
888894
}
889895

890896
// SAFETY: `info_proc` and `info_node` reference the same node, so we are inserting
891897
// `info_node` into the right node's `refs` list.
892898
unsafe { info_proc.node_ref2().node.insert_node_info(info_node) };
893899

894-
refs.by_node.insert(reserve1.into_node(gid, handle));
900+
by_node_slot.insert(handle, reserve1);
895901
by_handle_slot.insert(info_proc, reserve2);
896902
unused_id.acquire();
897903
Ok(handle)

0 commit comments

Comments
 (0)