search for: detach_unused_buf

virtqueue ops were introduced in the hope that we'll have multiple implementations besides virtio_ring, but none have surfaced so far, and given that existing virtio ring is deployed in production we are likely stuck with it now, so this layer just adds complexity and overhead. Further, the need to pass vq twice to each call (as in dev->vq->vq_ops->kick(dev->vq) ) adds potential

virtqueue ops were introduced in the hope that we'll have multiple implementations besides virtio_ring, but none have surfaced so far, and given that existing virtio ring is deployed in production we are likely stuck with it now, so this layer just adds complexity and overhead. Further, the need to pass vq twice to each call (as in dev->vq->vq_ops->kick(dev->vq) ) adds potential

...te: > Code is added to avoid calling blk_cleanup_queue() when the surprize_removal > flag is set due to a disappeared device. It avoid hangs due to incomplete > requests (e.g. in-flight requests). Such requests must be considered as lost. Ugh. Can't we complete these immediately using detach_unused_buf? If not why? > If the current remove callback was triggered due to an unregister driver, > and the surprize_removal is not already set (although the actual device > is already gone, e.g. virsh detach), blk_cleanup_queue() would be triggered > resulting in a possible hang. This hang is...

...te: > Code is added to avoid calling blk_cleanup_queue() when the surprize_removal > flag is set due to a disappeared device. It avoid hangs due to incomplete > requests (e.g. in-flight requests). Such requests must be considered as lost. Ugh. Can't we complete these immediately using detach_unused_buf? If not why? > If the current remove callback was triggered due to an unregister driver, > and the surprize_removal is not already set (although the actual device > is already gone, e.g. virsh detach), blk_cleanup_queue() would be triggered > resulting in a possible hang. This hang is...

Hi Dave, Nice driver optimization from Shirley, but requires a new virtio hook. Do you want to take both? I have nothing else overlapping it. Cheers, Rusty.

Hi Dave, Nice driver optimization from Shirley, but requires a new virtio hook. Do you want to take both? I have nothing else overlapping it. Cheers, Rusty.

...> > + desc = &vq->vring_packed.desc[i]; > > > > + vring_unmap_one_packed(vq, desc); > > > > + desc->flags = 0x0; > > > Looks like this is unnecessary. > > It's safer to zero it. If we don't zero it, after we > > call virtqueue_detach_unused_buf_packed() which calls > > this function, the desc is still available to the > > device. > > Well detach_unused_buf_packed() should be called after device is stopped, > otherwise even if you try to clear, there will still be a window that device > may use it. This is not abo...

...> > + desc = &vq->vring_packed.desc[i]; > > > > + vring_unmap_one_packed(vq, desc); > > > > + desc->flags = 0x0; > > > Looks like this is unnecessary. > > It's safer to zero it. If we don't zero it, after we > > call virtqueue_detach_unused_buf_packed() which calls > > this function, the desc is still available to the > > device. > > Well detach_unused_buf_packed() should be called after device is stopped, > otherwise even if you try to clear, there will still be a window that device > may use it. This is not abo...

...lling blk_cleanup_queue() when the surprize_removal > >>flag is set due to a disappeared device. It avoid hangs due to incomplete > >>requests (e.g. in-flight requests). Such requests must be considered as lost. > > > >Ugh. Can't we complete these immediately using detach_unused_buf? If not why? > > OK, I will try > > > > >>If the current remove callback was triggered due to an unregister driver, > >>and the surprize_removal is not already set (although the actual device > >>is already gone, e.g. virsh detach), blk_cleanup_queue() wo...

...lling blk_cleanup_queue() when the surprize_removal > >>flag is set due to a disappeared device. It avoid hangs due to incomplete > >>requests (e.g. in-flight requests). Such requests must be considered as lost. > > > >Ugh. Can't we complete these immediately using detach_unused_buf? If not why? > > OK, I will try > > > > >>If the current remove callback was triggered due to an unregister driver, > >>and the surprize_removal is not already set (although the actual device > >>is already gone, e.g. virsh detach), blk_cleanup_queue() wo...

Hey Rusty, Here are a few fixes for virtio and virtio_console. The first patch ensures the data elements of vqs are properly initialised at allocation-time so that we don't trigger BUG_ONs. I found this when hot-unplugging ports and there was just one unused buffer. detach_unused_buffers() kept returning pointers that were invalid. I didn't catch this earlier as I had the in_vq filled completely. Patches 2, 4 and 5 can be folded into the series as they are bugfixes for the functionality present there. About patch 5: When running a test that transfers a 260M file from the...

Hey Rusty, Here are a few fixes for virtio and virtio_console. The first patch ensures the data elements of vqs are properly initialised at allocation-time so that we don't trigger BUG_ONs. I found this when hot-unplugging ports and there was just one unused buffer. detach_unused_buffers() kept returning pointers that were invalid. I didn't catch this earlier as I had the in_vq filled completely. Patches 2, 4 and 5 can be folded into the series as they are bugfixes for the functionality present there. About patch 5: When running a test that transfers a 260M file from the...

...[i]; > > > > > > + vring_unmap_one_packed(vq, desc); > > > > > > + desc->flags = 0x0; > > > > > Looks like this is unnecessary. > > > > It's safer to zero it. If we don't zero it, after we > > > > call virtqueue_detach_unused_buf_packed() which calls > > > > this function, the desc is still available to the > > > > device. > > > > > > Well detach_unused_buf_packed() should be called after device is stopped, > > > otherwise even if you try to clear, there will still be a wi...

...[i]; > > > > > > + vring_unmap_one_packed(vq, desc); > > > > > > + desc->flags = 0x0; > > > > > Looks like this is unnecessary. > > > > It's safer to zero it. If we don't zero it, after we > > > > call virtqueue_detach_unused_buf_packed() which calls > > > > this function, the desc is still available to the > > > > device. > > > > > > Well detach_unused_buf_packed() should be called after device is stopped, > > > otherwise even if you try to clear, there will still be a wi...

...drivers/virtio/virtio_ring.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index cc2f73e..b0043fb 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -371,6 +371,7 @@ void *virtqueue_detach_unused_buf(struct virtqueue *_vq) /* detach_buf clears data, so grab it now. */ buf = vq->data[i]; detach_buf(vq, i); + vq->vring.avail->idx--; END_USE(vq); return buf; } -- 1.7.4

...drivers/virtio/virtio_ring.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index cc2f73e..b0043fb 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -371,6 +371,7 @@ void *virtqueue_detach_unused_buf(struct virtqueue *_vq) /* detach_buf clears data, so grab it now. */ buf = vq->data[i]; detach_buf(vq, i); + vq->vring.avail->idx--; END_USE(vq); return buf; } -- 1.7.4

...ing_unmap_one_packed(vq, desc); > > > > > > > > + desc->flags = 0x0; > > > > > > > Looks like this is unnecessary. > > > > > > It's safer to zero it. If we don't zero it, after we > > > > > > call virtqueue_detach_unused_buf_packed() which calls > > > > > > this function, the desc is still available to the > > > > > > device. > > > > > > > > > > Well detach_unused_buf_packed() should be called after device is stopped, > > > > > otherwise...

...ing_unmap_one_packed(vq, desc); > > > > > > > > + desc->flags = 0x0; > > > > > > > Looks like this is unnecessary. > > > > > > It's safer to zero it. If we don't zero it, after we > > > > > > call virtqueue_detach_unused_buf_packed() which calls > > > > > > this function, the desc is still available to the > > > > > > device. > > > > > > > > > > Well detach_unused_buf_packed() should be called after device is stopped, > > > > > otherwise...

...;vq->vring_packed.desc[i]; > > > > > + vring_unmap_one_packed(vq, desc); > > > > > + desc->flags = 0x0; > > > > Looks like this is unnecessary. > > > It's safer to zero it. If we don't zero it, after we > > > call virtqueue_detach_unused_buf_packed() which calls > > > this function, the desc is still available to the > > > device. > > > > Well detach_unused_buf_packed() should be called after device is stopped, > > otherwise even if you try to clear, there will still be a window that device > &gt...

...> > > + vring_unmap_one_packed(vq, desc); > > > > > > > + desc->flags = 0x0; > > > > > > Looks like this is unnecessary. > > > > > It's safer to zero it. If we don't zero it, after we > > > > > call virtqueue_detach_unused_buf_packed() which calls > > > > > this function, the desc is still available to the > > > > > device. > > > > > > > > Well detach_unused_buf_packed() should be called after device is stopped, > > > > otherwise even if you try to clear,...