Add Reference Count to Xen Event Channel

[akshay1994.leo_gmail.com]

Summary

Add a reference count to xenisrc. This is required for implementation of unmap-notifications in the grant table userspace device (gntdev). We need to hold a reference to the event channel port, in case the user deallocates the port, before we send the notification.

[royger]

I've been thinking a little bit about this, and AFAIK from the gntdev you need to send a signal to a specific event channel port that's provided from an ioctl. I don't think you need refcounting for this, can you simply add something like:

int
xen_intr_signal_port(evtchn_port_t port)
{
	struct xenisrc *isrc;
	struct evtchn_send send;

	if (!is_valid_evtchn(port) || port >= NR_EVENT_CHANNELS)
		return (EINVAL);

	mtx_lock(&xen_intr_isrc_lock);
	isrc = xen_intr_port_to_isrc[port];
	if (isrc == NULL || isrc->xi_type != EVTCHN_TYPE_PORT) {
		mtx_unlock(&xen_intr_isrc_lock);
		return (EINVAL);
	}
	send.port = isrc->xi_port;
	HYPERVISOR_event_channel_op(EVTCHNOP_send, &send);
	mtx_unlock(&xen_intr_isrc_lock);

	return (0);
}

This should work properly even if the event channel has been deallocated, in that case it will just return EINVAL. Note that I have not even tried to compile the above code snipped.

[akshay1994.leo_gmail.com]

I'm sorry, I have very little idea about event channels, but from what I think, keeping refcounted channels is a better idea.
The event channel might be used to signal the foreign domain that we are unmapping the grant. Keeping a refcounted channel ensures that the notification always goes through, which won't be the case if we go with the other approach. The other domain may be waiting for a signal before deallocating the grant, and the lack of signal might lead to ghost grants accumulating, wasting precious memory (which in case of FreeBSD would be wired pages).

[royger]

sys/x86/xen/xen_intr.c

330

Shouldn't this be done at xen_intr_bind_isrc? If you do it here the refcount is only going to be initialized when a new vector is allocated, but not if we are reusing a free vector.

354

IMHO, it would be better to do this at xen_intr_unbind, and only remove the handler if all refcounts have been dropped. And now that I think of it, this check for is_handlers should probably be transformed into an ASSERT, I don't think it's possible to get here with a handler still bound.

1592

Just using the unbind function should be fine, so I would remove this function and call xen_intr_unbind directly when you want to drop a reference. In the long term it might be better to rename unbind to xen_intr_put_evtchn or something similar.

[akshay1994.leo_gmail.com]

sys/x86/xen/xen_intr.c

330

Agreed. And done.
I shouldn't have missed this out.

354

Refcount_release shifted to unbind, and the if condition changed to an ASSERT.

1592

Agreed. And done.

[royger]

LGTM, just a minor comment.

sys/x86/xen/xen_intr.c

1593

This comment is slightly wrong, we assign the vector as the opaque handler, not the event channel port (I can fix this up when committing, no need for you to update the patch).

[akshay1994.leo_gmail.com]

sys/x86/xen/xen_intr.c

1593

Okay! :)
(The same comment is present on line 422 too.)

[royger]

Oh, thanks for noticing! Those are leftovers from when we used the port as the opaque handler.