* [PATCH 0/2] 9p/trans_virtio: handle request cancellation
@ 2017-12-20 17:46 Greg Kurz
2017-12-20 17:46 ` [PATCH 1/2] virtio: allow to detach a buffer from the virtqueue Greg Kurz
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Greg Kurz @ 2017-12-20 17:46 UTC (permalink / raw)
To: linux-kernel; +Cc: v9fs-developer, Michael S. Tsirkin, Jason Wang, Al Viro
The 9p protocol mostly relies on a request/reply dialog between the
client and the server. A notable exception to this rule is request
cancellation (ie, flush in 9p wording): when the client requests an
in-flight request to be flushed, the server should only reply to the
flush request and not to the flushed in-flight request (otherwise it
considers the in-flight request to have completed just like it has
never been flushed).
It is up to the client to inform the transport that the in-flight request
has been flushed and won't receive a reply. This is achieved with the
'cancelled' operation of the struct p9_trans_module.
This operation isn't currently implemented with the virtio transport.
As a consequence, flushed requests leave buffers in the used list
forever and the virtqueue ends up in being able to process only one
request at a time.
This issue never popped up because the 9p server in QEMU had a bug
and would always reply to flushed requests. But this will be fixed
soon, so it is time to implement the 'cancelled' operation in the
9p virtio transport.
--
Greg
---
Greg Kurz (2):
virtio: allow to detach a buffer from the virtqueue
9p/trans_virtio: implement cancelled callback
drivers/virtio/virtio_ring.c | 28 ++++++++++++++++++++++++++++
include/linux/virtio.h | 1 +
net/9p/trans_virtio.c | 18 ++++++++++++++++++
3 files changed, 47 insertions(+)
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/2] virtio: allow to detach a buffer from the virtqueue 2017-12-20 17:46 [PATCH 0/2] 9p/trans_virtio: handle request cancellation Greg Kurz @ 2017-12-20 17:46 ` Greg Kurz 2018-01-19 19:49 ` Michael S. Tsirkin 2017-12-20 17:46 ` [PATCH 2/2] 9p/trans_virtio: implement cancelled callback Greg Kurz 2018-01-08 9:18 ` [PATCH 0/2] 9p/trans_virtio: handle request cancellation Greg Kurz 2 siblings, 1 reply; 6+ messages in thread From: Greg Kurz @ 2017-12-20 17:46 UTC (permalink / raw) To: linux-kernel; +Cc: v9fs-developer, Michael S. Tsirkin, Jason Wang, Al Viro 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> --- 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); 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); ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] virtio: allow to detach a buffer from the virtqueue 2017-12-20 17:46 ` [PATCH 1/2] virtio: allow to detach a buffer from the virtqueue Greg Kurz @ 2018-01-19 19:49 ` Michael S. Tsirkin 2018-01-20 9:20 ` Greg Kurz 0 siblings, 1 reply; 6+ messages in thread From: Michael S. Tsirkin @ 2018-01-19 19:49 UTC (permalink / raw) To: Greg Kurz; +Cc: linux-kernel, v9fs-developer, Jason Wang, Al Viro 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? > --- > 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); > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] virtio: allow to detach a buffer from the virtqueue 2018-01-19 19:49 ` Michael S. Tsirkin @ 2018-01-20 9:20 ` Greg Kurz 0 siblings, 0 replies; 6+ messages in thread From: Greg Kurz @ 2018-01-20 9:20 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: linux-kernel, v9fs-developer, Jason Wang, Al Viro 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); > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] 9p/trans_virtio: implement cancelled callback 2017-12-20 17:46 [PATCH 0/2] 9p/trans_virtio: handle request cancellation Greg Kurz 2017-12-20 17:46 ` [PATCH 1/2] virtio: allow to detach a buffer from the virtqueue Greg Kurz @ 2017-12-20 17:46 ` Greg Kurz 2018-01-08 9:18 ` [PATCH 0/2] 9p/trans_virtio: handle request cancellation Greg Kurz 2 siblings, 0 replies; 6+ messages in thread From: Greg Kurz @ 2017-12-20 17:46 UTC (permalink / raw) To: linux-kernel; +Cc: v9fs-developer, Michael S. Tsirkin, Jason Wang, Al Viro When 9p requests are successfully flushed, we must manually move the associated buffers to the virtqueue freelist, since the server doesn't send a reply. Signed-off-by: Greg Kurz <groug@kaod.org> --- net/9p/trans_virtio.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c index f3a4efcf1456..d3216af124c4 100644 --- a/net/9p/trans_virtio.c +++ b/net/9p/trans_virtio.c @@ -206,6 +206,23 @@ static int p9_virtio_cancel(struct p9_client *client, struct p9_req_t *req) return 1; } +static int p9_virtio_cancelled(struct p9_client *client, struct p9_req_t *req) +{ + struct virtio_chan *chan = client->trans; + unsigned long flags; + + spin_lock_irqsave(&chan->lock, flags); + if (virtqueue_detach_buf(chan->vq, req) != req) { + spin_unlock_irqrestore(&chan->lock, flags); + return 0; + } + chan->ring_bufs_avail = 1; + spin_unlock_irqrestore(&chan->lock, flags); + /* Wakeup if anyone waiting for VirtIO ring space. */ + wake_up(chan->vc_wq); + return 0; +} + /** * pack_sg_list_p - Just like pack_sg_list. Instead of taking a buffer, * this takes a list of pages. @@ -737,6 +754,7 @@ static struct p9_trans_module p9_virtio_trans = { .request = p9_virtio_request, .zc_request = p9_virtio_zc_request, .cancel = p9_virtio_cancel, + .cancelled = p9_virtio_cancelled, /* * We leave one entry for input and one entry for response * headers. We also skip one more entry to accomodate, address ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] 9p/trans_virtio: handle request cancellation 2017-12-20 17:46 [PATCH 0/2] 9p/trans_virtio: handle request cancellation Greg Kurz 2017-12-20 17:46 ` [PATCH 1/2] virtio: allow to detach a buffer from the virtqueue Greg Kurz 2017-12-20 17:46 ` [PATCH 2/2] 9p/trans_virtio: implement cancelled callback Greg Kurz @ 2018-01-08 9:18 ` Greg Kurz 2 siblings, 0 replies; 6+ messages in thread From: Greg Kurz @ 2018-01-08 9:18 UTC (permalink / raw) To: linux-kernel; +Cc: v9fs-developer, Michael S. Tsirkin, Jason Wang, Al Viro Hi Michael, I wish you a happy new year, and I really need some feedback on this series because it holds down some patches on the QEMU side. Thanks, -- Greg On Wed, 20 Dec 2017 18:46:27 +0100 Greg Kurz <groug@kaod.org> wrote: > The 9p protocol mostly relies on a request/reply dialog between the > client and the server. A notable exception to this rule is request > cancellation (ie, flush in 9p wording): when the client requests an > in-flight request to be flushed, the server should only reply to the > flush request and not to the flushed in-flight request (otherwise it > considers the in-flight request to have completed just like it has > never been flushed). > > It is up to the client to inform the transport that the in-flight request > has been flushed and won't receive a reply. This is achieved with the > 'cancelled' operation of the struct p9_trans_module. > > This operation isn't currently implemented with the virtio transport. > As a consequence, flushed requests leave buffers in the used list > forever and the virtqueue ends up in being able to process only one > request at a time. > > This issue never popped up because the 9p server in QEMU had a bug > and would always reply to flushed requests. But this will be fixed > soon, so it is time to implement the 'cancelled' operation in the > 9p virtio transport. > > -- > Greg > > --- > > Greg Kurz (2): > virtio: allow to detach a buffer from the virtqueue > 9p/trans_virtio: implement cancelled callback > > > drivers/virtio/virtio_ring.c | 28 ++++++++++++++++++++++++++++ > include/linux/virtio.h | 1 + > net/9p/trans_virtio.c | 18 ++++++++++++++++++ > 3 files changed, 47 insertions(+) > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-01-20 9:58 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-12-20 17:46 [PATCH 0/2] 9p/trans_virtio: handle request cancellation Greg Kurz 2017-12-20 17:46 ` [PATCH 1/2] virtio: allow to detach a buffer from the virtqueue Greg Kurz 2018-01-19 19:49 ` Michael S. Tsirkin 2018-01-20 9:20 ` Greg Kurz 2017-12-20 17:46 ` [PATCH 2/2] 9p/trans_virtio: implement cancelled callback Greg Kurz 2018-01-08 9:18 ` [PATCH 0/2] 9p/trans_virtio: handle request cancellation Greg Kurz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).