Re: [PATCH v5 2/4] kvm: KVM_EOIFD, an eventfd for EOIs

[Alex Williamson]

On Tue, 2012-07-17 at 17:10 +0300, Michael S. Tsirkin wrote:
> On Tue, Jul 17, 2012 at 07:59:16AM -0600, Alex Williamson wrote:
> > On Tue, 2012-07-17 at 13:21 +0300, Michael S. Tsirkin wrote:
> > > On Mon, Jul 16, 2012 at 02:33:55PM -0600, Alex Williamson wrote:
> > > > +	if (args->flags & KVM_EOIFD_FLAG_LEVEL_IRQFD) {
> > > > +		struct _irqfd *irqfd = _irqfd_fdget_lock(kvm, args->irqfd);
> > > > +		if (IS_ERR(irqfd)) {
> > > > +			ret = PTR_ERR(irqfd);
> > > > +			goto fail;
> > > > +		}
> > > > +
> > > > +		gsi = irqfd->gsi;
> > > > +		level_irqfd = eventfd_ctx_get(irqfd->eventfd);
> > > > +		source = _irq_source_get(irqfd->source);
> > > > +		_irqfd_put_unlock(irqfd);
> > > > +		if (!source) {
> > > > +			ret = -EINVAL;
> > > > +			goto fail;
> > > > +		}
> > > > +	} else {
> > > > +		ret = -EINVAL;
> > > > +		goto fail;
> > > > +	}
> > > > +
> > > > +	INIT_LIST_HEAD(&eoifd->list);
> > > > +	eoifd->kvm = kvm;
> > > > +	eoifd->eventfd = eventfd;
> > > > +	eoifd->source = source;
> > > > +	eoifd->level_irqfd = level_irqfd;
> > > > +	eoifd->notifier.gsi = gsi;
> > > > +	eoifd->notifier.irq_acked = eoifd_event;
> > > 
> > > OK so this means eoifd keeps a reference to the irqfd.
> > > And since this is the case, can't we drop the reference counting
> > > around source ids now? Everything is referenced through irqfd.
> > 
> > Holding a reference and using it as a reference count are not the same
> > thing.  What if another module holds a reference to this eventfd?  How
> > do we do anything on release?
> 
> We don't as there is no release, and using kref on source id does not
> help: it just never gets invoked.

Please work out how you think it should work and let me know, I don't
see it.  We have an irq source id that needs to be allocated by irqfd
and returned when it's unused.  It becomes unused when neither irqfd nor
eoifd are making use of it.  irqfd and eoifd may be closed in any order.
Use of the source id is what we're reference counting, which is why it's
in struct _irq_source.  How can I use an eventfd reference for the same?
I don't know when it's unused.  I don't know who else holds a reference
to it...  Doesn't make sense to me.  Feels like attempting to squat on
someone else's object.



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/