LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] virtio_console: allocate inbufs in add_port() only if it is needed
@ 2019-10-18 16:47 Laurent Vivier
  2019-11-06 13:53 ` Laurent Vivier
  2019-11-06 13:56 ` Michael S. Tsirkin
  0 siblings, 2 replies; 7+ messages in thread
From: Laurent Vivier @ 2019-10-18 16:47 UTC (permalink / raw)
  To: linux-kernel
  Cc: Greg Kroah-Hartman, Amit Shah, virtualization, Arnd Bergmann,
	Michael S . Tsirkin, Laurent Vivier

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 only allocate the
buffers if the in_vq has available free slots.

Fixes: a7a69ec0d8e4 ("virtio_console: free buffers after reset")
Cc: mst@redhat.com
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 drivers/char/virtio_console.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 7270e7b69262..77105166fe01 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1421,12 +1421,17 @@ 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) {
-		dev_err(port->dev, "Error allocating inbufs\n");
-		err = -ENOMEM;
-		goto free_device;
+	/* if the in_vq has not already been filled (the port has already been
+	 * used and unplugged), fill the in_vq with buffers so the host can
+	 * send us data.
+	 */
+	if (port->in_vq->num_free != 0) {
+		nr_added_bufs = fill_queue(port->in_vq, &port->inbuf_lock);
+		if (!nr_added_bufs) {
+			dev_err(port->dev, "Error allocating inbufs\n");
+			err = -ENOMEM;
+			goto free_device;
+		}
 	}
 
 	if (is_rproc_serial(port->portdev->vdev))
-- 
2.21.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] virtio_console: allocate inbufs in add_port() only if it is needed
  2019-10-18 16:47 [PATCH] virtio_console: allocate inbufs in add_port() only if it is needed Laurent Vivier
@ 2019-11-06 13:53 ` Laurent Vivier
  2019-11-06 13:56 ` Michael S. Tsirkin
  1 sibling, 0 replies; 7+ messages in thread
From: Laurent Vivier @ 2019-11-06 13:53 UTC (permalink / raw)
  To: linux-kernel
  Cc: Greg Kroah-Hartman, Amit Shah, virtualization, Arnd Bergmann,
	Michael S . Tsirkin

Any comments?

Thanks,
Laurent

On 18/10/2019 18:47, 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 only allocate the
> buffers if the in_vq has available free slots.
> 
> Fixes: a7a69ec0d8e4 ("virtio_console: free buffers after reset")
> Cc: mst@redhat.com
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  drivers/char/virtio_console.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 7270e7b69262..77105166fe01 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1421,12 +1421,17 @@ 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) {
> -		dev_err(port->dev, "Error allocating inbufs\n");
> -		err = -ENOMEM;
> -		goto free_device;
> +	/* if the in_vq has not already been filled (the port has already been
> +	 * used and unplugged), fill the in_vq with buffers so the host can
> +	 * send us data.
> +	 */
> +	if (port->in_vq->num_free != 0) {
> +		nr_added_bufs = fill_queue(port->in_vq, &port->inbuf_lock);
> +		if (!nr_added_bufs) {
> +			dev_err(port->dev, "Error allocating inbufs\n");
> +			err = -ENOMEM;
> +			goto free_device;
> +		}
>  	}
>  
>  	if (is_rproc_serial(port->portdev->vdev))
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] virtio_console: allocate inbufs in add_port() only if it is needed
  2019-10-18 16:47 [PATCH] virtio_console: allocate inbufs in add_port() only if it is needed Laurent Vivier
  2019-11-06 13:53 ` Laurent Vivier
@ 2019-11-06 13:56 ` Michael S. Tsirkin
  2019-11-06 14:02   ` Laurent Vivier
  1 sibling, 1 reply; 7+ messages in thread
From: Michael S. Tsirkin @ 2019-11-06 13:56 UTC (permalink / raw)
  To: Laurent Vivier
  Cc: linux-kernel, Greg Kroah-Hartman, Amit Shah, virtualization,
	Arnd Bergmann

On Fri, Oct 18, 2019 at 06:47:18PM +0200, 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 only allocate the
> buffers if the in_vq has available free slots.
> 
> Fixes: a7a69ec0d8e4 ("virtio_console: free buffers after reset")
> Cc: mst@redhat.com
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  drivers/char/virtio_console.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 7270e7b69262..77105166fe01 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1421,12 +1421,17 @@ 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) {
> -		dev_err(port->dev, "Error allocating inbufs\n");
> -		err = -ENOMEM;
> -		goto free_device;
> +	/* if the in_vq has not already been filled (the port has already been
> +	 * used and unplugged), fill the in_vq with buffers so the host can
> +	 * send us data.
> +	 */
> +	if (port->in_vq->num_free != 0) {
> +		nr_added_bufs = fill_queue(port->in_vq, &port->inbuf_lock);
> +		if (!nr_added_bufs) {
> +			dev_err(port->dev, "Error allocating inbufs\n");
> +			err = -ENOMEM;
> +			goto free_device;
> +		}
>  	}
>  
>  	if (is_rproc_serial(port->portdev->vdev))

Well fill_queue will just add slots as long as it can.
So on a full queue it does nothing. How does this patch help?

> -- 
> 2.21.0

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] virtio_console: allocate inbufs in add_port() only if it is needed
  2019-11-06 13:56 ` Michael S. Tsirkin
@ 2019-11-06 14:02   ` Laurent Vivier
  2019-11-06 15:03     ` Michael S. Tsirkin
  0 siblings, 1 reply; 7+ messages in thread
From: Laurent Vivier @ 2019-11-06 14:02 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: linux-kernel, Greg Kroah-Hartman, Amit Shah, virtualization,
	Arnd Bergmann

On 06/11/2019 14:56, Michael S. Tsirkin wrote:
> On Fri, Oct 18, 2019 at 06:47:18PM +0200, 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 only allocate the
>> buffers if the in_vq has available free slots.
>>
>> Fixes: a7a69ec0d8e4 ("virtio_console: free buffers after reset")
>> Cc: mst@redhat.com
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>  drivers/char/virtio_console.c | 17 +++++++++++------
>>  1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
>> index 7270e7b69262..77105166fe01 100644
>> --- a/drivers/char/virtio_console.c
>> +++ b/drivers/char/virtio_console.c
>> @@ -1421,12 +1421,17 @@ 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) {
>> -		dev_err(port->dev, "Error allocating inbufs\n");
>> -		err = -ENOMEM;
>> -		goto free_device;
>> +	/* if the in_vq has not already been filled (the port has already been
>> +	 * used and unplugged), fill the in_vq with buffers so the host can
>> +	 * send us data.
>> +	 */
>> +	if (port->in_vq->num_free != 0) {
>> +		nr_added_bufs = fill_queue(port->in_vq, &port->inbuf_lock);
>> +		if (!nr_added_bufs) {
>> +			dev_err(port->dev, "Error allocating inbufs\n");
>> +			err = -ENOMEM;
>> +			goto free_device;
>> +		}
>>  	}
>>  
>>  	if (is_rproc_serial(port->portdev->vdev))
> 
> Well fill_queue will just add slots as long as it can.
> So on a full queue it does nothing. How does this patch help?

Yes, but in this case it returns 0 and so add_port() fails and exits
with -ENOMEM and the device is freed. It's what this patch tries to avoid.

Thanks,
Laurent


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] virtio_console: allocate inbufs in add_port() only if it is needed
  2019-11-06 14:02   ` Laurent Vivier
@ 2019-11-06 15:03     ` Michael S. Tsirkin
  2019-11-06 15:41       ` Laurent Vivier
  2019-11-09  5:49       ` Amit Shah
  0 siblings, 2 replies; 7+ messages in thread
From: Michael S. Tsirkin @ 2019-11-06 15:03 UTC (permalink / raw)
  To: Laurent Vivier
  Cc: linux-kernel, Greg Kroah-Hartman, Amit Shah, virtualization,
	Arnd Bergmann

On Wed, Nov 06, 2019 at 03:02:25PM +0100, Laurent Vivier wrote:
> On 06/11/2019 14:56, Michael S. Tsirkin wrote:
> > On Fri, Oct 18, 2019 at 06:47:18PM +0200, 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 only allocate the
> >> buffers if the in_vq has available free slots.
> >>
> >> Fixes: a7a69ec0d8e4 ("virtio_console: free buffers after reset")
> >> Cc: mst@redhat.com
> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> >> ---
> >>  drivers/char/virtio_console.c | 17 +++++++++++------
> >>  1 file changed, 11 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> >> index 7270e7b69262..77105166fe01 100644
> >> --- a/drivers/char/virtio_console.c
> >> +++ b/drivers/char/virtio_console.c
> >> @@ -1421,12 +1421,17 @@ 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) {
> >> -		dev_err(port->dev, "Error allocating inbufs\n");
> >> -		err = -ENOMEM;
> >> -		goto free_device;
> >> +	/* if the in_vq has not already been filled (the port has already been
> >> +	 * used and unplugged), fill the in_vq with buffers so the host can
> >> +	 * send us data.
> >> +	 */
> >> +	if (port->in_vq->num_free != 0) {
> >> +		nr_added_bufs = fill_queue(port->in_vq, &port->inbuf_lock);
> >> +		if (!nr_added_bufs) {
> >> +			dev_err(port->dev, "Error allocating inbufs\n");
> >> +			err = -ENOMEM;
> >> +			goto free_device;
> >> +		}
> >>  	}
> >>  
> >>  	if (is_rproc_serial(port->portdev->vdev))
> > 
> > Well fill_queue will just add slots as long as it can.
> > So on a full queue it does nothing. How does this patch help?
> 
> Yes, but in this case it returns 0 and so add_port() fails and exits
> with -ENOMEM and the device is freed. It's what this patch tries to avoid.
> 
> Thanks,
> Laurent

Oh I see. However it's a bit asymmetrical to special case ring full.
How about making fill_queue return int and testing return code for
-ENOSPC instead? Will also help propagate errors correctly.

And I guess CC stable?

-- 
MST


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] virtio_console: allocate inbufs in add_port() only if it is needed
  2019-11-06 15:03     ` Michael S. Tsirkin
@ 2019-11-06 15:41       ` Laurent Vivier
  2019-11-09  5:49       ` Amit Shah
  1 sibling, 0 replies; 7+ messages in thread
From: Laurent Vivier @ 2019-11-06 15:41 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: linux-kernel, Greg Kroah-Hartman, Amit Shah, virtualization,
	Arnd Bergmann

On 06/11/2019 16:03, Michael S. Tsirkin wrote:
> On Wed, Nov 06, 2019 at 03:02:25PM +0100, Laurent Vivier wrote:
>> On 06/11/2019 14:56, Michael S. Tsirkin wrote:
>>> On Fri, Oct 18, 2019 at 06:47:18PM +0200, 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 only allocate the
>>>> buffers if the in_vq has available free slots.
>>>>
>>>> Fixes: a7a69ec0d8e4 ("virtio_console: free buffers after reset")
>>>> Cc: mst@redhat.com
>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>>> ---
>>>>  drivers/char/virtio_console.c | 17 +++++++++++------
>>>>  1 file changed, 11 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
>>>> index 7270e7b69262..77105166fe01 100644
>>>> --- a/drivers/char/virtio_console.c
>>>> +++ b/drivers/char/virtio_console.c
>>>> @@ -1421,12 +1421,17 @@ 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) {
>>>> -		dev_err(port->dev, "Error allocating inbufs\n");
>>>> -		err = -ENOMEM;
>>>> -		goto free_device;
>>>> +	/* if the in_vq has not already been filled (the port has already been
>>>> +	 * used and unplugged), fill the in_vq with buffers so the host can
>>>> +	 * send us data.
>>>> +	 */
>>>> +	if (port->in_vq->num_free != 0) {
>>>> +		nr_added_bufs = fill_queue(port->in_vq, &port->inbuf_lock);
>>>> +		if (!nr_added_bufs) {
>>>> +			dev_err(port->dev, "Error allocating inbufs\n");
>>>> +			err = -ENOMEM;
>>>> +			goto free_device;
>>>> +		}
>>>>  	}
>>>>  
>>>>  	if (is_rproc_serial(port->portdev->vdev))
>>>
>>> Well fill_queue will just add slots as long as it can.
>>> So on a full queue it does nothing. How does this patch help?
>>
>> Yes, but in this case it returns 0 and so add_port() fails and exits
>> with -ENOMEM and the device is freed. It's what this patch tries to avoid.
>>
>> Thanks,
>> Laurent
> 
> Oh I see. However it's a bit asymmetrical to special case ring full.
> How about making fill_queue return int and testing return code for
> -ENOSPC instead? Will also help propagate errors correctly.

Good idea. I'm going to propose a new patch.

> And I guess CC stable?
Sure.

Thanks,
Laurent


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] virtio_console: allocate inbufs in add_port() only if it is needed
  2019-11-06 15:03     ` Michael S. Tsirkin
  2019-11-06 15:41       ` Laurent Vivier
@ 2019-11-09  5:49       ` Amit Shah
  1 sibling, 0 replies; 7+ messages in thread
From: Amit Shah @ 2019-11-09  5:49 UTC (permalink / raw)
  To: Michael S. Tsirkin, Laurent Vivier
  Cc: linux-kernel, Greg Kroah-Hartman, Amit Shah, virtualization,
	Arnd Bergmann

On Wed, 2019-11-06 at 10:03 -0500, Michael S. Tsirkin wrote:
> On Wed, Nov 06, 2019 at 03:02:25PM +0100, Laurent Vivier wrote:
> > On 06/11/2019 14:56, Michael S. Tsirkin wrote:
> > > On Fri, Oct 18, 2019 at 06:47:18PM +0200, 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 only
> > > > allocate the
> > > > buffers if the in_vq has available free slots.
> > > > 
> > > > Fixes: a7a69ec0d8e4 ("virtio_console: free buffers after
> > > > reset")
> > > > Cc: mst@redhat.com
> > > > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > > > ---
> > > >  drivers/char/virtio_console.c | 17 +++++++++++------
> > > >  1 file changed, 11 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/char/virtio_console.c
> > > > b/drivers/char/virtio_console.c
> > > > index 7270e7b69262..77105166fe01 100644
> > > > --- a/drivers/char/virtio_console.c
> > > > +++ b/drivers/char/virtio_console.c
> > > > @@ -1421,12 +1421,17 @@ 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) {
> > > > -		dev_err(port->dev, "Error allocating
> > > > inbufs\n");
> > > > -		err = -ENOMEM;
> > > > -		goto free_device;
> > > > +	/* if the in_vq has not already been filled (the port
> > > > has already been
> > > > +	 * used and unplugged), fill the in_vq with buffers so
> > > > the host can
> > > > +	 * send us data.
> > > > +	 */
> > > > +	if (port->in_vq->num_free != 0) {
> > > > +		nr_added_bufs = fill_queue(port->in_vq, &port-
> > > > >inbuf_lock);
> > > > +		if (!nr_added_bufs) {
> > > > +			dev_err(port->dev, "Error allocating
> > > > inbufs\n");
> > > > +			err = -ENOMEM;
> > > > +			goto free_device;
> > > > +		}
> > > >  	}
> > > >  
> > > >  	if (is_rproc_serial(port->portdev->vdev))
> > > 
> > > Well fill_queue will just add slots as long as it can.
> > > So on a full queue it does nothing. How does this patch help?
> > 
> > Yes, but in this case it returns 0 and so add_port() fails and
> > exits
> > with -ENOMEM and the device is freed. It's what this patch tries to
> > avoid.
> > 
> > Thanks,
> > Laurent
> 
> Oh I see. However it's a bit asymmetrical to special case ring full.
> How about making fill_queue return int and testing return code for
> -ENOSPC instead? Will also help propagate errors correctly.

Yes, I like this better too.

Can you call out which commit introduced this behaviour / regression?

> 
> And I guess CC stable?
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-18 16:47 [PATCH] virtio_console: allocate inbufs in add_port() only if it is needed Laurent Vivier
2019-11-06 13:53 ` Laurent Vivier
2019-11-06 13:56 ` Michael S. Tsirkin
2019-11-06 14:02   ` Laurent Vivier
2019-11-06 15:03     ` Michael S. Tsirkin
2019-11-06 15:41       ` Laurent Vivier
2019-11-09  5:49       ` Amit Shah

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git