Re: [PATCH 0/7] irqbypass: Cleanups and a perf improvement

[Michael S. Tsirkin]

On Fri, Apr 04, 2025 at 02:14:42PM -0700, Sean Christopherson wrote:
> The two primary goals of this series are to make the irqbypass concept
> easier to understand, and to address the terrible performance that can
> result from using a list to track connections.
> 
> For the first goal, track the producer/consumer "tokens" as eventfd context
> pointers instead of opaque "void *".  Supporting arbitrary token types was
> dead infrastructure when it was added 10 years ago, and nothing has changed
> since.  Taking an opaque token makes a *very* simple concept (device signals
> eventfd; KVM listens to eventfd) unnecessarily difficult to understand.
> 
> Burying that simple behind a layer of obfuscation also makes the overall
> code more brittle, as callers can pass in literally anything. I.e. passing
> in a token that will never be paired would go unnoticed.
> 
> For the performance issue, use an xarray.  I'm definitely not wedded to an
> xarray, but IMO it doesn't add meaningful complexity (even requires less
> code), and pretty much Just Works.  Like tried this a while back[1], but
> the implementation had undesirable behavior changes and stalled out.
> 
> To address the use case where huge numbers of VMs are being created without
> _any_ possibility for irqbypass, KVM should probably add a
> KVM_IRQFD_FLAG_NO_IRQBYPASS flag so that userspace can opt-out on a per-IRQ
> basis.  I already proposed a KVM module param[2] to let userspace disable
> IRQ bypass, but that obviously affects all IRQs in all VMs.  It might
> suffice for most use cases, but I can imagine scenarios where the VMM wants
> to be more selective, e.g. when it *knows* a KVM_IRQFD isn't eligible for
> bypass.  And both of those require userspace changes.
> 
> Note, I want to do more aggressive cleanups of irqbypass at some point,
> e.g. not reporting an error to userspace if connect() fails is *awful*
> behavior for environments that want/need irqbypass to always work.  But
> that's a future problem.
> 
> [1] https://lore.kernel.org/all/20230801115646.33990-1-likexu@xxxxxxxxxxx
> [2] https://lore.kernel.org/all/20250401161804.842968-1-seanjc@xxxxxxxxxx

vdpa changes seem minor, so

Acked-by: Michael S. Tsirkin <mst@xxxxxxxxxx>


> Sean Christopherson (7):
>   irqbypass: Drop pointless and misleading THIS_MODULE get/put
>   irqbypass: Drop superfluous might_sleep() annotations
>   irqbypass: Take ownership of producer/consumer token tracking
>   irqbypass: Explicitly track producer and consumer bindings
>   irqbypass: Use paired consumer/producer to disconnect during
>     unregister
>   irqbypass: Use guard(mutex) in lieu of manual lock+unlock
>   irqbypass: Use xarray to track producers and consumers
> 
>  drivers/vfio/pci/vfio_pci_intrs.c |   5 +-
>  drivers/vhost/vdpa.c              |   4 +-
>  include/linux/irqbypass.h         |  38 +++---
>  virt/kvm/eventfd.c                |   3 +-
>  virt/lib/irqbypass.c              | 185 ++++++++++--------------------
>  5 files changed, 88 insertions(+), 147 deletions(-)
> 
> 
> base-commit: 782f9feaa9517caf33186dcdd6b50a8f770ed29b
> -- 
> 2.49.0.504.g3bcea36a83-goog