linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Alex Williamson <alex.williamson@redhat.com>
To: Avi Kivity <avi@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>,
	gleb@redhat.com, kvm@vger.kernel.org,
	linux-kernel@vger.kernel.org, jan.kiszka@siemens.com
Subject: Re: [PATCH v7 2/2] kvm: KVM_EOIFD, an eventfd for EOIs
Date: Tue, 14 Aug 2012 16:01:15 -0600	[thread overview]
Message-ID: <1344981675.4683.338.camel@ul30vt.home> (raw)
In-Reply-To: <502A462A.1000600@redhat.com>

On Tue, 2012-08-14 at 15:35 +0300, Avi Kivity wrote:
> On 08/12/2012 12:33 PM, Michael S. Tsirkin wrote:
> >> 
> >> Michael, would the interface be more acceptable to you if we added
> >> separate ioctls to allocate and free some representation of an irq
> >> source ID, gsi pair?  For instance, an ioctl might return an idr entry
> >> for an irq source ID/gsi object which would then be passed as a
> >> parameter in struct kvm_irqfd and struct kvm_eoifd so that the object
> >> representing the source id/gsi isn't magically freed on it's own.  This
> >> would also allow us to deassign/close one end and reconfigure it later.
> >> Thanks,
> >> 
> >> Alex
> > 
> > It's acceptable to me either way. I was only pointing out that as
> > designed, the interface looks simple at first but then you find out some
> > subtle limitations which are implementation driven. This gives
> > an overall feeling the abstraction is too low level.
> > 
> > If we compare to the existing irqfd, isn't the difference
> > simply that irqfd deasserts immediately ATM, while we
> > want to delay this until later?
> > 
> > If yes, then along the lines that you proposed, and combining with my
> > idea of tracking deasserts, how do you like the following:
> > 
> > /* Keep line asserted until guest has handled the interrupt. */
> > #define KVM_IRQFD_FLAG_DEASSERT_ON_ACK (1 << 1)
> > /* Notify after line is deasserted. */
> > #define KVM_IRQFD_FLAG_DEASSERT_EVENTFD (2 << 1)
> > 
> > 	struct kvm_irqfd {
> > 		__u32 fd;
> > 		__u32 gsi;
> > 		__u32 flags;
> > 		/* eventfd to notify when line is deasserted */
> > 		__u32 deassert_eventfd;
> > 		__u8  pad[16];
> > 	};
> > 
> > now the only limitation is that KVM_IRQFD_FLAG_DEASSERT_ON_ACK is only
> > effective for level interrupts.
> > 
> > Notes about lifetime of objects:
> > 	- closing deassert_eventfd does nothing (we can keep
> > 	  reference to it from irqfd so no need for
> >           complex polling/flushing scheme)
> > 	- closing irqfd or deasserting dis-associates
> > 	  deassert_eventfd automatically
> > 	- source id is internal to irqfd and goes away with it
> > 
> > it looks harder to misuse and fits what we want to do nicely,
> > and needs less code to implement.
> > 
> > Avi, what do you think?
> 
> I think given all the complexity in the separate ioctl approach that
> this makes sense.  There are no lifetime issues or code to match the two
> eventfds.  Alex, would this API simplify the code?

It does though I'm concerned that it's a very specific solution that
only addresses this problem.  Generic userspace eoi/ack is not
addressed.  The latest version using separate ioctls does a lot of
simplification by exposing irq sourceids.  The bulk of the code there is
duplicating what irqfd does just so we can catch the POLLHUP for
cleanup.  If there was an easier way to do that, we don't care about
POLLIN/POLLOUT, much of the code could be removed.  Alternatively we
could make some common infrastructure to simplify both irqfd and
irq_ackfd, but how to frame the helpers isn't easy.

> Yet another option was raised in the past, and that was exiling ioapic
> and pic to userspace.  This moves the entire issue to userspace.  The
> cost is a new interface that implements the APIC bus (betweem APIC and
> IOAPIC) and the INTACK sequence (between APIC and PIC), and potential
> for performance regressions due to the PIC, IOAPIC, and PIT being in
> userspace.  We would still have to keep the IOAPIC/PIC in the kernel,
> but no new features would be added.

Doesn't this assure a performance regression or are we assuming anywhere
we care about performance we're using MSI?  Thanks,

Alex


  parent reply	other threads:[~2012-08-14 22:01 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-07-24 20:43 [PATCH v7 0/2] kvm: level irqfd and new eoifd Alex Williamson
2012-07-24 20:43 ` [PATCH v7 1/2] kvm: Extend irqfd to support level interrupts Alex Williamson
2012-07-29 15:01   ` Michael S. Tsirkin
2012-07-30 16:06     ` Alex Williamson
2012-07-24 20:43 ` [PATCH v7 2/2] kvm: KVM_EOIFD, an eventfd for EOIs Alex Williamson
2012-07-29 14:54   ` Michael S. Tsirkin
2012-07-30 16:22     ` Alex Williamson
2012-07-31  0:01       ` Michael S. Tsirkin
2012-07-31  0:26         ` Alex Williamson
2012-07-31  0:36           ` Michael S. Tsirkin
2012-07-31  1:12             ` Alex Williamson
2012-08-01 19:06               ` Alex Williamson
2012-08-12  7:49                 ` Michael S. Tsirkin
2012-08-13 16:48                   ` Alex Williamson
2012-08-13 16:59                     ` Michael S. Tsirkin
2012-08-13 18:17                       ` Alex Williamson
2012-08-13 19:50                         ` Michael S. Tsirkin
2012-08-13 20:48                           ` Alex Williamson
2012-08-13 21:50                             ` Michael S. Tsirkin
2012-08-13 22:22                               ` Alex Williamson
2012-08-13 22:52                                 ` Michael S. Tsirkin
2012-08-14 10:10                                   ` Gleb Natapov
2012-08-14 10:13                                     ` Gleb Natapov
2012-08-02  8:42               ` Michael S. Tsirkin
2012-08-06 10:17   ` Avi Kivity
2012-08-06 10:38     ` Avi Kivity
2012-08-06 10:40       ` Avi Kivity
2012-08-09 19:26         ` Alex Williamson
2012-08-12  8:36           ` Avi Kivity
2012-08-13 21:34             ` Alex Williamson
2012-08-13 22:06               ` Michael S. Tsirkin
2012-08-13 22:41                 ` Alex Williamson
2012-08-13 23:00                   ` Michael S. Tsirkin
2012-08-14  3:09                     ` Alex Williamson
2012-08-14  8:35                       ` Michael S. Tsirkin
2012-08-14 21:28                         ` Alex Williamson
2012-08-12  9:33           ` Michael S. Tsirkin
2012-08-13 21:23             ` Alex Williamson
2012-08-13 22:00               ` Michael S. Tsirkin
2012-08-14 12:35             ` Avi Kivity
2012-08-14 14:50               ` Michael S. Tsirkin
2012-08-14 22:01               ` Alex Williamson [this message]
2012-08-14 23:04                 ` Michael S. Tsirkin
2012-08-14 23:26                   ` Alex Williamson
2012-08-15 13:09                     ` Avi Kivity
2012-08-12  7:53     ` Michael S. Tsirkin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1344981675.4683.338.camel@ul30vt.home \
    --to=alex.williamson@redhat.com \
    --cc=avi@redhat.com \
    --cc=gleb@redhat.com \
    --cc=jan.kiszka@siemens.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mst@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).