Stefano, ping ?
On Wed, 20 Dec 2017 13:59:41 +0100
Greg Kurz <gr...@kaod.org> wrote:
> On Mon, 18 Dec 2017 11:41:23 +0100
> Greg Kurz <gr...@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): the server shouldn't send a
> > reply when the request was flushed.
> >
> > This patch changes the transport API so that the core 9p code may
> > choose to discard a reply instead of pushing it back to the client.
> > As a consequence, the push_and_notify naming isn't quite appropriate
> > anymore. And by the way, this operation also frees ring descriptors
> > allocated by the transport, ie, finalizes the completion of the PDU
> > in the transport layer. It is hence renamed to pdu_complete.
> >
> > This will be used by a subsequent patch to fix request cancellation.
> >
> > Signed-off-by: Greg Kurz <gr...@kaod.org>
> > ---
> > hw/9pfs/9p.c | 10 +++++-----
> > hw/9pfs/9p.h | 2 +-
> > hw/9pfs/virtio-9p-device.c | 11 +++++++----
> > hw/9pfs/xen-9p-backend.c | 14 ++++++++------
> > 4 files changed, 21 insertions(+), 16 deletions(-)
> >
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 1e4ebbe57687..dd8b7c6d69c6 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -642,7 +642,7 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu,
> > ssize_t len)
> >
> > ret = pdu_marshal(pdu, len, "s", &str);
> > if (ret < 0) {
> > - goto out_notify;
> > + goto out_complete;
> > }
> > len += ret;
> > id = P9_RERROR;
> > @@ -650,7 +650,7 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu,
> > ssize_t len)
> >
> > ret = pdu_marshal(pdu, len, "d", err);
> > if (ret < 0) {
> > - goto out_notify;
> > + goto out_complete;
> > }
> > len += ret;
> >
> > @@ -662,15 +662,15 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu,
> > ssize_t len)
> >
> > /* fill out the header */
> > if (pdu_marshal(pdu, 0, "dbw", (int32_t)len, id, pdu->tag) < 0) {
> > - goto out_notify;
> > + goto out_complete;
> > }
> >
> > /* keep these in sync */
> > pdu->size = len;
> > pdu->id = id;
> >
> > -out_notify:
> > - pdu->s->transport->push_and_notify(pdu);
> > +out_complete:
> > + pdu->s->transport->pdu_complete(pdu, false);
> >
> > /* Now wakeup anybody waiting in flush for this request */
> > if (!qemu_co_queue_next(&pdu->complete)) {
> > diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> > index ffe658ab8975..ea7657837784 100644
> > --- a/hw/9pfs/9p.h
> > +++ b/hw/9pfs/9p.h
> > @@ -363,7 +363,7 @@ struct V9fsTransport {
> > unsigned int *pniov, size_t size);
> > void (*init_out_iov_from_pdu)(V9fsPDU *pdu, struct iovec **piov,
> > unsigned int *pniov, size_t size);
> > - void (*push_and_notify)(V9fsPDU *pdu);
> > + void (*pdu_complete)(V9fsPDU *pdu, bool discard);
> > };
> >
> > static inline int v9fs_register_transport(V9fsState *s, const
> > V9fsTransport *t)
> > diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> > index 43f4e53f336f..083df987fd93 100644
> > --- a/hw/9pfs/virtio-9p-device.c
> > +++ b/hw/9pfs/virtio-9p-device.c
> > @@ -20,14 +20,17 @@
> > #include "hw/virtio/virtio-access.h"
> > #include "qemu/iov.h"
> >
> > -static void virtio_9p_push_and_notify(V9fsPDU *pdu)
> > +static void virtio_pdu_complete(V9fsPDU *pdu, bool discard)
> > {
> > V9fsState *s = pdu->s;
> > V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
> > VirtQueueElement *elem = v->elems[pdu->idx];
> >
> > - /* push onto queue and notify */
> > - virtqueue_push(v->vq, elem, pdu->size);
> > + if (discard) {
> > + virtqueue_detach_element(v->vq, elem, 0);
> > + } else {
> > + virtqueue_push(v->vq, elem, pdu->size);
>
> We should only notify the client if we actually push something
> back, ie, only call virtio_notify() if discard is false...
>
> > + }
> > g_free(elem);
> > v->elems[pdu->idx] = NULL;
> >
> > @@ -189,7 +192,7 @@ static const V9fsTransport virtio_9p_transport = {
> > .pdu_vunmarshal = virtio_pdu_vunmarshal,
> > .init_in_iov_from_pdu = virtio_init_in_iov_from_pdu,
> > .init_out_iov_from_pdu = virtio_init_out_iov_from_pdu,
> > - .push_and_notify = virtio_9p_push_and_notify,
> > + .pdu_complete = virtio_pdu_complete,
> > };
> >
> > static void virtio_9p_device_realize(DeviceState *dev, Error **errp)
> > diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
> > index df2a4100bf55..dc1e6c88885d 100644
> > --- a/hw/9pfs/xen-9p-backend.c
> > +++ b/hw/9pfs/xen-9p-backend.c
> > @@ -210,7 +210,7 @@ static void xen_9pfs_init_in_iov_from_pdu(V9fsPDU *pdu,
> > *pniov = num;
> > }
> >
> > -static void xen_9pfs_push_and_notify(V9fsPDU *pdu)
> > +static void xen_9pfs_pdu_complete(V9fsPDU *pdu, bool discard)
> > {
> > RING_IDX prod;
> > Xen9pfsDev *priv = container_of(pdu->s, Xen9pfsDev, state);
> > @@ -222,10 +222,12 @@ static void xen_9pfs_push_and_notify(V9fsPDU *pdu)
> > ring->intf->out_cons = ring->out_cons;
> > xen_wmb();
> >
> > - prod = ring->intf->in_prod;
> > - xen_rmb();
> > - ring->intf->in_prod = prod + pdu->size;
> > - xen_wmb();
> > + if (!discard) {
> > + prod = ring->intf->in_prod;
> > + xen_rmb();
> > + ring->intf->in_prod = prod + pdu->size;
> > + xen_wmb();
> > + }
> >
> > ring->inprogress = false;
> > xenevtchn_notify(ring->evtchndev, ring->local_port);
>
> ... should xenevtchn_notify() be moved to the !discard block ?
>
> > @@ -238,7 +240,7 @@ static const V9fsTransport xen_9p_transport = {
> > .pdu_vunmarshal = xen_9pfs_pdu_vunmarshal,
> > .init_in_iov_from_pdu = xen_9pfs_init_in_iov_from_pdu,
> > .init_out_iov_from_pdu = xen_9pfs_init_out_iov_from_pdu,
> > - .push_and_notify = xen_9pfs_push_and_notify,
> > + .pdu_complete = xen_9pfs_pdu_complete,
> > };
> >
> > static int xen_9pfs_init(struct XenDevice *xendev)
> >
>
>
