From: "Michael S. Tsirkin" <mst@redhat.com>
To: Alex Williamson <alex.williamson@redhat.com>
Cc: Avi Kivity <avi@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 01:00:04 +0300 [thread overview]
Message-ID: <20120813220004.GC15639@redhat.com> (raw)
In-Reply-To: <1344893004.4683.136.camel@ul30vt.home>
On Mon, Aug 13, 2012 at 03:23:24PM -0600, Alex Williamson wrote:
> On Sun, 2012-08-12 at 12:33 +0300, Michael S. Tsirkin wrote:
> > On Thu, Aug 09, 2012 at 01:26:15PM -0600, Alex Williamson wrote:
> > > On Mon, 2012-08-06 at 13:40 +0300, Avi Kivity wrote:
> > > > On 08/06/2012 01:38 PM, Avi Kivity wrote:
> > > >
> > > > > Regarding the implementation, instead of a linked list, would an array
> > > > > of counters parallel to the bitmap make it simpler?
> > > >
> > > > Or even, replace the bitmap with an array of counters.
> > >
> > > I'm not sure a counter array is what we're really after. That gives us
> > > reference counting for the irq source IDs, but not the key->gsi lookup.
> > > It also highlights another issue, that we have a limited set of source
> > > IDs. Looks like we have BITS_PER_LONG IDs, with two already used, one
> > > for the shared userspace ID and another for the PIT. How happy are we
> > > going to be with a limit of 62 level interrupts in use at one time?
> > >
> > > It's arguably a reasonable number since the most virtualization friendly
> > > devices (sr-iov VFs) don't even support this kind of interrupt. It's
> > > also very wasteful allocating an entire source ID for a single GSI
> > > within that source ID. PCI supports interrupts A, B, C, and D, which,
> > > in the most optimal config, each go to different GSIs. So we could
> > > theoretically be more efficient in our use and allocation of irq source
> > > IDs if we tracked use by the source ID, gsi pair.
> > >
> > > That probably makes it less practical to replace anything at the top
> > > level with a counter array. The key that we pass back is currently the
> > > actual source ID, but we don't specify what it is, so we could split it
> > > and have it encode a 16bit source ID plus 16 bit GSI. It could also be
> > > an idr entry.
> > >
> > > 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.
>
> This is effectively what I meant when I suggested we either need to a)
> pull eoifd into irqfd or b) implement them as modular components. I
> chose to implement b) because I think that non-irqfd related ack
> notification to userspace will be useful and a) does not provide that.
> So this interface enables exactly the use case for device assignment and
> no more. I feel like this is the start of an ioctl that will be quickly
> deprecated, but if that's the direction we want to go, I'll write the
> code. Thanks,
>
> Alex
Sorry I wrote this before I knew we really do not need
the deassert on ack at all, existing irqfd is fine for level.
--
MST
next prev parent reply other threads:[~2012-08-13 21:59 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 [this message]
2012-08-14 12:35 ` Avi Kivity
2012-08-14 14:50 ` Michael S. Tsirkin
2012-08-14 22:01 ` Alex Williamson
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=20120813220004.GC15639@redhat.com \
--to=mst@redhat.com \
--cc=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 \
/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).