[PATCH 0/2] revert 9pfs reply truncation, wait for free room to reply

[PATCH 0/2] revert 9pfs reply truncation, wait for free room to reply

[Stefano Stabellini]

Hi all,

This short series reverts commit 16724a173049ac29c7b5ade741da93a0f46edff
becauses it is the cause for https://bugs.launchpad.net/bugs/1877688.

The original issue addressed by 16724a173049ac29c7b5ade741da93a0f46edff
is solved differently in this series by using qemu_coroutine_yield() to
wait for the client to free more data from the ring before sending the
reply.

Cheers,

Stefano

[PATCH 1/2] Revert "9p: init_in_iov_from_pdu can truncate the size"

[Stefano Stabellini]

From: Stefano Stabellini <stefano.stabellini@xilinx.com>

This reverts commit 16724a173049ac29c7b5ade741da93a0f46edff7.
It causes https://bugs.launchpad.net/bugs/1877688.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
 hw/9pfs/9p.c               | 33 +++++++++++----------------------
 hw/9pfs/9p.h               |  2 +-
 hw/9pfs/virtio-9p-device.c | 11 ++++-------
 hw/9pfs/xen-9p-backend.c   | 15 ++++++---------
 4 files changed, 22 insertions(+), 39 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index a2a14b5979..d39bfee462 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -2102,29 +2102,22 @@ out_nofid:
  * with qemu_iovec_destroy().
  */
 static void v9fs_init_qiov_from_pdu(QEMUIOVector *qiov, V9fsPDU *pdu,
-                                    size_t skip, size_t *size,
+                                    size_t skip, size_t size,
                                     bool is_write)
 {
     QEMUIOVector elem;
     struct iovec *iov;
     unsigned int niov;
-    size_t alloc_size = *size + skip;
 
     if (is_write) {
-        pdu->s->transport->init_out_iov_from_pdu(pdu, &iov, &niov, alloc_size);
+        pdu->s->transport->init_out_iov_from_pdu(pdu, &iov, &niov, size + skip);
     } else {
-        pdu->s->transport->init_in_iov_from_pdu(pdu, &iov, &niov, &alloc_size);
-    }
-
-    if (alloc_size < skip) {
-        *size = 0;
-    } else {
-        *size = alloc_size - skip;
+        pdu->s->transport->init_in_iov_from_pdu(pdu, &iov, &niov, size + skip);
     }
 
     qemu_iovec_init_external(&elem, iov, niov);
     qemu_iovec_init(qiov, niov);
-    qemu_iovec_concat(qiov, &elem, skip, *size);
+    qemu_iovec_concat(qiov, &elem, skip, size);
 }
 
 static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
@@ -2132,14 +2125,15 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
 {
     ssize_t err;
     size_t offset = 7;
-    size_t read_count;
+    uint64_t read_count;
     QEMUIOVector qiov_full;
 
     if (fidp->fs.xattr.len < off) {
         read_count = 0;
-    } else if (fidp->fs.xattr.len - off < max_count) {
-        read_count = fidp->fs.xattr.len - off;
     } else {
+        read_count = fidp->fs.xattr.len - off;
+    }
+    if (read_count > max_count) {
         read_count = max_count;
     }
     err = pdu_marshal(pdu, offset, "d", read_count);
@@ -2148,7 +2142,7 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
     }
     offset += err;
 
-    v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, &read_count, false);
+    v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, read_count, false);
     err = v9fs_pack(qiov_full.iov, qiov_full.niov, 0,
                     ((char *)fidp->fs.xattr.value) + off,
                     read_count);
@@ -2277,11 +2271,9 @@ static void coroutine_fn v9fs_read(void *opaque)
         QEMUIOVector qiov_full;
         QEMUIOVector qiov;
         int32_t len;
-        size_t size = max_count;
 
-        v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset + 4, &size, false);
+        v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset + 4, max_count, false);
         qemu_iovec_init(&qiov, qiov_full.niov);
-        max_count = size;
         do {
             qemu_iovec_reset(&qiov);
             qemu_iovec_concat(&qiov, &qiov_full, count, qiov_full.size - count);
@@ -2532,7 +2524,6 @@ static void coroutine_fn v9fs_write(void *opaque)
     int32_t len = 0;
     int32_t total = 0;
     size_t offset = 7;
-    size_t size;
     V9fsFidState *fidp;
     V9fsPDU *pdu = opaque;
     V9fsState *s = pdu->s;
@@ -2545,9 +2536,7 @@ static void coroutine_fn v9fs_write(void *opaque)
         return;
     }
     offset += err;
-    size = count;
-    v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, &size, true);
-    count = size;
+    v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, count, true);
     trace_v9fs_write(pdu->tag, pdu->id, fid, off, count, qiov_full.niov);
 
     fidp = get_fid(pdu, fid);
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index dd1c6cb8d2..1b9e110605 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -436,7 +436,7 @@ struct V9fsTransport {
     ssize_t     (*pdu_vunmarshal)(V9fsPDU *pdu, size_t offset, const char *fmt,
                                   va_list ap);
     void        (*init_in_iov_from_pdu)(V9fsPDU *pdu, struct iovec **piov,
-                                        unsigned int *pniov, size_t *size);
+                                        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);
diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index e5b44977c7..36f3aa9352 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -147,22 +147,19 @@ static ssize_t virtio_pdu_vunmarshal(V9fsPDU *pdu, size_t offset,
 }
 
 static void virtio_init_in_iov_from_pdu(V9fsPDU *pdu, struct iovec **piov,
-                                        unsigned int *pniov, size_t *size)
+                                        unsigned int *pniov, size_t size)
 {
     V9fsState *s = pdu->s;
     V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
     VirtQueueElement *elem = v->elems[pdu->idx];
     size_t buf_size = iov_size(elem->in_sg, elem->in_num);
 
-    if (buf_size < P9_IOHDRSZ) {
+    if (buf_size < size) {
         VirtIODevice *vdev = VIRTIO_DEVICE(v);
 
         virtio_error(vdev,
-                     "VirtFS reply type %d needs %zu bytes, buffer has %zu, less than minimum",
-                     pdu->id + 1, *size, buf_size);
-    }
-    if (buf_size < *size) {
-        *size = buf_size;
+                     "VirtFS reply type %d needs %zu bytes, buffer has %zu",
+                     pdu->id + 1, size, buf_size);
     }
 
     *piov = elem->in_sg;
diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index f04caabfe5..fc197f6c8a 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -188,7 +188,7 @@ static void xen_9pfs_init_out_iov_from_pdu(V9fsPDU *pdu,
 static void xen_9pfs_init_in_iov_from_pdu(V9fsPDU *pdu,
                                           struct iovec **piov,
                                           unsigned int *pniov,
-                                          size_t *size)
+                                          size_t size)
 {
     Xen9pfsDev *xen_9pfs = container_of(pdu->s, Xen9pfsDev, state);
     Xen9pfsRing *ring = &xen_9pfs->rings[pdu->tag % xen_9pfs->num_rings];
@@ -198,19 +198,16 @@ static void xen_9pfs_init_in_iov_from_pdu(V9fsPDU *pdu,
     g_free(ring->sg);
 
     ring->sg = g_new0(struct iovec, 2);
-    xen_9pfs_in_sg(ring, ring->sg, &num, pdu->idx, *size);
+    xen_9pfs_in_sg(ring, ring->sg, &num, pdu->idx, size);
 
     buf_size = iov_size(ring->sg, num);
-    if (buf_size  < P9_IOHDRSZ) {
-        xen_pv_printf(&xen_9pfs->xendev, 0, "Xen 9pfs reply type %d needs "
-                      "%zu bytes, buffer has %zu, less than minimum\n",
-                      pdu->id + 1, *size, buf_size);
+    if (buf_size  < size) {
+        xen_pv_printf(&xen_9pfs->xendev, 0, "Xen 9pfs request type %d"
+                "needs %zu bytes, buffer has %zu\n", pdu->id, size,
+                buf_size);
         xen_be_set_state(&xen_9pfs->xendev, XenbusStateClosing);
         xen_9pfs_disconnect(&xen_9pfs->xendev);
     }
-    if (buf_size  < *size) {
-        *size = buf_size;
-    }
 
     *piov = ring->sg;
     *pniov = num;
-- 
2.17.1

Re: [PATCH 1/2] Revert "9p: init_in_iov_from_pdu can truncate the size"

[Christian Schoenebeck]

On Mittwoch, 20. Mai 2020 03:47:11 CEST Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> This reverts commit 16724a173049ac29c7b5ade741da93a0f46edff7.
> It causes https://bugs.launchpad.net/bugs/1877688.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> ---

Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

>  hw/9pfs/9p.c               | 33 +++++++++++----------------------
>  hw/9pfs/9p.h               |  2 +-
>  hw/9pfs/virtio-9p-device.c | 11 ++++-------
>  hw/9pfs/xen-9p-backend.c   | 15 ++++++---------
>  4 files changed, 22 insertions(+), 39 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index a2a14b5979..d39bfee462 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -2102,29 +2102,22 @@ out_nofid:
>   * with qemu_iovec_destroy().
>   */
>  static void v9fs_init_qiov_from_pdu(QEMUIOVector *qiov, V9fsPDU *pdu,
> -                                    size_t skip, size_t *size,
> +                                    size_t skip, size_t size,
>                                      bool is_write)
>  {
>      QEMUIOVector elem;
>      struct iovec *iov;
>      unsigned int niov;
> -    size_t alloc_size = *size + skip;
> 
>      if (is_write) {
> -        pdu->s->transport->init_out_iov_from_pdu(pdu, &iov, &niov,
> alloc_size); +        pdu->s->transport->init_out_iov_from_pdu(pdu, &iov,
> &niov, size + skip); } else {
> -        pdu->s->transport->init_in_iov_from_pdu(pdu, &iov, &niov,
> &alloc_size); -    }
> -
> -    if (alloc_size < skip) {
> -        *size = 0;
> -    } else {
> -        *size = alloc_size - skip;
> +        pdu->s->transport->init_in_iov_from_pdu(pdu, &iov, &niov, size +
> skip); }
> 
>      qemu_iovec_init_external(&elem, iov, niov);
>      qemu_iovec_init(qiov, niov);
> -    qemu_iovec_concat(qiov, &elem, skip, *size);
> +    qemu_iovec_concat(qiov, &elem, skip, size);
>  }
> 
>  static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
> @@ -2132,14 +2125,15 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU
> *pdu, V9fsFidState *fidp, {
>      ssize_t err;
>      size_t offset = 7;
> -    size_t read_count;
> +    uint64_t read_count;
>      QEMUIOVector qiov_full;
> 
>      if (fidp->fs.xattr.len < off) {
>          read_count = 0;
> -    } else if (fidp->fs.xattr.len - off < max_count) {
> -        read_count = fidp->fs.xattr.len - off;
>      } else {
> +        read_count = fidp->fs.xattr.len - off;
> +    }
> +    if (read_count > max_count) {
>          read_count = max_count;
>      }
>      err = pdu_marshal(pdu, offset, "d", read_count);
> @@ -2148,7 +2142,7 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu,
> V9fsFidState *fidp, }
>      offset += err;
> 
> -    v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, &read_count, false);
> +    v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, read_count, false);
>      err = v9fs_pack(qiov_full.iov, qiov_full.niov, 0,
>                      ((char *)fidp->fs.xattr.value) + off,
>                      read_count);
> @@ -2277,11 +2271,9 @@ static void coroutine_fn v9fs_read(void *opaque)
>          QEMUIOVector qiov_full;
>          QEMUIOVector qiov;
>          int32_t len;
> -        size_t size = max_count;
> 
> -        v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset + 4, &size, false);
> +        v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset + 4, max_count,
> false); qemu_iovec_init(&qiov, qiov_full.niov);
> -        max_count = size;
>          do {
>              qemu_iovec_reset(&qiov);
>              qemu_iovec_concat(&qiov, &qiov_full, count, qiov_full.size -
> count); @@ -2532,7 +2524,6 @@ static void coroutine_fn v9fs_write(void
> *opaque) int32_t len = 0;
>      int32_t total = 0;
>      size_t offset = 7;
> -    size_t size;
>      V9fsFidState *fidp;
>      V9fsPDU *pdu = opaque;
>      V9fsState *s = pdu->s;
> @@ -2545,9 +2536,7 @@ static void coroutine_fn v9fs_write(void *opaque)
>          return;
>      }
>      offset += err;
> -    size = count;
> -    v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, &size, true);
> -    count = size;
> +    v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, count, true);
>      trace_v9fs_write(pdu->tag, pdu->id, fid, off, count, qiov_full.niov);
> 
>      fidp = get_fid(pdu, fid);
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index dd1c6cb8d2..1b9e110605 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -436,7 +436,7 @@ struct V9fsTransport {
>      ssize_t     (*pdu_vunmarshal)(V9fsPDU *pdu, size_t offset, const char
> *fmt, va_list ap);
>      void        (*init_in_iov_from_pdu)(V9fsPDU *pdu, struct iovec **piov,
> -                                        unsigned int *pniov, size_t *size);
> +                                        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);
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index e5b44977c7..36f3aa9352 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -147,22 +147,19 @@ static ssize_t virtio_pdu_vunmarshal(V9fsPDU *pdu,
> size_t offset, }
> 
>  static void virtio_init_in_iov_from_pdu(V9fsPDU *pdu, struct iovec **piov,
> -                                        unsigned int *pniov, size_t *size)
> +                                        unsigned int *pniov, size_t size)
>  {
>      V9fsState *s = pdu->s;
>      V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
>      VirtQueueElement *elem = v->elems[pdu->idx];
>      size_t buf_size = iov_size(elem->in_sg, elem->in_num);
> 
> -    if (buf_size < P9_IOHDRSZ) {
> +    if (buf_size < size) {
>          VirtIODevice *vdev = VIRTIO_DEVICE(v);
> 
>          virtio_error(vdev,
> -                     "VirtFS reply type %d needs %zu bytes, buffer has %zu,
> less than minimum", -                     pdu->id + 1, *size, buf_size);
> -    }
> -    if (buf_size < *size) {
> -        *size = buf_size;
> +                     "VirtFS reply type %d needs %zu bytes, buffer has
> %zu", +                     pdu->id + 1, size, buf_size);
>      }
> 
>      *piov = elem->in_sg;
> diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
> index f04caabfe5..fc197f6c8a 100644
> --- a/hw/9pfs/xen-9p-backend.c
> +++ b/hw/9pfs/xen-9p-backend.c
> @@ -188,7 +188,7 @@ static void xen_9pfs_init_out_iov_from_pdu(V9fsPDU *pdu,
> static void xen_9pfs_init_in_iov_from_pdu(V9fsPDU *pdu,
>                                            struct iovec **piov,
>                                            unsigned int *pniov,
> -                                          size_t *size)
> +                                          size_t size)
>  {
>      Xen9pfsDev *xen_9pfs = container_of(pdu->s, Xen9pfsDev, state);
>      Xen9pfsRing *ring = &xen_9pfs->rings[pdu->tag % xen_9pfs->num_rings];
> @@ -198,19 +198,16 @@ static void xen_9pfs_init_in_iov_from_pdu(V9fsPDU
> *pdu, g_free(ring->sg);
> 
>      ring->sg = g_new0(struct iovec, 2);
> -    xen_9pfs_in_sg(ring, ring->sg, &num, pdu->idx, *size);
> +    xen_9pfs_in_sg(ring, ring->sg, &num, pdu->idx, size);
> 
>      buf_size = iov_size(ring->sg, num);
> -    if (buf_size  < P9_IOHDRSZ) {
> -        xen_pv_printf(&xen_9pfs->xendev, 0, "Xen 9pfs reply type %d needs "
> -                      "%zu bytes, buffer has %zu, less than minimum\n", - 
>                     pdu->id + 1, *size, buf_size);
> +    if (buf_size  < size) {
> +        xen_pv_printf(&xen_9pfs->xendev, 0, "Xen 9pfs request type %d"
> +                "needs %zu bytes, buffer has %zu\n", pdu->id, size,
> +                buf_size);
>          xen_be_set_state(&xen_9pfs->xendev, XenbusStateClosing);
>          xen_9pfs_disconnect(&xen_9pfs->xendev);
>      }
> -    if (buf_size  < *size) {
> -        *size = buf_size;
> -    }
> 
>      *piov = ring->sg;
>      *pniov = num;

Best regards,
Christian Schoenebeck

[PATCH 2/2] xen/9pfs: yield when there isn't enough room on the ring

[Stefano Stabellini]

From: Stefano Stabellini <stefano.stabellini@xilinx.com>

Instead of truncating replies, which is problematic, wait until the
client reads more data and frees bytes on the reply ring.

Do that by calling qemu_coroutine_yield(). The corresponding
qemu_coroutine_enter_if_inactive() is called from xen_9pfs_bh upon
receiving the next notification from the client.

We need to be careful to avoid races in case xen_9pfs_bh and the
coroutine are both active at the same time. In xen_9pfs_bh, wait until
either the critical section is over (ring->co == NULL) or until the
coroutine becomes inactive (qemu_coroutine_yield() was called) before
continuing. Then, simply wake up the coroutine if it is inactive.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
 hw/9pfs/xen-9p-backend.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index fc197f6c8a..3939539028 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -37,6 +37,7 @@ typedef struct Xen9pfsRing {
 
     struct iovec *sg;
     QEMUBH *bh;
+    Coroutine *co;
 
     /* local copies, so that we can read/write PDU data directly from
      * the ring */
@@ -198,16 +199,18 @@ static void xen_9pfs_init_in_iov_from_pdu(V9fsPDU *pdu,
     g_free(ring->sg);
 
     ring->sg = g_new0(struct iovec, 2);
-    xen_9pfs_in_sg(ring, ring->sg, &num, pdu->idx, size);
+    ring->co = qemu_coroutine_self();
+    smp_wmb();
 
+again:
+    xen_9pfs_in_sg(ring, ring->sg, &num, pdu->idx, size);
     buf_size = iov_size(ring->sg, num);
     if (buf_size  < size) {
-        xen_pv_printf(&xen_9pfs->xendev, 0, "Xen 9pfs request type %d"
-                "needs %zu bytes, buffer has %zu\n", pdu->id, size,
-                buf_size);
-        xen_be_set_state(&xen_9pfs->xendev, XenbusStateClosing);
-        xen_9pfs_disconnect(&xen_9pfs->xendev);
+        qemu_coroutine_yield();
+        goto again;
     }
+    ring->co = NULL;
+    smp_wmb();
 
     *piov = ring->sg;
     *pniov = num;
@@ -292,6 +295,19 @@ static int xen_9pfs_receive(Xen9pfsRing *ring)
 static void xen_9pfs_bh(void *opaque)
 {
     Xen9pfsRing *ring = opaque;
+    bool wait;
+
+again:
+    wait = ring->co != NULL && qemu_coroutine_entered(ring->co);
+    smp_rmb();
+    if (wait) {
+        cpu_relax();
+        goto again;
+    }
+
+    if (ring->co != NULL) {
+        qemu_coroutine_enter_if_inactive(ring->co);
+    }
     xen_9pfs_receive(ring);
 }
 
-- 
2.17.1

Re: [PATCH 2/2] xen/9pfs: yield when there isn't enough room on the ring

[Christian Schoenebeck]

On Mittwoch, 20. Mai 2020 03:47:12 CEST Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> Instead of truncating replies, which is problematic, wait until the
> client reads more data and frees bytes on the reply ring.
> 
> Do that by calling qemu_coroutine_yield(). The corresponding
> qemu_coroutine_enter_if_inactive() is called from xen_9pfs_bh upon
> receiving the next notification from the client.
> 
> We need to be careful to avoid races in case xen_9pfs_bh and the
> coroutine are both active at the same time. In xen_9pfs_bh, wait until
> either the critical section is over (ring->co == NULL) or until the
> coroutine becomes inactive (qemu_coroutine_yield() was called) before
> continuing. Then, simply wake up the coroutine if it is inactive.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> ---

In general this patch makes sense to me, and much better and cleaner solution 
than what we discussed before. Just one detail ...

>  hw/9pfs/xen-9p-backend.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
> index fc197f6c8a..3939539028 100644
> --- a/hw/9pfs/xen-9p-backend.c
> +++ b/hw/9pfs/xen-9p-backend.c
> @@ -37,6 +37,7 @@ typedef struct Xen9pfsRing {
> 
>      struct iovec *sg;
>      QEMUBH *bh;
> +    Coroutine *co;
> 
>      /* local copies, so that we can read/write PDU data directly from
>       * the ring */
> @@ -198,16 +199,18 @@ static void xen_9pfs_init_in_iov_from_pdu(V9fsPDU
> *pdu, g_free(ring->sg);
> 
>      ring->sg = g_new0(struct iovec, 2);
> -    xen_9pfs_in_sg(ring, ring->sg, &num, pdu->idx, size);
> +    ring->co = qemu_coroutine_self();
> +    smp_wmb();
> 
> +again:
> +    xen_9pfs_in_sg(ring, ring->sg, &num, pdu->idx, size);
>      buf_size = iov_size(ring->sg, num);
>      if (buf_size  < size) {
> -        xen_pv_printf(&xen_9pfs->xendev, 0, "Xen 9pfs request type %d"
> -                "needs %zu bytes, buffer has %zu\n", pdu->id, size,
> -                buf_size);
> -        xen_be_set_state(&xen_9pfs->xendev, XenbusStateClosing);
> -        xen_9pfs_disconnect(&xen_9pfs->xendev);
> +        qemu_coroutine_yield();
> +        goto again;
>      }
> +    ring->co = NULL;
> +    smp_wmb();
> 
>      *piov = ring->sg;
>      *pniov = num;
> @@ -292,6 +295,19 @@ static int xen_9pfs_receive(Xen9pfsRing *ring)
>  static void xen_9pfs_bh(void *opaque)
>  {
>      Xen9pfsRing *ring = opaque;
> +    bool wait;
> +
> +again:
> +    wait = ring->co != NULL && qemu_coroutine_entered(ring->co);
> +    smp_rmb();
> +    if (wait) {
> +        cpu_relax();
> +        goto again;
> +    }
> +
> +    if (ring->co != NULL) {
> +        qemu_coroutine_enter_if_inactive(ring->co);

... correct me if I am wrong, but AFAIK qemu_coroutine_enter_if_inactive() 
will simply run the coroutine directly on caller's thread, it will not 
dispatch the coroutine onto the thread which yielded the coroutine before.

> +    }
>      xen_9pfs_receive(ring);
>  }

AFAICS you have not addressed the problem msize >> xen ringbuffer size, in 
which case I would expect the Xen driver to loop forever. Am I missing 
something or have you postponed addressing this?

Best regards,
Christian Schoenebeck

Re: [PATCH 2/2] xen/9pfs: yield when there isn't enough room on the ring

[Stefano Stabellini]

On Wed, 20 May 2020, Christian Schoenebeck wrote:
> On Mittwoch, 20. Mai 2020 03:47:12 CEST Stefano Stabellini wrote:
> > From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > 
> > Instead of truncating replies, which is problematic, wait until the
> > client reads more data and frees bytes on the reply ring.
> > 
> > Do that by calling qemu_coroutine_yield(). The corresponding
> > qemu_coroutine_enter_if_inactive() is called from xen_9pfs_bh upon
> > receiving the next notification from the client.
> > 
> > We need to be careful to avoid races in case xen_9pfs_bh and the
> > coroutine are both active at the same time. In xen_9pfs_bh, wait until
> > either the critical section is over (ring->co == NULL) or until the
> > coroutine becomes inactive (qemu_coroutine_yield() was called) before
> > continuing. Then, simply wake up the coroutine if it is inactive.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > ---
> 
> In general this patch makes sense to me, and much better and cleaner solution 
> than what we discussed before. Just one detail ...
> 
> >  hw/9pfs/xen-9p-backend.c | 28 ++++++++++++++++++++++------
> >  1 file changed, 22 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
> > index fc197f6c8a..3939539028 100644
> > --- a/hw/9pfs/xen-9p-backend.c
> > +++ b/hw/9pfs/xen-9p-backend.c
> > @@ -37,6 +37,7 @@ typedef struct Xen9pfsRing {
> > 
> >      struct iovec *sg;
> >      QEMUBH *bh;
> > +    Coroutine *co;
> > 
> >      /* local copies, so that we can read/write PDU data directly from
> >       * the ring */
> > @@ -198,16 +199,18 @@ static void xen_9pfs_init_in_iov_from_pdu(V9fsPDU
> > *pdu, g_free(ring->sg);
> > 
> >      ring->sg = g_new0(struct iovec, 2);
> > -    xen_9pfs_in_sg(ring, ring->sg, &num, pdu->idx, size);
> > +    ring->co = qemu_coroutine_self();
> > +    smp_wmb();
> > 
> > +again:
> > +    xen_9pfs_in_sg(ring, ring->sg, &num, pdu->idx, size);
> >      buf_size = iov_size(ring->sg, num);
> >      if (buf_size  < size) {
> > -        xen_pv_printf(&xen_9pfs->xendev, 0, "Xen 9pfs request type %d"
> > -                "needs %zu bytes, buffer has %zu\n", pdu->id, size,
> > -                buf_size);
> > -        xen_be_set_state(&xen_9pfs->xendev, XenbusStateClosing);
> > -        xen_9pfs_disconnect(&xen_9pfs->xendev);
> > +        qemu_coroutine_yield();
> > +        goto again;
> >      }
> > +    ring->co = NULL;
> > +    smp_wmb();
> > 
> >      *piov = ring->sg;
> >      *pniov = num;
> > @@ -292,6 +295,19 @@ static int xen_9pfs_receive(Xen9pfsRing *ring)
> >  static void xen_9pfs_bh(void *opaque)
> >  {
> >      Xen9pfsRing *ring = opaque;
> > +    bool wait;
> > +
> > +again:
> > +    wait = ring->co != NULL && qemu_coroutine_entered(ring->co);
> > +    smp_rmb();
> > +    if (wait) {
> > +        cpu_relax();
> > +        goto again;
> > +    }
> > +
> > +    if (ring->co != NULL) {
> > +        qemu_coroutine_enter_if_inactive(ring->co);
> 
> ... correct me if I am wrong, but AFAIK qemu_coroutine_enter_if_inactive() 
> will simply run the coroutine directly on caller's thread, it will not 
> dispatch the coroutine onto the thread which yielded the coroutine before.

Yes, that is correct. I thought it would be fine because the caller here
is a bh function so it should have no problems entering the coroutine.

But I am not that much of an expert on coroutines... Do you think there
could be issues?


> > +    }
> >      xen_9pfs_receive(ring);
> >  }
> 
> AFAICS you have not addressed the problem msize >> xen ringbuffer size, in 
> which case I would expect the Xen driver to loop forever. Am I missing 
> something or have you postponed addressing this?

Yes, I postponed addressing that issue because of a couple of reasons.

For starter strictly speaking it should not be required: msize cannot be
bigger than the ring, but it can be equal to the ring increasing the
chances of having to wait in QEMU. It should still be correct but the
performance might not be great.

The other reason is that I already have the patches for both QEMU and
Linux, but I am seeing a strange error setting order = 10. Order = 9
works fine. I would like to do a bit more investigation before sending
those patches.

Re: [PATCH 2/2] xen/9pfs: yield when there isn't enough room on the ring

[Stefano Stabellini]

On Wed, 20 May 2020, Stefano Stabellini wrote:
> On Wed, 20 May 2020, Christian Schoenebeck wrote:
> > On Mittwoch, 20. Mai 2020 03:47:12 CEST Stefano Stabellini wrote:
> > > From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > > 
> > > Instead of truncating replies, which is problematic, wait until the
> > > client reads more data and frees bytes on the reply ring.
> > > 
> > > Do that by calling qemu_coroutine_yield(). The corresponding
> > > qemu_coroutine_enter_if_inactive() is called from xen_9pfs_bh upon
> > > receiving the next notification from the client.
> > > 
> > > We need to be careful to avoid races in case xen_9pfs_bh and the
> > > coroutine are both active at the same time. In xen_9pfs_bh, wait until
> > > either the critical section is over (ring->co == NULL) or until the
> > > coroutine becomes inactive (qemu_coroutine_yield() was called) before
> > > continuing. Then, simply wake up the coroutine if it is inactive.
> > > 
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > > ---
> > 
> > In general this patch makes sense to me, and much better and cleaner solution 
> > than what we discussed before. Just one detail ...
> > 
> > >  hw/9pfs/xen-9p-backend.c | 28 ++++++++++++++++++++++------
> > >  1 file changed, 22 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
> > > index fc197f6c8a..3939539028 100644
> > > --- a/hw/9pfs/xen-9p-backend.c
> > > +++ b/hw/9pfs/xen-9p-backend.c
> > > @@ -37,6 +37,7 @@ typedef struct Xen9pfsRing {
> > > 
> > >      struct iovec *sg;
> > >      QEMUBH *bh;
> > > +    Coroutine *co;
> > > 
> > >      /* local copies, so that we can read/write PDU data directly from
> > >       * the ring */
> > > @@ -198,16 +199,18 @@ static void xen_9pfs_init_in_iov_from_pdu(V9fsPDU
> > > *pdu, g_free(ring->sg);
> > > 
> > >      ring->sg = g_new0(struct iovec, 2);
> > > -    xen_9pfs_in_sg(ring, ring->sg, &num, pdu->idx, size);
> > > +    ring->co = qemu_coroutine_self();
> > > +    smp_wmb();
> > > 
> > > +again:
> > > +    xen_9pfs_in_sg(ring, ring->sg, &num, pdu->idx, size);
> > >      buf_size = iov_size(ring->sg, num);
> > >      if (buf_size  < size) {
> > > -        xen_pv_printf(&xen_9pfs->xendev, 0, "Xen 9pfs request type %d"
> > > -                "needs %zu bytes, buffer has %zu\n", pdu->id, size,
> > > -                buf_size);
> > > -        xen_be_set_state(&xen_9pfs->xendev, XenbusStateClosing);
> > > -        xen_9pfs_disconnect(&xen_9pfs->xendev);
> > > +        qemu_coroutine_yield();
> > > +        goto again;
> > >      }
> > > +    ring->co = NULL;
> > > +    smp_wmb();
> > > 
> > >      *piov = ring->sg;
> > >      *pniov = num;
> > > @@ -292,6 +295,19 @@ static int xen_9pfs_receive(Xen9pfsRing *ring)
> > >  static void xen_9pfs_bh(void *opaque)
> > >  {
> > >      Xen9pfsRing *ring = opaque;
> > > +    bool wait;
> > > +
> > > +again:
> > > +    wait = ring->co != NULL && qemu_coroutine_entered(ring->co);
> > > +    smp_rmb();
> > > +    if (wait) {
> > > +        cpu_relax();
> > > +        goto again;
> > > +    }
> > > +
> > > +    if (ring->co != NULL) {
> > > +        qemu_coroutine_enter_if_inactive(ring->co);
> > 
> > ... correct me if I am wrong, but AFAIK qemu_coroutine_enter_if_inactive() 
> > will simply run the coroutine directly on caller's thread, it will not 
> > dispatch the coroutine onto the thread which yielded the coroutine before.
> 
> Yes, that is correct. I thought it would be fine because the caller here
> is a bh function so it should have no problems entering the coroutine.
> 
> But I am not that much of an expert on coroutines... Do you think there
> could be issues?
> 
> 
> > > +    }
> > >      xen_9pfs_receive(ring);
> > >  }
> > 
> > AFAICS you have not addressed the problem msize >> xen ringbuffer size, in 
> > which case I would expect the Xen driver to loop forever. Am I missing 
> > something or have you postponed addressing this?
> 
> Yes, I postponed addressing that issue because of a couple of reasons.
> 
> For starter strictly speaking it should not be required: msize cannot be
> bigger than the ring, but it can be equal to the ring increasing the
> chances of having to wait in QEMU. It should still be correct but the
> performance might not be great.
> 
> The other reason is that I already have the patches for both QEMU and
> Linux, but I am seeing a strange error setting order = 10. Order = 9
> works fine. I would like to do a bit more investigation before sending
> those patches.

As it turns out, order 9 is the max that the protocol allows at the
moment, so I am going to add a patch increasing the max order supported
by QEMU to 9 in the next version of this series.

The msize limitation to 8 (order 9 - 1) is done at the Linux side
(https://marc.info/?l=linux-kernel&m=159000008631935&w=2).

Re: [PATCH 2/2] xen/9pfs: yield when there isn't enough room on the ring

[Christian Schoenebeck]

On Mittwoch, 20. Mai 2020 19:35:00 CEST Stefano Stabellini wrote:
> On Wed, 20 May 2020, Christian Schoenebeck wrote:
> > On Mittwoch, 20. Mai 2020 03:47:12 CEST Stefano Stabellini wrote:
> > > From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > > 
> > > Instead of truncating replies, which is problematic, wait until the
> > > client reads more data and frees bytes on the reply ring.
> > > 
> > > Do that by calling qemu_coroutine_yield(). The corresponding
> > > qemu_coroutine_enter_if_inactive() is called from xen_9pfs_bh upon
> > > receiving the next notification from the client.
> > > 
> > > We need to be careful to avoid races in case xen_9pfs_bh and the
> > > coroutine are both active at the same time. In xen_9pfs_bh, wait until
> > > either the critical section is over (ring->co == NULL) or until the
> > > coroutine becomes inactive (qemu_coroutine_yield() was called) before
> > > continuing. Then, simply wake up the coroutine if it is inactive.
> > > 
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > > ---
> > 
> > In general this patch makes sense to me, and much better and cleaner
> > solution than what we discussed before. Just one detail ...
> > 
> > >  hw/9pfs/xen-9p-backend.c | 28 ++++++++++++++++++++++------
> > >  1 file changed, 22 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
> > > index fc197f6c8a..3939539028 100644
> > > --- a/hw/9pfs/xen-9p-backend.c
> > > +++ b/hw/9pfs/xen-9p-backend.c
> > > @@ -37,6 +37,7 @@ typedef struct Xen9pfsRing {
> > > 
> > >      struct iovec *sg;
> > >      QEMUBH *bh;
> > > 
> > > +    Coroutine *co;
> > > 
> > >      /* local copies, so that we can read/write PDU data directly from
> > >      
> > >       * the ring */
> > > 
> > > @@ -198,16 +199,18 @@ static void xen_9pfs_init_in_iov_from_pdu(V9fsPDU
> > > *pdu, g_free(ring->sg);
> > > 
> > >      ring->sg = g_new0(struct iovec, 2);
> > > 
> > > -    xen_9pfs_in_sg(ring, ring->sg, &num, pdu->idx, size);
> > > +    ring->co = qemu_coroutine_self();
> > > +    smp_wmb();
> > > 
> > > +again:
> > > +    xen_9pfs_in_sg(ring, ring->sg, &num, pdu->idx, size);
> > > 
> > >      buf_size = iov_size(ring->sg, num);
> > >      if (buf_size  < size) {
> > > 
> > > -        xen_pv_printf(&xen_9pfs->xendev, 0, "Xen 9pfs request type %d"
> > > -                "needs %zu bytes, buffer has %zu\n", pdu->id, size,
> > > -                buf_size);
> > > -        xen_be_set_state(&xen_9pfs->xendev, XenbusStateClosing);
> > > -        xen_9pfs_disconnect(&xen_9pfs->xendev);
> > > +        qemu_coroutine_yield();
> > > +        goto again;
> > > 
> > >      }
> > > 
> > > +    ring->co = NULL;
> > > +    smp_wmb();
> > > 
> > >      *piov = ring->sg;
> > >      *pniov = num;
> > > 
> > > @@ -292,6 +295,19 @@ static int xen_9pfs_receive(Xen9pfsRing *ring)
> > > 
> > >  static void xen_9pfs_bh(void *opaque)
> > >  {
> > >  
> > >      Xen9pfsRing *ring = opaque;
> > > 
> > > +    bool wait;
> > > +
> > > +again:
> > > +    wait = ring->co != NULL && qemu_coroutine_entered(ring->co);
> > > +    smp_rmb();
> > > +    if (wait) {
> > > +        cpu_relax();
> > > +        goto again;
> > > +    }
> > > +
> > > +    if (ring->co != NULL) {
> > > +        qemu_coroutine_enter_if_inactive(ring->co);
> > 
> > ... correct me if I am wrong, but AFAIK qemu_coroutine_enter_if_inactive()
> > will simply run the coroutine directly on caller's thread, it will not
> > dispatch the coroutine onto the thread which yielded the coroutine before.
> 
> Yes, that is correct. I thought it would be fine because the caller here
> is a bh function so it should have no problems entering the coroutine.
> 
> But I am not that much of an expert on coroutines... Do you think there
> could be issues?

There is not something specific that would come to my mind right now, no. I 
just wanted to make sure you are aware on which thread that's going to be 
executed.

Remember we're all working on 9pfs only on a side channel, so nobody right now 
has a 100% code base coverage in his head. Most of the 9pfs code assumes to be 
run on main I/O thread only. On other projects I use to guard those functions 
like:

#define MAIN_THREAD_ONLY() \
	assert(isMainThread())

void foo() {
	MAIN_THREAD_ONLY();
	...
}

Might make sense to add something like that to 9pfs as well at some point.

> > > +    }
> > > 
> > >      xen_9pfs_receive(ring);
> > >  
> > >  }
> > 
> > AFAICS you have not addressed the problem msize >> xen ringbuffer size, in
> > which case I would expect the Xen driver to loop forever. Am I missing
> > something or have you postponed addressing this?
> 
> Yes, I postponed addressing that issue because of a couple of reasons.
> 
> For starter strictly speaking it should not be required: msize cannot be
> bigger than the ring, but it can be equal to the ring increasing the
> chances of having to wait in QEMU. It should still be correct but the
> performance might not be great.

Ah right, I actually mixed some things up on Xen side, my bad! You're right, 
the introduced loop should be sufficient to avoid misbehaviours now. So there 
would really just be a potential performance penalty, but that's more of a 
luxury issue that could be addressed in future.

Good then, maybe just add some comment on the memory barriers to shut up the 
code style warning as suggested by Greg, except of that ...

Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

Best regards,
Christian Schoenebeck

Re: [PATCH 0/2] revert 9pfs reply truncation, wait for free room to reply

[no-reply]

Patchew URL: https://patchew.org/QEMU/alpine.DEB.2.21.2005191651130.27502@sstabellini-ThinkPad-T480s/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: alpine.DEB.2.21.2005191651130.27502@sstabellini-ThinkPad-T480s
Subject: [PATCH 0/2] revert 9pfs reply truncation, wait for free room to reply
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
1fcf375 xen/9pfs: yield when there isn't enough room on the ring
8e197ec Revert "9p: init_in_iov_from_pdu can truncate the size"

=== OUTPUT BEGIN ===
1/2 Checking commit 8e197ec8340d (Revert "9p: init_in_iov_from_pdu can truncate the size")
2/2 Checking commit 1fcf3751db74 (xen/9pfs: yield when there isn't enough room on the ring)
ERROR: memory barrier without comment
#41: FILE: hw/9pfs/xen-9p-backend.c:203:
+    smp_wmb();

ERROR: memory barrier without comment
#56: FILE: hw/9pfs/xen-9p-backend.c:213:
+    smp_wmb();

ERROR: memory barrier without comment
#68: FILE: hw/9pfs/xen-9p-backend.c:302:
+    smp_rmb();

total: 3 errors, 0 warnings, 50 lines checked

Patch 2/2 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/alpine.DEB.2.21.2005191651130.27502@sstabellini-ThinkPad-T480s/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 0/2] revert 9pfs reply truncation, wait for free room to reply

[Greg Kurz]

On Wed, 20 May 2020 00:57:42 -0700 (PDT)
no-reply@patchew.org wrote:

> Patchew URL: https://patchew.org/QEMU/alpine.DEB.2.21.2005191651130.27502@sstabellini-ThinkPad-T480s/
> 
> 
> 
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Message-id: alpine.DEB.2.21.2005191651130.27502@sstabellini-ThinkPad-T480s
> Subject: [PATCH 0/2] revert 9pfs reply truncation, wait for free room to reply
> Type: series
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> git rev-parse base > /dev/null || exit 0
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> git config --local diff.algorithm histogram
> ./scripts/checkpatch.pl --mailback base..
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> Switched to a new branch 'test'
> 1fcf375 xen/9pfs: yield when there isn't enough room on the ring
> 8e197ec Revert "9p: init_in_iov_from_pdu can truncate the size"
> 
> === OUTPUT BEGIN ===
> 1/2 Checking commit 8e197ec8340d (Revert "9p: init_in_iov_from_pdu can truncate the size")
> 2/2 Checking commit 1fcf3751db74 (xen/9pfs: yield when there isn't enough room on the ring)
> ERROR: memory barrier without comment
> #41: FILE: hw/9pfs/xen-9p-backend.c:203:
> +    smp_wmb();
> 
> ERROR: memory barrier without comment
> #56: FILE: hw/9pfs/xen-9p-backend.c:213:
> +    smp_wmb();
> 
> ERROR: memory barrier without comment
> #68: FILE: hw/9pfs/xen-9p-backend.c:302:
> +    smp_rmb();
> 

Indeed some comments would definitely provide better understanding.

> total: 3 errors, 0 warnings, 50 lines checked
> 
> Patch 2/2 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> === OUTPUT END ===
> 
> Test command exited with code: 1
> 
> 
> The full log is available at
> http://patchew.org/logs/alpine.DEB.2.21.2005191651130.27502@sstabellini-ThinkPad-T480s/testing.checkpatch/?type=message.
> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-devel@redhat.com