linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] virtio_console: free unused buffers with virtio port
@ 2019-03-04 13:05 Pankaj Gupta
  2019-03-04 13:28 ` Pankaj Gupta
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Pankaj Gupta @ 2019-03-04 13:05 UTC (permalink / raw)
  To: linux-kernel, virtualization; +Cc: amit, arnd, gregkh, mst, pagupta, xiaohli

  The commit a7a69ec0d8e4 ("virtio_console: free buffers after reset")
  deffered detaching of unused buffer to virtio device unplug time. 

  This causes unplug/replug of single port in virtio device with an 
  error "Error allocating inbufs\n". As we don't free the unused buffers 
  attached with the port. Re-plug the same port tries to allocate new
  buffers in virtqueue and results in this error if queue is full. 

  This patch removes the unused buffers in vq's when we unplug the port.
  This is the best we can do as we cannot call device_reset because virtio 
  device is still active. This was the working behaviour before the change 
  introduced in commit b3258ff1d6.

  Reported-by: Xiaohui Li <xiaohli@redhat.com>
  Fixes: b3258ff1d6 ("virtio_console: free buffers after reset")
  Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
---
 drivers/char/virtio_console.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index fbeb71953526..5fbf2ac73111 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1506,15 +1506,25 @@ static void remove_port(struct kref *kref)
 	kfree(port);
 }
 
+static void remove_unused_bufs(struct virtqueue *vq)
+{
+	struct port_buffer *buf;
+
+	while ((buf = virtqueue_detach_unused_buf(vq)))
+		free_buf(buf, true);
+}
+
 static void remove_port_data(struct port *port)
 {
 	spin_lock_irq(&port->inbuf_lock);
 	/* Remove unused data this port might have received. */
 	discard_port_data(port);
+	remove_unused_bufs(port->in_vq);
 	spin_unlock_irq(&port->inbuf_lock);
 
 	spin_lock_irq(&port->outvq_lock);
 	reclaim_consumed_buffers(port);
+	remove_unused_bufs(port->out_vq);
 	spin_unlock_irq(&port->outvq_lock);
 }
 
@@ -1950,11 +1960,9 @@ static void remove_vqs(struct ports_device *portdev)
 	struct virtqueue *vq;
 
 	virtio_device_for_each_vq(portdev->vdev, vq) {
-		struct port_buffer *buf;
 
 		flush_bufs(vq, true);
-		while ((buf = virtqueue_detach_unused_buf(vq)))
-			free_buf(buf, true);
+		remove_unused_bufs(vq);
 	}
 	portdev->vdev->config->del_vqs(portdev->vdev);
 	kfree(portdev->in_vqs);
-- 
2.20.1


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

* Re: [PATCH] virtio_console: free unused buffers with virtio port
  2019-03-04 13:05 [PATCH] virtio_console: free unused buffers with virtio port Pankaj Gupta
@ 2019-03-04 13:28 ` Pankaj Gupta
  2019-03-04 19:25 ` Michael S. Tsirkin
  2019-03-11  5:58 ` Pankaj Gupta
  2 siblings, 0 replies; 8+ messages in thread
From: Pankaj Gupta @ 2019-03-04 13:28 UTC (permalink / raw)
  To: linux-kernel, virtualization; +Cc: amit, arnd, gregkh, mst, xiaohli, Gal Hammer


Adding Gal in CC [Forgot to add]

> 
> The commit a7a69ec0d8e4 ("virtio_console: free buffers after reset")
>   deffered detaching of unused buffer to virtio device unplug time.
> 
>   This causes unplug/replug of single port in virtio device with an
>   error "Error allocating inbufs\n". As we don't free the unused buffers
>   attached with the port. Re-plug the same port tries to allocate new
>   buffers in virtqueue and results in this error if queue is full.
> 
>   This patch removes the unused buffers in vq's when we unplug the port.
>   This is the best we can do as we cannot call device_reset because virtio
>   device is still active. This was the working behaviour before the change
>   introduced in commit b3258ff1d6.
> 
>   Reported-by: Xiaohui Li <xiaohli@redhat.com>
>   Fixes: b3258ff1d6 ("virtio_console: free buffers after reset")
>   Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> ---
>  drivers/char/virtio_console.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index fbeb71953526..5fbf2ac73111 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1506,15 +1506,25 @@ static void remove_port(struct kref *kref)
>          kfree(port);
>  }
>  
> +static void remove_unused_bufs(struct virtqueue *vq)
> +{
> +        struct port_buffer *buf;
> +
> +        while ((buf = virtqueue_detach_unused_buf(vq)))
> +                free_buf(buf, true);
> +}
> +
>  static void remove_port_data(struct port *port)
>  {
>          spin_lock_irq(&port->inbuf_lock);
>          /* Remove unused data this port might have received. */
>          discard_port_data(port);
> +        remove_unused_bufs(port->in_vq);
>          spin_unlock_irq(&port->inbuf_lock);
>  
>          spin_lock_irq(&port->outvq_lock);
>          reclaim_consumed_buffers(port);
> +        remove_unused_bufs(port->out_vq);
>          spin_unlock_irq(&port->outvq_lock);
>  }
>  
> @@ -1950,11 +1960,9 @@ static void remove_vqs(struct ports_device *portdev)
>          struct virtqueue *vq;
>  
>          virtio_device_for_each_vq(portdev->vdev, vq) {
> -                struct port_buffer *buf;
>  
>                  flush_bufs(vq, true);
> -                while ((buf = virtqueue_detach_unused_buf(vq)))
> -                        free_buf(buf, true);
> +                remove_unused_bufs(vq);
>          }
>          portdev->vdev->config->del_vqs(portdev->vdev);
>          kfree(portdev->in_vqs);
> --
> 2.20.1
> 
> 

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

* Re: [PATCH] virtio_console: free unused buffers with virtio port
  2019-03-04 13:05 [PATCH] virtio_console: free unused buffers with virtio port Pankaj Gupta
  2019-03-04 13:28 ` Pankaj Gupta
@ 2019-03-04 19:25 ` Michael S. Tsirkin
  2019-03-05  7:09   ` Pankaj Gupta
  2019-03-11  5:58 ` Pankaj Gupta
  2 siblings, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2019-03-04 19:25 UTC (permalink / raw)
  To: Pankaj Gupta; +Cc: linux-kernel, virtualization, amit, arnd, gregkh, xiaohli

On Mon, Mar 04, 2019 at 06:35:11PM +0530, Pankaj Gupta wrote:
>   The commit a7a69ec0d8e4 ("virtio_console: free buffers after reset")
>   deffered detaching of unused buffer to virtio device unplug time. 
> 
>   This causes unplug/replug of single port in virtio device with an 
>   error "Error allocating inbufs\n". As we don't free the unused buffers 
>   attached with the port. Re-plug the same port tries to allocate new
>   buffers in virtqueue and results in this error if queue is full. 
> 
>   This patch removes the unused buffers in vq's when we unplug the port.
>   This is the best we can do as we cannot call device_reset because virtio 
>   device is still active. This was the working behaviour before the change 
>   introduced in commit b3258ff1d6.
> 
>   Reported-by: Xiaohui Li <xiaohli@redhat.com>
>   Fixes: b3258ff1d6 ("virtio_console: free buffers after reset")
>   Signed-off-by: Pankaj Gupta <pagupta@redhat.com>

I think if you do this you need to add support
in the packed ring.

> ---
>  drivers/char/virtio_console.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index fbeb71953526..5fbf2ac73111 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1506,15 +1506,25 @@ static void remove_port(struct kref *kref)
>  	kfree(port);
>  }
>  
> +static void remove_unused_bufs(struct virtqueue *vq)
> +{
> +	struct port_buffer *buf;
> +
> +	while ((buf = virtqueue_detach_unused_buf(vq)))
> +		free_buf(buf, true);
> +}
> +
>  static void remove_port_data(struct port *port)
>  {
>  	spin_lock_irq(&port->inbuf_lock);
>  	/* Remove unused data this port might have received. */
>  	discard_port_data(port);
> +	remove_unused_bufs(port->in_vq);
>  	spin_unlock_irq(&port->inbuf_lock);
>  
>  	spin_lock_irq(&port->outvq_lock);
>  	reclaim_consumed_buffers(port);
> +	remove_unused_bufs(port->out_vq);
>  	spin_unlock_irq(&port->outvq_lock);
>  }
>  
> @@ -1950,11 +1960,9 @@ static void remove_vqs(struct ports_device *portdev)
>  	struct virtqueue *vq;
>  
>  	virtio_device_for_each_vq(portdev->vdev, vq) {
> -		struct port_buffer *buf;
>  
>  		flush_bufs(vq, true);
> -		while ((buf = virtqueue_detach_unused_buf(vq)))
> -			free_buf(buf, true);
> +		remove_unused_bufs(vq);
>  	}
>  	portdev->vdev->config->del_vqs(portdev->vdev);
>  	kfree(portdev->in_vqs);
> -- 
> 2.20.1

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

* Re: [PATCH] virtio_console: free unused buffers with virtio port
  2019-03-04 19:25 ` Michael S. Tsirkin
@ 2019-03-05  7:09   ` Pankaj Gupta
  2019-03-11  6:35     ` Michael S. Tsirkin
  0 siblings, 1 reply; 8+ messages in thread
From: Pankaj Gupta @ 2019-03-05  7:09 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: linux-kernel, virtualization, amit, arnd, gregkh, xiaohli, Gal Hammer


Hello Michael,

Thanks for your reply.

> 
> On Mon, Mar 04, 2019 at 06:35:11PM +0530, Pankaj Gupta wrote:
> >   The commit a7a69ec0d8e4 ("virtio_console: free buffers after reset")
> >   deffered detaching of unused buffer to virtio device unplug time.
> > 
> >   This causes unplug/replug of single port in virtio device with an
> >   error "Error allocating inbufs\n". As we don't free the unused buffers
> >   attached with the port. Re-plug the same port tries to allocate new
> >   buffers in virtqueue and results in this error if queue is full.
> > 
> >   This patch removes the unused buffers in vq's when we unplug the port.
> >   This is the best we can do as we cannot call device_reset because virtio
> >   device is still active. This was the working behaviour before the change
> >   introduced in commit b3258ff1d6.
> > 
> >   Reported-by: Xiaohui Li <xiaohli@redhat.com>
> >   Fixes: b3258ff1d6 ("virtio_console: free buffers after reset")
> >   Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> 
> I think if you do this you need to add support
> in the packed ring.

o.k. I will look at the implementation details for "support
of packed ring" for virtio_console. This will take some time.

Meanwhile "virtio_console" port hotplug/unplug is broken in upstream.
Can we accept this patch as it fixes the upstream and together
with parent patch(b3258ff1d6) does nice cleanups as well.

Thanks,
Pankaj  

> 
> > ---
> >  drivers/char/virtio_console.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > index fbeb71953526..5fbf2ac73111 100644
> > --- a/drivers/char/virtio_console.c
> > +++ b/drivers/char/virtio_console.c
> > @@ -1506,15 +1506,25 @@ static void remove_port(struct kref *kref)
> >          kfree(port);
> >  }
> >  
> > +static void remove_unused_bufs(struct virtqueue *vq)
> > +{
> > +        struct port_buffer *buf;
> > +
> > +        while ((buf = virtqueue_detach_unused_buf(vq)))
> > +                free_buf(buf, true);
> > +}
> > +
> >  static void remove_port_data(struct port *port)
> >  {
> >          spin_lock_irq(&port->inbuf_lock);
> >          /* Remove unused data this port might have received. */
> >          discard_port_data(port);
> > +        remove_unused_bufs(port->in_vq);
> >          spin_unlock_irq(&port->inbuf_lock);
> >  
> >          spin_lock_irq(&port->outvq_lock);
> >          reclaim_consumed_buffers(port);
> > +        remove_unused_bufs(port->out_vq);
> >          spin_unlock_irq(&port->outvq_lock);
> >  }
> >  
> > @@ -1950,11 +1960,9 @@ static void remove_vqs(struct ports_device *portdev)
> >          struct virtqueue *vq;
> >  
> >          virtio_device_for_each_vq(portdev->vdev, vq) {
> > -                struct port_buffer *buf;
> >  
> >                  flush_bufs(vq, true);
> > -                while ((buf = virtqueue_detach_unused_buf(vq)))
> > -                        free_buf(buf, true);
> > +                remove_unused_bufs(vq);
> >          }
> >          portdev->vdev->config->del_vqs(portdev->vdev);
> >          kfree(portdev->in_vqs);
> > --
> > 2.20.1
> 

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

* Re: [PATCH] virtio_console: free unused buffers with virtio port
  2019-03-04 13:05 [PATCH] virtio_console: free unused buffers with virtio port Pankaj Gupta
  2019-03-04 13:28 ` Pankaj Gupta
  2019-03-04 19:25 ` Michael S. Tsirkin
@ 2019-03-11  5:58 ` Pankaj Gupta
  2019-03-11  6:36   ` Michael S. Tsirkin
  2 siblings, 1 reply; 8+ messages in thread
From: Pankaj Gupta @ 2019-03-11  5:58 UTC (permalink / raw)
  To: linux-kernel, virtualization; +Cc: amit, arnd, gregkh, mst, xiaohli, Gal Hammer


> 
> The commit a7a69ec0d8e4 ("virtio_console: free buffers after reset")
>   deffered detaching of unused buffer to virtio device unplug time.
> 
>   This causes unplug/replug of single port in virtio device with an
>   error "Error allocating inbufs\n". As we don't free the unused buffers
>   attached with the port. Re-plug the same port tries to allocate new
>   buffers in virtqueue and results in this error if queue is full.
> 
>   This patch removes the unused buffers in vq's when we unplug the port.
>   This is the best we can do as we cannot call device_reset because virtio
>   device is still active. This was the working behaviour before the change
>   introduced in commit b3258ff1d6.

ping.

This patch fixes a regression in upstream kernel, virtio_console driver.
Can we please have this merged.

Thanks,
Pankaj

> 
>   Reported-by: Xiaohui Li <xiaohli@redhat.com>
>   Fixes: b3258ff1d6 ("virtio_console: free buffers after reset")
>   Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> ---
>  drivers/char/virtio_console.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index fbeb71953526..5fbf2ac73111 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1506,15 +1506,25 @@ static void remove_port(struct kref *kref)
>  	kfree(port);
>  }
>  
> +static void remove_unused_bufs(struct virtqueue *vq)
> +{
> +	struct port_buffer *buf;
> +
> +	while ((buf = virtqueue_detach_unused_buf(vq)))
> +		free_buf(buf, true);
> +}
> +
>  static void remove_port_data(struct port *port)
>  {
>  	spin_lock_irq(&port->inbuf_lock);
>  	/* Remove unused data this port might have received. */
>  	discard_port_data(port);
> +	remove_unused_bufs(port->in_vq);
>  	spin_unlock_irq(&port->inbuf_lock);
>  
>  	spin_lock_irq(&port->outvq_lock);
>  	reclaim_consumed_buffers(port);
> +	remove_unused_bufs(port->out_vq);
>  	spin_unlock_irq(&port->outvq_lock);
>  }
>  
> @@ -1950,11 +1960,9 @@ static void remove_vqs(struct ports_device *portdev)
>  	struct virtqueue *vq;
>  
>  	virtio_device_for_each_vq(portdev->vdev, vq) {
> -		struct port_buffer *buf;
>  
>  		flush_bufs(vq, true);
> -		while ((buf = virtqueue_detach_unused_buf(vq)))
> -			free_buf(buf, true);
> +		remove_unused_bufs(vq);
>  	}
>  	portdev->vdev->config->del_vqs(portdev->vdev);
>  	kfree(portdev->in_vqs);
> --
> 2.20.1
> 
> 

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

* Re: [PATCH] virtio_console: free unused buffers with virtio port
  2019-03-05  7:09   ` Pankaj Gupta
@ 2019-03-11  6:35     ` Michael S. Tsirkin
  2019-03-11  6:53       ` Pankaj Gupta
  0 siblings, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2019-03-11  6:35 UTC (permalink / raw)
  To: Pankaj Gupta
  Cc: linux-kernel, virtualization, amit, arnd, gregkh, xiaohli, Gal Hammer

On Tue, Mar 05, 2019 at 02:09:06AM -0500, Pankaj Gupta wrote:
> 
> Hello Michael,
> 
> Thanks for your reply.
> 
> > 
> > On Mon, Mar 04, 2019 at 06:35:11PM +0530, Pankaj Gupta wrote:
> > >   The commit a7a69ec0d8e4 ("virtio_console: free buffers after reset")
> > >   deffered detaching of unused buffer to virtio device unplug time.
> > > 
> > >   This causes unplug/replug of single port in virtio device with an
> > >   error "Error allocating inbufs\n". As we don't free the unused buffers
> > >   attached with the port. Re-plug the same port tries to allocate new
> > >   buffers in virtqueue and results in this error if queue is full.

That's the basic issue, isn't it? Why aren't we
reusing buffers that are already there?


> > > 
> > >   This patch removes the unused buffers in vq's when we unplug the port.
> > >   This is the best we can do as we cannot call device_reset because virtio
> > >   device is still active. This was the working behaviour before the change
> > >   introduced in commit b3258ff1d6.
> > > 
> > >   Reported-by: Xiaohui Li <xiaohli@redhat.com>
> > >   Fixes: b3258ff1d6 ("virtio_console: free buffers after reset")
> > >   Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> > 
> > I think if you do this you need to add support
> > in the packed ring.
> 
> o.k. I will look at the implementation details for "support
> of packed ring" for virtio_console. This will take some time.
> 
> Meanwhile "virtio_console" port hotplug/unplug is broken in upstream.
> Can we accept this patch as it fixes the upstream and together
> with parent patch(b3258ff1d6) does nice cleanups as well.
> 
> Thanks,
> Pankaj  

Sorry, no - I don't think we should fix one configuration by breaking the other.
If you want to go back, then that's a spec violation, but I guess we can
fix the spec to match.  OK, but code-wise if you call
virtqueue_detach_unused_buf without device reset then you need to teach
packed ring code to support that.



> > 
> > > ---
> > >  drivers/char/virtio_console.c | 14 +++++++++++---
> > >  1 file changed, 11 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > > index fbeb71953526..5fbf2ac73111 100644
> > > --- a/drivers/char/virtio_console.c
> > > +++ b/drivers/char/virtio_console.c
> > > @@ -1506,15 +1506,25 @@ static void remove_port(struct kref *kref)
> > >          kfree(port);
> > >  }
> > >  
> > > +static void remove_unused_bufs(struct virtqueue *vq)
> > > +{
> > > +        struct port_buffer *buf;
> > > +
> > > +        while ((buf = virtqueue_detach_unused_buf(vq)))
> > > +                free_buf(buf, true);
> > > +}
> > > +
> > >  static void remove_port_data(struct port *port)
> > >  {
> > >          spin_lock_irq(&port->inbuf_lock);
> > >          /* Remove unused data this port might have received. */
> > >          discard_port_data(port);
> > > +        remove_unused_bufs(port->in_vq);
> > >          spin_unlock_irq(&port->inbuf_lock);
> > >  
> > >          spin_lock_irq(&port->outvq_lock);
> > >          reclaim_consumed_buffers(port);
> > > +        remove_unused_bufs(port->out_vq);
> > >          spin_unlock_irq(&port->outvq_lock);
> > >  }
> > >  
> > > @@ -1950,11 +1960,9 @@ static void remove_vqs(struct ports_device *portdev)
> > >          struct virtqueue *vq;
> > >  
> > >          virtio_device_for_each_vq(portdev->vdev, vq) {
> > > -                struct port_buffer *buf;
> > >  
> > >                  flush_bufs(vq, true);
> > > -                while ((buf = virtqueue_detach_unused_buf(vq)))
> > > -                        free_buf(buf, true);
> > > +                remove_unused_bufs(vq);
> > >          }
> > >          portdev->vdev->config->del_vqs(portdev->vdev);
> > >          kfree(portdev->in_vqs);
> > > --
> > > 2.20.1
> > 

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

* Re: [PATCH] virtio_console: free unused buffers with virtio port
  2019-03-11  5:58 ` Pankaj Gupta
@ 2019-03-11  6:36   ` Michael S. Tsirkin
  0 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2019-03-11  6:36 UTC (permalink / raw)
  To: Pankaj Gupta
  Cc: linux-kernel, virtualization, amit, arnd, gregkh, xiaohli, Gal Hammer

On Mon, Mar 11, 2019 at 01:58:11AM -0400, Pankaj Gupta wrote:
> 
> > 
> > The commit a7a69ec0d8e4 ("virtio_console: free buffers after reset")
> >   deffered detaching of unused buffer to virtio device unplug time.
> > 
> >   This causes unplug/replug of single port in virtio device with an
> >   error "Error allocating inbufs\n". As we don't free the unused buffers
> >   attached with the port. Re-plug the same port tries to allocate new
> >   buffers in virtqueue and results in this error if queue is full.
> > 
> >   This patch removes the unused buffers in vq's when we unplug the port.
> >   This is the best we can do as we cannot call device_reset because virtio
> >   device is still active. This was the working behaviour before the change
> >   introduced in commit b3258ff1d6.
> 
> ping.
> 
> This patch fixes a regression in upstream kernel, virtio_console driver.
> Can we please have this merged.
> 
> Thanks,
> Pankaj

Sorry, no, I don't think that's a full fix.

> > 
> >   Reported-by: Xiaohui Li <xiaohli@redhat.com>
> >   Fixes: b3258ff1d6 ("virtio_console: free buffers after reset")
> >   Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> > ---
> >  drivers/char/virtio_console.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > index fbeb71953526..5fbf2ac73111 100644
> > --- a/drivers/char/virtio_console.c
> > +++ b/drivers/char/virtio_console.c
> > @@ -1506,15 +1506,25 @@ static void remove_port(struct kref *kref)
> >  	kfree(port);
> >  }
> >  
> > +static void remove_unused_bufs(struct virtqueue *vq)
> > +{
> > +	struct port_buffer *buf;
> > +
> > +	while ((buf = virtqueue_detach_unused_buf(vq)))
> > +		free_buf(buf, true);
> > +}
> > +
> >  static void remove_port_data(struct port *port)
> >  {
> >  	spin_lock_irq(&port->inbuf_lock);
> >  	/* Remove unused data this port might have received. */
> >  	discard_port_data(port);
> > +	remove_unused_bufs(port->in_vq);
> >  	spin_unlock_irq(&port->inbuf_lock);
> >  
> >  	spin_lock_irq(&port->outvq_lock);
> >  	reclaim_consumed_buffers(port);
> > +	remove_unused_bufs(port->out_vq);
> >  	spin_unlock_irq(&port->outvq_lock);
> >  }
> >  
> > @@ -1950,11 +1960,9 @@ static void remove_vqs(struct ports_device *portdev)
> >  	struct virtqueue *vq;
> >  
> >  	virtio_device_for_each_vq(portdev->vdev, vq) {
> > -		struct port_buffer *buf;
> >  
> >  		flush_bufs(vq, true);
> > -		while ((buf = virtqueue_detach_unused_buf(vq)))
> > -			free_buf(buf, true);
> > +		remove_unused_bufs(vq);
> >  	}
> >  	portdev->vdev->config->del_vqs(portdev->vdev);
> >  	kfree(portdev->in_vqs);
> > --
> > 2.20.1
> > 
> > 

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

* Re: [PATCH] virtio_console: free unused buffers with virtio port
  2019-03-11  6:35     ` Michael S. Tsirkin
@ 2019-03-11  6:53       ` Pankaj Gupta
  0 siblings, 0 replies; 8+ messages in thread
From: Pankaj Gupta @ 2019-03-11  6:53 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: linux-kernel, virtualization, amit, arnd, gregkh, xiaohli, Gal Hammer


> > 
> > Hello Michael,
> > 
> > Thanks for your reply.
> > 
> > > 
> > > On Mon, Mar 04, 2019 at 06:35:11PM +0530, Pankaj Gupta wrote:
> > > >   The commit a7a69ec0d8e4 ("virtio_console: free buffers after reset")
> > > >   deffered detaching of unused buffer to virtio device unplug time.
> > > > 
> > > >   This causes unplug/replug of single port in virtio device with an
> > > >   error "Error allocating inbufs\n". As we don't free the unused
> > > >   buffers
> > > >   attached with the port. Re-plug the same port tries to allocate new
> > > >   buffers in virtqueue and results in this error if queue is full.
> 
> That's the basic issue, isn't it? Why aren't we
> reusing buffers that are already there?

I think that that's how initial design has been. Will see if I can fix this.

> 
> 
> > > > 
> > > >   This patch removes the unused buffers in vq's when we unplug the
> > > >   port.
> > > >   This is the best we can do as we cannot call device_reset because
> > > >   virtio
> > > >   device is still active. This was the working behaviour before the
> > > >   change
> > > >   introduced in commit b3258ff1d6.
> > > > 
> > > >   Reported-by: Xiaohui Li <xiaohli@redhat.com>
> > > >   Fixes: b3258ff1d6 ("virtio_console: free buffers after reset")
> > > >   Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> > > 
> > > I think if you do this you need to add support
> > > in the packed ring.
> > 
> > o.k. I will look at the implementation details for "support
> > of packed ring" for virtio_console. This will take some time.
> > 
> > Meanwhile "virtio_console" port hotplug/unplug is broken in upstream.
> > Can we accept this patch as it fixes the upstream and together
> > with parent patch(b3258ff1d6) does nice cleanups as well.
> > 
> > Thanks,
> > Pankaj
> 
> Sorry, no - I don't think we should fix one configuration by breaking the
> other.
> If you want to go back, then that's a spec violation, but I guess we can
> fix the spec to match.  OK, but code-wise if you call
> virtqueue_detach_unused_buf without device reset then you need to teach
> packed ring code to support that.

o.k. Will look at this.

Thanks for the pointers.

Thanks,
Pankaj

> 
> 
> 
> > > 
> > > > ---
> > > >  drivers/char/virtio_console.c | 14 +++++++++++---
> > > >  1 file changed, 11 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/char/virtio_console.c
> > > > b/drivers/char/virtio_console.c
> > > > index fbeb71953526..5fbf2ac73111 100644
> > > > --- a/drivers/char/virtio_console.c
> > > > +++ b/drivers/char/virtio_console.c
> > > > @@ -1506,15 +1506,25 @@ static void remove_port(struct kref *kref)
> > > >          kfree(port);
> > > >  }
> > > >  
> > > > +static void remove_unused_bufs(struct virtqueue *vq)
> > > > +{
> > > > +        struct port_buffer *buf;
> > > > +
> > > > +        while ((buf = virtqueue_detach_unused_buf(vq)))
> > > > +                free_buf(buf, true);
> > > > +}
> > > > +
> > > >  static void remove_port_data(struct port *port)
> > > >  {
> > > >          spin_lock_irq(&port->inbuf_lock);
> > > >          /* Remove unused data this port might have received. */
> > > >          discard_port_data(port);
> > > > +        remove_unused_bufs(port->in_vq);
> > > >          spin_unlock_irq(&port->inbuf_lock);
> > > >  
> > > >          spin_lock_irq(&port->outvq_lock);
> > > >          reclaim_consumed_buffers(port);
> > > > +        remove_unused_bufs(port->out_vq);
> > > >          spin_unlock_irq(&port->outvq_lock);
> > > >  }
> > > >  
> > > > @@ -1950,11 +1960,9 @@ static void remove_vqs(struct ports_device
> > > > *portdev)
> > > >          struct virtqueue *vq;
> > > >  
> > > >          virtio_device_for_each_vq(portdev->vdev, vq) {
> > > > -                struct port_buffer *buf;
> > > >  
> > > >                  flush_bufs(vq, true);
> > > > -                while ((buf = virtqueue_detach_unused_buf(vq)))
> > > > -                        free_buf(buf, true);
> > > > +                remove_unused_bufs(vq);
> > > >          }
> > > >          portdev->vdev->config->del_vqs(portdev->vdev);
> > > >          kfree(portdev->in_vqs);
> > > > --
> > > > 2.20.1
> > > 
> 

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

end of thread, other threads:[~2019-03-11  6:53 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-04 13:05 [PATCH] virtio_console: free unused buffers with virtio port Pankaj Gupta
2019-03-04 13:28 ` Pankaj Gupta
2019-03-04 19:25 ` Michael S. Tsirkin
2019-03-05  7:09   ` Pankaj Gupta
2019-03-11  6:35     ` Michael S. Tsirkin
2019-03-11  6:53       ` Pankaj Gupta
2019-03-11  5:58 ` Pankaj Gupta
2019-03-11  6:36   ` Michael S. Tsirkin

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).