netdev - Re: [PATCH v2 0/8] irqbypass: Cleanups and a perf improvement

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aD40BIYA1ecnbX73@google.com>
Date: Mon, 2 Jun 2025 16:30:12 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Alex Williamson <alex.williamson@...hat.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, "Michael S. Tsirkin" <mst@...hat.com>, 
	Jason Wang <jasowang@...hat.com>, kvm@...r.kernel.org, virtualization@...ts.linux.dev, 
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Kevin Tian <kevin.tian@...el.com>, Oliver Upton <oliver.upton@...ux.dev>, 
	David Matlack <dmatlack@...gle.com>, Like Xu <like.xu.linux@...il.com>, 
	Binbin Wu <binbin.wu@...ux.intel.com>, Yong He <alexyonghe@...cent.com>
Subject: Re: [PATCH v2 0/8] irqbypass: Cleanups and a perf improvement

On Mon, Jun 02, 2025, Alex Williamson wrote:
> On Fri, 16 May 2025 16:07:26 -0700
> Sean Christopherson <seanjc@...gle.com> 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.
> > 
> > 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.  And
> > KVM shold probably have a KVM_IRQFD_FLAG_NO_IRQBYPASS if a VM is never going
> > to use device posted interrupts.  But those are future problems.
> > 
> > v2:
> >  - Collect reviews. [Kevin, Michael]
> >  - Track the pointer as "struct eventfd_ctx *eventfd" instead of "void *token".
> >    [Alex]
> >  - Fix typos and stale comments. [Kevin, Binbin]
> >  - Use "trigger" instead of the null token/eventfd pointer on failure in
> >    vfio_msi_set_vector_signal(). [Kevin]
> >  - Drop a redundant "tmp == consumer" check from patch 3. [Kevin]
> >  - Require producers to pass in the line IRQ number.
> > 
> > v1: https://lore.kernel.org/all/20250404211449.1443336-1-seanjc@google.com
> > 
> > [1] https://lore.kernel.org/all/20230801115646.33990-1-likexu@tencent.com
> > [2] https://lore.kernel.org/all/20250401161804.842968-1-seanjc@google.com
> > 
> > Sean Christopherson (8):
> >   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
> >   irqbypass: Require producers to pass in Linux IRQ number during
> >     registration
> > 
> >  arch/x86/kvm/x86.c                |   4 +-
> >  drivers/vfio/pci/vfio_pci_intrs.c |  10 +-
> >  drivers/vhost/vdpa.c              |  10 +-
> >  include/linux/irqbypass.h         |  46 ++++----
> >  virt/kvm/eventfd.c                |   7 +-
> >  virt/lib/irqbypass.c              | 190 +++++++++++-------------------
> >  6 files changed, 107 insertions(+), 160 deletions(-)
> > 
> > 
> > base-commit: 7ef51a41466bc846ad794d505e2e34ff97157f7f
> 
> Sorry for the delay.

Heh, no worries.  ~2 weeks is downright prompt by my standards ;-)

> Do you intend to take this through your trees?

Yes, ideally, it would go into Paolo's kvm/next sooner than later (I'll start
poking him if necessary).  The s/token/eventfd rename creates an annoying conflict
in kvm/x86.c with an in-flight patch (significant code movement between files).
It would be nice to be able to rebase the in-flight patch instead of having to
resolve a merge confict (the conflict itself isn't difficult to resolve, I just
find it hard to visually review/audit the resolution due to the code movement).