Re: usb: xhci: add optional MSI-X interrupters for isoc and bulk

Re: usb: xhci: add optional MSI-X interrupters for isoc and bulk

[Michal Pecio]

Hi,

USB patches should be copied to linux-usb@vger.kernel.org,
please run ./scripts/get_maintainer on submitted patches.


On Wed,  3 Dec 2025 18:08:24 +0800, raoxu wrote:
> From: Xu Rao <raoxu@uniontech.com>
> 
> Some xHCI controllers expose multiple MSI-X vectors (e.g. AMD 1022:15e0
> with 8 vectors) but the driver still routes all traffic through the
> primary interrupter. In mixed isochronous and bulk workloads this means
> all completions share a single IRQ, leading to higher context switching
> and CPU migrations.
> 
> This patch introduces an optional quirk, XHCI_BULK_ISOC_SECONDARY_IRQ,
> that allows the driver to allocate dedicated interrupters for isoc and
> bulk endpoints when the host supports at least three interrupters /
> MSI-X vectors. URBs are routed to these interrupters via TRB_INTR_TARGET()
> and the corresponding event rings, so that isoc and bulk traffic can
> interrupt on different CPUs.
> 
> The implementation is generic and can in principle be used on any xHCI
> host that supports multiple interrupters. For now the new quirk is only
> enabled for the AMD 1022:15e0 controller to be conservative and to
> limit the initial behavioral change to this tested device. Other
> multi-MSI-X controllers could be opted in later by setting the same
> quirk or by introducing a more generic policy.

That's apparently one generation of AMD APUs from 7 years ago, so it's
a little weird to single it out. If the change really is an improvement
it would make sense to apply it on all capable hardware.

This quirk ensures that the code will get little use in the field and
most likely exactly zero testing by developers in the future.

> The single primary-interrupt path is preserved:
> 
>   - If the quirk is not set, the driver behaves exactly as before and
>     only uses interrupter[0].
>   - If MSI/MSI-X setup fails, or fewer than three vectors are available,
>     the driver clears the isoc/bulk interrupter indices and falls back
>     to interrupter[0] for all traffic.
>   - When MSI-X is disabled (e.g. INTx or single MSI), TRB_INTR_TARGET()
>     is forced to 0 so all events still go through the primary
>     interrupter and existing logic is unchanged.
> 
> With the quirk enabled on an AMD 1022:15e0 xHCI controller
> (03:00.3, MSI-X Count=8), and a workload that runs a USB camera
> (isochronous) and a USB flash drive (bulk) simultaneously, the secondary
> MSI-X vectors are used as follows:
> 
> cat /proc/interrupts | grep xhci
>  32:          0          0          0      11917 IR-PCI-MSIX-0000:03:00.3
> 						0-edge     xhci_hcd
>  33:     267794          0          0          0 IR-PCI-MSIX-0000:03:00.3
> 						1-edge     xhci_hcd
>  34:          0      33441          0          0 IR-PCI-MSIX-0000:03:00.3
> 						2-edge     xhci_hcd
> 
> Vector 32 is the primary interrupter, while vectors 33 and 34 serve as
> dedicated secondary interrupters for isoc and bulk traffic.
> 
> perf stat over 30 seconds, with the USB camera and USB flash drive both
> active, shows the following system-wide numbers:
> 
>   With dedicated MSI-X interrupters for isoc and bulk:
>     perf stat -a -e context-switches,cpu-migrations -- sleep 30
> 
>       context-switches:        389,068
>       cpu-migrations :         18,665
> 
>   Original single-primary-interrupt case:
>     perf stat -a -e context-switches,cpu-migrations -- sleep 30
> 
>       context-switches:        509,633
>       cpu-migrations :         42,644
> 
> In this workload, using separate MSI-X interrupters for isoc and bulk
> reduces cpu-migrations by roughly 56% and context-switches by about 24%
> compared to routing all traffic through the primary interrupter.

What's the practical consequence of that?

Some isochronous users may care about latency (maybe interrupt too),
but is it important to separate bulk from control for example?

Regards,
Michal

Re: usb: xhci: add optional MSI-X interrupters for isoc and bulk

[Mathias Nyman]

Hi

(adding linux-usb mailing list)

On 12/3/25 12:08, raoxu wrote:
> From: Xu Rao <raoxu@uniontech.com>
> 
> Some xHCI controllers expose multiple MSI-X vectors (e.g. AMD 1022:15e0
> with 8 vectors) but the driver still routes all traffic through the
> primary interrupter. In mixed isochronous and bulk workloads this means
> all completions share a single IRQ, leading to higher context switching
> and CPU migrations.
> 
> This patch introduces an optional quirk, XHCI_BULK_ISOC_SECONDARY_IRQ,
> that allows the driver to allocate dedicated interrupters for isoc and
> bulk endpoints when the host supports at least three interrupters /
> MSI-X vectors. URBs are routed to these interrupters via TRB_INTR_TARGET()
> and the corresponding event rings, so that isoc and bulk traffic can
> interrupt on different CPUs.
> 
> The implementation is generic and can in principle be used on any xHCI
> host that supports multiple interrupters. For now the new quirk is only
> enabled for the AMD 1022:15e0 controller to be conservative and to
> limit the initial behavioral change to this tested device. Other
> multi-MSI-X controllers could be opted in later by setting the same
> quirk or by introducing a more generic policy.
> 
> The single primary-interrupt path is preserved:
> 
>    - If the quirk is not set, the driver behaves exactly as before and
>      only uses interrupter[0].
>    - If MSI/MSI-X setup fails, or fewer than three vectors are available,
>      the driver clears the isoc/bulk interrupter indices and falls back
>      to interrupter[0] for all traffic.
>    - When MSI-X is disabled (e.g. INTx or single MSI), TRB_INTR_TARGET()
>      is forced to 0 so all events still go through the primary
>      interrupter and existing logic is unchanged.
> 
> With the quirk enabled on an AMD 1022:15e0 xHCI controller
> (03:00.3, MSI-X Count=8), and a workload that runs a USB camera
> (isochronous) and a USB flash drive (bulk) simultaneously, the secondary
> MSI-X vectors are used as follows:
> 
> cat /proc/interrupts | grep xhci
>   32:          0          0          0      11917 IR-PCI-MSIX-0000:03:00.3
> 						0-edge     xhci_hcd
>   33:     267794          0          0          0 IR-PCI-MSIX-0000:03:00.3
> 						1-edge     xhci_hcd
>   34:          0      33441          0          0 IR-PCI-MSIX-0000:03:00.3
> 						2-edge     xhci_hcd
> 
> Vector 32 is the primary interrupter, while vectors 33 and 34 serve as
> dedicated secondary interrupters for isoc and bulk traffic.
> 
> perf stat over 30 seconds, with the USB camera and USB flash drive both
> active, shows the following system-wide numbers:
> 
>    With dedicated MSI-X interrupters for isoc and bulk:
>      perf stat -a -e context-switches,cpu-migrations -- sleep 30
> 
>        context-switches:        389,068
>        cpu-migrations :         18,665
> 
>    Original single-primary-interrupt case:
>      perf stat -a -e context-switches,cpu-migrations -- sleep 30
> 
>        context-switches:        509,633
>        cpu-migrations :         42,644
> 
> In this workload, using separate MSI-X interrupters for isoc and bulk
> reduces cpu-migrations by roughly 56% and context-switches by about 24%
> compared to routing all traffic through the primary interrupter.

Thanks, good to get some numbers on secondary interrupter usage.

> 
> Signed-off-by: Xu Rao <raoxu@uniontech.com>
> ---
>   drivers/usb/host/xhci-mem.c  | 17 +++++++
>   drivers/usb/host/xhci-pci.c  | 40 +++++++++++++++
>   drivers/usb/host/xhci-ring.c | 99 ++++++++++++++++++++++++++++++++++--
>   drivers/usb/host/xhci.c      | 20 ++++++++
>   drivers/usb/host/xhci.h      |  5 ++
>   5 files changed, 177 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index c4a6544aa107..a0c04015d12e 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -2412,6 +2412,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
>   {
>   	struct device	*dev = xhci_to_hcd(xhci)->self.sysdev;
>   	dma_addr_t	dma;
> +	struct xhci_interrupter *ir;
> 
>   	/*
>   	 * xHCI section 5.4.6 - Device Context array must be
> @@ -2505,6 +2506,22 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
>   	if (!xhci->interrupters[0])
>   		goto fail;
> 
> +	/* Default to single-interrupt mode */
> +	xhci->msix_isoc_interrupter = 0;
> +	xhci->msix_bulk_interrupter = 0;

we could start with just one extra interrupter, use a struct xhci_interrupter pointer
here instead of an integer, and not name it according to any specific transfer type

xhci->xfer_interrupter

> +
> +	if (xhci->max_interrupters >= 3 && xhci->quirks & XHCI_BULK_ISOC_SECONDARY_IRQ) {
> +		ir = xhci_create_secondary_interrupter(xhci_to_hcd(xhci), 0, xhci->imod_interval,
> +				0);

xhci->xfer_interrupter = xhci_create_secondary_interrupter(..)

> +		if (ir)
> +			xhci->msix_isoc_interrupter = ir->intr_num;
> +
> +		ir = xhci_create_secondary_interrupter(xhci_to_hcd(xhci), 0, xhci->imod_interval,
> +				0);
> +		if (ir)
> +			xhci->msix_bulk_interrupter = ir->intr_num;
> +	}
> +
>   	if (scratchpad_alloc(xhci, flags))
>   		goto fail;
> 
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index f67a4d956204..a333f6765b9c 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -135,6 +135,13 @@ static void xhci_cleanup_msix(struct xhci_hcd *xhci)
>   		return;
> 
>   	free_irq(pci_irq_vector(pdev, 0), xhci_to_hcd(xhci));
> +
> +	if (xhci->msix_isoc_interrupter != 0)
> +		free_irq(pci_irq_vector(pdev, xhci->msix_isoc_interrupter), xhci_to_hcd(xhci));
> +

xhci_remove_secondary_interrupter(hcd, xhci->xfer_interrupter)

> +	if (xhci->msix_bulk_interrupter != 0)
> +		free_irq(pci_irq_vector(pdev, xhci->msix_bulk_interrupter), xhci_to_hcd(xhci));
> +
>   	pci_free_irq_vectors(pdev);
>   	hcd->msix_enabled = 0;
>   }
> @@ -179,8 +186,36 @@ static int xhci_try_enable_msi(struct usb_hcd *hcd)
>   	if (ret)
>   		goto free_irq_vectors;
> 
> +	if (xhci->nvecs < 3) {
> +		xhci->msix_isoc_interrupter = 0;
> +		xhci->msix_bulk_interrupter = 0;
> +	}
> +
> +	if (xhci->msix_isoc_interrupter) {
> +		ret = request_irq(pci_irq_vector(pdev, xhci->msix_isoc_interrupter),
> +				xhci_msix_isoc_irq, 0, "xhci_hcd", xhci_to_hcd(xhci));
> +		if (ret) {
> +			xhci_dbg_trace(xhci, trace_xhci_dbg_init,
> +					"failed to request isoc msix irq %d, fallback to primary",
> +					xhci->msix_isoc_interrupter);
> +			xhci->msix_isoc_interrupter = 0;

xhci_remove_secondary_interrupter(hcd, xhci->xfer_interrupter)
xhci->xfer_interrupter = NULL;


> +		}
> +	}
> +
> +	if (xhci->msix_bulk_interrupter) {
> +		ret = request_irq(pci_irq_vector(pdev, xhci->msix_bulk_interrupter),
> +				xhci_msix_bulk_irq, 0, "xhci_hcd", xhci_to_hcd(xhci));
> +		if (ret) {
> +			xhci_dbg_trace(xhci, trace_xhci_dbg_init,
> +					"failed to request bulk msix irq %d, fallback to primary",
> +					xhci->msix_bulk_interrupter);
> +			xhci->msix_bulk_interrupter = 0;
> +		}
> +	}
> +
>   	hcd->msi_enabled = 1;
>   	hcd->msix_enabled = pdev->msix_enabled;
> +
>   	return 0;
> 
>   free_irq_vectors:
> @@ -514,6 +549,11 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
>   	if (xhci->quirks & XHCI_RESET_ON_RESUME)
>   		xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
>   				"QUIRK: Resetting on resume");
> +
> +	if (pdev->vendor == 0x1022 &&
> +	    pdev->device == 0x15e0)
> +		xhci->quirks |= XHCI_BULK_ISOC_SECONDARY_IRQ;
> +
>   }
> 
>   #ifdef CONFIG_ACPI
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 6309200e93dc..9427c447d269 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -3244,6 +3244,93 @@ irqreturn_t xhci_msi_irq(int irq, void *hcd)
>   }
>   EXPORT_SYMBOL_GPL(xhci_msi_irq);
> 
> +static irqreturn_t xhci_msix_secondary_irq(struct usb_hcd *hcd,
> +		unsigned int intr_num)
> +{
> +	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> +	struct xhci_interrupter *ir;
> +	irqreturn_t ret = IRQ_HANDLED;
> +	u32 status;
> +
> +	if (!xhci->interrupters || intr_num >= xhci->max_interrupters)
> +		return IRQ_NONE;
> +
> +	ir = xhci->interrupters[intr_num];
> +	if (!ir || !ir->event_ring)
> +		return IRQ_NONE;
> +
> +	spin_lock(&xhci->lock);
> +
> +	/* Check if the xHC generated the interrupt, or the irq is shared */
> +	status = readl(&xhci->op_regs->status);
> +	if (status == ~(u32)0) {
> +		xhci_hc_died(xhci);
> +		goto out;
> +	}
> +
> +	if (status & STS_HCE) {
> +		xhci_warn(xhci, "WARNING: Host Controller Error\n");
> +		goto out;
> +	}
> +
> +	if (status & STS_FATAL) {
> +		xhci_warn(xhci, "WARNING: Host System Error\n");
> +		xhci_halt(xhci);
> +		goto out;
> +	}
> +
> +	xhci_handle_events(xhci, ir, false);
> +out:
> +	spin_unlock(&xhci->lock);
> +
> +	return ret;
> +}
> +

The handler above looks very similar to the existing handler


> +irqreturn_t xhci_msix_isoc_irq(int irq, void *hcd)
> +{
> +	struct usb_hcd *usb_hcd = hcd;
> +	struct xhci_hcd *xhci = hcd_to_xhci(usb_hcd);
> +
> +	if (!xhci->msix_isoc_interrupter)
> +		return IRQ_NONE;
> +
> +	/* Handler for secondary interrupter: isoc */
> +	return xhci_msix_secondary_irq(usb_hcd, xhci->msix_isoc_interrupter);
> +}
> +EXPORT_SYMBOL_GPL(xhci_msix_isoc_irq);
> +
> +irqreturn_t xhci_msix_bulk_irq(int irq, void *hcd)
> +{
> +	struct usb_hcd *usb_hcd = hcd;
> +	struct xhci_hcd *xhci = hcd_to_xhci(usb_hcd);
> +
> +	if (!xhci->msix_bulk_interrupter)
> +		return IRQ_NONE;
> +
> +	/* Handler for secondary interrupter: bulk */
> +	return xhci_msix_secondary_irq(usb_hcd, xhci->msix_bulk_interrupter);
> +}
> +EXPORT_SYMBOL_GPL(xhci_msix_bulk_irq);

Could we skip all three interrupt handling relater functions above, and change the
existing handler to take a struct xhci_interrupter *pointer instead of struct
usb_hcd *hcd

Then add the hcd pointer to struct xhci_interrupter, and dig it out from there when
needed.

i.e.
ret = request_irq(pci_irq_vector(pdev, 0), xhci_msi_irq, 0, "xhci_hcd",
                           &xhci->interrupters[0]);

if (xhci->xfer_interrupter) {
	ret = request_irq(pci_irq_vector(pdev, xhci->xfer_interrupter->intr_num),
				xhci_msix_irq, 0, "xhci_hcd", xhci->xfer_interrupter);
}

irqreturn_t xhci_msi_irq(int irq, void *ir)


> +
> +static unsigned int xhci_choose_msix_irqnum(struct xhci_hcd *xhci, struct urb *urb)
> +{
> +	u8 xfer_type = usb_endpoint_type(&urb->ep->desc);
> +	struct usb_hcd *hcd = xhci_to_hcd(xhci);
> +
> +	if (!hcd->msix_enabled)
> +		return 0;
> +
> +	if (xfer_type == USB_ENDPOINT_XFER_ISOC &&
> +			xhci->msix_isoc_interrupter)
> +		return xhci->msix_isoc_interrupter;
> +
> +	if (xfer_type == USB_ENDPOINT_XFER_BULK &&
> +			xhci->msix_bulk_interrupter)
> +		return xhci->msix_bulk_interrupter;
> +
> +	return 0;
> +}
> +
>   /****		Endpoint Ring Operations	****/
> 
>   /*
> @@ -3642,6 +3729,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
>   	int sent_len, ret;
>   	u32 field, length_field, remainder;
>   	u64 addr, send_addr;
> +	unsigned int intr_num = 0;
> 
>   	ring = xhci_urb_to_transfer_ring(xhci, urb);
>   	if (!ring)
> @@ -3682,6 +3770,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
>   	start_trb = &ring->enqueue->generic;
>   	start_cycle = ring->cycle_state;
>   	send_addr = addr;
> +	intr_num = xhci_choose_msix_irqnum(xhci, urb);

I don't think we should choose the interupter for every transfer

How about adding a pointer to the interrupter in struct xhci_virt_ep, and set it
to the desired interrupter when we enable the endpoint.

we should add reference counting to the interrupter as well, increase it
when its set for an endpoint, and decrease when endpoint is dropped.

This way we can prevent interrupter removal when it's still in use

> 
>   	/* Queue the TRBs, even if they are zero-length */
>   	for (enqd_len = 0; first_trb || enqd_len < full_len;
> @@ -3742,7 +3831,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> 
>   		length_field = TRB_LEN(trb_buff_len) |
>   			TRB_TD_SIZE(remainder) |
> -			TRB_INTR_TARGET(0);
> +			TRB_INTR_TARGET(intr_num);

TRB_INTR_TARGET(ep->interrupter->intr_num);

Thanks
Mathias