[lkml]   [2009]   [May]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [KVM PATCH v10] kvm: add support for irqfd
    Michael S. Tsirkin wrote:
    > On Wed, May 27, 2009 at 10:06:50AM -0400, Gregory Haskins wrote:
    >> Michael S. Tsirkin wrote:
    >>> On Wed, May 20, 2009 at 10:30:49AM -0400, Gregory Haskins wrote:
    >>>> +static int
    >>>> +kvm_assign_irqfd(struct kvm *kvm, int fd, int gsi)
    >>>> +{
    >>>> + struct _irqfd *irqfd;
    >>>> + struct file *file = NULL;
    >>>> + int ret;
    >>>> +
    >>>> + irqfd = kzalloc(sizeof(*irqfd), GFP_KERNEL);
    >>>> + if (!irqfd)
    >>>> + return -ENOMEM;
    >>>> +
    >>>> + irqfd->kvm = kvm;
    >>>> + irqfd->gsi = gsi;
    >>>> + INIT_LIST_HEAD(&irqfd->list);
    >>>> + INIT_WORK(&irqfd->work, irqfd_inject);
    >>>> +
    >>>> + /*
    >>>> + * Embed the file* lifetime in the irqfd.
    >>>> + */
    >>>> + file = fget(fd);
    >>>> + if (IS_ERR(file)) {
    >>>> + ret = PTR_ERR(file);
    >>>> + goto fail;
    >>>> + }
    >>> So we get a reference to a file, and unless the user is nice to us, it
    >>> will only be dropped when kvm char device file is closed?
    >>> I think this will deadlock if the fd in question is the open kvm char device.
    >> Hmm...I hadn't considered this possibility, though I am not sure if it
    >> would cause a deadlock in the pattern you suggest. It seems more like
    >> it would result in, at worst, an extra reference to itself (and thus a
    >> leak) rather than a deadlock...
    >> I digress. In either case, perhaps I should s/fget/eventfd_fget to at
    >> least limit the type of fd to eventfd. I was trying to be "slick" by
    >> not needing the eventfd_fget() exported, but I am going to need to
    >> export it later anyway for iosignalfd, so its probably a moot point.
    >> Thanks Michael,
    >> -Greg
    > This only works as long as eventfd does not do fget on some fd as well.
    > Which it does not do now, and may never do - but we create a fragile
    > system this way.
    > I think it's really wrong, fundamentally, to keep a reference to a
    > file until another file is closed, unless you are code under fs/.
    > We will get nasty circular references sooner or later.

    Hmm.. I understand your concern, but I respectfully disagree.

    One object referencing another is a natural expression, regardless of
    what type they may be. The fact is that introducing the concept of
    irqfd creates a relationship between an eventfd instance and a kvm
    instance whether we like it or not, and this relationship needs to be
    managed. It is therefore IMO perfectly natural to express that
    relationship with a reference count, and I do not currently see anything
    wrong or even particularly fragile about how I've currently done this.
    I'm sure there are other ways, however. Do you have a particular
    suggestion in mind?

    > Isn't the real reason we use fd to be able to support the same interface
    > on top of both kvm and lguest?

    Actually, the reason why we use an fd is to decouple the
    interrupt-producing end-point from the KVM core. Ignoring eventfd in
    specific for a moment, one convenient way to do that is with an fd
    because it provides a nice, already written/tested handle-to-pointer
    translation, and a polymorphic interface (e.g. f_ops). Choosing to use
    eventfd flavored fd's buys us additional advantages in terms of
    leveraging already tested f_ops code, and compatibility with an
    interface that is designed-for/used-by other established subsystems for
    > And if so, wouldn't some kind of bus be a better solution?

    Ultimately I aim to implement a bus (vbus, specifically) in terms of
    irqfd (and iosignalfd, for that matter). However, the eventfd
    interfaces are general purpose and can be used in other areas as well
    (for instance, virtio-pci, or the shared-mem driver recently
    discussed). I realize this is probably not the point you were making
    here, but fyi.


    [unhandled content-type:application/pgp-signature]
     \ /
      Last update: 2009-05-27 22:11    [W:0.027 / U:19.936 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site