Re: [PATCH v11 2/2] usb: xhci: enable secondary interrupters and route transfers per slot

[Mathias Nyman <mathias.nyman@linux.intel.com>]

On 1/29/26 16:22, Michal Pecio wrote:
> On Wed, 28 Jan 2026 16:09:39 +0800, raoxu wrote:
>> By "hot spot" I do not mean a CPU cache problem. I mean a
>> software-side serialization point: all devices share a single event
>> ring and a single IRQ handler which processes completions under the
>> global xhci->lock. That centralizes the work into one place
>> regardless of cache locality.
>> [...]
>> Even if USB link latency is high, the IRQ/event path is still a
>> CPU-side serialization point: all completions land in one event ring
>> and are drained by one handler under xhci->lock. Under mixed
>> workloads (e.g. isoc video plus periodic audio/bulk), events from
>> unrelated devices can queue behind the same lock/handler and increase
>> jitter/tail latency, even if the bus is not saturated.
> 
> But this patch doesn't address such contetion. It's still one big lock
> per xHCI chip and a few CPUs take turns to do all work sequentially.
> 
> I find Greg's counterargument convincing. This change seems to only
> harm xhci_hcd performance, and if your bottleneck truly were in this
> driver, it's hard to imagine how anything could improve.

We would need more fine-grained xhci locking, and dynamically on-demand
created and removed secondary interrupters to get any real performance
benefit here.

I'm have another, virtualization and VTIO related interest in seeing this
working.
Having a per-device interrupter is one step forward in implementing
xHCI section 8 virtualization, allowing xHIC to pass a usb device over from
VM host to a VM guest

> 
> I suspect it's somewhere else, and considering the results you report,
> quite likely in URB completion handlers. They used to be called from
> this IRQ handler, but nowadays it's delegated to USB core.
> 
> Core uses BH workqueues. AFAIU this means completions run later on the
> same CPU which handled the IRQ. So if you spread IRQs, you also spread
> completions. It's easy to see how this could increase throughput or
> remedy latency spikes caused by sluggish class drivers, because unlike
> this IRQ, completions can usually run in parallel on multiple CPUs.

Sounds plausible. Let me see if I understood this correctly.

When xhci only has one interrupt it will process all transfer events from
all  devices in the interrupt handler in one go, with xhci->lock held for
a fairly long time.
During this processing the interrupt handler calls usb core to queue all
URB completions on the system_bh_wq (BH workqueue) on that same CPU.

This workqueue only stars processing URBs in softirq context, which
is _after_ completing xhci interrupt handler.

With several interrupts on different CPUs there will be fewer events to
handle per interrupt handler in one go with xhci->lock held.
There will also be several BH workqueues, one per CPU? which can start
earlier and have fewer URB completions per workqueue.

As a downside we will have several CPUs spinning in interupt context,
waiting for their turn to get the xhci->lock and handle the interrupt.

So we get a bit shorter URB completion latency traded for hogging more
overall system resources

> 
> If that's the real problem here then no xHCI driver surgery should be
> necessary to work around it, only updating the core.

Could be worth trying to reduce xHCI interrupt moderation interval.
A xHC interrupter can't trigger a new interrupr until 40 microseconds has
passed since previous interrupt completed (EHB bit cleared).
This might cause some tranfer event build up and thus latency.

Xu Rao, can I ask you you run the same test as before with only primary
interrupter with interrupt moderation the change below?

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 585b2f3117b0..9a2a4d17ed68 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -573,7 +573,7 @@ static int xhci_pci_setup(struct usb_hcd *hcd)
  	xhci = hcd_to_xhci(hcd);
  
  	/* imod_interval is the interrupt moderation value in nanoseconds. */
-	xhci->imod_interval = 40000;
+	xhci->imod_interval = 10000;
  
  	retval = xhci_gen_setup(hcd, xhci_pci_quirks);
  	if (retval)

> 
>> the upstream xhci-sideband work has already exercised and validated
>> the core multi-interrupter/event-ring plumbing in the driver. So
>> using additional interrupters is a proven in-tree mechanism, not an
>> experimental path unique to this series.
> 
> I'm afraid the only driver functionality exercised by xhci-sideband is
> allocation and freeing of extra event rings. Handling those rings in SW
> and solving race conditions is new, as is having no way to tell which
> order some events were generated in when things go wrong.

Agree, commands like 'stop endpoint' would trigger a transfer event on one cpu
and the command completion on another. PATCH V12 2/2 attempts to resolve
this by handling transfer events on the other interrupter before handling
specific command completions. This will get complicated and messy,
especially if we implement more fine-grained xhci locking.

The reward is too small compared to the risk of turning USB (more) unreliable.

Maybe we could add support for several interrupters but just use the
primary one as default.
Brave developers and testers could enable more interrupters via Kconfig,
debugfs, sysfs, or some other way.

Thanks
Mathias