From dfc1e724b1ddaba44d0f6235b963f2bc6b5ab14a Mon Sep 17 00:00:00 2001 From: Yuuki Takano Date: Mon, 1 Jun 2026 17:56:10 +0900 Subject: [PATCH 1/3] feat(igc): enable multi-queue tx/rx Co-Authored-By: Claude Sonnet 4.6 --- awkernel_drivers/src/pcie/intel/igc.rs | 94 ++++++++++++++++---------- 1 file changed, 60 insertions(+), 34 deletions(-) diff --git a/awkernel_drivers/src/pcie/intel/igc.rs b/awkernel_drivers/src/pcie/intel/igc.rs index d8e8bbb02..5cd18aee2 100644 --- a/awkernel_drivers/src/pcie/intel/igc.rs +++ b/awkernel_drivers/src/pcie/intel/igc.rs @@ -189,6 +189,20 @@ pub struct Igc { } impl Igc { + fn service_queue(inner: &IgcInner, que_id: usize) -> Result<(), IgcDriverErr> { + { + let mut node = MCSNode::new(); + let mut rx = inner.queue_info.que[que_id].rx.lock(&mut node); + inner.igc_rx_recv(que_id, &mut rx)?; + } + + let mut node = MCSNode::new(); + let mut tx = inner.queue_info.que[que_id].tx.lock(&mut node); + inner.igc_txeof(que_id, &mut tx)?; + + Ok(()) + } + fn new(mut info: PCIeInfo) -> Result { use PCIeDeviceErr::InitFailure; @@ -265,21 +279,11 @@ impl Igc { let igc_icr = read_reg(&inner.info, igc_regs::IGC_ICR)?; let irq_queue = irq.and_then(|irq| inner.queue_info.irqs_to_queues.get(&irq).copied()); - let que_result = if let Some(que_id) = irq_queue { - let rx_result = { - let mut node = MCSNode::new(); - let mut rx = inner.queue_info.que[que_id].rx.lock(&mut node); - inner.igc_rx_recv(que_id, &mut rx) - }; - let tx_result = { - let mut node = MCSNode::new(); - let mut tx = inner.queue_info.que[que_id].tx.lock(&mut node); - inner.igc_txeof(&mut tx) - }; - rx_result.and(tx_result) - } else { - Ok(()) - }; + if let Some(que_id) = irq_queue { + Self::service_queue(&inner, que_id)?; + } + + let should_poll_link = irq.is_none() && (igc_icr & igc_defines::IGC_ICR_LSC) == 0; if (igc_icr & igc_defines::IGC_ICR_LSC) != 0 { // Link status change interrupt. @@ -289,7 +293,7 @@ impl Igc { inner.igc_intr_link()?; } inner = self.inner.read(); - } else if irq.is_none() { + } else if should_poll_link { drop(inner); { let mut inner = self.inner.write(); @@ -298,14 +302,22 @@ impl Igc { inner = self.inner.read(); } + if irq.is_none() { + for que_id in 0..inner.queue_info.que.len() { + Self::service_queue(&inner, que_id)?; + } + } + + let msix_linkmask = 1 << inner.queue_info.que.len(); + let msix_queuesmask = (1 << inner.queue_info.que.len()) - 1; write_reg(&inner.info, igc_regs::IGC_IMS, igc_defines::IGC_IMS_LSC)?; write_reg( &inner.info, igc_regs::IGC_EIMS, - 1 << inner.queue_info.que.len(), + msix_queuesmask | msix_linkmask, )?; - que_result + Ok(()) } } @@ -365,9 +377,15 @@ impl NetDevice for Igc { return false; } - let mut node = MCSNode::new(); - let mut tx = inner.queue_info.que[0].tx.lock(&mut node); - inner.igc_txeof(&mut tx).is_ok() && tx.igc_desc_unused() > 0 + for que_id in 0..inner.queue_info.que.len() { + let mut node = MCSNode::new(); + let mut tx = inner.queue_info.que[que_id].tx.lock(&mut node); + if inner.igc_txeof(que_id, &mut tx).is_ok() && tx.igc_desc_unused() > 0 { + return true; + } + } + + false } fn capabilities(&self) -> net_device::NetCapabilities { @@ -896,7 +914,7 @@ impl IgcInner { Ok(()) } - fn igc_txeof(&self, tx: &mut Tx) -> Result<(), IgcDriverErr> { + fn igc_txeof(&self, _que_id: usize, tx: &mut Tx) -> Result<(), IgcDriverErr> { membar_sync(); loop { @@ -925,10 +943,6 @@ impl IgcInner { que_id: usize, ether_frame: net_device::EtherFrameRef, ) -> Result<(), IgcDriverErr> { - if que_id != 0 { - return Err(IgcDriverErr::Param); - } - if !self.link_info.link_active { return Ok(()); } @@ -943,7 +957,7 @@ impl IgcInner { let mut node = MCSNode::new(); let mut tx = self.queue_info.que[que_id].tx.lock(&mut node); - self.igc_txeof(&mut tx)?; + self.igc_txeof(que_id, &mut tx)?; if tx.igc_desc_unused() == 0 { return Ok(()); @@ -984,8 +998,6 @@ impl IgcInner { } fn igc_rx_recv(&self, que_id: usize, rx: &mut Rx) -> Result<(), IgcDriverErr> { - debug_assert_eq!(que_id, 0, "multi-queue not yet supported (planned for PR5)"); - if rx.read_buf.is_none() { return Ok(()); } @@ -1258,6 +1270,22 @@ fn igc_is_valid_ether_addr(addr: &[u8; 6]) -> bool { !(addr[0] & 1 != 0 || addr.iter().all(|&x| x == 0)) } +fn igc_select_num_queues(available_vectors: usize) -> usize { + let cpu_count = match awkernel_lib::cpu::num_cpu() { + 0 => 4, + n => n, + }; + let available = core::cmp::min(core::cmp::min(available_vectors, cpu_count), 4); + + if available >= 4 { + 4 + } else if available >= 2 { + 2 + } else { + 1 + } +} + /// Allocate PCI resources for the IGC device. /// This function initialize IRQs for the IGC device, /// and returns IRQs for the Rx/Tx queues and an IRQ for events. @@ -1275,12 +1303,10 @@ fn igc_allocate_pci_resources(info: &mut PCIeInfo) -> Result<(Vec, IRQ), PC } let nmsix = nmsix - 1; // Give one vector to events. - - // Limit the driver to a single Rx/Tx queue for now. - let nqueues = core::cmp::min(nmsix, 1); + let nqueues = igc_select_num_queues(nmsix as usize); // Initialize the IRQs for the Rx/Tx queues. - let mut irqs_queues = Vec::with_capacity(nqueues as usize); + let mut irqs_queues = Vec::with_capacity(nqueues); for q in 0..nqueues { let irq_name_rxtx = format!("{DEVICE_SHORT_NAME}-{bdf}-RxTx{q}"); @@ -1292,7 +1318,7 @@ fn igc_allocate_pci_resources(info: &mut PCIeInfo) -> Result<(Vec, IRQ), PC }), segment_number, awkernel_lib::cpu::raw_cpu_id() as u32, - q as usize, + q, ) .or(Err(PCIeDeviceErr::InitFailure))?; irq_rxtx.enable(); From 1e4ba90d58196de6332fe92ff2ef002f201470f2 Mon Sep 17 00:00:00 2001 From: ytakano Date: Mon, 1 Jun 2026 18:41:42 +0900 Subject: [PATCH 2/3] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- awkernel_drivers/src/pcie/intel/igc.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/awkernel_drivers/src/pcie/intel/igc.rs b/awkernel_drivers/src/pcie/intel/igc.rs index 5cd18aee2..a3214585a 100644 --- a/awkernel_drivers/src/pcie/intel/igc.rs +++ b/awkernel_drivers/src/pcie/intel/igc.rs @@ -1271,10 +1271,7 @@ fn igc_is_valid_ether_addr(addr: &[u8; 6]) -> bool { } fn igc_select_num_queues(available_vectors: usize) -> usize { - let cpu_count = match awkernel_lib::cpu::num_cpu() { - 0 => 4, - n => n, - }; + let cpu_count = core::cmp::max(1, awkernel_lib::cpu::num_cpu()); let available = core::cmp::min(core::cmp::min(available_vectors, cpu_count), 4); if available >= 4 { From 524331378baf0fc4c2543484de004a675eb7b2b1 Mon Sep 17 00:00:00 2001 From: Yuuki Takano Date: Mon, 8 Jun 2026 12:57:34 +0900 Subject: [PATCH 3/3] fix(igc): address atsushi421's review comments on multi-queue PR - intr: accumulate errors from service_queue/link handlers instead of using `?` so EIMS/IMS re-arm writes always execute and the interrupt line is never left permanently masked - igc_send, igc_rx_recv: guard against out-of-range que_id with an early Err(Param) return to prevent index-out-of-bounds panic - can_send: remove igc_txeof call; descriptor reclaim is the ISR's responsibility and calling it here contends with the ISR TX lock path - igc_select_num_queues: add doc comment explaining the three constraints (MSI-X vectors, CPU count, hard cap of 4) and the rationale for power-of-two rounding for even RETA distribution - igc_txeof: add comment explaining why _que_id is unused Co-Authored-By: Claude Sonnet 4.6 --- awkernel_drivers/src/pcie/intel/igc.rs | 48 ++++++++++++++++++++++---- 1 file changed, 41 insertions(+), 7 deletions(-) diff --git a/awkernel_drivers/src/pcie/intel/igc.rs b/awkernel_drivers/src/pcie/intel/igc.rs index a3214585a..25da1e6fa 100644 --- a/awkernel_drivers/src/pcie/intel/igc.rs +++ b/awkernel_drivers/src/pcie/intel/igc.rs @@ -279,8 +279,15 @@ impl Igc { let igc_icr = read_reg(&inner.info, igc_regs::IGC_ICR)?; let irq_queue = irq.and_then(|irq| inner.queue_info.irqs_to_queues.get(&irq).copied()); + // Accumulate errors rather than short-circuiting with `?`: the EIMS/IMS re-arm + // writes below must execute regardless of queue-service failures, otherwise the + // interrupt line stays masked permanently. + let mut result: Result<(), IgcDriverErr> = Ok(()); + if let Some(que_id) = irq_queue { - Self::service_queue(&inner, que_id)?; + if let Err(e) = Self::service_queue(&inner, que_id) { + result = Err(e); + } } let should_poll_link = irq.is_none() && (igc_icr & igc_defines::IGC_ICR_LSC) == 0; @@ -290,21 +297,27 @@ impl Igc { drop(inner); { let mut inner = self.inner.write(); - inner.igc_intr_link()?; + if let Err(e) = inner.igc_intr_link() { + result = Err(e); + } } inner = self.inner.read(); } else if should_poll_link { drop(inner); { let mut inner = self.inner.write(); - inner.igc_poll_link()?; + if let Err(e) = inner.igc_poll_link() { + result = Err(e); + } } inner = self.inner.read(); } if irq.is_none() { for que_id in 0..inner.queue_info.que.len() { - Self::service_queue(&inner, que_id)?; + if let Err(e) = Self::service_queue(&inner, que_id) { + result = Err(e); + } } } @@ -317,7 +330,7 @@ impl Igc { msix_queuesmask | msix_linkmask, )?; - Ok(()) + result } } @@ -379,8 +392,8 @@ impl NetDevice for Igc { for que_id in 0..inner.queue_info.que.len() { let mut node = MCSNode::new(); - let mut tx = inner.queue_info.que[que_id].tx.lock(&mut node); - if inner.igc_txeof(que_id, &mut tx).is_ok() && tx.igc_desc_unused() > 0 { + let tx = inner.queue_info.que[que_id].tx.lock(&mut node); + if tx.igc_desc_unused() > 0 { return true; } } @@ -914,6 +927,9 @@ impl IgcInner { Ok(()) } + // _que_id is unused: descriptor reclaim is driven entirely by the DD writeback bit in + // descriptor memory; the hardware head register is not required. The parameter is + // retained so all queue-scoped helpers share a uniform signature. fn igc_txeof(&self, _que_id: usize, tx: &mut Tx) -> Result<(), IgcDriverErr> { membar_sync(); @@ -943,6 +959,10 @@ impl IgcInner { que_id: usize, ether_frame: net_device::EtherFrameRef, ) -> Result<(), IgcDriverErr> { + if que_id >= self.queue_info.que.len() { + return Err(IgcDriverErr::Param); + } + if !self.link_info.link_active { return Ok(()); } @@ -998,6 +1018,10 @@ impl IgcInner { } fn igc_rx_recv(&self, que_id: usize, rx: &mut Rx) -> Result<(), IgcDriverErr> { + if que_id >= self.queue_info.que.len() { + return Err(IgcDriverErr::Param); + } + if rx.read_buf.is_none() { return Ok(()); } @@ -1270,6 +1294,16 @@ fn igc_is_valid_ether_addr(addr: &[u8; 6]) -> bool { !(addr[0] & 1 != 0 || addr.iter().all(|&x| x == 0)) } +/// Select the number of Rx/Tx queue pairs to enable. +/// +/// The result is constrained by three factors: +/// 1. **MSI-X vectors**: one vector per queue plus one for link events. +/// 2. **CPU count**: no benefit in having more queues than CPUs. +/// 3. **Hardware cap**: IGC/I225 supports at most 4 RSS queues. +/// +/// The result is rounded down to the nearest power of two (1, 2, or 4) so that +/// the 128-entry RSS redirection table divides evenly among queues, giving each +/// queue an equal share of hashed flows. fn igc_select_num_queues(available_vectors: usize) -> usize { let cpu_count = core::cmp::max(1, awkernel_lib::cpu::num_cpu()); let available = core::cmp::min(core::cmp::min(available_vectors, cpu_count), 4);