On 09/20/2014 06:00 PM, Paolo Bonzini wrote:

Il 19/09/2014 09:10, Jason Wang ha scritto:

quoted

quoted

quoted

 
-	if (!vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX)) {
+	if (vq->urgent || !vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX)) {

So the urgent descriptor only work when event index was not enabled?
This seems suboptimal, we may still want to benefit from event index
even if urgent descriptor is used. Looks like we need return true here
when vq->urgent is true?

Its ||, not &&.

Without event index, all descriptors are treated as urgent.

Paolo

The problem is if vq->urgent is true, the patch checks
VRING_AVAIL_F_NO_INTERRUPT bit. This bit were set unconditionally in
virtqueue_enable_cb() regardless of event index feature and cleared
unconditionally in virtqueue_disable_cb(). So virtqueue_enable_cb() was
used to not only publish a new event index but also enable the urgent
descriptor. And virtqueue_disable_cb() disabled all interrupts including
the urgent descriptor. Guest won't get urgent interrupts by just adding
virtqueue_add_outbuf_urgent() since what it needs is to enable and
disable interrupt for !urgent descriptor.

Btw, not sure "urgent" is a suitable name, since interrupt is often slow
in kvm guest. And in fact virtio-net will probably use "urgent"
descriptor for those packets (e.g stream packets who can be delayed a
little bit to batch more bytes from userspace) who was not urgent
compared to other packets.