Re: [PATCH RFC 2/2] vhost: support urgent descriptors

On Mon, Sep 22, 2014 at 05:55:23PM +0800, Jason Wang wrote:

On 09/22/2014 02:55 PM, Michael S. Tsirkin wrote:

quoted

On Mon, Sep 22, 2014 at 11:30:23AM +0800, Jason Wang wrote:

quoted

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

quoted

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().

The reverse actually, right?

Ah, right.

quoted

quoted

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.

Right, we want a new API that advances event index but does not
set VRING_AVAIL_F_NO_INTERRUPT.
IMO still want to set VRING_AVAIL_F_NO_INTERRUPT when handling tx
interrupts, to avoid interrupt storms.

I see, so urgent descriptor needs to be disabled in this case. But vhost
parts need a little big changes, we could not just check
VRING_AVAIL_F_NO_INTERRUPT when vq->urgent is true. If event index is
enabled, we still need to check used event to make sure the current tx
delayed interrupt works.

But just re-using VRING_AVAIL_F_NO_INTERRUPT for urgent descriptor may
not work in some case. I see codes of virtqueue_get_buf() that may
breaks this:

        /* If we expect an interrupt for the next entry, tell
host                                                                                                       

         * by writing event index and flush out the write
before                                                                                                         

         * the read in the next get_buf call. */
        if (!(vq->vring.avail->flags & VRING_AVAIL_F_NO_INTERRUPT)) {
                vring_used_event(&vq->vring) = vq->last_used_idx;
                virtio_mb(vq->weak_barriers);
        }

Right, we need to change this path to use a private flag.

Consider if only urgent descriptor is enabled, this will publish used
event which in fact enable lots of unnecessary interrupt. In fact I
don't quite understand how the above lines is used. Virtio-net stop the
queue before enable the tx interrupt in start_xmit(), so the above lines
will not run at all.

True - this code is for drivers which process VQ without
disabling interrupts first. No rule says this is not allowed.

quoted

quoted

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.

Yes but we are asking for an interrupt before event index is reached
because something is waiting for the packet to be transmitted.
I couldn't come up with a better name.

Ok.