[PATCH 0/1] virtio: console: regression in virtqueue_add_buf() change

[PATCH 0/1] virtio: console: regression in virtqueue_add_buf() change

[Amit Shah]

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

[PATCH 1/1] virtio: console: virtqueue_add_buf() no longer returns > 0

[Amit Shah]

From: Rusty Russell <rusty@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@rustcorp.com.au>
Signed-off-by: Amit Shah <amit.shah@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

Re: [PATCH 0/1] virtio: console: regression in virtqueue_add_buf() change

[Rusty Russell]

Amit Shah <amit.shah@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@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@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.