From a2ff484d6e3d490fc897ebbc68b8c2b68b91c74e Mon Sep 17 00:00:00 2001 From: Steve Karam Date: Thu, 27 Aug 2026 01:23:22 +0000 Subject: [PATCH] Don't hold pipeline mutex across a blocking operation On the external ingress path, the lock will only cover `process_packet`. A syscall inside the critical section descheduled the holder and every packet has wait during scheduler wakeup. --- lib/propolis/src/hw/virtio/softnpu.rs | 98 +++++++++++++-------------- 1 file changed, 48 insertions(+), 50 deletions(-) diff --git a/lib/propolis/src/hw/virtio/softnpu.rs b/lib/propolis/src/hw/virtio/softnpu.rs index 66546c543..fe64e085a 100644 --- a/lib/propolis/src/hw/virtio/softnpu.rs +++ b/lib/propolis/src/hw/virtio/softnpu.rs @@ -346,27 +346,29 @@ impl PciVirtioSoftNpuPort { } push_used = true; - let pkt = packet_in::new(&frame[..n]); + let mut pkt = packet_in::new(&frame[..n]); - let mut pipeline = match self.pipeline.lock() { - Ok(pipe) => pipe, - Err(e) => { - error!(self.log, "failed to lock pipeline: {}", e); - break; - } - }; - let pl: &mut Box = match &mut *pipeline { - Some(ref mut x) => &mut x.1, - None => { - // This just means no P4 program has been set by the guest. - break; + // As on the ingress path: process locked, forward unlocked. + let outs = { + let mut pipeline = match self.pipeline.lock() { + Ok(pipe) => pipe, + Err(e) => { + error!(self.log, "failed to lock pipeline: {}", e); + break; + } + }; + match &mut *pipeline { + Some(ref mut x) => x.1.process_packet(0, &mut pkt), + None => { + // This just means no P4 program has been set by the + // guest. + break; + } } }; - - PacketHandler::process_guest_packet( - pkt, + PacketHandler::forward_guest_packets( + outs, &self.data_handles, - pl, &self.log, ); } @@ -506,37 +508,32 @@ impl PacketHandler { // // TODO pipeline should not need to be mutable for packet handling? - let pkt = packet_in::new(&msg[..n]); - let mut p = pipeline.lock().unwrap(); - let pl = match &mut *p { - Some(ref mut pl) => &mut pl.1, - None => continue, // no program is loaded + let mut pkt = packet_in::new(&msg[..n]); + + // Process under the pipeline lock; forward with it dropped, so + // no syscall runs inside the critical section. + let outs = { + let mut p = pipeline.lock().unwrap(); + match &mut *p { + Some(ref mut pl) => { + pl.1.process_packet((index + 1) as u16, &mut pkt) + } + None => continue, // no program is loaded + } }; - - Self::process_external_packet( - index + 1, - pkt, - &data_handles, - &virtio, - pl, - &log, - ) + Self::forward_packets(outs, &data_handles, &virtio, &log) } } - /// Run a packet coming into the ASIC from an external port through the - /// loaded pipeline and forward it on to its destination. - fn process_external_packet( - index: usize, - mut pkt: packet_in<'_>, + /// Forward pipeline output for a packet that came in on an external port. + /// Runs with the pipeline lock dropped; see run_ingress_packet_handler. + fn forward_packets( + outs: Vec<(packet_out<'_>, u16)>, data_handles: &Vec, virtio: &Arc, - pipeline: &mut Box, log: &Logger, ) { - for (mut out_pkt, port) in - pipeline.process_packet(index as u16, &mut pkt) - { + for (mut out_pkt, port) in outs { // packet is going to CPU port if port == 0 { Self::send_packet_to_cpu_port(&mut out_pkt, virtio, &log); @@ -553,17 +550,15 @@ impl PacketHandler { } } - /// Run a packet coming into the ASIC from the guest pci port through the - /// loaded pipeline and forward it on to its destination. - fn process_guest_packet( - mut pkt: packet_in<'_>, + /// Forward pipeline output for a packet that came in on the guest pci + /// port. Runs with the pipeline lock dropped. + fn forward_guest_packets( + outs: Vec<(packet_out<'_>, u16)>, data_handles: &Vec, - pipeline: &mut Box, log: &Logger, ) { - for (mut out_pkt, port) in pipeline.process_packet(0, &mut pkt) { + for (mut out_pkt, port) in outs { if port == 0 { - // no looping packets back to the guest return; } if port == SOFTNPU_CPU_AUX_PORT { @@ -672,10 +667,9 @@ fn handle_management_message( radix: usize, log: Logger, ) { - let mut pl_opt = pipeline.lock().unwrap(); - match msg { ManagementRequest::TableAdd(tm) => { + let mut pl_opt = pipeline.lock().unwrap(); let pl = match &mut *pl_opt { Some(pl) => pl, None => return, @@ -689,6 +683,7 @@ fn handle_management_message( ); } ManagementRequest::TableRemove(tm) => { + let mut pl_opt = pipeline.lock().unwrap(); let pl = match &mut *pl_opt { Some(pl) => pl, None => return, @@ -714,7 +709,9 @@ fn handle_management_message( } ManagementRequest::DumpRequest => { info!(log, "dumping state"); + // Collect under the lock; write the reply with it dropped. let result = { + let mut pl_opt = pipeline.lock().unwrap(); let pl = match &mut *pl_opt { Some(pl) => &pl.1, None => return, @@ -726,7 +723,8 @@ fn handle_management_message( for id in pl.get_table_ids() { let entries = pl.get_table_entries(id); - result.insert(id, entries); + // Owned keys: the reply outlives the pipeline lock. + result.insert(id.to_string(), entries); } result };