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 <xiao...@redhat.com>
> > > Fixes: a7a69ec0d8e4 ("virtio_console: free buffers after reset")
> > > Cc: sta...@vger.kernel.org
> > > Signed-off-by: Pankaj Gupta <pagu...@redhat.com>
> >
> > 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
> >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
