* [PATCH 0/6] virtio: console: Fixes
@ 2010-02-12 5:02 Amit Shah
2010-02-12 5:02 ` [PATCH 1/6] virtio: Initialize vq->data entries to NULL Amit Shah
2010-02-12 9:04 ` [PATCH 0/6] virtio: console: Fixes Rusty Russell
0 siblings, 2 replies; 10+ messages in thread
From: Amit Shah @ 2010-02-12 5:02 UTC (permalink / raw)
To: rusty; +Cc: Amit Shah, virtualization
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(-)
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/6] virtio: Initialize vq->data entries to NULL 2010-02-12 5:02 [PATCH 0/6] virtio: console: Fixes Amit Shah @ 2010-02-12 5:02 ` Amit Shah 2010-02-12 5:02 ` [PATCH 2/6] virtio: console: Ensure no memleaks in case of unused buffers Amit Shah 2010-02-12 9:58 ` [PATCH 1/6] virtio: Initialize vq->data entries to NULL Rusty Russell 2010-02-12 9:04 ` [PATCH 0/6] virtio: console: Fixes Rusty Russell 1 sibling, 2 replies; 10+ messages in thread From: Amit Shah @ 2010-02-12 5:02 UTC (permalink / raw) To: rusty; +Cc: Amit Shah, virtualization 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/6] virtio: console: Ensure no memleaks in case of unused buffers 2010-02-12 5:02 ` [PATCH 1/6] virtio: Initialize vq->data entries to NULL Amit Shah @ 2010-02-12 5:02 ` Amit Shah 2010-02-12 5:02 ` [PATCH 3/6] virtio: console: Add ability to remove module Amit Shah 2010-02-12 9:58 ` [PATCH 1/6] virtio: Initialize vq->data entries to NULL Rusty Russell 1 sibling, 1 reply; 10+ messages in thread From: Amit Shah @ 2010-02-12 5:02 UTC (permalink / raw) To: rusty; +Cc: Amit Shah, virtualization 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/6] virtio: console: Add ability to remove module 2010-02-12 5:02 ` [PATCH 2/6] virtio: console: Ensure no memleaks in case of unused buffers Amit Shah @ 2010-02-12 5:02 ` Amit Shah 2010-02-12 5:02 ` [PATCH 4/6] virtio: console: Error out if we can't allocate buffers for control queue Amit Shah 0 siblings, 1 reply; 10+ messages in thread From: Amit Shah @ 2010-02-12 5:02 UTC (permalink / raw) To: rusty; +Cc: Amit Shah, virtualization 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/6] virtio: console: Error out if we can't allocate buffers for control queue 2010-02-12 5:02 ` [PATCH 3/6] virtio: console: Add ability to remove module Amit Shah @ 2010-02-12 5:02 ` Amit Shah 2010-02-12 5:02 ` [PATCH 5/6] virtio: console: Fill ports' entire in_vq with buffers Amit Shah 0 siblings, 1 reply; 10+ messages in thread From: Amit Shah @ 2010-02-12 5:02 UTC (permalink / raw) To: rusty; +Cc: Amit Shah, virtualization 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 5/6] virtio: console: Fill ports' entire in_vq with buffers 2010-02-12 5:02 ` [PATCH 4/6] virtio: console: Error out if we can't allocate buffers for control queue Amit Shah @ 2010-02-12 5:02 ` Amit Shah 2010-02-12 5:02 ` [PATCH 6/6] Add MAINTAINERS entry for virtio_console Amit Shah 0 siblings, 1 reply; 10+ messages in thread From: Amit Shah @ 2010-02-12 5:02 UTC (permalink / raw) To: rusty; +Cc: Amit Shah, virtualization 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 6/6] Add MAINTAINERS entry for virtio_console 2010-02-12 5:02 ` [PATCH 5/6] virtio: console: Fill ports' entire in_vq with buffers Amit Shah @ 2010-02-12 5:02 ` Amit Shah 0 siblings, 0 replies; 10+ messages in thread From: Amit Shah @ 2010-02-12 5:02 UTC (permalink / raw) To: rusty; +Cc: Amit Shah, virtualization 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/6] virtio: Initialize vq->data entries to NULL 2010-02-12 5:02 ` [PATCH 1/6] virtio: Initialize vq->data entries to NULL Amit Shah 2010-02-12 5:02 ` [PATCH 2/6] virtio: console: Ensure no memleaks in case of unused buffers Amit Shah @ 2010-02-12 9:58 ` Rusty Russell 1 sibling, 0 replies; 10+ messages in thread From: Rusty Russell @ 2010-02-12 9:58 UTC (permalink / raw) To: Amit Shah; +Cc: Shirley Ma, virtualization 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/6] virtio: console: Fixes 2010-02-12 5:02 [PATCH 0/6] virtio: console: Fixes Amit Shah 2010-02-12 5:02 ` [PATCH 1/6] virtio: Initialize vq->data entries to NULL Amit Shah @ 2010-02-12 9:04 ` Rusty Russell 2010-02-12 9:07 ` Amit Shah 1 sibling, 1 reply; 10+ messages in thread From: Rusty Russell @ 2010-02-12 9:04 UTC (permalink / raw) To: Amit Shah; +Cc: virtualization 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/6] virtio: console: Fixes 2010-02-12 9:04 ` [PATCH 0/6] virtio: console: Fixes Rusty Russell @ 2010-02-12 9:07 ` Amit Shah 0 siblings, 0 replies; 10+ messages in thread From: Amit Shah @ 2010-02-12 9:07 UTC (permalink / raw) To: Rusty Russell; +Cc: virtualization 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2010-02-12 9:58 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-02-12 5:02 [PATCH 0/6] virtio: console: Fixes Amit Shah 2010-02-12 5:02 ` [PATCH 1/6] virtio: Initialize vq->data entries to NULL Amit Shah 2010-02-12 5:02 ` [PATCH 2/6] virtio: console: Ensure no memleaks in case of unused buffers Amit Shah 2010-02-12 5:02 ` [PATCH 3/6] virtio: console: Add ability to remove module Amit Shah 2010-02-12 5:02 ` [PATCH 4/6] virtio: console: Error out if we can't allocate buffers for control queue Amit Shah 2010-02-12 5:02 ` [PATCH 5/6] virtio: console: Fill ports' entire in_vq with buffers Amit Shah 2010-02-12 5:02 ` [PATCH 6/6] Add MAINTAINERS entry for virtio_console Amit Shah 2010-02-12 9:58 ` [PATCH 1/6] virtio: Initialize vq->data entries to NULL Rusty Russell 2010-02-12 9:04 ` [PATCH 0/6] virtio: console: Fixes Rusty Russell 2010-02-12 9:07 ` Amit Shah
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).