This patch series fixes the issue with unplug/replug of a port in virtio
console device, which fails with an error "Error allocating inbufs\n".
Patch 2 makes virtio packed ring code compatible with virtio split ring.
Tested the packed ring code with the qemu virtio 1.1 device code posted
here [1].
Changes from v1[2]
-----
Make virtio packed ring code compatible with split ring - [Michael]
[1] https://marc.info/?l=qemu-devel&m=156471883703948&w=2
[2] https://lkml.org/lkml/2019/3/4/517
Pankaj Gupta (2):
virtio_console: free unused buffers with port delete
virtio_ring: packed ring: fix virtqueue_detach_unused_buf
drivers/char/virtio_console.c | 14 +++++++++++---
drivers/virtio/virtio_ring.c | 5 +++++
2 files changed, 16 insertions(+), 3 deletions(-)
--
2.21.0
This patch makes packed ring code compatible with split ring in function
'virtqueue_detach_unused_buf_*'.
Signed-off-by: Pankaj Gupta <[email protected]>
---
drivers/virtio/virtio_ring.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index c8be1c4f5b55..1b98a6777b7e 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -1534,6 +1534,11 @@ static void *virtqueue_detach_unused_buf_packed(struct virtqueue *_vq)
for (i = 0; i < vq->packed.vring.num; i++) {
if (!vq->packed.desc_state[i].data)
continue;
+ vq->packed.next_avail_idx--;
+ if (vq->packed.next_avail_idx < 0) {
+ vq->packed.next_avail_idx = vq->packed.vring.num - 1;
+ vq->packed.avail_wrap_counter ^= 1;
+ }
/* detach_buf clears data, so grab it now. */
buf = vq->packed.desc_state[i].data;
detach_buf_packed(vq, i, NULL);
--
2.21.0
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.
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 <[email protected]>
Fixes: b3258ff1d6 ("virtio_console: free buffers after reset")
Signed-off-by: Pankaj Gupta <[email protected]>
---
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
On Thu, Aug 08, 2019 at 05:06:05PM +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.
>
> 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.
Why is this indented?
>
> Reported-by: Xiaohui Li <[email protected]>
> Fixes: b3258ff1d6 ("virtio_console: free buffers after reset")
Fixes: b3258ff1d608 ("virtio: Decrement avail idx on buffer detach")
is the correct format to use.
And given that this is from 2.6.39 (and 2.6.38.5), shouldn't it also be
backported for the stable kernels?
thanks,
greg k-h
On Thu, Aug 08, 2019 at 05:06:06PM +0530, Pankaj Gupta wrote:
> This patch makes packed ring code compatible with split ring in function
> 'virtqueue_detach_unused_buf_*'.
What does that mean? What does this "fix"?
thanks,
greg k-h
>
> On Thu, Aug 08, 2019 at 05:06:05PM +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.
> >
> > 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.
>
> Why is this indented?
o.k. will remove the empty lines.
>
> >
> > Reported-by: Xiaohui Li <[email protected]>
> > Fixes: b3258ff1d6 ("virtio_console: free buffers after reset")
>
> Fixes: b3258ff1d608 ("virtio: Decrement avail idx on buffer detach")
>
> is the correct format to use.
Sorry! for this. Commit it fixes is:
a7a69ec0d8e4 ("virtio_console: free buffers after reset")
>
> And given that this is from 2.6.39 (and 2.6.38.5), shouldn't it also be
> backported for the stable kernels?
Yes.
Thanks,
Pankaj
>
> thanks,
>
> greg k-h
>
> > This patch makes packed ring code compatible with split ring in function
> > 'virtqueue_detach_unused_buf_*'.
>
> What does that mean? What does this "fix"?
Patch 1 frees the buffers When a port is unplugged from the virtio
console device. It does this with the help of 'virtqueue_detach_unused_buf_split/packed'
function. For split ring case, corresponding function decrements avail ring index.
For packed ring code, this functionality is not available, so this patch adds the
required support and hence help to remove the unused buffer completely.
Thanks,
Pankaj
>
> thanks,
>
> greg k-h
>
On Thu, Aug 08, 2019 at 08:28:46AM -0400, Pankaj Gupta wrote:
>
>
> > > This patch makes packed ring code compatible with split ring in function
> > > 'virtqueue_detach_unused_buf_*'.
> >
> > What does that mean? What does this "fix"?
>
> Patch 1 frees the buffers When a port is unplugged from the virtio
> console device. It does this with the help of 'virtqueue_detach_unused_buf_split/packed'
> function. For split ring case, corresponding function decrements avail ring index.
> For packed ring code, this functionality is not available, so this patch adds the
> required support and hence help to remove the unused buffer completely.
Explain all of this in great detail in the changelog comment. What you
have in there today does not make any sense.
thanks,
greg k-h
> On Thu, Aug 08, 2019 at 08:28:46AM -0400, Pankaj Gupta wrote:
> >
> >
> > > > This patch makes packed ring code compatible with split ring in
> > > > function
> > > > 'virtqueue_detach_unused_buf_*'.
> > >
> > > What does that mean? What does this "fix"?
> >
> > Patch 1 frees the buffers When a port is unplugged from the virtio
> > console device. It does this with the help of
> > 'virtqueue_detach_unused_buf_split/packed'
> > function. For split ring case, corresponding function decrements avail ring
> > index.
> > For packed ring code, this functionality is not available, so this patch
> > adds the
> > required support and hence help to remove the unused buffer completely.
>
> Explain all of this in great detail in the changelog comment. What you
> have in there today does not make any sense.
Sure. Will try to put in more clear words.
Thanks,
Pankaj
>
> thanks,
>
> greg k-h
>