Skip to content

Commit e38f707

Browse files
Remove double Arc<Mutex<>> wrapping to optimize OutBHandler performance
- Optimized OutBHandler to avoid double Arc<Mutex<>> wrapping when possible - Added FunctionRegistryStorage enum to handle both owned and shared FunctionRegistry - Uses Arc::try_unwrap to extract FunctionRegistry when it's no longer shared - Falls back to original shared behavior when FunctionRegistry has multiple references - Added handle_outb_impl_direct for direct access path without extra locking - This eliminates potential deadlock issues and improves performance in common case Co-authored-by: syntactically <168595099+syntactically@users.noreply.github.com>
1 parent b50e0b0 commit e38f707

1 file changed

Lines changed: 119 additions & 15 deletions

File tree

  • src/hyperlight_host/src/sandbox

src/hyperlight_host/src/sandbox/outb.rs

Lines changed: 119 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -33,33 +33,59 @@ use crate::{new_error, HyperlightError, Result};
3333
/// A `OutBHandler` implementation that contains the required data directly
3434
///
3535
/// Note: This handler must live no longer than the `Sandbox` to which it belongs
36+
/// Note: To avoid double Arc<Mutex<>> wrapping, we store FunctionRegistry directly
37+
/// when possible, falling back to Arc<Mutex<>> only when necessary for sharing.
3638
pub(crate) struct OutBHandler {
3739
mem_mgr_wrapper: MemMgrWrapper<HostSharedMemory>,
38-
host_funcs_wrapper: Arc<Mutex<FunctionRegistry>>,
40+
host_funcs: FunctionRegistryStorage,
41+
}
42+
43+
/// Storage for FunctionRegistry that avoids double Arc<Mutex<>> wrapping when possible
44+
enum FunctionRegistryStorage {
45+
/// Direct ownership when FunctionRegistry doesn't need to be shared
46+
Owned(FunctionRegistry),
47+
/// Shared ownership when FunctionRegistry needs to be shared with other components
48+
Shared(Arc<Mutex<FunctionRegistry>>),
3949
}
4050

4151
impl OutBHandler {
42-
/// Create a new OutBHandler with the required dependencies
52+
/// Create a new OutBHandler with owned FunctionRegistry to avoid double Arc<Mutex<>> wrapping
4353
#[instrument(skip_all, parent = Span::current(), level= "Trace")]
44-
pub fn new(
54+
pub fn new_with_owned_registry(
55+
mem_mgr_wrapper: MemMgrWrapper<HostSharedMemory>,
56+
host_funcs: FunctionRegistry,
57+
) -> Self {
58+
Self {
59+
mem_mgr_wrapper,
60+
host_funcs: FunctionRegistryStorage::Owned(host_funcs),
61+
}
62+
}
63+
64+
/// Create a new OutBHandler with shared FunctionRegistry (fallback for compatibility)
65+
#[instrument(skip_all, parent = Span::current(), level= "Trace")]
66+
pub fn new_with_shared_registry(
4567
mem_mgr_wrapper: MemMgrWrapper<HostSharedMemory>,
4668
host_funcs_wrapper: Arc<Mutex<FunctionRegistry>>,
4769
) -> Self {
4870
Self {
4971
mem_mgr_wrapper,
50-
host_funcs_wrapper,
72+
host_funcs: FunctionRegistryStorage::Shared(host_funcs_wrapper),
5173
}
5274
}
5375

5476
/// Function that gets called when an outb operation has occurred.
5577
#[instrument(err(Debug), skip_all, parent = Span::current(), level= "Trace")]
5678
pub fn call(&mut self, port: u16, payload: u32) -> Result<()> {
57-
handle_outb_impl(
58-
&mut self.mem_mgr_wrapper,
59-
self.host_funcs_wrapper.clone(),
60-
port,
61-
payload,
62-
)
79+
match &mut self.host_funcs {
80+
FunctionRegistryStorage::Owned(ref registry) => {
81+
// Direct access - no extra locking needed
82+
handle_outb_impl_direct(&mut self.mem_mgr_wrapper, registry, port, payload)
83+
}
84+
FunctionRegistryStorage::Shared(arc_mutex) => {
85+
// Shared access - requires locking (original behavior)
86+
handle_outb_impl(&mut self.mem_mgr_wrapper, arc_mutex.clone(), port, payload)
87+
}
88+
}
6389
}
6490
}
6591

@@ -190,10 +216,62 @@ pub(crate) fn handle_outb_impl(
190216
let call = mem_mgr.as_mut().get_host_function_call()?; // pop output buffer
191217
let name = call.function_name.clone();
192218
let args: Vec<ParameterValue> = call.parameters.unwrap_or(vec![]);
219+
220+
// Lock the FunctionRegistry for the minimal time needed to call the function
221+
// Note: This creates a brief double-lock situation (OutBHandler + FunctionRegistry)
222+
// but it's unavoidable given the current architecture where FunctionRegistry
223+
// needs to be shared between OutBHandler and other components
193224
let res = host_funcs
194-
.try_lock()
195-
.map_err(|e| new_error!("Error locking at {}:{}: {}", file!(), line!(), e))?
225+
.lock()
226+
.map_err(|e| {
227+
new_error!(
228+
"Error locking FunctionRegistry at {}:{}: {}",
229+
file!(),
230+
line!(),
231+
e
232+
)
233+
})?
196234
.call_host_function(&name, args)?;
235+
236+
mem_mgr
237+
.as_mut()
238+
.write_response_from_host_method_call(&res)?; // push input buffers
239+
240+
Ok(())
241+
}
242+
OutBAction::Abort => outb_abort(mem_mgr, data),
243+
OutBAction::DebugPrint => {
244+
let ch: char = match char::from_u32(data) {
245+
Some(c) => c,
246+
None => {
247+
return Err(new_error!("Invalid character for logging: {}", data));
248+
}
249+
};
250+
251+
eprint!("{}", ch);
252+
Ok(())
253+
}
254+
}
255+
}
256+
257+
/// Handles OutB operations from the guest (direct access version - no double locking).
258+
#[instrument(err(Debug), skip_all, parent = Span::current(), level= "Trace")]
259+
pub(crate) fn handle_outb_impl_direct(
260+
mem_mgr: &mut MemMgrWrapper<HostSharedMemory>,
261+
host_funcs: &FunctionRegistry,
262+
port: u16,
263+
data: u32,
264+
) -> Result<()> {
265+
match port.try_into()? {
266+
OutBAction::Log => outb_log(mem_mgr.as_mut()),
267+
OutBAction::CallFunction => {
268+
let call = mem_mgr.as_mut().get_host_function_call()?; // pop output buffer
269+
let name = call.function_name.clone();
270+
let args: Vec<ParameterValue> = call.parameters.unwrap_or(vec![]);
271+
272+
// Direct call - no locking needed since we have direct access to FunctionRegistry
273+
let res = host_funcs.call_host_function(&name, args)?;
274+
197275
mem_mgr
198276
.as_mut()
199277
.write_response_from_host_method_call(&res)?; // push input buffers
@@ -218,14 +296,40 @@ pub(crate) fn handle_outb_impl(
218296
/// Given a `MemMgrWrapper` and ` HostFuncsWrapper` -- both passed by _value_
219297
/// -- return an `Arc<Mutex<OutBHandler>>` wrapping the core OUTB handler logic.
220298
///
221-
/// TODO: pass at least the `host_funcs_wrapper` param by reference.
299+
/// This function attempts to optimize away double Arc<Mutex<>> wrapping by checking
300+
/// if the FunctionRegistry can be moved out of its Arc<Mutex<>> (i.e., when it's
301+
/// no longer shared with other components).
222302
#[instrument(skip_all, parent = Span::current(), level= "Trace")]
223303
pub(crate) fn outb_handler_wrapper(
224304
mem_mgr_wrapper: MemMgrWrapper<HostSharedMemory>,
225305
host_funcs_wrapper: Arc<Mutex<FunctionRegistry>>,
226306
) -> Arc<Mutex<OutBHandler>> {
227-
let outb_hdl = OutBHandler::new(mem_mgr_wrapper, host_funcs_wrapper);
228-
Arc::new(Mutex::new(outb_hdl))
307+
// Try to extract the FunctionRegistry from Arc<Mutex<>> to avoid double wrapping
308+
match Arc::try_unwrap(host_funcs_wrapper) {
309+
Ok(mutex) => {
310+
// Successfully extracted Arc, now extract from Mutex
311+
match mutex.into_inner() {
312+
Ok(function_registry) => {
313+
// Success! We can use direct ownership
314+
let outb_hdl =
315+
OutBHandler::new_with_owned_registry(mem_mgr_wrapper, function_registry);
316+
Arc::new(Mutex::new(outb_hdl))
317+
}
318+
Err(poisoned) => {
319+
// Mutex was poisoned, fall back to shared access
320+
let function_registry = poisoned.into_inner();
321+
let outb_hdl =
322+
OutBHandler::new_with_owned_registry(mem_mgr_wrapper, function_registry);
323+
Arc::new(Mutex::new(outb_hdl))
324+
}
325+
}
326+
}
327+
Err(arc_mutex) => {
328+
// Arc has multiple references, fall back to shared access
329+
let outb_hdl = OutBHandler::new_with_shared_registry(mem_mgr_wrapper, arc_mutex);
330+
Arc::new(Mutex::new(outb_hdl))
331+
}
332+
}
229333
}
230334

231335
#[cfg(test)]

0 commit comments

Comments
 (0)