Laurent Vivier
2019-Nov-14 12:25 UTC
[PATCH v3] virtio_console: allocate inbufs in add_port() only if it is needed
When we hot unplug a virtserialport and then try to hot plug again, it fails: (qemu) chardev-add socket,id=serial0,path=/tmp/serial0,server,nowait (qemu) device_add virtserialport,bus=virtio-serial0.0,nr=2,\ chardev=serial0,id=serial0,name=serial0 (qemu) device_del serial0 (qemu) device_add virtserialport,bus=virtio-serial0.0,nr=2,\ chardev=serial0,id=serial0,name=serial0 kernel error: virtio-ports vport2p2: Error allocating inbufs qemu error: virtio-serial-bus: Guest failure in adding port 2 for device \ virtio-serial0.0 This happens because buffers for the in_vq are allocated when the port is added but are not released when the port is unplugged. They are only released when virtconsole is removed (see a7a69ec0d8e4) To avoid the problem and to be symmetric, we could allocate all the buffers in init_vqs() as they are released in remove_vqs(), but it sounds like a waste of memory. Rather than that, this patch changes add_port() logic to ignore ENOSPC error in fill_queue(), which means queue has already been filled. Fixes: a7a69ec0d8e4 ("virtio_console: free buffers after reset") Cc: mst at redhat.com Cc: stable at vger.kernel.org Signed-off-by: Laurent Vivier <lvivier at redhat.com> --- Notes: v3: add a comment about ENOSPC error v2: making fill_queue return int and testing return code for -ENOSPC drivers/char/virtio_console.c | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c index 7270e7b69262..3259426f01dc 100644 --- a/drivers/char/virtio_console.c +++ b/drivers/char/virtio_console.c @@ -1325,24 +1325,24 @@ static void set_console_size(struct port *port, u16 rows, u16 cols) port->cons.ws.ws_col = cols; } -static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock) +static int fill_queue(struct virtqueue *vq, spinlock_t *lock) { struct port_buffer *buf; - unsigned int nr_added_bufs; + int nr_added_bufs; int ret; nr_added_bufs = 0; do { buf = alloc_buf(vq->vdev, PAGE_SIZE, 0); if (!buf) - break; + return -ENOMEM; spin_lock_irq(lock); ret = add_inbuf(vq, buf); if (ret < 0) { spin_unlock_irq(lock); free_buf(buf, true); - break; + return ret; } nr_added_bufs++; spin_unlock_irq(lock); @@ -1362,7 +1362,6 @@ static int add_port(struct ports_device *portdev, u32 id) char debugfs_name[16]; struct port *port; dev_t devt; - unsigned int nr_added_bufs; int err; port = kmalloc(sizeof(*port), GFP_KERNEL); @@ -1421,11 +1420,13 @@ static int add_port(struct ports_device *portdev, u32 id) spin_lock_init(&port->outvq_lock); init_waitqueue_head(&port->waitqueue); - /* Fill the in_vq with buffers so the host can send us data. */ - nr_added_bufs = fill_queue(port->in_vq, &port->inbuf_lock); - if (!nr_added_bufs) { + /* We can safely ignore ENOSPC because it means + * the queue already has buffers. Buffers are removed + * only by virtcons_remove(), not by unplug_port() + */ + err = fill_queue(port->in_vq, &port->inbuf_lock); + if (err < 0 && err != -ENOSPC) { dev_err(port->dev, "Error allocating inbufs\n"); - err = -ENOMEM; goto free_device; } @@ -2059,14 +2060,11 @@ static int virtcons_probe(struct virtio_device *vdev) INIT_WORK(&portdev->control_work, &control_work_handler); if (multiport) { - unsigned int nr_added_bufs; - spin_lock_init(&portdev->c_ivq_lock); spin_lock_init(&portdev->c_ovq_lock); - nr_added_bufs = fill_queue(portdev->c_ivq, - &portdev->c_ivq_lock); - if (!nr_added_bufs) { + err = fill_queue(portdev->c_ivq, &portdev->c_ivq_lock); + if (err < 0) { dev_err(&vdev->dev, "Error allocating buffers for control queue\n"); /* @@ -2077,7 +2075,7 @@ static int virtcons_probe(struct virtio_device *vdev) VIRTIO_CONSOLE_DEVICE_READY, 0); /* Device was functional: we need full cleanup. */ virtcons_remove(vdev); - return -ENOMEM; + return err; } } else { /* -- 2.23.0
Sasha Levin
2019-Nov-22 07:00 UTC
[PATCH v3] virtio_console: allocate inbufs in add_port() only if it is needed
Hi, [This is an automated email] This commit has been processed because it contains a "Fixes:" tag, fixing commit: a7a69ec0d8e4 ("virtio_console: free buffers after reset"). The bot has tested the following trees: v5.3.11, v4.19.84, v4.14.154, v4.9.201, v4.4.201. v5.3.11: Build OK! v4.19.84: Build OK! v4.14.154: Build OK! v4.9.201: Failed to apply! Possible dependencies: 5c60300d68da ("virtio_console: reset on out of memory") v4.4.201: Failed to apply! Possible dependencies: 5c60300d68da ("virtio_console: reset on out of memory") NOTE: The patch will not be queued to stable trees until it is upstream. How should we proceed with this patch? -- Thanks, Sasha
Amit Shah
2019-Dec-03 14:46 UTC
[PATCH v3] virtio_console: allocate inbufs in add_port() only if it is needed
On Thu, 2019-11-14 at 13:25 +0100, Laurent Vivier wrote:> When we hot unplug a virtserialport and then try to hot plug again, > it fails: > > (qemu) chardev-add socket,id=serial0,path=/tmp/serial0,server,nowait > (qemu) device_add virtserialport,bus=virtio-serial0.0,nr=2,\ > chardev=serial0,id=serial0,name=serial0 > (qemu) device_del serial0 > (qemu) device_add virtserialport,bus=virtio-serial0.0,nr=2,\ > chardev=serial0,id=serial0,name=serial0 > kernel error: > virtio-ports vport2p2: Error allocating inbufs > qemu error: > virtio-serial-bus: Guest failure in adding port 2 for device \ > virtio-serial0.0 > > This happens because buffers for the in_vq are allocated when the > port is > added but are not released when the port is unplugged. > > They are only released when virtconsole is removed (see a7a69ec0d8e4) > > To avoid the problem and to be symmetric, we could allocate all the > buffers > in init_vqs() as they are released in remove_vqs(), but it sounds > like > a waste of memory. > > Rather than that, this patch changes add_port() logic to ignore > ENOSPC > error in fill_queue(), which means queue has already been filled. > > Fixes: a7a69ec0d8e4 ("virtio_console: free buffers after reset") > Cc: mst at redhat.com > Cc: stable at vger.kernel.org > Signed-off-by: Laurent Vivier <lvivier at redhat.com>Reviewed-by: Amit Shah <amit at kernel.org> Thanks!> --- > > Notes: > v3: add a comment about ENOSPC error > v2: making fill_queue return int and testing return code for > -ENOSPC > > drivers/char/virtio_console.c | 28 +++++++++++++--------------- > 1 file changed, 13 insertions(+), 15 deletions(-) > > diff --git a/drivers/char/virtio_console.c > b/drivers/char/virtio_console.c > index 7270e7b69262..3259426f01dc 100644 > --- a/drivers/char/virtio_console.c > +++ b/drivers/char/virtio_console.c > @@ -1325,24 +1325,24 @@ static void set_console_size(struct port > *port, u16 rows, u16 cols) > port->cons.ws.ws_col = cols; > } > > -static unsigned int fill_queue(struct virtqueue *vq, spinlock_t > *lock) > +static int fill_queue(struct virtqueue *vq, spinlock_t *lock) > { > struct port_buffer *buf; > - unsigned int nr_added_bufs; > + int nr_added_bufs; > int ret; > > nr_added_bufs = 0; > do { > buf = alloc_buf(vq->vdev, PAGE_SIZE, 0); > if (!buf) > - break; > + return -ENOMEM; > > spin_lock_irq(lock); > ret = add_inbuf(vq, buf); > if (ret < 0) { > spin_unlock_irq(lock); > free_buf(buf, true); > - break; > + return ret; > } > nr_added_bufs++; > spin_unlock_irq(lock); > @@ -1362,7 +1362,6 @@ static int add_port(struct ports_device > *portdev, u32 id) > char debugfs_name[16]; > struct port *port; > dev_t devt; > - unsigned int nr_added_bufs; > int err; > > port = kmalloc(sizeof(*port), GFP_KERNEL); > @@ -1421,11 +1420,13 @@ static int add_port(struct ports_device > *portdev, u32 id) > spin_lock_init(&port->outvq_lock); > init_waitqueue_head(&port->waitqueue); > > - /* Fill the in_vq with buffers so the host can send us data. */ > - nr_added_bufs = fill_queue(port->in_vq, &port->inbuf_lock); > - if (!nr_added_bufs) { > + /* We can safely ignore ENOSPC because it means > + * the queue already has buffers. Buffers are removed > + * only by virtcons_remove(), not by unplug_port() > + */ > + err = fill_queue(port->in_vq, &port->inbuf_lock); > + if (err < 0 && err != -ENOSPC) { > dev_err(port->dev, "Error allocating inbufs\n"); > - err = -ENOMEM; > goto free_device; > } > > @@ -2059,14 +2060,11 @@ static int virtcons_probe(struct > virtio_device *vdev) > INIT_WORK(&portdev->control_work, &control_work_handler); > > if (multiport) { > - unsigned int nr_added_bufs; > - > spin_lock_init(&portdev->c_ivq_lock); > spin_lock_init(&portdev->c_ovq_lock); > > - nr_added_bufs = fill_queue(portdev->c_ivq, > - &portdev->c_ivq_lock); > - if (!nr_added_bufs) { > + err = fill_queue(portdev->c_ivq, &portdev->c_ivq_lock); > + if (err < 0) { > dev_err(&vdev->dev, > "Error allocating buffers for control > queue\n"); > /* > @@ -2077,7 +2075,7 @@ static int virtcons_probe(struct virtio_device > *vdev) > VIRTIO_CONSOLE_DEVICE_READY, > 0); > /* Device was functional: we need full cleanup. > */ > virtcons_remove(vdev); > - return -ENOMEM; > + return err; > } > } else { > /*
Michael S. Tsirkin
2019-Dec-03 15:42 UTC
[PATCH v3] virtio_console: allocate inbufs in add_port() only if it is needed
On Tue, Dec 03, 2019 at 03:46:50PM +0100, Amit Shah wrote:> On Thu, 2019-11-14 at 13:25 +0100, Laurent Vivier wrote: > > When we hot unplug a virtserialport and then try to hot plug again, > > it fails: > > > > (qemu) chardev-add socket,id=serial0,path=/tmp/serial0,server,nowait > > (qemu) device_add virtserialport,bus=virtio-serial0.0,nr=2,\ > > chardev=serial0,id=serial0,name=serial0 > > (qemu) device_del serial0 > > (qemu) device_add virtserialport,bus=virtio-serial0.0,nr=2,\ > > chardev=serial0,id=serial0,name=serial0 > > kernel error: > > virtio-ports vport2p2: Error allocating inbufs > > qemu error: > > virtio-serial-bus: Guest failure in adding port 2 for device \ > > virtio-serial0.0 > > > > This happens because buffers for the in_vq are allocated when the > > port is > > added but are not released when the port is unplugged. > > > > They are only released when virtconsole is removed (see a7a69ec0d8e4) > > > > To avoid the problem and to be symmetric, we could allocate all the > > buffers > > in init_vqs() as they are released in remove_vqs(), but it sounds > > like > > a waste of memory. > > > > Rather than that, this patch changes add_port() logic to ignore > > ENOSPC > > error in fill_queue(), which means queue has already been filled. > > > > Fixes: a7a69ec0d8e4 ("virtio_console: free buffers after reset") > > Cc: mst at redhat.com > > Cc: stable at vger.kernel.org > > Signed-off-by: Laurent Vivier <lvivier at redhat.com> > > Reviewed-by: Amit Shah <amit at kernel.org> > > Thanks!Thanks, however this has already been merged by Linus. I can't add the tag retroactively, sorry about that. For bugfix patches like that, I think we can reasonably target a turn around of a couple of days, these shouldn't really need to wait weeks for review.> > --- > > > > Notes: > > v3: add a comment about ENOSPC error > > v2: making fill_queue return int and testing return code for > > -ENOSPC > > > > drivers/char/virtio_console.c | 28 +++++++++++++--------------- > > 1 file changed, 13 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/char/virtio_console.c > > b/drivers/char/virtio_console.c > > index 7270e7b69262..3259426f01dc 100644 > > --- a/drivers/char/virtio_console.c > > +++ b/drivers/char/virtio_console.c > > @@ -1325,24 +1325,24 @@ static void set_console_size(struct port > > *port, u16 rows, u16 cols) > > port->cons.ws.ws_col = cols; > > } > > > > -static unsigned int fill_queue(struct virtqueue *vq, spinlock_t > > *lock) > > +static int fill_queue(struct virtqueue *vq, spinlock_t *lock) > > { > > struct port_buffer *buf; > > - unsigned int nr_added_bufs; > > + int nr_added_bufs; > > int ret; > > > > nr_added_bufs = 0; > > do { > > buf = alloc_buf(vq->vdev, PAGE_SIZE, 0); > > if (!buf) > > - break; > > + return -ENOMEM; > > > > spin_lock_irq(lock); > > ret = add_inbuf(vq, buf); > > if (ret < 0) { > > spin_unlock_irq(lock); > > free_buf(buf, true); > > - break; > > + return ret; > > } > > nr_added_bufs++; > > spin_unlock_irq(lock); > > @@ -1362,7 +1362,6 @@ static int add_port(struct ports_device > > *portdev, u32 id) > > char debugfs_name[16]; > > struct port *port; > > dev_t devt; > > - unsigned int nr_added_bufs; > > int err; > > > > port = kmalloc(sizeof(*port), GFP_KERNEL); > > @@ -1421,11 +1420,13 @@ static int add_port(struct ports_device > > *portdev, u32 id) > > spin_lock_init(&port->outvq_lock); > > init_waitqueue_head(&port->waitqueue); > > > > - /* Fill the in_vq with buffers so the host can send us data. */ > > - nr_added_bufs = fill_queue(port->in_vq, &port->inbuf_lock); > > - if (!nr_added_bufs) { > > + /* We can safely ignore ENOSPC because it means > > + * the queue already has buffers. Buffers are removed > > + * only by virtcons_remove(), not by unplug_port() > > + */ > > + err = fill_queue(port->in_vq, &port->inbuf_lock); > > + if (err < 0 && err != -ENOSPC) { > > dev_err(port->dev, "Error allocating inbufs\n"); > > - err = -ENOMEM; > > goto free_device; > > } > > > > @@ -2059,14 +2060,11 @@ static int virtcons_probe(struct > > virtio_device *vdev) > > INIT_WORK(&portdev->control_work, &control_work_handler); > > > > if (multiport) { > > - unsigned int nr_added_bufs; > > - > > spin_lock_init(&portdev->c_ivq_lock); > > spin_lock_init(&portdev->c_ovq_lock); > > > > - nr_added_bufs = fill_queue(portdev->c_ivq, > > - &portdev->c_ivq_lock); > > - if (!nr_added_bufs) { > > + err = fill_queue(portdev->c_ivq, &portdev->c_ivq_lock); > > + if (err < 0) { > > dev_err(&vdev->dev, > > "Error allocating buffers for control > > queue\n"); > > /* > > @@ -2077,7 +2075,7 @@ static int virtcons_probe(struct virtio_device > > *vdev) > > VIRTIO_CONSOLE_DEVICE_READY, > > 0); > > /* Device was functional: we need full cleanup. > > */ > > virtcons_remove(vdev); > > - return -ENOMEM; > > + return err; > > } > > } else { > > /*
Seemingly Similar Threads
- [PATCH v2] virtio_console: allocate inbufs in add_port() only if it is needed
- [PATCH v2] virtio_console: allocate inbufs in add_port() only if it is needed
- [PATCH v2] virtio_console: allocate inbufs in add_port() only if it is needed
- [PATCH v2] virtio_console: allocate inbufs in add_port() only if it is needed
- [PATCH v3] virtio_console: allocate inbufs in add_port() only if it is needed