virtio: document functions better

The old documentation is left over from when we used a structure with strategy pointers.

Remove wrapper functions. This makes the allocation type explicit in all callers; I used GPF_KERNEL where it seemed obvious, left it at GFP_ATOMIC otherwise.

Since we know vq->vring.num is a power of 2, modulus is lazy (it's asserted in vring_new_virtqueue()).

Signed-off-by Rusty Russell <[email protected]>

---
 drivers/virtio/virtio_ring.c |   10 ++++++----

 1 file changed, 6 insertions(+), 4 deletions(-)

Since we know vq->vring.num is a power of 2, modulus is lazy (it's asserted in vring_new_virtqueue()).

Signed-off-by Rusty Russell <[email protected]>

---
 drivers/virtio/virtio_ring.c |   10 ++++++----

 1 file changed, 6 insertions(+), 4 deletions(-)

Based on patch by Christoph for virtio_blk speedup:

Based on patch by Christoph for virtio_blk speedup:

Quoted 14 lines · Rusty Russell

Since we know vq->vring.num is a power of 2, modulus is lazy (it's asserted in vring_new_virtqueue()).

Signed-off-by Rusty Russell <[email protected]>

---
 drivers/virtio/virtio_ring.c |   10 ++++++----

 1 file changed, 6 insertions(+), 4 deletions(-)

arch/x86/lguest/boot.c +0 -0

@@ -1420,7 +1420,7 @@ __init void lguest_init(void)
        add_preferred_console("hvc", 0, NULL);
 

arch/x86/lguest/boot.c __init void lguest_init(void) · line 1423

Quoted 3 lines

arch/x86/lguest/boot.c +1 -1

        /* Register our very early console. */
-       virtio_cons_early_init(early_put_chars);
+//     virtio_cons_early_init(early_put_chars);

What's this hunk here?

Quoted 14 lines · Rusty Russell

Since we know vq->vring.num is a power of 2, modulus is lazy (it's asserted in vring_new_virtqueue()).

Signed-off-by Rusty Russell <[email protected]>

---
 drivers/virtio/virtio_ring.c |   10 ++++++----

 1 file changed, 6 insertions(+), 4 deletions(-)

arch/x86/lguest/boot.c +0 -0

@@ -1420,7 +1420,7 @@ __init void lguest_init(void)
        add_preferred_console("hvc", 0, NULL);
 

arch/x86/lguest/boot.c __init void lguest_init(void) · line 1423

Quoted 3 lines

arch/x86/lguest/boot.c +1 -1

        /* Register our very early console. */
-       virtio_cons_early_init(early_put_chars);
+//     virtio_cons_early_init(early_put_chars);

What's this hunk here?

Quoted 1 line · Rusty Russell

Based on patch by Christoph for virtio_blk speedup:

Please credit it to Stefan - he also sent a pointer to his original version in reply to the previous thread.

Quoted 1 line · Rusty Russell

Based on patch by Christoph for virtio_blk speedup:

Please credit it to Stefan - he also sent a pointer to his original version in reply to the previous thread.

Also shouldn't virtqueue_kick have kerneldoc comments?

I also notices that you documented the functions bother here and in the first patch in the headers. At least historically the kerneldoc tools didn't parse comments at declarations, but only at the function defintions. Did you check these actually get picked up?

A virtio driver does virtqueue_add_buf() multiple times before finally calling virtqueue_kick(); previously we only exposed the added buffers in the virtqueue_kick() call. This means we don't need a memory barrier in virtqueue_add_buf(), but it reduces concurrency as the device (ie. host) can't see the buffers until the kick.

Signed-off-by Rusty Russell <[email protected]>

---
 drivers/virtio/virtio_ring.c |   37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

drivers/virtio/virtio_ring.c +9 -6

@@ -227,9 +227,15 @@ add_head:
 
 	/* Put entry in available array (but don't update avail->idx until they
 	 * do sync). */
-	avail = ((vq->vring.avail->idx + vq->num_added++) & (vq->vring.num-1));
+	avail = (vq->vring.avail->idx & (vq->vring.num-1));
 	vq->vring.avail->ring[avail] = head;
 
+	/* Descriptors and available array need to be set before we expose the
+	 * new available array entries. */
+	virtio_wmb();
+	vq->vring.avail->idx++;
+	vq->num_added++;
+
 	pr_debug("Added buffer head %i to %p\n", head, vq);
 	END_USE(vq);
 

@@ -248,13 +254,10 @@ bool virtqueue_kick_prepare(struct virtq
 	 * new available array entries. */
 	virtio_wmb();
 
-	old = vq->vring.avail->idx;
-	new = vq->vring.avail->idx = old + vq->num_added;
+	old = vq->vring.avail->idx - vq->num_added;
+	new = vq->vring.avail->idx;
 	vq->num_added = 0;
 
-	/* Need to update avail index before checking if we should notify */
-	virtio_mb();
-
 	if (vq->event) {
 		needs_kick = vring_need_event(vring_avail_event(&vq->vring),
 					      new, old);

_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Quoted 7 lines · Rusty Russell

A virtio driver does virtqueue_add_buf() multiple times before finally calling virtqueue_kick(); previously we only exposed the added buffers in the virtqueue_kick() call. This means we don't need a memory barrier in virtqueue_add_buf(), but it reduces concurrency as the device (ie. host) can't see the buffers until the kick.

Signed-off-by Rusty Russell <[email protected]>

In the past I played with a patch like this, but I didn't see a performance gain either way. Do you see any gain?

I'm a bit concerned that with this patch, a buggy driver that adds more than 2^16 descriptors without a kick
would seem to work sometimes. Let's add WARN_ON(vq->num_added > (1 << 16))?

Quoted 6 lines · Rusty Russell

A virtio driver does virtqueue_add_buf() multiple times before finally calling virtqueue_kick(); previously we only exposed the added buffers in the virtqueue_kick() call. This means we don't need a memory barrier in virtqueue_add_buf(), but it reduces concurrency as the device (ie. host) can't see the buffers until the kick.

Quoted 1 line

Signed-off-by Rusty Russell <[email protected]>

In the past I played with a patch like this, but I didn't see a performance gain either way. Do you see any gain?

I'm a bit concerned that with this patch, a buggy driver that adds more than 2^16 descriptors without a kick
would seem to work sometimes. Let's add WARN_ON(vq->num_added > (1 << 16))?

Quoted 46 lines

---
 drivers/virtio/virtio_ring.c |   37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

drivers/virtio/virtio_ring.c +9 -6

@@ -227,9 +227,15 @@ add_head:
 
 	/* Put entry in available array (but don't update avail->idx until they
 	 * do sync). */
-	avail = ((vq->vring.avail->idx + vq->num_added++) & (vq->vring.num-1));
+	avail = (vq->vring.avail->idx & (vq->vring.num-1));
 	vq->vring.avail->ring[avail] = head;
 
+	/* Descriptors and available array need to be set before we expose the
+	 * new available array entries. */
+	virtio_wmb();
+	vq->vring.avail->idx++;
+	vq->num_added++;
+
 	pr_debug("Added buffer head %i to %p\n", head, vq);
 	END_USE(vq);
 

@@ -248,13 +254,10 @@ bool virtqueue_kick_prepare(struct virtq
 	 * new available array entries. */
 	virtio_wmb();
 
-	old = vq->vring.avail->idx;
-	new = vq->vring.avail->idx = old + vq->num_added;
+	old = vq->vring.avail->idx - vq->num_added;
+	new = vq->vring.avail->idx;
 	vq->num_added = 0;
 
-	/* Need to update avail index before checking if we should notify */
-	virtio_mb();
-
 	if (vq->event) {
 		needs_kick = vring_need_event(vring_avail_event(&vq->vring),
 					      new, old);

_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Quoted 7 lines · Michael S. Tsirkin

Quoted 6 lines · Rusty Russell

A virtio driver does virtqueue_add_buf() multiple times before finally calling virtqueue_kick(); previously we only exposed the added buffers in the virtqueue_kick() call. This means we don't need a memory barrier in virtqueue_add_buf(), but it reduces concurrency as the device (ie. host) can't see the buffers until the kick.

Quoted 8 lines

Quoted 1 line

Signed-off-by Rusty Russell <[email protected]>

In the past I played with a patch like this, but I didn't see a performance gain either way. Do you see any gain?

I'm a bit concerned that with this patch, a buggy driver that adds more than 2^16 descriptors without a kick
would seem to work sometimes. Let's add WARN_ON(vq->num_added > (1 << 16))?

Thinking about it more - it might be tricky for drivers to ensure this. add used to fail when vq is full, but now driver might do get between add and notify:
lock
add_buf * N
prep
unlock
lock
get_buf * N
unlock
lock
add_buf
prep
unlock
notify

and since add was followed by get, this doesn't fail.

Quoted 33 lines · Michael S. Tsirkin

Quoted 7 lines · Michael S. Tsirkin

Quoted 6 lines · Rusty Russell

A virtio driver does virtqueue_add_buf() multiple times before finally calling virtqueue_kick(); previously we only exposed the added buffers in the virtqueue_kick() call. This means we don't need a memory barrier in virtqueue_add_buf(), but it reduces concurrency as the device (ie. host) can't see the buffers until the kick.

Quoted 8 lines

Quoted 1 line

Signed-off-by Rusty Russell <[email protected]>

In the past I played with a patch like this, but I didn't see a performance gain either way. Do you see any gain?

I'm a bit concerned that with this patch, a buggy driver that adds more than 2^16 descriptors without a kick
would seem to work sometimes. Let's add WARN_ON(vq->num_added > (1 << 16))?

Thinking about it more - it might be tricky for drivers to ensure this. add used to fail when vq is full, but now driver might do get between add and notify:
lock
add_buf * N
prep
unlock
lock
get_buf * N
unlock
lock
add_buf
prep
unlock
notify

Quoted 1 line

and since add was followed by get, this doesn't fail.

Right, the driver could, in theory, do:

        add_buf()
        if (!get_buf())
                notify()

But we don't allow that at the moment in our API: we insist on a notify occasionally. Noone does this at the moment, so a WARN_ON is correct.

If you're just add_buf() without the get_buf() then add_buf() will fail already.

Here's my current variant:

drivers/virtio/virtio_ring.c +11 -1

@@ -245,9 +245,19 @@ add_head:
 
 	/* Put entry in available array (but don't update avail->idx until they
 	 * do sync). */
-	avail = ((vq->vring.avail->idx + vq->num_added++) & (vq->vring.num-1));
+	avail = (vq->vring.avail->idx & (vq->vring.num-1));
 	vq->vring.avail->ring[avail] = head;
 
+	/* Descriptors and available array need to be set before we expose the
+	 * new available array entries. */
+	virtio_wmb();
+	vq->vring.avail->idx++;
+	vq->num_added++;
+
+	/* If you haven't kicked in this long, you're probably doing something
+	 * wrong. */
+	WARN_ON(vq->num_added > vq->vring.num);
+
 	pr_debug("Added buffer head %i to %p\n", head, vq);
 	END_USE(vq);
 

It's hard to write a useful WARN_ON() for the "you should kick more regularly" case (we could take timestamps if DEBUG is defined, I guess), so let's leave this until someone actually trips it.

Thanks,
Rusty.

My unlocked kick patches will trip this warning: they make virtio-net do add + get without kick.

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;

Quoted 2 lines · Michael S. Tsirkin

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 :)

Quoted 7 lines

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 <[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(+)

drivers/virtio/virtio_ring.c +31 -0

@@ -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);

Quoted 6 lines · Rusty Russell

Quoted 2 lines · Michael S. Tsirkin

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.

Quoted 12 lines

Quoted 7 lines

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.

Quoted 1 line

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.

Quoted 22 lines · Michael S. Tsirkin

Quoted 6 lines · Rusty Russell

Quoted 2 lines · Michael S. Tsirkin

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.

Quoted 12 lines

Quoted 7 lines

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.

Quoted 6 lines

Quoted 1 line

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.

You're right. Thankyou. My immediate reaction of "make it an unsigned long" doesn't work.

Here's the diff to what I posted before:

drivers/virtio/virtio_ring.c add_head: · line 259

Quoted 6 lines · Rusty Russell

drivers/virtio/virtio_ring.c +3 -3

-	/* If you haven't kicked in this long, you're probably doing something
-	 * wrong. */
-	WARN_ON(vq->num_added > vq->vring.num);
+	/* This is very unlikely, but theoretically possible.  Kick
+	 * just in case. */
+	if (unlikely(vq->num_added == 65535))

This is 0xffff but why use the decimal notation?

drivers/virtio/virtio_ring.c add_head: · line 263

Quoted 4 lines

drivers/virtio/virtio_ring.c +1 -0

+		virtqueue_kick(_vq);
 
 	pr_debug("Added buffer head %i to %p\n", head, vq);
 	END_USE(vq);

We also still need to reset vq->num_added, right?

Quoted 9 lines · Michael S. Tsirkin

drivers/virtio/virtio_ring.c add_head: · line 259

Quoted 6 lines · Rusty Russell

drivers/virtio/virtio_ring.c +3 -3

-	/* If you haven't kicked in this long, you're probably doing something
-	 * wrong. */
-	WARN_ON(vq->num_added > vq->vring.num);
+	/* This is very unlikely, but theoretically possible.  Kick
+	 * just in case. */
+	if (unlikely(vq->num_added == 65535))

This is 0xffff but why use the decimal notation?

Interesting. Why use hex? Feels more like binary?

But I've changed it to "(1 << 16) - 1" to be clear.

Quoted 6 lines

drivers/virtio/virtio_ring.c add_head: · line 263

Quoted 4 lines

drivers/virtio/virtio_ring.c +1 -0

+		virtqueue_kick(_vq);
 
 	pr_debug("Added buffer head %i to %p\n", head, vq);
 	END_USE(vq);

We also still need to reset vq->num_added, right?

virtqueue_kick does that for us.

Cheers,
Rusty.

A virtio driver does virtqueue_add_buf() multiple times before finally calling virtqueue_kick(); previously we only exposed the added buffers in the virtqueue_kick() call. This means we don't need a memory barrier in virtqueue_add_buf(), but it reduces concurrency as the device (ie. host) can't see the buffers until the kick.

Signed-off-by Rusty Russell <[email protected]>

Quoted 7 lines · Rusty Russell

A virtio driver does virtqueue_add_buf() multiple times before finally calling virtqueue_kick(); previously we only exposed the added buffers in the virtqueue_kick() call. This means we don't need a memory barrier in virtqueue_add_buf(), but it reduces concurrency as the device (ie. host) can't see the buffers until the kick.

Signed-off-by Rusty Russell <[email protected]>

Looking at recent mm compaction patches made me look at locking in balloon closely. And I noticed the referenced patch (commit ee7cd8981e15bcb365fc762afe3fc47b8242f630 upstream) interacts strangely with virtio balloon; balloon currently does:

static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq)

{
        struct scatterlist sg;

        sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);

        init_completion(&vb->acked);

        /* We should always be able to add one buffer to an empty queue. */
        if (virtqueue_add_buf(vq, &sg, 1, 0, vb, GFP_KERNEL) < 0)
                BUG();
        virtqueue_kick(vq);

        /* When host has read buffer, this completes via balloon_ack */
        wait_for_completion(&vb->acked);
}

While vq callback does:

static void balloon_ack(struct virtqueue *vq)

{
        struct virtio_balloon *vb;
        unsigned int len;

        vb = virtqueue_get_buf(vq, &len);
        if (vb)
                complete(&vb->acked);
}

So virtqueue_get_buf might now run concurrently with virtqueue_kick. I audited both and this seems safe in practice but I think

we need to either declare this legal at the API level or add locking in driver.

Further, is there a guarantee that we never get spurious callbacks? We currently check ring not empty but esp for non shared MSI this might not be needed.

If a spurious callback triggers, virtqueue_get_buf can run concurrently with virtqueue_add_buf which is known to be racy. Again I think this is currently safe as no spurious callbacks in practice but should we guarantee no spurious callbacks at the API level or add locking in driver?

Quoted 45 lines · Michael S. Tsirkin

Quoted 7 lines · Rusty Russell

A virtio driver does virtqueue_add_buf() multiple times before finally calling virtqueue_kick(); previously we only exposed the added buffers in the virtqueue_kick() call. This means we don't need a memory barrier in virtqueue_add_buf(), but it reduces concurrency as the device (ie. host) can't see the buffers until the kick.

Signed-off-by Rusty Russell <[email protected]>

Looking at recent mm compaction patches made me look at locking in balloon closely. And I noticed the referenced patch (commit ee7cd8981e15bcb365fc762afe3fc47b8242f630 upstream) interacts strangely with virtio balloon; balloon currently does:

static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq)

{
        struct scatterlist sg;

        sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);

        init_completion(&vb->acked);

        /* We should always be able to add one buffer to an empty queue. */
        if (virtqueue_add_buf(vq, &sg, 1, 0, vb, GFP_KERNEL) < 0)
                BUG();
        virtqueue_kick(vq);

        /* When host has read buffer, this completes via balloon_ack */
        wait_for_completion(&vb->acked);
}

While vq callback does:

static void balloon_ack(struct virtqueue *vq)

{
        struct virtio_balloon *vb;
        unsigned int len;

        vb = virtqueue_get_buf(vq, &len);
        if (vb)
                complete(&vb->acked);
}

Quoted 2 lines

So virtqueue_get_buf might now run concurrently with virtqueue_kick. I audited both and this seems safe in practice but I think

Good spotting!

Agreed. Because there's only add_buf, we get away with it: the add_buf must be almost finished by the time get_buf runs because the device has seen the buffer.

Quoted 2 lines

we need to either declare this legal at the API level or add locking in driver.

I wonder if we should just lock in the balloon driver, rather than document this corner case and set a bad example.

Are there other
drivers which take the same shortcut?

Quoted 3 lines

Further, is there a guarantee that we never get spurious callbacks? We currently check ring not empty but esp for non shared MSI this might not be needed.

Yes, I think this saves us. A spurious interrupt won't trigger a spurious callback.

Quoted 5 lines

If a spurious callback triggers, virtqueue_get_buf can run concurrently with virtqueue_add_buf which is known to be racy. Again I think this is currently safe as no spurious callbacks in practice but should we guarantee no spurious callbacks at the API level or add locking in driver?

I think we should guarantee it, but is there a hole in the current implementation?

Thanks,
Rusty.

Quoted 55 lines · Rusty Russell

Quoted 45 lines · Michael S. Tsirkin

Quoted 7 lines · Rusty Russell

A virtio driver does virtqueue_add_buf() multiple times before finally calling virtqueue_kick(); previously we only exposed the added buffers in the virtqueue_kick() call. This means we don't need a memory barrier in virtqueue_add_buf(), but it reduces concurrency as the device (ie. host) can't see the buffers until the kick.

Signed-off-by Rusty Russell <[email protected]>

Looking at recent mm compaction patches made me look at locking in balloon closely. And I noticed the referenced patch (commit ee7cd8981e15bcb365fc762afe3fc47b8242f630 upstream) interacts strangely with virtio balloon; balloon currently does:

static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq)

{
        struct scatterlist sg;

        sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);

        init_completion(&vb->acked);

        /* We should always be able to add one buffer to an empty queue. */
        if (virtqueue_add_buf(vq, &sg, 1, 0, vb, GFP_KERNEL) < 0)
                BUG();
        virtqueue_kick(vq);

        /* When host has read buffer, this completes via balloon_ack */
        wait_for_completion(&vb->acked);
}

While vq callback does:

static void balloon_ack(struct virtqueue *vq)

{
        struct virtio_balloon *vb;
        unsigned int len;

        vb = virtqueue_get_buf(vq, &len);
        if (vb)
                complete(&vb->acked);
}

Quoted 2 lines

So virtqueue_get_buf might now run concurrently with virtqueue_kick. I audited both and this seems safe in practice but I think

Good spotting!

Agreed. Because there's only add_buf, we get away with it: the add_buf must be almost finished by the time get_buf runs because the device has seen the buffer.

Quoted 5 lines

Quoted 2 lines

we need to either declare this legal at the API level or add locking in driver.

I wonder if we should just lock in the balloon driver, rather than document this corner case and set a bad example.

We'll need to replace &vb->acked with a waitqueue and do get_buf from the same thread.
But I note that stats_request hash the same issue. Let's see if we can fix it.

Quoted 2 lines

Are there other
drivers which take the same shortcut?

Not that I know.

Quoted 13 lines

Quoted 3 lines

Further, is there a guarantee that we never get spurious callbacks? We currently check ring not empty but esp for non shared MSI this might not be needed.

Yes, I think this saves us. A spurious interrupt won't trigger a spurious callback.

Quoted 5 lines

If a spurious callback triggers, virtqueue_get_buf can run concurrently with virtqueue_add_buf which is known to be racy. Again I think this is currently safe as no spurious callbacks in practice but should we guarantee no spurious callbacks at the API level or add locking in driver?

Quoted 5 lines

I think we should guarantee it, but is there a hole in the current implementation?

Thanks,
Rusty.

Could be. The check for ring empty looks somewhat suspicious. It might be expensive to make it 100% robust - that check was intended as an optimization for shared interrupts. Whith per vq interrupts we IMO do not need the check. If we add locking in balloon I think there's no need to guarantee no spurious interrupts.

As 'locking in balloon', may I assume the approach I took for the compaction case is OK and aligned to address these concerns of yours? If not, do not hesitate in giving me your thoughts, please. I'm respinning a V3 series to address a couple of extra nitpicks from the compaction standpoint, and I'd love to be able to address any extra concern you might have on the balloon side of that work.

Quoted 5 lines · Rafael Aquini

As 'locking in balloon', may I assume the approach I took for the compaction case is OK and aligned to address these concerns of yours? If not, do not hesitate in giving me your thoughts, please. I'm respinning a V3 series to address a couple of extra nitpicks from the compaction standpoint, and I'd love to be able to address any extra concern you might have on the balloon side of that work.

It's orthogonal, though looks like they clash textually :(

I'll re-spin MST's patch on top of yours, and include both in my tree, otherwise linux-next will have to do the merge. But I'll await your push before pushing to Linus next merge window.

Thanks,
Rusty.

Quoted 5 lines · Rafael Aquini

As 'locking in balloon', may I assume the approach I took for the compaction case is OK and aligned to address these concerns of yours? If not, do not hesitate in giving me your thoughts, please. I'm respinning a V3 series to address a couple of extra nitpicks from the compaction standpoint, and I'd love to be able to address any extra concern you might have on the balloon side of that work.

It's orthogonal, though looks like they clash textually :(

I'll re-spin MST's patch on top of yours, and include both in my tree, otherwise linux-next will have to do the merge. But I'll await your push before pushing to Linus next merge window.

Thanks,
Rusty.

In virtio balloon virtqueue_get_buf might now run concurrently with virtqueue_kick. I audited both and this seems safe in practice but this is not guaranteed by the API.
Additionally, a spurious interrupt might in theory make virtqueue_get_buf run in parallel with virtqueue_add_buf, which is racy.

While we might try to protect against spurious callbacks it's easier to fix the driver: balloon seems to be the only one (mis)using the API like this, so let's just fix balloon.

Signed-off-by Michael S. Tsirkin <[email protected]>

Quoted 5 lines · Rafael Aquini

As 'locking in balloon', may I assume the approach I took for the compaction case is OK and aligned to address these concerns of yours? If not, do not hesitate in giving me your thoughts, please. I'm respinning a V3 series to address a couple of extra nitpicks from the compaction standpoint, and I'd love to be able to address any extra concern you might have on the balloon side of that work.

It's orthogonal, though looks like they clash textually :(

I'll re-spin MST's patch on top of yours, and include both in my tree, otherwise linux-next will have to do the merge. But I'll await your push before pushing to Linus next merge window.

Thanks,
Rusty.

Quoted 91 lines · Michael S. Tsirkin

Quoted 55 lines · Rusty Russell

Quoted 45 lines · Michael S. Tsirkin

Quoted 7 lines · Rusty Russell

A virtio driver does virtqueue_add_buf() multiple times before finally calling virtqueue_kick(); previously we only exposed the added buffers in the virtqueue_kick() call. This means we don't need a memory barrier in virtqueue_add_buf(), but it reduces concurrency as the device (ie. host) can't see the buffers until the kick.

Signed-off-by Rusty Russell <[email protected]>

Looking at recent mm compaction patches made me look at locking in balloon closely. And I noticed the referenced patch (commit ee7cd8981e15bcb365fc762afe3fc47b8242f630 upstream) interacts strangely with virtio balloon; balloon currently does:

static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq)

{
        struct scatterlist sg;

        sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);

        init_completion(&vb->acked);

        /* We should always be able to add one buffer to an empty queue. */
        if (virtqueue_add_buf(vq, &sg, 1, 0, vb, GFP_KERNEL) < 0)
                BUG();
        virtqueue_kick(vq);

        /* When host has read buffer, this completes via balloon_ack */
        wait_for_completion(&vb->acked);
}

While vq callback does:

static void balloon_ack(struct virtqueue *vq)

{
        struct virtio_balloon *vb;
        unsigned int len;

        vb = virtqueue_get_buf(vq, &len);
        if (vb)
                complete(&vb->acked);
}

Quoted 2 lines

So virtqueue_get_buf might now run concurrently with virtqueue_kick. I audited both and this seems safe in practice but I think

Good spotting!

Agreed. Because there's only add_buf, we get away with it: the add_buf must be almost finished by the time get_buf runs because the device has seen the buffer.

Quoted 5 lines

Quoted 2 lines

we need to either declare this legal at the API level or add locking in driver.

I wonder if we should just lock in the balloon driver, rather than document this corner case and set a bad example.

We'll need to replace &vb->acked with a waitqueue and do get_buf from the same thread.
But I note that stats_request hash the same issue. Let's see if we can fix it.

Quoted 2 lines

Are there other
drivers which take the same shortcut?

Not that I know.

Quoted 13 lines

Quoted 3 lines

Further, is there a guarantee that we never get spurious callbacks? We currently check ring not empty but esp for non shared MSI this might not be needed.

Yes, I think this saves us. A spurious interrupt won't trigger a spurious callback.

Quoted 5 lines

If a spurious callback triggers, virtqueue_get_buf can run concurrently with virtqueue_add_buf which is known to be racy. Again I think this is currently safe as no spurious callbacks in practice but should we guarantee no spurious callbacks at the API level or add locking in driver?

Quoted 5 lines

I think we should guarantee it, but is there a hole in the current implementation?

Thanks,
Rusty.

Quoted 7 lines

Could be. The check for ring empty looks somewhat suspicious. It might be expensive to make it 100% robust - that check was intended as an optimization for shared interrupts. Whith per vq interrupts we IMO do not need the check. If we add locking in balloon I think there's no need to guarantee no spurious interrupts.

As 'locking in balloon', may I assume the approach I took for the compaction case is OK and aligned to address these concerns of yours?

If not, do not hesitate in
giving me your thoughts, please. I'm respinning a V3 series to address a couple of extra nitpicks from the compaction standpoint, and I'd love to be able to address any extra concern you might have on the balloon side of that work.

Thanks!
Rafael.

Quoted 9 lines · Rusty Russell

Quoted 5 lines · Rafael Aquini

As 'locking in balloon', may I assume the approach I took for the compaction case is OK and aligned to address these concerns of yours? If not, do not hesitate in giving me your thoughts, please. I'm respinning a V3 series to address a couple of extra nitpicks from the compaction standpoint, and I'd love to be able to address any extra concern you might have on the balloon side of that work.

It's orthogonal, though looks like they clash textually :(

Quoted 6 lines

I'll re-spin MST's patch on top of yours, and include both in my tree, otherwise linux-next will have to do the merge. But I'll await your push before pushing to Linus next merge window.

Thanks,
Rusty.

While theoretical mine is a bugfix so could be 3.5 material, no?