[PATCH 0/6] virtio: console: Fixes

[PATCH 0/6] virtio: console: Fixes

[Amit Shah]

Hey Rusty,

Here are a few fixes for virtio and virtio_console.

The first patch ensures the data elements of vqs are properly
initialised at allocation-time so that we don't trigger BUG_ONs. I found
this when hot-unplugging ports and there was just one unused buffer.
detach_unused_buffers() kept returning pointers that were invalid. I
didn't catch this earlier as I had the in_vq filled completely.

Patches 2, 4 and 5 can be folded into the series as they are bugfixes
for the functionality present there.

About patch 5: When running a test that transfers a 260M file from the
host to the guest, qemu-kvm.git takes 17m with a single outstanding
buffer in the in_vq vs. 1m when the entire in_vq is filled. This is a
bug in qemu-kvm.git's scheduling, but since it's a big difference and
not much change involved, we could merge this now.

Comments?

If these patches are favourable, I could send you a tarball in private
so that the bugfixes are folded in the series and just patches 1, 3 and
6 are added.

Thanks,

		Amit.

Amit Shah (6):
  virtio: Initialize vq->data entries to NULL
  virtio: console: Ensure no memleaks in case of unused buffers
  virtio: console: Add ability to remove module
  virtio: console: Error out if we can't allocate buffers for control
    queue
  virtio: console: Fill ports' entire in_vq with buffers
  Add MAINTAINERS entry for virtio_console

 MAINTAINERS                   |    6 ++
 drivers/char/virtio_console.c |  132 +++++++++++++++++++++++++++++++---------
 drivers/virtio/virtio_ring.c  |    5 +-
 3 files changed, 112 insertions(+), 31 deletions(-)

[PATCH 1/6] virtio: Initialize vq->data entries to NULL

[Amit Shah]

vq operations depend on vq->data[i] being NULL to figure out if the vq
entry is in use.

We have to initialize them to NULL to ensure we don't work with junk
data and trigger false BUG_ONs.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/virtio/virtio_ring.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 71929ee..9bcfe95 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -431,8 +431,11 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
 	/* Put everything in free lists. */
 	vq->num_free = num;
 	vq->free_head = 0;
-	for (i = 0; i < num-1; i++)
+	for (i = 0; i < num-1; i++) {
 		vq->vring.desc[i].next = i+1;
+		vq->data[i] = NULL;
+	}
+	vq->data[i] = NULL;
 
 	return &vq->vq;
 }
-- 
1.6.2.5

[PATCH 2/6] virtio: console: Ensure no memleaks in case of unused buffers

[Amit Shah]

If unused data exists in in_vq, ensure we flush that first and then
detach unused buffers, which will ensure all buffers from the in_vq are
removed.

Also ensure we free the buffers after detaching them.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index a3f1f73..69d2e61 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -838,6 +838,8 @@ static const struct file_operations port_debugfs_ops = {
 /* Remove all port-specific data. */
 static int remove_port(struct port *port)
 {
+	struct port_buffer *buf;
+
 	spin_lock_irq(&port->portdev->ports_lock);
 	list_del(&port->list);
 	spin_unlock_irq(&port->portdev->ports_lock);
@@ -851,14 +853,17 @@ static int remove_port(struct port *port)
 	if (port->guest_connected)
 		send_control_msg(port, VIRTIO_CONSOLE_PORT_OPEN, 0);
 
-	while (port->in_vq->vq_ops->detach_unused_buf(port->in_vq))
-		;
-
 	sysfs_remove_group(&port->dev->kobj, &port_attribute_group);
 	device_destroy(pdrvdata.class, port->dev->devt);
 	cdev_del(&port->cdev);
 
+	/* Remove unused data this port might have received. */
 	discard_port_data(port);
+
+	/* Remove buffers we queued up for the Host to send us data in. */
+	while ((buf = port->in_vq->vq_ops->detach_unused_buf(port->in_vq)))
+		free_buf(buf);
+
 	kfree(port->name);
 
 	debugfs_remove(port->debugfs_file);
-- 
1.6.2.5

[PATCH 3/6] virtio: console: Add ability to remove module

[Amit Shah]

Add the ability to remove the virtio_console module.

This aids debugging.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c |   41 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 69d2e61..0057bae 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1448,6 +1448,36 @@ fail:
 	return err;
 }
 
+static void virtcons_remove(struct virtio_device *vdev)
+{
+	struct ports_device *portdev;
+	struct port *port, *port2;
+	struct port_buffer *buf;
+	unsigned int len;
+
+	portdev = vdev->priv;
+
+	cancel_work_sync(&portdev->control_work);
+	cancel_work_sync(&portdev->config_work);
+
+	list_for_each_entry_safe(port, port2, &portdev->ports, list)
+		remove_port(port);
+
+	unregister_chrdev(portdev->chr_major, "virtio-portsdev");
+
+	while ((buf = portdev->c_ivq->vq_ops->get_buf(portdev->c_ivq, &len)))
+		free_buf(buf);
+
+	while ((buf = portdev->c_ivq->vq_ops->detach_unused_buf(portdev->c_ivq)))
+		free_buf(buf);
+
+	vdev->config->del_vqs(vdev);
+	kfree(portdev->in_vqs);
+	kfree(portdev->out_vqs);
+
+	kfree(portdev);
+}
+
 static struct virtio_device_id id_table[] = {
 	{ VIRTIO_ID_CONSOLE, VIRTIO_DEV_ANY_ID },
 	{ 0 },
@@ -1465,6 +1495,7 @@ static struct virtio_driver virtio_console = {
 	.driver.owner =	THIS_MODULE,
 	.id_table =	id_table,
 	.probe =	virtcons_probe,
+	.remove =	virtcons_remove,
 	.config_changed = config_intr,
 };
 
@@ -1488,7 +1519,17 @@ static int __init init(void)
 
 	return register_virtio_driver(&virtio_console);
 }
+
+static void __exit fini(void)
+{
+	unregister_virtio_driver(&virtio_console);
+
+	class_destroy(pdrvdata.class);
+	if (pdrvdata.debugfs_dir)
+		debugfs_remove_recursive(pdrvdata.debugfs_dir);
+}
 module_init(init);
+module_exit(fini);
 
 MODULE_DEVICE_TABLE(virtio, id_table);
 MODULE_DESCRIPTION("Virtio console driver");
-- 
1.6.2.5

[PATCH 4/6] virtio: console: Error out if we can't allocate buffers for control queue

[Amit Shah]

With MULTIPORT support, the control queue is an integral part of the
functioning of the device. If we can't get any buffers allocated, the
host won't be able to relay important information and the device may not
function as intended.

Ensure 'probe' doesn't succeed until the control queue has at least one
buffer allocated for its ivq.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c |   27 +++++++++++++++++++++------
 1 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 0057bae..c407037 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1057,25 +1057,30 @@ static void config_intr(struct virtio_device *vdev)
 	resize_console(find_port_by_id(portdev, 0));
 }
 
-static void fill_queue(struct virtqueue *vq, spinlock_t *lock)
+static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
 {
 	struct port_buffer *buf;
-	int ret;
+	unsigned int ret;
+	int err;
 
+	ret = 0;
 	do {
 		buf = alloc_buf(PAGE_SIZE);
 		if (!buf)
 			break;
 
 		spin_lock_irq(lock);
-		ret = add_inbuf(vq, buf);
-		if (ret < 0) {
+		err = add_inbuf(vq, buf);
+		if (err < 0) {
 			spin_unlock_irq(lock);
 			free_buf(buf);
 			break;
 		}
+		ret++;
 		spin_unlock_irq(lock);
-	} while (ret > 0);
+	} while (err > 0);
+
+	return ret;
 }
 
 static int add_port(struct ports_device *portdev, u32 id)
@@ -1430,7 +1435,13 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 		INIT_WORK(&portdev->control_work, &control_work_handler);
 		INIT_WORK(&portdev->config_work, &config_work_handler);
 
-		fill_queue(portdev->c_ivq, &portdev->cvq_lock);
+		err = fill_queue(portdev->c_ivq, &portdev->cvq_lock);
+		if (!err) {
+			dev_err(&vdev->dev,
+				"Error allocating buffers for control queue\n");
+			err = -ENOMEM;
+			goto free_vqs;
+		}
 	}
 
 	for (i = 0; i < portdev->config.nr_ports; i++)
@@ -1440,6 +1451,10 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 	early_put_chars = NULL;
 	return 0;
 
+free_vqs:
+	vdev->config->del_vqs(vdev);
+	kfree(portdev->in_vqs);
+	kfree(portdev->out_vqs);
 free_chrdev:
 	unregister_chrdev(portdev->chr_major, "virtio-portsdev");
 free:
-- 
1.6.2.5

[PATCH 5/6] virtio: console: Fill ports' entire in_vq with buffers

[Amit Shah]

Instead of allocating just one buffer for a port's in_vq, fill
the entire in_vq with buffers so the host need not stall while
an application consumes the data and makes the buffer available
again for the host.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c |   53 ++++++++++++++++++++++++----------------
 1 files changed, 32 insertions(+), 21 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index c407037..213373b 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -330,6 +330,7 @@ static void discard_port_data(struct port *port)
 	struct port_buffer *buf;
 	struct virtqueue *vq;
 	unsigned int len;
+	int ret;
 
 	vq = port->in_vq;
 	if (port->inbuf)
@@ -337,16 +338,18 @@ static void discard_port_data(struct port *port)
 	else
 		buf = vq->vq_ops->get_buf(vq, &len);
 
-	if (!buf)
-		return;
-
-	if (add_inbuf(vq, buf) < 0) {
-		buf->len = buf->offset = 0;
-		dev_warn(port->dev, "Error adding buffer back to vq\n");
-		return;
+	ret = 0;
+	while (buf) {
+		if (add_inbuf(vq, buf) < 0) {
+			ret++;
+			free_buf(buf);
+		}
+		buf = vq->vq_ops->get_buf(vq, &len);
 	}
-
 	port->inbuf = NULL;
+	if (ret)
+		dev_warn(port->dev, "Errors adding %d buffers back to vq\n",
+			 ret);
 }
 
 static bool port_has_data(struct port *port)
@@ -354,12 +357,19 @@ static bool port_has_data(struct port *port)
 	unsigned long flags;
 	bool ret;
 
-	ret = false;
 	spin_lock_irqsave(&port->inbuf_lock, flags);
-	if (port->inbuf)
+	if (port->inbuf) {
 		ret = true;
+		goto out;
+	}
+	port->inbuf = get_inbuf(port);
+	if (port->inbuf) {
+		ret = true;
+		goto out;
+	}
+	ret = false;
+out:
 	spin_unlock_irqrestore(&port->inbuf_lock, flags);
-
 	return ret;
 }
 
@@ -1011,7 +1021,8 @@ static void in_intr(struct virtqueue *vq)
 		return;
 
 	spin_lock_irqsave(&port->inbuf_lock, flags);
-	port->inbuf = get_inbuf(port);
+	if (!port->inbuf)
+		port->inbuf = get_inbuf(port);
 
 	/*
 	 * Don't queue up data when port is closed.  This condition
@@ -1087,7 +1098,7 @@ static int add_port(struct ports_device *portdev, u32 id)
 {
 	char debugfs_name[16];
 	struct port *port;
-	struct port_buffer *inbuf;
+	struct port_buffer *buf;
 	dev_t devt;
 	int err;
 
@@ -1132,22 +1143,21 @@ static int add_port(struct ports_device *portdev, u32 id)
 	spin_lock_init(&port->inbuf_lock);
 	init_waitqueue_head(&port->waitqueue);
 
-	inbuf = alloc_buf(PAGE_SIZE);
-	if (!inbuf) {
+	/* Fill the in_vq with buffers so the host can send us data. */
+	err = fill_queue(port->in_vq, &port->inbuf_lock);
+	if (!err) {
+		dev_err(port->dev, "Error allocating inbufs\n");
 		err = -ENOMEM;
 		goto free_device;
 	}
 
-	/* Register the input buffer the first time. */
-	add_inbuf(port->in_vq, inbuf);
-
 	/*
 	 * If we're not using multiport support, this has to be a console port
 	 */
 	if (!use_multiport(port->portdev)) {
 		err = init_port_console(port);
 		if (err)
-			goto free_inbuf;
+			goto free_inbufs;
 	}
 
 	spin_lock_irq(&portdev->ports_lock);
@@ -1175,8 +1185,9 @@ static int add_port(struct ports_device *portdev, u32 id)
 	}
 	return 0;
 
-free_inbuf:
-	free_buf(inbuf);
+free_inbufs:
+	while ((buf = port->in_vq->vq_ops->detach_unused_buf(port->in_vq)))
+		free_buf(buf);
 free_device:
 	device_destroy(pdrvdata.class, port->dev->devt);
 free_cdev:
-- 
1.6.2.5

[PATCH 6/6] Add MAINTAINERS entry for virtio_console

[Amit Shah]

I'm taking ownership of the virtio_console module; but I'll continue
feeding patches via Rusty.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 MAINTAINERS |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 03f38c1..3118dfa 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2394,6 +2394,12 @@ L:	linuxppc-dev@ozlabs.org
 S:	Odd Fixes
 F:	drivers/char/hvc_*
 
+VIRTIO CONSOLE DRIVER
+M:	Amit Shah <amit.shah@redhat.com>
+L:	virtualization@lists.linux-foundation.org
+S:	Maintained
+F:	drivers/char/virtio_console.c
+
 GSPCA FINEPIX SUBDRIVER
 M:	Frank Zago <frank@zago.net>
 L:	linux-media@vger.kernel.org
-- 
1.6.2.5

Re: [PATCH 0/6] virtio: console: Fixes

[Rusty Russell]

On Fri, 12 Feb 2010 03:32:13 pm Amit Shah wrote:
> Hey Rusty,
> 
> Here are a few fixes for virtio and virtio_console.
> 
> The first patch ensures the data elements of vqs are properly
> initialised at allocation-time so that we don't trigger BUG_ONs. I found
> this when hot-unplugging ports and there was just one unused buffer.
> detach_unused_buffers() kept returning pointers that were invalid. I
> didn't catch this earlier as I had the in_vq filled completely.
> 
> Patches 2, 4 and 5 can be folded into the series as they are bugfixes
> for the functionality present there.
> 
> About patch 5: When running a test that transfers a 260M file from the
> host to the guest, qemu-kvm.git takes 17m with a single outstanding
> buffer in the in_vq vs. 1m when the entire in_vq is filled. This is a
> bug in qemu-kvm.git's scheduling, but since it's a big difference and
> not much change involved, we could merge this now.
> 
> Comments?
> 
> If these patches are favourable, I could send you a tarball in private
> so that the bugfixes are folded in the series and just patches 1, 3 and
> 6 are added.

I prefer to fold them myself, after they've spent some time in linux-next.

Thanks!
Rusty.

Re: [PATCH 0/6] virtio: console: Fixes

[Amit Shah]

On (Fri) Feb 12 2010 [19:34:58], Rusty Russell wrote:
> On Fri, 12 Feb 2010 03:32:13 pm Amit Shah wrote:
> > Hey Rusty,
> > 
> > Here are a few fixes for virtio and virtio_console.
> > 
> > The first patch ensures the data elements of vqs are properly
> > initialised at allocation-time so that we don't trigger BUG_ONs. I found
> > this when hot-unplugging ports and there was just one unused buffer.
> > detach_unused_buffers() kept returning pointers that were invalid. I
> > didn't catch this earlier as I had the in_vq filled completely.
> > 
> > Patches 2, 4 and 5 can be folded into the series as they are bugfixes
> > for the functionality present there.
> > 
> > About patch 5: When running a test that transfers a 260M file from the
> > host to the guest, qemu-kvm.git takes 17m with a single outstanding
> > buffer in the in_vq vs. 1m when the entire in_vq is filled. This is a
> > bug in qemu-kvm.git's scheduling, but since it's a big difference and
> > not much change involved, we could merge this now.
> > 
> > Comments?
> > 
> > If these patches are favourable, I could send you a tarball in private
> > so that the bugfixes are folded in the series and just patches 1, 3 and
> > 6 are added.
> 
> I prefer to fold them myself, after they've spent some time in linux-next.

Fine by me.

BTW should patch 1 be considered for stable?

Thanks!

		Amit

Re: [PATCH 1/6] virtio: Initialize vq->data entries to NULL

[Rusty Russell]

On Fri, 12 Feb 2010 03:32:14 pm Amit Shah wrote:
> vq operations depend on vq->data[i] being NULL to figure out if the vq
> entry is in use.
> 
> We have to initialize them to NULL to ensure we don't work with junk
> data and trigger false BUG_ONs.

Not really.  We wouldn't detect some bogus get_buf() calls if it's not NULL,
but we only *need* this since detach_bufs was added.

I've updated the commit message to reflect it.

Thanks,
Rusty.