Linux Virtualization - May 2008 - [PATCH 1/3] virtio: VIRTIO_F_NOTIFY_ON_EMPTY to force callback on empty

virtio allows drivers to suppress callbacks (ie. interrupts) for
efficiency (no locking, it's just an optimization).

There's a similar mechanism for the host to suppress notifications
coming from the guest: in that case, we ignore the suppression if the
ring is completely full.

It turns out that life is simpler if the host similarly ignores
callback suppression when the ring is completely empty: the network
driver wants to free up old packets in a timely manner, and otherwise
has to use a timer to poll.

We have to remove the code which ignores interrupts when the driver
has disabled them (again, it had no locking and hence was unreliable
anyway).

Signed-off-by: Rusty Russell <rusty at rustcorp.com.au>
---
 drivers/virtio/virtio_ring.c  |    7 -------
 include/linux/virtio_config.h |    4 ++++
 2 files changed, 4 insertions(+), 7 deletions(-)

diff -r 95a02f0e0e21 drivers/virtio/virtio_ring.c
--- a/drivers/virtio/virtio_ring.c	Tue May 27 12:45:17 2008 +1000
+++ b/drivers/virtio/virtio_ring.c	Tue May 27 15:53:41 2008 +1000
@@ -253,13 +253,6 @@ irqreturn_t vring_interrupt(int irq, voi
 	if (unlikely(vq->broken))
 		return IRQ_HANDLED;
 
-	/* Other side may have missed us turning off the interrupt,
-	 * but we should preserve disable semantic for virtio users. */
-	if (unlikely(vq->vring.avail->flags & VRING_AVAIL_F_NO_INTERRUPT)) {
-		pr_debug("virtqueue interrupt after disable for %p\n", vq);
-		return IRQ_HANDLED;
-	}
-
 	pr_debug("virtqueue callback for %p (%p)\n", vq,
vq->vq.callback);
 	if (vq->vq.callback)
 		vq->vq.callback(&vq->vq);
diff -r 95a02f0e0e21 include/linux/virtio_config.h
--- a/include/linux/virtio_config.h	Tue May 27 12:45:17 2008 +1000
+++ b/include/linux/virtio_config.h	Tue May 27 15:53:41 2008 +1000
@@ -14,6 +14,10 @@
 #define VIRTIO_CONFIG_S_DRIVER_OK	4
 /* We've given up on this device. */
 #define VIRTIO_CONFIG_S_FAILED		0x80
+
+/* Do we get callbacks when the ring is completely used, even if we've
+ * suppressed them? */
+#define VIRTIO_F_NOTIFY_ON_EMPTY	24
 
 #ifdef __KERNEL__
 #include <linux/virtio.h>

This is the lguest implementation of the VIRTIO_F_NOTIFY_ON_EMPTY feature.
It is currently only published for network devices, but it is turned on for
everyone.

Signed-off-by: Rusty Russell <rusty at rustcorp.com.au>
---
 Documentation/lguest/lguest.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff -r a2e94bbd1589 Documentation/lguest/lguest.c
--- a/Documentation/lguest/lguest.c	Tue May 27 16:06:10 2008 +1000
+++ b/Documentation/lguest/lguest.c	Tue May 27 16:15:43 2008 +1000
@@ -157,6 +157,9 @@ struct virtqueue
 
 	/* The routine to call when the Guest pings us. */
 	void (*handle_output)(int fd, struct virtqueue *me);
+
+	/* Outstanding buffers */
+	unsigned int inflight;
 };
 
 /* Remember the arguments to the program so we can "reboot" */
@@ -702,6 +705,7 @@ static unsigned get_vq_desc(struct virtq
 			errx(1, "Looped descriptor");
 	} while ((i = next_desc(vq, i)) != vq->vring.num);
 
+	vq->inflight++;
 	return head;
 }
 
@@ -719,6 +723,7 @@ static void add_used(struct virtqueue *v
 	/* Make sure buffer is written before we update index. */
 	wmb();
 	vq->vring.used->idx++;
+	vq->inflight--;
 }
 
 /* This actually sends the interrupt for this virtqueue */
@@ -726,8 +731,9 @@ static void trigger_irq(int fd, struct v
 {
 	unsigned long buf[] = { LHREQ_IRQ, vq->config.irq };
 
-	/* If they don't want an interrupt, don't send one. */
-	if (vq->vring.avail->flags & VRING_AVAIL_F_NO_INTERRUPT)
+	/* If they don't want an interrupt, don't send one, unless empty. */
+	if ((vq->vring.avail->flags & VRING_AVAIL_F_NO_INTERRUPT)
+	    && vq->inflight)
 		return;
 
 	/* Send the Guest an interrupt tell them we used something up. */
@@ -1107,6 +1113,7 @@ static void add_virtqueue(struct device 
 	vq->next = NULL;
 	vq->last_avail_idx = 0;
 	vq->dev = dev;
+	vq->inflight = 0;
 
 	/* Initialize the configuration. */
 	vq->config.num = num_descs;
@@ -1368,6 +1375,7 @@ static void setup_tun_net(const char *ar
 
 	/* Tell Guest what MAC address to use. */
 	add_feature(dev, VIRTIO_NET_F_MAC);
+	add_feature(dev, VIRTIO_F_NOTIFY_ON_EMPTY);
 	set_config(dev, sizeof(conf), &conf);
 
 	/* We don't need the socket any more; setup is done. */

Am Freitag, 30. Mai 2008 schrieb Rusty Russell:
> It turns out that life is simpler if the host similarly ignores
> callback suppression when the ring is completely empty: the network
> driver wants to free up old packets in a timely manner, and otherwise
> has to use a timer to poll.
> 
> We have to remove the code which ignores interrupts when the driver
> has disabled them (again, it had no locking and hence was unreliable
> anyway).
> 
> Signed-off-by: Rusty Russell <rusty at rustcorp.com.au>
> ---
>  drivers/virtio/virtio_ring.c  |    7 -------
>  include/linux/virtio_config.h |    4 ++++
>  2 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff -r 95a02f0e0e21 drivers/virtio/virtio_ring.c
> --- a/drivers/virtio/virtio_ring.c	Tue May 27 12:45:17 2008 +1000
> +++ b/drivers/virtio/virtio_ring.c	Tue May 27 15:53:41 2008 +1000
> @@ -253,13 +253,6 @@ irqreturn_t vring_interrupt(int irq, voi
>  	if (unlikely(vq->broken))
>  		return IRQ_HANDLED;
> 
> -	/* Other side may have missed us turning off the interrupt,
> -	 * but we should preserve disable semantic for virtio users. */
> -	if (unlikely(vq->vring.avail->flags &
VRING_AVAIL_F_NO_INTERRUPT)) {
> -		pr_debug("virtqueue interrupt after disable for %p\n", vq);
> -		return IRQ_HANDLED;
> -	}
> -
>  	pr_debug("virtqueue callback for %p (%p)\n", vq,
vq->vq.callback);
>  	if (vq->vq.callback)
>  		vq->vq.callback(&vq->vq);
that part makes sense.
> diff -r 95a02f0e0e21 include/linux/virtio_config.h
> --- a/include/linux/virtio_config.h	Tue May 27 12:45:17 2008 +1000
> +++ b/include/linux/virtio_config.h	Tue May 27 15:53:41 2008 +1000
> @@ -14,6 +14,10 @@
>  #define VIRTIO_CONFIG_S_DRIVER_OK	4
>  /* We've given up on this device. */
>  #define VIRTIO_CONFIG_S_FAILED		0x80
> +
> +/* Do we get callbacks when the ring is completely used, even if we've
> + * suppressed them? */
> +#define VIRTIO_F_NOTIFY_ON_EMPTY	24
24?

Does that mean the first 24 bits are driver owned and 24-31 are for the
common virtio code?

Christian