Linux Virtualization - Dec 2012 - [PATCH 0/1] virtio: console: regression in virtqueue_add_buf() change

Hi Rusty,

The linux-next kernel was failing my virtio-console test suite for a
while.  I looked into it today, and it's due to the
virtqueue_add_buf() change that doesn't return > 0 values anymore.  I
found your commit that adjusts virtio_console.c, but you missed one
instance where the return value mattered, and as a result not enough
buffers were queued for the host to send in data.

This resulted in the port's name to be not populated in sysfs, which
meant udev didn't create any symlinks in /dev/virtio-ports/.

I've just updated your commit with the small diff (attached below).
Please put this commit after 

commit e794093a52cdfef09b3fdb6294b75ab8cacb30a8
Author: Rusty Russell <rusty at rustcorp.com.au>
Date:   Tue Oct 16 23:56:14 2012 +1030

    virtio_net: don't rely on virtqueue_add_buf() returning capacity.
    
and before

commit 08d088e8357b3c031db7de006247f613c7f136ab
Author: Rusty Russell <rusty at rustcorp.com.au>
Date:   Tue Oct 16 23:56:15 2012 +1030

    virtio: make virtqueue_add_buf() returning 0 on success, not capacity.
    
in the pull request you send so that there's no regression in
virtio_console on bisection.

(commit ids from linux-next/master).

I didn't see your linux-next commit

commit 80162488188c239733433cff98ece5cde18b8b3b
Author: Rusty Russell <rusty at rustcorp.com.au>
Date:   Tue Oct 16 23:56:15 2012 +1030

    virtio: console: make it clear that virtqueue_add_buf() no longer returns
> 0

on a mailing list (please CC me!), and I missed reviewing the virtio
changes, but glad that the testsuite caught this :)

Diff relative to your patch:

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 82ebe02..6a36994 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -396,6 +396,8 @@ static int add_inbuf(struct virtqueue *vq, struct
port_buffer *buf)
 
 	ret = virtqueue_add_buf(vq, sg, 0, 1, buf, GFP_ATOMIC);
 	virtqueue_kick(vq);
+	if (!ret)
+		ret = vq->num_free;
 	return ret;
 }
 

Please apply.


Rusty Russell (1):
  virtio: console: virtqueue_add_buf() no longer returns > 0

 drivers/char/virtio_console.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

-- 
1.8.0.1

From: Rusty Russell <rusty at rustcorp.com.au>

We simplified virtqueue_add_buf(), make it clear in the callers.  Use
the newly-exposed vq->num_free in the two places that used the previous
+ve return value.

Signed-off-by: Rusty Russell <rusty at rustcorp.com.au>
Signed-off-by: Amit Shah <amit.shah at redhat.com> # Update add_inbuf()
---
 drivers/char/virtio_console.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 8ab9c3d..6a36994 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -396,6 +396,8 @@ static int add_inbuf(struct virtqueue *vq, struct
port_buffer *buf)
 
 	ret = virtqueue_add_buf(vq, sg, 0, 1, buf, GFP_ATOMIC);
 	virtqueue_kick(vq);
+	if (!ret)
+		ret = vq->num_free;
 	return ret;
 }
 
@@ -459,7 +461,7 @@ static ssize_t __send_control_msg(struct ports_device
*portdev, u32 port_id,
 	vq = portdev->c_ovq;
 
 	sg_init_one(sg, &cpkt, sizeof(cpkt));
-	if (virtqueue_add_buf(vq, sg, 1, 0, &cpkt, GFP_ATOMIC) >= 0) {
+	if (virtqueue_add_buf(vq, sg, 1, 0, &cpkt, GFP_ATOMIC) == 0) {
 		virtqueue_kick(vq);
 		while (!virtqueue_get_buf(vq, &len))
 			cpu_relax();
@@ -524,7 +526,7 @@ static ssize_t __send_to_port(struct port *port, struct
scatterlist *sg,
 			      struct buffer_token *tok, bool nonblock)
 {
 	struct virtqueue *out_vq;
-	ssize_t ret;
+	int err;
 	unsigned long flags;
 	unsigned int len;
 
@@ -534,17 +536,17 @@ static ssize_t __send_to_port(struct port *port, struct
scatterlist *sg,
 
 	reclaim_consumed_buffers(port);
 
-	ret = virtqueue_add_buf(out_vq, sg, nents, 0, tok, GFP_ATOMIC);
+	err = virtqueue_add_buf(out_vq, sg, nents, 0, tok, GFP_ATOMIC);
 
 	/* Tell Host to go! */
 	virtqueue_kick(out_vq);
 
-	if (ret < 0) {
+	if (err) {
 		in_count = 0;
 		goto done;
 	}
 
-	if (ret == 0)
+	if (out_vq->num_free == 0)
 		port->outvq_full = true;
 
 	if (nonblock)
-- 
1.8.0.1

Amit Shah <amit.shah at redhat.com> writes:
> Hi Rusty,
>
> The linux-next kernel was failing my virtio-console test suite for a
> while.  I looked into it today, and it's due to the
> virtqueue_add_buf() change that doesn't return > 0 values anymore. 
I
> found your commit that adjusts virtio_console.c, but you missed one
> instance where the return value mattered, and as a result not enough
> buffers were queued for the host to send in data.
>
> This resulted in the port's name to be not populated in sysfs, which
> meant udev didn't create any symlinks in /dev/virtio-ports/.
>
> I've just updated your commit with the small diff (attached below).
> Please put this commit after 
>
> commit e794093a52cdfef09b3fdb6294b75ab8cacb30a8
> Author: Rusty Russell <rusty at rustcorp.com.au>
> Date:   Tue Oct 16 23:56:14 2012 +1030
>
>     virtio_net: don't rely on virtqueue_add_buf() returning capacity.
>     
> and before
>
> commit 08d088e8357b3c031db7de006247f613c7f136ab
> Author: Rusty Russell <rusty at rustcorp.com.au>
> Date:   Tue Oct 16 23:56:15 2012 +1030
>
>     virtio: make virtqueue_add_buf() returning 0 on success, not capacity.
>     
> in the pull request you send so that there's no regression in
> virtio_console on bisection.
>
> (commit ids from linux-next/master).
I will append it for now, then fold it before pushing to Linus.  I try
not to rebase linux-next until just before the push.

Thanks,
Rusty.