On Mon, Nov 21, 2011 at 12:18:45PM +1030, Rusty Russell wrote: > On Wed, 16 Nov 2011 09:18:38 +0200, "Michael S. Tsirkin" <[email protected]> > 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 :)
They don't in fact, in my testing :(. But I think they can with luck. > > 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: Absolutely. But I think we also need to handle num_added overflow of a 15 bit counter, no? Otherwise the vring_need_event logic might give us false negatives .... I'm guessing we can just assume we need a kick in that case. > From: Rusty Russell <[email protected]> > 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 <[email protected]> > --- > 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); _______________________________________________ Virtualization mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/virtualization
