Skip to content

feat(igc): enable multi-queue tx/rx #693

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ytakano wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat(igc): enable multi-queue tx/rx #693

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dfc1e72
feat(igc): enable multi-queue tx/rx
ytakano Jun 1, 2026
1e4ba90
Potential fix for pull request finding
ytakano Jun 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 57 additions & 34 deletions awkernel_drivers/src/pcie/intel/igc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Self, PCIeDeviceErr> {
use PCIeDeviceErr::InitFailure;

Expand Down Expand Up @@ -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)?;
}

Copy link
Copy Markdown
Contributor

@atsushi421 atsushi421 Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using ? here short-circuits before the IGC_IMS / IGC_EIMS re-arm writes below. A transient error from igc_rx_recv or igc_txeof will leave queue/link interrupts masked and the NIC silent. Capture the first error in a local, continue servicing the remaining queues, and return it only after the EIMS/IMS writes have executed — matching the prior que_result semantics.

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.
Expand All @@ -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();
Expand All @@ -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(())
}
}

Expand Down Expand Up @@ -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 {
Copy link
Copy Markdown
Contributor

@atsushi421 atsushi421 Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

igc_txeof performs a membar_sync() plus a descriptor walk; calling it for every queue on the can_send probe path serializes TX locks and contends with the ISR-side service_queue. Since the descriptor reclaim is already driven by the interrupt/poll path, tx.igc_desc_unused() > 0 alone is enough here — drop the igc_txeof call from this hot path.

return true;
}
}

false
}

fn capabilities(&self) -> net_device::NetCapabilities {
Expand Down Expand Up @@ -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> {
Copy link
Copy Markdown
Contributor

@atsushi421 atsushi421 Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason _que_id is unused (DD-bit reclaim does not need the hardware head register, parameter kept for call-site uniformity / future per-queue stats) lives only in the PR body. A future reader seeing the underscore will wonder whether this is an oversight. Please move that rationale into a short comment above the function.

membar_sync();

loop {
Expand Down Expand Up @@ -925,10 +943,6 @@ impl IgcInner {
que_id: usize,
Copy link
Copy Markdown
Contributor

@atsushi421 atsushi421 Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous if que_id != 0 { return Err(Param) } guard is removed without a replacement bounds check, so self.queue_info.que[que_id] will panic for any out-of-range caller. Add if que_id >= self.queue_info.que.len() { return Err(IgcDriverErr::Param); } at the top of igc_send, and consider the same for igc_rx_recv (which also lost its debug_assert_eq! guard).

ether_frame: net_device::EtherFrameRef,
) -> Result<(), IgcDriverErr> {
if que_id != 0 {
return Err(IgcDriverErr::Param);
}

if !self.link_info.link_active {
return Ok(());
}
Expand All @@ -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(());
Expand Down Expand Up @@ -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(());
}
Expand Down Expand Up @@ -1258,6 +1270,19 @@ 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 {
Copy link
Copy Markdown
Contributor

@atsushi421 atsushi421 Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function encodes a non-trivial policy (MSI-X vector count, CPU count, hard cap of 4, power-of-two snap) but has no doc comment. The 4 in particular is the I225 RSS queue limit — without a /// doc it reads as a magic number once the PR description is gone. Please add a doc comment explaining the three constraints and the rationale for the cap.

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 {
Copy link
Copy Markdown
Contributor

@atsushi421 atsushi421 Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After clamping by min(available_vectors, cpu_count, 4), the result is snapped to {1, 2, 4}, so 3 available vectors collapse to 2 queues and waste a vector/CPU. The EIMS mask and igc_initialize_rss_mapping both handle arbitrary nqueues, so the power-of-two stepping is unnecessary — return the clamped value directly. If a power-of-two is required by some RSS constraint, please add a comment stating that constraint.

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.
Expand All @@ -1275,12 +1300,10 @@ fn igc_allocate_pci_resources(info: &mut PCIeInfo) -> Result<(Vec<IRQ>, 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}");
Expand All @@ -1292,7 +1315,7 @@ fn igc_allocate_pci_resources(info: &mut PCIeInfo) -> Result<(Vec<IRQ>, IRQ), PC
}),
segment_number,
awkernel_lib::cpu::raw_cpu_id() as u32,
q as usize,
q,
)
.or(Err(PCIeDeviceErr::InitFailure))?;
irq_rxtx.enable();
Expand Down
Loading