On Wed, 16 Nov 2011 09:18:38 +0200, "Michael S. Tsirkin" <mst@xxxxxxxxxx> wrote:
> My unlocked kick patches will trip this warning: they make
> virtio-net do add + get without kick.
Heh, it's a good sign if they do, since that means you're running really
well :)
> I think block with unlocked kick can trip it too:
> add, lock is dropped and then an interrupt can get.
>
> We also don't need a kick each num - each 2^15 is enough.
> Why don't we do this at start of add_buf:
> if (vq->num_added >= 0x7fff)
> return -ENOSPC;
The warning was there in case a driver is never doing a kick, and
getting away with it (mostly) because the device is polling. Let's not
penalize good drivers to catch bad ones.
How about we do this properly, like so:
From: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
Subject: virtio: add debugging if driver doesn't kick.
Under the existing #ifdef DEBUG, check that they don't have more than
1/10 of a second between an add_buf() and a
virtqueue_notify()/virtqueue_kick_prepare() call.
We could get false positives on a really busy system, but good for
development.
Signed-off-by: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
---
drivers/virtio/virtio_ring.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -22,6 +22,7 @@
#include <linux/device.h>
#include <linux/slab.h>
#include <linux/module.h>
+#include <linux/hrtimer.h>
/* virtio guest is communicating with a virtual "device" that actually runs on
* a host processor. Memory barriers are used to control SMP effects. */
@@ -102,6 +103,10 @@ struct vring_virtqueue
#ifdef DEBUG
/* They're supposed to lock for us. */
unsigned int in_use;
+
+ /* Figure out if their kicks are too delayed. */
+ bool last_add_time_valid;
+ ktime_t last_add_time;
#endif
/* Tokens for callbacks. */
@@ -192,6 +197,19 @@ int virtqueue_add_buf(struct virtqueue *
BUG_ON(data == NULL);
+#ifdef DEBUG
+ {
+ ktime_t now = ktime_get();
+
+ /* No kick or get, with .1 second between? Warn. */
+ if (vq->last_add_time_valid)
+ WARN_ON(ktime_to_ms(ktime_sub(now, vq->last_add_time))
+ > 100);
+ vq->last_add_time = now;
+ vq->last_add_time_valid = true;
+ }
+#endif
+
/* If the host supports indirect descriptor tables, and we have multiple
* buffers, then go indirect. FIXME: tune this threshold */
if (vq->indirect && (out + in) > 1 && vq->num_free) {
@@ -291,6 +309,14 @@ bool virtqueue_kick_prepare(struct virtq
new = vq->vring.avail->idx;
vq->num_added = 0;
+#ifdef DEBUG
+ if (vq->last_add_time_valid) {
+ WARN_ON(ktime_to_ms(ktime_sub(ktime_get(),
+ vq->last_add_time)) > 100);
+ }
+ vq->last_add_time_valid = false;
+#endif
+
if (vq->event) {
needs_kick = vring_need_event(vring_avail_event(&vq->vring),
new, old);
@@ -428,6 +454,10 @@ void *virtqueue_get_buf(struct virtqueue
virtio_mb();
}
+#ifdef DEBUG
+ vq->last_add_time_valid = false;
+#endif
+
END_USE(vq);
return ret;
}
@@ -611,6 +641,7 @@ struct virtqueue *vring_new_virtqueue(un
list_add_tail(&vq->vq.list, &vdev->vqs);
#ifdef DEBUG
vq->in_use = false;
+ vq->last_add_time_valid = false;
#endif
vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
