Messages in this thread | | | Date | Sat, 20 Jan 2018 10:20:44 +0100 | From | Greg Kurz <> | Subject | Re: [PATCH 1/2] virtio: allow to detach a buffer from the virtqueue |
| |
On Fri, 19 Jan 2018 21:49:38 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Dec 20, 2017 at 06:46:42PM +0100, Greg Kurz wrote: > > It is possible for a device to stop using buffers without pushing them > > back to the driver. This is the case for example with the 9p virtio > > device: if the driver flushes an in-flight request, the 9p specification > > specification [*] mandates the server to "to purge the pending response". > > The reply to the flush request indicates that the 9p server has stopped > > using the buffers of the flushed in-flight request. But since the server > > doesn't push back the associated buffers, they don't go back to the free > > list. This leads the virtqueue to end up with a single slot to handle all > > the dialog with the device, ie, serialize all I/Os. > > > > This patch hence gives the possibility for device specific code to > > explicitly detach a given buffer from the used ring and put it back > > to the free list. > > > > [*] http://man.cat-v.org/plan_9/5/flush > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > It would be better to just change the server to mark all flushed > requests as used. Why isn't that an option? >
It is just because I started to look at this from a 9p client code perspective. It supports several other transports than virtio, and has a set of transport hooks. One of this hook is called when the server is supposed to have cancelled a request. My first thought was to rely on this 'cancelled' hook, like it is done for the RDMA and fd-based transport.
But now, I realize I should be able to come up with something simpler in the virtio case, if I do like you suggest. It would require a change in the client though, as the current code assumes anything pushed by the other end contains a 9p message, which would be wrong if QEMU does something like virtqueue_push(vq, elem, 0).
I'll give a try.
Thanks!
> > --- > > drivers/virtio/virtio_ring.c | 28 ++++++++++++++++++++++++++++ > > include/linux/virtio.h | 1 + > > 2 files changed, 29 insertions(+) > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index eb30f3e09a47..886e9d054de3 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -936,6 +936,34 @@ void *virtqueue_detach_unused_buf(struct virtqueue *_vq) > > } > > EXPORT_SYMBOL_GPL(virtqueue_detach_unused_buf); > > > > +/** > > + * virtqueue_detach_buf - detach specific buffer > > + * @vq: the struct virtqueue we're talking about. > > + * > > + * Returns NULL or the "data" token handed to virtqueue_add_*(). > > + * This should only be used if the driver really knows the buffer > > + * isn't needed anymore by the device. > > + */ > > +void *virtqueue_detach_buf(struct virtqueue *_vq, void *buf) > > +{ > > + struct vring_virtqueue *vq = to_vvq(_vq); > > + unsigned int i; > > + > > + START_USE(vq); > > + > > + for (i = 0; i < vq->vring.num; i++) { > > + if (vq->desc_state[i].data != buf) > > + continue; > > + detach_buf(vq, i, NULL); > > + END_USE(vq); > > + return buf; > > + } > > + > > + END_USE(vq); > > + return NULL; > > +} > > +EXPORT_SYMBOL_GPL(virtqueue_detach_buf); > > + > > irqreturn_t vring_interrupt(int irq, void *_vq) > > { > > struct vring_virtqueue *vq = to_vvq(_vq); > > Unfortunately the used index gets out of sync with the available index > then. > > E.g. you are breaking the invariant that used == avail means ring empty. > > Any chance to preserve this invariant? > > > > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h > > index 988c7355bc22..850158518ce5 100644 > > --- a/include/linux/virtio.h > > +++ b/include/linux/virtio.h > > @@ -80,6 +80,7 @@ bool virtqueue_poll(struct virtqueue *vq, unsigned); > > bool virtqueue_enable_cb_delayed(struct virtqueue *vq); > > > > void *virtqueue_detach_unused_buf(struct virtqueue *vq); > > +void *virtqueue_detach_buf(struct virtqueue *vq, void *buf); > > > > unsigned int virtqueue_get_vring_size(struct virtqueue *vq); > >
| |