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(-)
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 at 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
Amit Shah
2010-Feb-12 05:02 UTC
[PATCH 2/6] virtio: console: Ensure no memleaks in case of unused buffers
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 at 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
Add the ability to remove the virtio_console module.
This aids debugging.
Signed-off-by: Amit Shah <amit.shah at 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
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.