Skip to contentSkip to conversation

virtio: don't always force a notification when ring is full

PATCH In review Rusty Russell 3 participants 3 messages Jun 18, 2008 – Jun 19, 2008 virtio core

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 virtio: don't always force a notification when ring is full

We force notification when the ring is full, even if the host has indicated it doesn't want to know. This seemed like a good idea at the time: if we fill the transmit ring, we should tell the host immediately.

Unfortunately this logic also applies to the receiving ring, which is refilled constantly. We should introduce real notification thesholds to replace this logic. Meanwhile, removing the logic altogether breaks the heuristics which KVM uses, so we use a hack: only notify if there are outgoing parts of the new buffer.

It even makes a difference with lguest's crappy network implementation:

	1GB guest->host before: 30.0844
	1GB guest->host after: 27.6167
drivers/virtio/virtio_ring.c +5 -2
diff -r 89bf4894cc36 drivers/virtio/virtio_ring.c
@@ -87,8 +87,11 @@ static int vring_add_buf(struct virtqueu
 	if (vq->num_free < out + in) {
 		pr_debug("Can't add buf len %i - avail = %i\n",
 			 out + in, vq->num_free);
-		/* We notify *even if* VRING_USED_F_NO_NOTIFY is set here. */
-		vq->notify(&vq->vq);
+		/* FIXME: for historical reasons, we force a notify here if
+		 * there are outgoing parts to the buffer.  Presumably the
+		 * host should service the ring ASAP. */
+		if (out)
+			vq->notify(&vq->vq);
 		END_USE(vq);
 		return -ENOSPC;
 	}
Acked-by on v1 1/1
#1 lore raw
Quoted 11 lines · Rusty Russell

We force notification when the ring is full, even if the host has indicated it doesn't want to know. This seemed like a good idea at the time: if we fill the transmit ring, we should tell the host immediately.

Unfortunately this logic also applies to the receiving ring, which is refilled constantly. We should introduce real notification thesholds to replace this logic. Meanwhile, removing the logic altogether breaks the heuristics which KVM uses, so we use a hack: only notify if there are outgoing parts of the new buffer.

Quoted 4 lines

It even makes a difference with lguest's crappy network implementation:

	1GB guest->host before: 30.0844
	1GB guest->host after: 27.6167

A little ugly, but no more performance regression in KVM, so

Regards,

Anthony Liguori

Quoted 20 lines

diff -r 89bf4894cc36 drivers/virtio/virtio_ring.c

drivers/virtio/virtio_ring.c +5 -2
@@ -87,8 +87,11 @@ static int vring_add_buf(struct virtqueu
 	if (vq->num_free < out + in) {
 		pr_debug("Can't add buf len %i - avail = %i\n",
 			 out + in, vq->num_free);
-		/* We notify *even if* VRING_USED_F_NO_NOTIFY is set here. */
-		vq->notify(&vq->vq);
+		/* FIXME: for historical reasons, we force a notify here if
+		 * there are outgoing parts to the buffer.  Presumably the
+		 * host should service the ring ASAP. */
+		if (out)
+			vq->notify(&vq->vq);
 		END_USE(vq);
 		return -ENOSPC;
 	}
#2 lore raw
In reply to Rusty Russell

Am Donnerstag, 19. Juni 2008 schrieb Rusty Russell:

Quoted 10 lines

We force notification when the ring is full, even if the host has indicated it doesn't want to know. This seemed like a good idea at the time: if we fill the transmit ring, we should tell the host immediately.

Unfortunately this logic also applies to the receiving ring, which is refilled constantly. We should introduce real notification thesholds to replace this logic. Meanwhile, removing the logic altogether breaks the heuristics which KVM uses, so we use a hack: only notify if there are outgoing parts of the new buffer.

I did not yet finish the VIRTIO_F_NOTIFY_ON_EMPTY implementation for s390, so I cant tell for sure if it makes a difference.

Nevertheless, the patch looks good to me.