Re: [PATCH v3 1/2] virtio_console: free unused buffers with port delete

[Michael S. Tsirkin]

On Mon, Aug 12, 2019 at 01:36:48AM -0400, Pankaj Gupta wrote:
> 
> > 
> > On Fri, Aug 09, 2019 at 12:18:46PM +0530, Pankaj Gupta wrote:
> > > The commit a7a69ec0d8e4 ("virtio_console: free buffers after reset")
> > > deferred 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.
> > 
> > So why not reuse the buffers that are already there in this case?
> > Seems quite possible.
> 
> I took this approach because reusing the buffers will involve tweaking
> the existing core functionality like managing the the virt queue indexes.

I don't see why frankly, if you keep a list of outstanding
buffers on plug you can assume they have been added.

> Compared to that deleting the buffers while hot-unplugging port is simple
> and was working fine before. It seems logically correct as well.   
> 
> I agree we need a spec change for 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.
> > > 
> > > Reported-by: Xiaohui Li <xiaohli@xxxxxxxxxx>
> > > Fixes: a7a69ec0d8e4 ("virtio_console: free buffers after reset")
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Signed-off-by: Pankaj Gupta <pagupta@xxxxxxxxxx>
> > 
> > This is really a revert of a7a69ec0d8e4, just tagged confusingly.
> > 
> > And the original is also supposed to be a bugfix.
> > So how will the original bug be fixed?
> 
> Yes, Even I was confused while adding this tag.
> I will remove remove 'fixes' tag completely for this patch?
> because its a revert to original behavior which also is a bugfix.
> 
> > 
> > "this is the best we can do" is rarely the case.
> > 
> > I am not necessarily against the revert. But if we go that way then what
> > we need to do is specify the behaviour in the spec, since strict spec
> > compliance is exactly what the original patch was addressing.
> 
> Agree.
> 
> > 
> > In particular, we'd document that console has a special property that
> > when port is detached virtqueue is considered stopped, device must not
> > use any buffers, and it is legal to take buffers out of the device.
> 
> Yes. This documents the exact scenario. Thanks.
> You want me to send a patch for the spec change?
> 
> Best regards,
> Pankaj

Go ahead.

> > 
> > 
> > 
> > > ---
> > >  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 7270e7b69262..e8be82f1bae9 100644
> > > --- a/drivers/char/virtio_console.c
> > > +++ b/drivers/char/virtio_console.c
> > > @@ -1494,15 +1494,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);
> > >  }
> > >  
> > > @@ -1938,11 +1948,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.21.0
> >