Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1593682 > unrolled thread
| Started by | Stefano Stabellini <sstabellini@kernel.org> |
|---|---|
| First post | 2017-03-06 21:10 +0100 |
| Last post | 2017-03-13 23:40 +0100 |
| Articles | 19 on this page of 39 — 5 participants |
Back to article view | Back to linux.kernel
[PATCH 0/7] Xen transport for 9pfs frontend driver Stefano Stabellini <sstabellini@kernel.org> - 2017-03-06 21:10 +0100
[PATCH 1/7] xen: import new ring macros in ring.h Stefano Stabellini <sstabellini@kernel.org> - 2017-03-06 21:20 +0100
[PATCH 7/7] xen/9pfs: build 9pfs Xen transport driver Stefano Stabellini <sstabellini@kernel.org> - 2017-03-06 21:20 +0100
[PATCH 6/7] xen/9pfs: receive responses Stefano Stabellini <sstabellini@kernel.org> - 2017-03-06 21:20 +0100
Re: [PATCH 6/7] xen/9pfs: receive responses Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-03-07 17:00 +0100
Re: [PATCH 6/7] xen/9pfs: receive responses Stefano Stabellini <sstabellini@kernel.org> - 2017-03-08 02:30 +0100
Re: [Xen-devel] [PATCH 6/7] xen/9pfs: receive responses Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> - 2017-03-08 21:20 +0100
Re: [PATCH 6/7] xen/9pfs: receive responses Stefano Stabellini <sstabellini@kernel.org> - 2017-03-08 06:30 +0100
Re: [PATCH 6/7] xen/9pfs: receive responses Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-03-08 15:40 +0100
Re: [PATCH 6/7] xen/9pfs: receive responses Stefano Stabellini <sstabellini@kernel.org> - 2017-03-08 20:30 +0100
[PATCH 2/7] xen: introduce the header file for the Xen 9pfs transport protocol Stefano Stabellini <sstabellini@kernel.org> - 2017-03-06 21:20 +0100
Re: [PATCH 2/7] xen: introduce the header file for the Xen 9pfs transport protocol Stefano Stabellini <sstabellini@kernel.org> - 2017-03-06 22:40 +0100
Re: [PATCH 2/7] xen: introduce the header file for the Xen 9pfs transport protocol Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-03-06 22:50 +0100
Re: [PATCH 2/7] xen: introduce the header file for the Xen 9pfs transport protocol Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-03-06 23:40 +0100
[PATCH 4/7] xen/9pfs: connect to the backend Stefano Stabellini <sstabellini@kernel.org> - 2017-03-06 21:20 +0100
Re: [PATCH 4/7] xen/9pfs: connect to the backend Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-03-07 16:10 +0100
Re: [PATCH 4/7] xen/9pfs: connect to the backend Stefano Stabellini <sstabellini@kernel.org> - 2017-03-08 01:30 +0100
Re: [PATCH 4/7] xen/9pfs: connect to the backend Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-03-08 15:00 +0100
Re: [PATCH 4/7] xen/9pfs: connect to the backend Stefano Stabellini <sstabellini@kernel.org> - 2017-03-08 20:30 +0100
Re: [Xen-devel] [PATCH 4/7] xen/9pfs: connect to the backend Julien Grall <julien.grall@arm.com> - 2017-03-07 20:00 +0100
Re: [Xen-devel] [PATCH 4/7] xen/9pfs: connect to the backend Stefano Stabellini <sstabellini@kernel.org> - 2017-03-08 03:20 +0100
Re: [Xen-devel] [PATCH 4/7] xen/9pfs: connect to the backend Julien Grall <julien.grall@arm.com> - 2017-03-08 13:40 +0100
Re: [Xen-devel] [PATCH 4/7] xen/9pfs: connect to the backend Stefano Stabellini <sstabellini@kernel.org> - 2017-03-08 20:10 +0100
[PATCH 3/7] xen/9pfs: introduce Xen 9pfs transport driver Stefano Stabellini <sstabellini@kernel.org> - 2017-03-06 21:20 +0100
[PATCH 5/7] xen/9pfs: send requests to the backend Stefano Stabellini <sstabellini@kernel.org> - 2017-03-06 21:20 +0100
Re: [PATCH 5/7] xen/9pfs: send requests to the backend Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-03-07 17:40 +0100
Re: [PATCH 5/7] xen/9pfs: send requests to the backend Stefano Stabellini <sstabellini@kernel.org> - 2017-03-08 02:00 +0100
Re: [PATCH 5/7] xen/9pfs: send requests to the backend Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-03-08 15:10 +0100
Re: [PATCH 5/7] xen/9pfs: send requests to the backend Stefano Stabellini <sstabellini@kernel.org> - 2017-03-08 20:50 +0100
Re: [PATCH 5/7] xen/9pfs: send requests to the backend Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-03-08 21:10 +0100
Re: [PATCH 5/7] xen/9pfs: send requests to the backend Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-03-08 22:10 +0100
Re: [PATCH 5/7] xen/9pfs: send requests to the backend Stefano Stabellini <sstabellini@kernel.org> - 2017-03-08 22:30 +0100
Re: [Xen-devel] [PATCH 1/7] xen: import new ring macros in ring.h Julien Grall <julien.grall@arm.com> - 2017-03-07 20:20 +0100
Re: [Xen-devel] [PATCH 1/7] xen: import new ring macros in ring.h Stefano Stabellini <sstabellini@kernel.org> - 2017-03-08 01:30 +0100
Re: [Xen-devel] [PATCH 1/7] xen: import new ring macros in ring.h Julien Grall <julien.grall@arm.com> - 2017-03-08 12:50 +0100
Re: [Xen-devel] [PATCH 0/7] Xen transport for 9pfs frontend driver Roger Pau Monné <roger.pau@citrix.com> - 2017-03-07 17:50 +0100
Re: [Xen-devel] [PATCH 0/7] Xen transport for 9pfs frontend driver Stefano Stabellini <sstabellini@kernel.org> - 2017-03-07 19:30 +0100
Re: [Xen-devel] [PATCH 0/7] Xen transport for 9pfs frontend driver Roger Pau Monné <roger.pau@citrix.com> - 2017-03-09 04:10 +0100
Re: [Xen-devel] [PATCH 0/7] Xen transport for 9pfs frontend driver Stefano Stabellini <sstabellini@kernel.org> - 2017-03-13 23:40 +0100
Page 2 of 2 — ← Prev page 1 [2]
| From | Stefano Stabellini <sstabellini@kernel.org> |
|---|---|
| Date | 2017-03-08 03:20 +0100 |
| Subject | Re: [Xen-devel] [PATCH 4/7] xen/9pfs: connect to the backend |
| Message-ID | <tizmN-8ev-7@gated-at.bofh.it> |
| In reply to | #1594546 |
On Tue, 7 Mar 2017, Julien Grall wrote:
> Hi Stefano,
>
> On 03/06/2017 08:01 PM, Stefano Stabellini wrote:
> > +static int xen_9pfs_front_alloc_dataring(struct xenbus_device *dev,
> > + struct xen_9pfs_dataring *ring)
> > +{
> > + int i;
> > + int ret = -ENOMEM;
> > +
> > + init_waitqueue_head(&ring->wq);
> > + spin_lock_init(&ring->lock);
> > + INIT_WORK(&ring->work, p9_xen_response);
> > +
> > + ring->intf = (struct xen_9pfs_data_intf *) __get_free_page(GFP_KERNEL
> > | __GFP_ZERO);
> > + if (!ring->intf)
> > + goto error;
> > + memset(ring->intf, 0, XEN_PAGE_SIZE);
> > + ring->bytes = (void*)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
> > XEN_9PFS_RING_ORDER);
>
> The ring order will be in term of Xen page size and not Linux. So you are
> going to allocate much more memory than expected on 64KB kernel.
I'll fix.
> > + if (ring->bytes == NULL)
> > + goto error;
> > + for (i = 0; i < (1 << XEN_9PFS_RING_ORDER); i++)
> > + ring->intf->ref[i] =
> > gnttab_grant_foreign_access(dev->otherend_id,
> > pfn_to_gfn(virt_to_pfn((void*)ring->bytes) + i), 0);.
>
> Please use virt_to_gfn rather than pfn_to_gfn(virt_to_pfn).
OK
> Also, this is not going to work on 64K kernel because you will grant access to
> noncontiguous memory (e.g 0-4K, 64K-68K,...).
By using virt_to_gfn like you suggested, the loop will correctly iterate
on a 4K by 4K basis, even on a 64K kernel:
ring->bytes = (void*)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
XEN_9PFS_RING_ORDER - (PAGE_SHIFT - XEN_PAGE_SHIFT));
for (i = 0; i < (1 << XEN_9PFS_RING_ORDER); i++)
ring->intf->ref[i] = gnttab_grant_foreign_access(dev->otherend_id, virt_to_gfn((void*)ring->bytes) + i, 0);
where XEN_9PFS_RING_ORDER specifies the order at 4K granularity. Am I
missing something?
> We have various helper to break-down the page for you, see
> gnttab_for_one_grant, gnttab_foreach_grant, gnttab_count_grant,
> xen_for_each_gfn (though this one it is internal to xlate_mmu.c so far)
>
> Please use them to avoid any further.
>
> > + ring->ref = gnttab_grant_foreign_access(dev->otherend_id,
> > pfn_to_gfn(virt_to_pfn((void*)ring->intf)), 0);
>
> Please use virt_to_gfn rather than pfn_to_gfn(virt_to_pfn).
Sure
> > + ring->ring.in = ring->bytes;
> > + ring->ring.out = ring->bytes + XEN_9PFS_RING_SIZE;
> > +
> > + ret = xenbus_alloc_evtchn(dev, &ring->evtchn);
> > + if (ret)
> > + goto error;
> > + ring->irq = bind_evtchn_to_irqhandler(ring->evtchn,
> > xen_9pfs_front_event_handler,
> > + 0, "xen_9pfs-frontend", ring);
> > + if (ring->irq < 0) {
> > + xenbus_free_evtchn(dev, ring->evtchn);
> > + ret = ring->irq;
> > + goto error;
> > + }
> > return 0;
> > +
> > +error:
> > + if (ring->intf != NULL)
> > + kfree(ring->intf);
> > + if (ring->bytes != NULL)
> > + kfree(ring->bytes);
> > + return ret;
> > }
[toc] | [prev] | [next] | [standalone]
| From | Julien Grall <julien.grall@arm.com> |
|---|---|
| Date | 2017-03-08 13:40 +0100 |
| Subject | Re: [Xen-devel] [PATCH 4/7] xen/9pfs: connect to the backend |
| Message-ID | <tiJ2N-6yt-3@gated-at.bofh.it> |
| In reply to | #1594800 |
Hi Stefano, On 08/03/17 00:49, Stefano Stabellini wrote: > On Tue, 7 Mar 2017, Julien Grall wrote: >> On 03/06/2017 08:01 PM, Stefano Stabellini wrote: >>> + if (ring->bytes == NULL) >>> + goto error; >>> + for (i = 0; i < (1 << XEN_9PFS_RING_ORDER); i++) >>> + ring->intf->ref[i] = >>> gnttab_grant_foreign_access(dev->otherend_id, >>> pfn_to_gfn(virt_to_pfn((void*)ring->bytes) + i), 0);. >> >> Please use virt_to_gfn rather than pfn_to_gfn(virt_to_pfn). > > OK > > >> Also, this is not going to work on 64K kernel because you will grant access to >> noncontiguous memory (e.g 0-4K, 64K-68K,...). > > By using virt_to_gfn like you suggested, the loop will correctly iterate > on a 4K by 4K basis, even on a 64K kernel: > > ring->bytes = (void*)__get_free_pages(GFP_KERNEL | __GFP_ZERO, > XEN_9PFS_RING_ORDER - (PAGE_SHIFT - XEN_PAGE_SHIFT)); > for (i = 0; i < (1 << XEN_9PFS_RING_ORDER); i++) > ring->intf->ref[i] = gnttab_grant_foreign_access(dev->otherend_id, virt_to_gfn((void*)ring->bytes) + i, 0); BTW, the cast (void *) is not necessary. > > where XEN_9PFS_RING_ORDER specifies the order at 4K granularity. Am I > missing something? I think it is fine. You could move virt_to_gfn(...) outside and avoid to do the translation everytime. Cheers, -- Julien Grall
[toc] | [prev] | [next] | [standalone]
| From | Stefano Stabellini <sstabellini@kernel.org> |
|---|---|
| Date | 2017-03-08 20:10 +0100 |
| Subject | Re: [Xen-devel] [PATCH 4/7] xen/9pfs: connect to the backend |
| Message-ID | <tiP8d-2nO-9@gated-at.bofh.it> |
| In reply to | #1595117 |
On Wed, 8 Mar 2017, Julien Grall wrote: > Hi Stefano, > > On 08/03/17 00:49, Stefano Stabellini wrote: > > On Tue, 7 Mar 2017, Julien Grall wrote: > > > On 03/06/2017 08:01 PM, Stefano Stabellini wrote: > > > > + if (ring->bytes == NULL) > > > > + goto error; > > > > + for (i = 0; i < (1 << XEN_9PFS_RING_ORDER); i++) > > > > + ring->intf->ref[i] = > > > > gnttab_grant_foreign_access(dev->otherend_id, > > > > pfn_to_gfn(virt_to_pfn((void*)ring->bytes) + i), 0);. > > > > > > Please use virt_to_gfn rather than pfn_to_gfn(virt_to_pfn). > > > > OK > > > > > > > Also, this is not going to work on 64K kernel because you will grant > > > access to > > > noncontiguous memory (e.g 0-4K, 64K-68K,...). > > > > By using virt_to_gfn like you suggested, the loop will correctly iterate > > on a 4K by 4K basis, even on a 64K kernel: > > > > ring->bytes = (void*)__get_free_pages(GFP_KERNEL | __GFP_ZERO, > > XEN_9PFS_RING_ORDER - (PAGE_SHIFT - XEN_PAGE_SHIFT)); > > for (i = 0; i < (1 << XEN_9PFS_RING_ORDER); i++) > > ring->intf->ref[i] = gnttab_grant_foreign_access(dev->otherend_id, > > virt_to_gfn((void*)ring->bytes) + i, 0); > > BTW, the cast (void *) is not necessary. > > > > > where XEN_9PFS_RING_ORDER specifies the order at 4K granularity. Am I > > missing something? > > I think it is fine. You could move virt_to_gfn(...) outside and avoid to do > the translation everytime. All right, thanks.
[toc] | [prev] | [next] | [standalone]
| From | Stefano Stabellini <sstabellini@kernel.org> |
|---|---|
| Date | 2017-03-06 21:20 +0100 |
| Subject | [PATCH 3/7] xen/9pfs: introduce Xen 9pfs transport driver |
| Message-ID | <ti7gS-4VX-21@gated-at.bofh.it> |
| In reply to | #1593688 |
Introduce the Xen 9pfs transport driver: add struct xenbus_driver to
register as a xenbus driver and add struct p9_trans_module to register
as v9fs driver.
All functions are empty stubs for now.
Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: boris.ostrovsky@oracle.com
CC: jgross@suse.com
CC: Eric Van Hensbergen <ericvh@gmail.com>
CC: Ron Minnich <rminnich@sandia.gov>
CC: Latchesar Ionkov <lucho@ionkov.net>
CC: v9fs-developer@lists.sourceforge.net
---
net/9p/trans_xen.c | 101 +++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 101 insertions(+)
create mode 100644 net/9p/trans_xen.c
diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
new file mode 100644
index 0000000..877dfd0
--- /dev/null
+++ b/net/9p/trans_xen.c
@@ -0,0 +1,101 @@
+/*
+ * linux/fs/9p/trans_xen
+ *
+ * Xen transport layer.
+ *
+ * Copyright (C) 2017 by Stefano Stabellini <stefano@aporeto.com>
+ */
+
+#include <xen/events.h>
+#include <xen/grant_table.h>
+#include <xen/xen.h>
+#include <xen/xenbus.h>
+#include <xen/interface/io/9pfs.h>
+
+#include <linux/module.h>
+#include <net/9p/9p.h>
+#include <net/9p/client.h>
+#include <net/9p/transport.h>
+
+static int p9_xen_cancel(struct p9_client *client, struct p9_req_t *req)
+{
+ return 0;
+}
+
+static int p9_xen_create(struct p9_client *client, const char *addr, char *args)
+{
+ return 0;
+}
+
+static void p9_xen_close(struct p9_client *client)
+{
+}
+
+static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
+{
+ return 0;
+}
+
+static struct p9_trans_module p9_xen_trans = {
+ .name = "xen",
+ .maxsize = (1 << (XEN_9PFS_RING_ORDER + XEN_PAGE_SHIFT)),
+ .def = 1,
+ .create = p9_xen_create,
+ .close = p9_xen_close,
+ .request = p9_xen_request,
+ .cancel = p9_xen_cancel,
+ .owner = THIS_MODULE,
+};
+
+static const struct xenbus_device_id xen_9pfs_front_ids[] = {
+ { "9pfs" },
+ { "" }
+};
+
+static int xen_9pfs_front_remove(struct xenbus_device *dev)
+{
+ return 0;
+}
+
+static int xen_9pfs_front_probe(struct xenbus_device *dev,
+ const struct xenbus_device_id *id)
+{
+ return 0;
+}
+
+static int xen_9pfs_front_resume(struct xenbus_device *dev)
+{
+ return 0;
+}
+
+static void xen_9pfs_front_changed(struct xenbus_device *dev,
+ enum xenbus_state backend_state)
+{
+}
+
+static struct xenbus_driver xen_9pfs_front_driver = {
+ .ids = xen_9pfs_front_ids,
+ .probe = xen_9pfs_front_probe,
+ .remove = xen_9pfs_front_remove,
+ .resume = xen_9pfs_front_resume,
+ .otherend_changed = xen_9pfs_front_changed,
+};
+
+int p9_trans_xen_init(void)
+{
+ if (!xen_domain())
+ return -ENODEV;
+
+ pr_info("Initialising Xen transport for 9pfs\n");
+
+ v9fs_register_trans(&p9_xen_trans);
+ return xenbus_register_frontend(&xen_9pfs_front_driver);
+}
+module_init(p9_trans_xen_init);
+
+void p9_trans_xen_exit(void)
+{
+ v9fs_unregister_trans(&p9_xen_trans);
+ return xenbus_unregister_driver(&xen_9pfs_front_driver);
+}
+module_exit(p9_trans_xen_exit);
--
1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Stefano Stabellini <sstabellini@kernel.org> |
|---|---|
| Date | 2017-03-06 21:20 +0100 |
| Subject | [PATCH 5/7] xen/9pfs: send requests to the backend |
| Message-ID | <ti7gR-4VX-13@gated-at.bofh.it> |
| In reply to | #1593688 |
Implement struct p9_trans_module create and close functions by looking
at the available Xen 9pfs frontend-backend connections. We don't expect
many frontend-backend connections, thus walking a list is OK.
Send requests to the backend by copying each request to one of the
available rings (each frontend-backend connection comes with multiple
rings). Handle the ring and notifications following the 9pfs
specification. If there are not enough free bytes on the ring for the
request, wait on the wait_queue: the backend will send a notification
after consuming more requests.
Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: boris.ostrovsky@oracle.com
CC: jgross@suse.com
CC: Eric Van Hensbergen <ericvh@gmail.com>
CC: Ron Minnich <rminnich@sandia.gov>
CC: Latchesar Ionkov <lucho@ionkov.net>
CC: v9fs-developer@lists.sourceforge.net
---
net/9p/trans_xen.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 82 insertions(+), 1 deletion(-)
diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
index 9f6cf8d..4e26556 100644
--- a/net/9p/trans_xen.c
+++ b/net/9p/trans_xen.c
@@ -47,22 +47,103 @@ struct xen_9pfs_front_priv {
};
static LIST_HEAD(xen_9pfs_devs);
+/* We don't currently allow canceling of requests */
static int p9_xen_cancel(struct p9_client *client, struct p9_req_t *req)
{
- return 0;
+ return 1;
}
static int p9_xen_create(struct p9_client *client, const char *addr, char *args)
{
+ struct xen_9pfs_front_priv *priv = NULL;
+
+ list_for_each_entry(priv, &xen_9pfs_devs, list) {
+ if (!strcmp(priv->tag, addr))
+ break;
+ }
+ if (!priv || strcmp(priv->tag, addr))
+ return -EINVAL;
+
+ priv->client = client;
return 0;
}
static void p9_xen_close(struct p9_client *client)
{
+ struct xen_9pfs_front_priv *priv = NULL;
+
+ list_for_each_entry(priv, &xen_9pfs_devs, list) {
+ if (priv->client == client)
+ break;
+ }
+ if (!priv || priv->client != client)
+ return;
+
+ priv->client = NULL;
+ return;
+}
+
+static int p9_xen_write_todo(struct xen_9pfs_dataring *ring, RING_IDX size)
+{
+ RING_IDX cons, prod;
+
+ cons = ring->intf->out_cons;
+ prod = ring->intf->out_prod;
+ mb();
+
+ if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) >= size)
+ return 1;
+ else
+ return 0;
}
static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
{
+ struct xen_9pfs_front_priv *priv = NULL;
+ RING_IDX cons, prod, masked_cons, masked_prod;
+ unsigned long flags;
+ uint32_t size = p9_req->tc->size;
+ struct xen_9pfs_dataring *ring;
+ int num;
+
+ list_for_each_entry(priv, &xen_9pfs_devs, list) {
+ if (priv->client == client)
+ break;
+ }
+ if (priv == NULL || priv->client != client)
+ return -EINVAL;
+
+ num = p9_req->tc->tag % priv->num_rings;
+ ring = &priv->rings[num];
+
+again:
+ while (wait_event_interruptible(ring->wq,
+ p9_xen_write_todo(ring, size) > 0) != 0);
+
+ spin_lock_irqsave(&ring->lock, flags);
+ cons = ring->intf->out_cons;
+ prod = ring->intf->out_prod;
+ mb();
+
+ if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) < size) {
+ spin_unlock_irqrestore(&ring->lock, flags);
+ goto again;
+ }
+
+ masked_prod = xen_9pfs_mask(prod, XEN_9PFS_RING_SIZE);
+ masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE);
+
+ xen_9pfs_write_packet(ring->ring.out,
+ &masked_prod, masked_cons,
+ XEN_9PFS_RING_SIZE, p9_req->tc->sdata, size);
+
+ p9_req->status = REQ_STATUS_SENT;
+ wmb(); /* write ring before updating pointer */
+ prod += size;
+ ring->intf->out_prod = prod;
+ spin_unlock_irqrestore(&ring->lock, flags);
+ notify_remote_via_irq(ring->irq);
+
return 0;
}
--
1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Boris Ostrovsky <boris.ostrovsky@oracle.com> |
|---|---|
| Date | 2017-03-07 17:40 +0100 |
| Subject | Re: [PATCH 5/7] xen/9pfs: send requests to the backend |
| Message-ID | <tiqjx-1Hb-33@gated-at.bofh.it> |
| In reply to | #1593696 |
On 03/06/2017 03:01 PM, Stefano Stabellini wrote:
> Implement struct p9_trans_module create and close functions by looking
> at the available Xen 9pfs frontend-backend connections. We don't expect
> many frontend-backend connections, thus walking a list is OK.
>
> Send requests to the backend by copying each request to one of the
> available rings (each frontend-backend connection comes with multiple
> rings). Handle the ring and notifications following the 9pfs
> specification. If there are not enough free bytes on the ring for the
> request, wait on the wait_queue: the backend will send a notification
> after consuming more requests.
>
> Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> CC: boris.ostrovsky@oracle.com
> CC: jgross@suse.com
> CC: Eric Van Hensbergen <ericvh@gmail.com>
> CC: Ron Minnich <rminnich@sandia.gov>
> CC: Latchesar Ionkov <lucho@ionkov.net>
> CC: v9fs-developer@lists.sourceforge.net
> ---
> net/9p/trans_xen.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 82 insertions(+), 1 deletion(-)
>
> diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
> index 9f6cf8d..4e26556 100644
> --- a/net/9p/trans_xen.c
> +++ b/net/9p/trans_xen.c
> @@ -47,22 +47,103 @@ struct xen_9pfs_front_priv {
> };
> static LIST_HEAD(xen_9pfs_devs);
>
> +/* We don't currently allow canceling of requests */
> static int p9_xen_cancel(struct p9_client *client, struct p9_req_t *req)
> {
> - return 0;
> + return 1;
> }
>
> static int p9_xen_create(struct p9_client *client, const char *addr, char *args)
> {
> + struct xen_9pfs_front_priv *priv = NULL;
> +
> + list_for_each_entry(priv, &xen_9pfs_devs, list) {
> + if (!strcmp(priv->tag, addr))
> + break;
> + }
You could simplify this (and p9_xen_close()) but assigning client and
returning from inside the 'if' statement.
I am also not sure you need to initialize priv.
> + if (!priv || strcmp(priv->tag, addr))
> + return -EINVAL;
> +
> + priv->client = client;
> return 0;
> }
>
> static void p9_xen_close(struct p9_client *client)
> {
> + struct xen_9pfs_front_priv *priv = NULL;
> +
> + list_for_each_entry(priv, &xen_9pfs_devs, list) {
> + if (priv->client == client)
> + break;
> + }
> + if (!priv || priv->client != client)
> + return;
> +
> + priv->client = NULL;
> + return;
> +}
> +
> +static int p9_xen_write_todo(struct xen_9pfs_dataring *ring, RING_IDX size)
> +{
> + RING_IDX cons, prod;
> +
> + cons = ring->intf->out_cons;
> + prod = ring->intf->out_prod;
> + mb();
> +
> + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) >= size)
> + return 1;
> + else
> + return 0;
> }
>
> static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
> {
> + struct xen_9pfs_front_priv *priv = NULL;
> + RING_IDX cons, prod, masked_cons, masked_prod;
> + unsigned long flags;
> + uint32_t size = p9_req->tc->size;
> + struct xen_9pfs_dataring *ring;
> + int num;
> +
> + list_for_each_entry(priv, &xen_9pfs_devs, list) {
> + if (priv->client == client)
> + break;
> + }
> + if (priv == NULL || priv->client != client)
> + return -EINVAL;
> +
> + num = p9_req->tc->tag % priv->num_rings;
> + ring = &priv->rings[num];
> +
> +again:
> + while (wait_event_interruptible(ring->wq,
> + p9_xen_write_todo(ring, size) > 0) != 0);
> +
> + spin_lock_irqsave(&ring->lock, flags);
> + cons = ring->intf->out_cons;
> + prod = ring->intf->out_prod;
> + mb();
> +
> + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) < size) {
This looks like p9_xen_write_todo(). BTW, where is xen_9pfs_queued()
defined? I couldn't find it. Same for xen_9pfs_mask() and
xen_9pfs_write_packet().
-boris
> + spin_unlock_irqrestore(&ring->lock, flags);
> + goto again;
> + }
> +
> + masked_prod = xen_9pfs_mask(prod, XEN_9PFS_RING_SIZE);
> + masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE);
> +
> + xen_9pfs_write_packet(ring->ring.out,
> + &masked_prod, masked_cons,
> + XEN_9PFS_RING_SIZE, p9_req->tc->sdata, size);
> +
> + p9_req->status = REQ_STATUS_SENT;
> + wmb(); /* write ring before updating pointer */
> + prod += size;
> + ring->intf->out_prod = prod;
> + spin_unlock_irqrestore(&ring->lock, flags);
> + notify_remote_via_irq(ring->irq);
> +
> return 0;
> }
>
>
[toc] | [prev] | [next] | [standalone]
| From | Stefano Stabellini <sstabellini@kernel.org> |
|---|---|
| Date | 2017-03-08 02:00 +0100 |
| Subject | Re: [PATCH 5/7] xen/9pfs: send requests to the backend |
| Message-ID | <tiy7n-7dF-9@gated-at.bofh.it> |
| In reply to | #1594411 |
On Tue, 7 Mar 2017, Boris Ostrovsky wrote:
> On 03/06/2017 03:01 PM, Stefano Stabellini wrote:
> > Implement struct p9_trans_module create and close functions by looking
> > at the available Xen 9pfs frontend-backend connections. We don't expect
> > many frontend-backend connections, thus walking a list is OK.
> >
> > Send requests to the backend by copying each request to one of the
> > available rings (each frontend-backend connection comes with multiple
> > rings). Handle the ring and notifications following the 9pfs
> > specification. If there are not enough free bytes on the ring for the
> > request, wait on the wait_queue: the backend will send a notification
> > after consuming more requests.
> >
> > Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> > CC: boris.ostrovsky@oracle.com
> > CC: jgross@suse.com
> > CC: Eric Van Hensbergen <ericvh@gmail.com>
> > CC: Ron Minnich <rminnich@sandia.gov>
> > CC: Latchesar Ionkov <lucho@ionkov.net>
> > CC: v9fs-developer@lists.sourceforge.net
> > ---
> > net/9p/trans_xen.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 82 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
> > index 9f6cf8d..4e26556 100644
> > --- a/net/9p/trans_xen.c
> > +++ b/net/9p/trans_xen.c
> > @@ -47,22 +47,103 @@ struct xen_9pfs_front_priv {
> > };
> > static LIST_HEAD(xen_9pfs_devs);
> >
> > +/* We don't currently allow canceling of requests */
> > static int p9_xen_cancel(struct p9_client *client, struct p9_req_t *req)
> > {
> > - return 0;
> > + return 1;
> > }
> >
> > static int p9_xen_create(struct p9_client *client, const char *addr, char *args)
> > {
> > + struct xen_9pfs_front_priv *priv = NULL;
> > +
> > + list_for_each_entry(priv, &xen_9pfs_devs, list) {
> > + if (!strcmp(priv->tag, addr))
> > + break;
> > + }
>
>
> You could simplify this (and p9_xen_close()) but assigning client and
> returning from inside the 'if' statement.
I'll do that.
> I am also not sure you need to initialize priv.
With the new changes, I won't need to.
> > + if (!priv || strcmp(priv->tag, addr))
> > + return -EINVAL;
> > +
> > + priv->client = client;
> > return 0;
> > }
> >
> > static void p9_xen_close(struct p9_client *client)
> > {
> > + struct xen_9pfs_front_priv *priv = NULL;
> > +
> > + list_for_each_entry(priv, &xen_9pfs_devs, list) {
> > + if (priv->client == client)
> > + break;
> > + }
> > + if (!priv || priv->client != client)
> > + return;
> > +
> > + priv->client = NULL;
> > + return;
> > +}
> > +
> > +static int p9_xen_write_todo(struct xen_9pfs_dataring *ring, RING_IDX size)
> > +{
> > + RING_IDX cons, prod;
> > +
> > + cons = ring->intf->out_cons;
> > + prod = ring->intf->out_prod;
> > + mb();
> > +
> > + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) >= size)
> > + return 1;
> > + else
> > + return 0;
> > }
> >
> > static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
> > {
> > + struct xen_9pfs_front_priv *priv = NULL;
> > + RING_IDX cons, prod, masked_cons, masked_prod;
> > + unsigned long flags;
> > + uint32_t size = p9_req->tc->size;
> > + struct xen_9pfs_dataring *ring;
> > + int num;
> > +
> > + list_for_each_entry(priv, &xen_9pfs_devs, list) {
> > + if (priv->client == client)
> > + break;
> > + }
> > + if (priv == NULL || priv->client != client)
> > + return -EINVAL;
> > +
> > + num = p9_req->tc->tag % priv->num_rings;
> > + ring = &priv->rings[num];
> > +
> > +again:
> > + while (wait_event_interruptible(ring->wq,
> > + p9_xen_write_todo(ring, size) > 0) != 0);
> > +
> > + spin_lock_irqsave(&ring->lock, flags);
> > + cons = ring->intf->out_cons;
> > + prod = ring->intf->out_prod;
> > + mb();
> > +
> > + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) < size) {
>
>
> This looks like p9_xen_write_todo().
p9_xen_write_todo is just a wrapper around xen_9pfs_queued to provide
a return value that works well with wait_event_interruptible.
I would prefer not to call p9_xen_write_todo here, because it's simpler
if we don't read prod and cons twice.
> BTW, where is xen_9pfs_queued()
> defined? I couldn't find it. Same for xen_9pfs_mask() and
> xen_9pfs_write_packet().
They are provided by the new ring macros, see
include/xen/interface/io/ring.h (the first patch).
> > + spin_unlock_irqrestore(&ring->lock, flags);
> > + goto again;
> > + }
> > +
> > + masked_prod = xen_9pfs_mask(prod, XEN_9PFS_RING_SIZE);
> > + masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE);
> > +
> > + xen_9pfs_write_packet(ring->ring.out,
> > + &masked_prod, masked_cons,
> > + XEN_9PFS_RING_SIZE, p9_req->tc->sdata, size);
> > +
> > + p9_req->status = REQ_STATUS_SENT;
> > + wmb(); /* write ring before updating pointer */
> > + prod += size;
> > + ring->intf->out_prod = prod;
> > + spin_unlock_irqrestore(&ring->lock, flags);
> > + notify_remote_via_irq(ring->irq);
> > +
> > return 0;
> > }
> >
> >
>
[toc] | [prev] | [next] | [standalone]
| From | Boris Ostrovsky <boris.ostrovsky@oracle.com> |
|---|---|
| Date | 2017-03-08 15:10 +0100 |
| Subject | Re: [PATCH 5/7] xen/9pfs: send requests to the backend |
| Message-ID | <tiKrU-7Ca-27@gated-at.bofh.it> |
| In reply to | #1594777 |
>>> +}
>>> +
>>> +static int p9_xen_write_todo(struct xen_9pfs_dataring *ring, RING_IDX size)
>>> +{
>>> + RING_IDX cons, prod;
>>> +
>>> + cons = ring->intf->out_cons;
>>> + prod = ring->intf->out_prod;
>>> + mb();
>>> +
>>> + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) >= size)
>>> + return 1;
>>> + else
>>> + return 0;
>>> }
>>>
>>> static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
>>> {
>>> + struct xen_9pfs_front_priv *priv = NULL;
>>> + RING_IDX cons, prod, masked_cons, masked_prod;
>>> + unsigned long flags;
>>> + uint32_t size = p9_req->tc->size;
>>> + struct xen_9pfs_dataring *ring;
>>> + int num;
>>> +
>>> + list_for_each_entry(priv, &xen_9pfs_devs, list) {
>>> + if (priv->client == client)
>>> + break;
>>> + }
>>> + if (priv == NULL || priv->client != client)
>>> + return -EINVAL;
>>> +
>>> + num = p9_req->tc->tag % priv->num_rings;
>>> + ring = &priv->rings[num];
>>> +
>>> +again:
>>> + while (wait_event_interruptible(ring->wq,
>>> + p9_xen_write_todo(ring, size) > 0) != 0);
>>> +
>>> + spin_lock_irqsave(&ring->lock, flags);
>>> + cons = ring->intf->out_cons;
>>> + prod = ring->intf->out_prod;
>>> + mb();
>>> +
>>> + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) < size) {
>>
>> This looks like p9_xen_write_todo().
> p9_xen_write_todo is just a wrapper around xen_9pfs_queued to provide
> a return value that works well with wait_event_interruptible.
>
> I would prefer not to call p9_xen_write_todo here, because it's simpler
> if we don't read prod and cons twice.
I was referring to the whole code fragment after spin_lock_irqsave(),
not just the last line. Isn't it exactly !p9_xen_write_todo()?
>
>
>> BTW, where is xen_9pfs_queued()
>> defined? I couldn't find it. Same for xen_9pfs_mask() and
>> xen_9pfs_write_packet().
> They are provided by the new ring macros, see
> include/xen/interface/io/ring.h (the first patch).
Oh, right. I was searching for the string literally.
-boris
[toc] | [prev] | [next] | [standalone]
| From | Stefano Stabellini <sstabellini@kernel.org> |
|---|---|
| Date | 2017-03-08 20:50 +0100 |
| Subject | Re: [PATCH 5/7] xen/9pfs: send requests to the backend |
| Message-ID | <tiPKV-2DT-7@gated-at.bofh.it> |
| In reply to | #1595204 |
On Wed, 8 Mar 2017, Boris Ostrovsky wrote:
> >>> +}
> >>> +
> >>> +static int p9_xen_write_todo(struct xen_9pfs_dataring *ring, RING_IDX size)
> >>> +{
> >>> + RING_IDX cons, prod;
> >>> +
> >>> + cons = ring->intf->out_cons;
> >>> + prod = ring->intf->out_prod;
> >>> + mb();
> >>> +
> >>> + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) >= size)
> >>> + return 1;
> >>> + else
> >>> + return 0;
> >>> }
> >>>
> >>> static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
> >>> {
> >>> + struct xen_9pfs_front_priv *priv = NULL;
> >>> + RING_IDX cons, prod, masked_cons, masked_prod;
> >>> + unsigned long flags;
> >>> + uint32_t size = p9_req->tc->size;
> >>> + struct xen_9pfs_dataring *ring;
> >>> + int num;
> >>> +
> >>> + list_for_each_entry(priv, &xen_9pfs_devs, list) {
> >>> + if (priv->client == client)
> >>> + break;
> >>> + }
> >>> + if (priv == NULL || priv->client != client)
> >>> + return -EINVAL;
> >>> +
> >>> + num = p9_req->tc->tag % priv->num_rings;
> >>> + ring = &priv->rings[num];
> >>> +
> >>> +again:
> >>> + while (wait_event_interruptible(ring->wq,
> >>> + p9_xen_write_todo(ring, size) > 0) != 0);
> >>> +
> >>> + spin_lock_irqsave(&ring->lock, flags);
> >>> + cons = ring->intf->out_cons;
> >>> + prod = ring->intf->out_prod;
> >>> + mb();
> >>> +
> >>> + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) < size) {
> >>
> >> This looks like p9_xen_write_todo().
> > p9_xen_write_todo is just a wrapper around xen_9pfs_queued to provide
> > a return value that works well with wait_event_interruptible.
> >
> > I would prefer not to call p9_xen_write_todo here, because it's simpler
> > if we don't read prod and cons twice.
>
> I was referring to the whole code fragment after spin_lock_irqsave(),
> not just the last line. Isn't it exactly !p9_xen_write_todo()?
Yes, it is true they are almost the same. The difference, and the reason
for p9_xen_write_todo to exist, is that p9_xen_write_todo is called in
the wait_event_interruptible loop, as such it needs to read prod and
cons every time. On the other end, here we want to read them once. Does
it make sense?
[toc] | [prev] | [next] | [standalone]
| From | Boris Ostrovsky <boris.ostrovsky@oracle.com> |
|---|---|
| Date | 2017-03-08 21:10 +0100 |
| Subject | Re: [PATCH 5/7] xen/9pfs: send requests to the backend |
| Message-ID | <tiQ4h-2ZW-3@gated-at.bofh.it> |
| In reply to | #1595466 |
On 03/08/2017 02:33 PM, Stefano Stabellini wrote:
> On Wed, 8 Mar 2017, Boris Ostrovsky wrote:
>>>>> +}
>>>>> +
>>>>> +static int p9_xen_write_todo(struct xen_9pfs_dataring *ring, RING_IDX size)
>>>>> +{
>>>>> + RING_IDX cons, prod;
>>>>> +
>>>>> + cons = ring->intf->out_cons;
>>>>> + prod = ring->intf->out_prod;
>>>>> + mb();
>>>>> +
>>>>> + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) >= size)
>>>>> + return 1;
>>>>> + else
>>>>> + return 0;
>>>>> }
>>>>>
>>>>> static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
>>>>> {
>>>>> + struct xen_9pfs_front_priv *priv = NULL;
>>>>> + RING_IDX cons, prod, masked_cons, masked_prod;
>>>>> + unsigned long flags;
>>>>> + uint32_t size = p9_req->tc->size;
>>>>> + struct xen_9pfs_dataring *ring;
>>>>> + int num;
>>>>> +
>>>>> + list_for_each_entry(priv, &xen_9pfs_devs, list) {
>>>>> + if (priv->client == client)
>>>>> + break;
>>>>> + }
>>>>> + if (priv == NULL || priv->client != client)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + num = p9_req->tc->tag % priv->num_rings;
>>>>> + ring = &priv->rings[num];
>>>>> +
>>>>> +again:
>>>>> + while (wait_event_interruptible(ring->wq,
>>>>> + p9_xen_write_todo(ring, size) > 0) != 0);
>>>>> +
>>>>> + spin_lock_irqsave(&ring->lock, flags);
>>>>> + cons = ring->intf->out_cons;
>>>>> + prod = ring->intf->out_prod;
>>>>> + mb();
>>>>> +
>>>>> + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) < size) {
>>>> This looks like p9_xen_write_todo().
>>> p9_xen_write_todo is just a wrapper around xen_9pfs_queued to provide
>>> a return value that works well with wait_event_interruptible.
>>>
>>> I would prefer not to call p9_xen_write_todo here, because it's simpler
>>> if we don't read prod and cons twice.
>> I was referring to the whole code fragment after spin_lock_irqsave(),
>> not just the last line. Isn't it exactly !p9_xen_write_todo()?
> Yes, it is true they are almost the same. The difference, and the reason
> for p9_xen_write_todo to exist, is that p9_xen_write_todo is called in
> the wait_event_interruptible loop, as such it needs to read prod and
> cons every time. On the other end, here we want to read them once. Does
> it make sense?
I am clearly being particularly dense here but what I was thinking was:
again:
while (wait_event_interruptible(ring->wq,
p9_xen_write_todo(ring, size) > 0) != 0);
spin_lock_irqsave(&ring->lock, flags);
if (!p9_xen_write_todo(ring, size)) {
spin_unlock_irqrestore(&ring->lock, flags);
goto again;
}
There is no extra read of prod/cons.
-boris
[toc] | [prev] | [next] | [standalone]
| From | Boris Ostrovsky <boris.ostrovsky@oracle.com> |
|---|---|
| Date | 2017-03-08 22:10 +0100 |
| Subject | Re: [PATCH 5/7] xen/9pfs: send requests to the backend |
| Message-ID | <tiR0m-3EK-13@gated-at.bofh.it> |
| In reply to | #1595471 |
>> There is no extra read of prod/cons. > Yes, there are: just after this if statement we would have to read them > again to calculate masked_prod and masked_cons. Ah, of course. Thanks. -boris
[toc] | [prev] | [next] | [standalone]
| From | Stefano Stabellini <sstabellini@kernel.org> |
|---|---|
| Date | 2017-03-08 22:30 +0100 |
| Subject | Re: [PATCH 5/7] xen/9pfs: send requests to the backend |
| Message-ID | <tiR0m-3EK-15@gated-at.bofh.it> |
| In reply to | #1595471 |
On Wed, 8 Mar 2017, Boris Ostrovsky wrote:
> On 03/08/2017 02:33 PM, Stefano Stabellini wrote:
> > On Wed, 8 Mar 2017, Boris Ostrovsky wrote:
> >>>>> +}
> >>>>> +
> >>>>> +static int p9_xen_write_todo(struct xen_9pfs_dataring *ring, RING_IDX size)
> >>>>> +{
> >>>>> + RING_IDX cons, prod;
> >>>>> +
> >>>>> + cons = ring->intf->out_cons;
> >>>>> + prod = ring->intf->out_prod;
> >>>>> + mb();
> >>>>> +
> >>>>> + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) >= size)
> >>>>> + return 1;
> >>>>> + else
> >>>>> + return 0;
> >>>>> }
> >>>>>
> >>>>> static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
> >>>>> {
> >>>>> + struct xen_9pfs_front_priv *priv = NULL;
> >>>>> + RING_IDX cons, prod, masked_cons, masked_prod;
> >>>>> + unsigned long flags;
> >>>>> + uint32_t size = p9_req->tc->size;
> >>>>> + struct xen_9pfs_dataring *ring;
> >>>>> + int num;
> >>>>> +
> >>>>> + list_for_each_entry(priv, &xen_9pfs_devs, list) {
> >>>>> + if (priv->client == client)
> >>>>> + break;
> >>>>> + }
> >>>>> + if (priv == NULL || priv->client != client)
> >>>>> + return -EINVAL;
> >>>>> +
> >>>>> + num = p9_req->tc->tag % priv->num_rings;
> >>>>> + ring = &priv->rings[num];
> >>>>> +
> >>>>> +again:
> >>>>> + while (wait_event_interruptible(ring->wq,
> >>>>> + p9_xen_write_todo(ring, size) > 0) != 0);
> >>>>> +
> >>>>> + spin_lock_irqsave(&ring->lock, flags);
> >>>>> + cons = ring->intf->out_cons;
> >>>>> + prod = ring->intf->out_prod;
> >>>>> + mb();
> >>>>> +
> >>>>> + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) < size) {
> >>>> This looks like p9_xen_write_todo().
> >>> p9_xen_write_todo is just a wrapper around xen_9pfs_queued to provide
> >>> a return value that works well with wait_event_interruptible.
> >>>
> >>> I would prefer not to call p9_xen_write_todo here, because it's simpler
> >>> if we don't read prod and cons twice.
> >> I was referring to the whole code fragment after spin_lock_irqsave(),
> >> not just the last line. Isn't it exactly !p9_xen_write_todo()?
> > Yes, it is true they are almost the same. The difference, and the reason
> > for p9_xen_write_todo to exist, is that p9_xen_write_todo is called in
> > the wait_event_interruptible loop, as such it needs to read prod and
> > cons every time. On the other end, here we want to read them once. Does
> > it make sense?
>
>
> I am clearly being particularly dense here but what I was thinking was:
>
> again:
> while (wait_event_interruptible(ring->wq,
> p9_xen_write_todo(ring, size) > 0) != 0);
>
> spin_lock_irqsave(&ring->lock, flags);
> if (!p9_xen_write_todo(ring, size)) {
> spin_unlock_irqrestore(&ring->lock, flags);
> goto again;
> }
>
> There is no extra read of prod/cons.
Yes, there are: just after this if statement we would have to read them
again to calculate masked_prod and masked_cons.
[toc] | [prev] | [next] | [standalone]
| From | Julien Grall <julien.grall@arm.com> |
|---|---|
| Date | 2017-03-07 20:20 +0100 |
| Subject | Re: [Xen-devel] [PATCH 1/7] xen: import new ring macros in ring.h |
| Message-ID | <tisOm-3ze-31@gated-at.bofh.it> |
| In reply to | #1593688 |
Hi Stefano, On 03/06/2017 08:01 PM, Stefano Stabellini wrote: > Sync the ring.h file with upstream Xen, to introduce the new ring macros. > They will be used by the Xen transport for 9pfs. > > Signed-off-by: Stefano Stabellini <stefano@aporeto.com> > CC: konrad.wilk@oracle.com > CC: boris.ostrovsky@oracle.com > CC: jgross@suse.com > > --- > NB: The new macros have not been committed to Xen yet. Do not apply this > patch until they do. > --- > --- > include/xen/interface/io/ring.h | 131 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 131 insertions(+) > > diff --git a/include/xen/interface/io/ring.h b/include/xen/interface/io/ring.h > index 21f4fbd..e16aa92 100644 > --- a/include/xen/interface/io/ring.h > +++ b/include/xen/interface/io/ring.h [...] > +#define XEN_FLEX_RING_SIZE(order) \ > + (1UL << (order + PAGE_SHIFT - 1)) This will need to be XEN_PAGE_SHIFT in order to works with 64K kernel. Cheers, -- Julien Grall
[toc] | [prev] | [next] | [standalone]
| From | Stefano Stabellini <sstabellini@kernel.org> |
|---|---|
| Date | 2017-03-08 01:30 +0100 |
| Subject | Re: [Xen-devel] [PATCH 1/7] xen: import new ring macros in ring.h |
| Message-ID | <tixEm-70H-13@gated-at.bofh.it> |
| In reply to | #1594563 |
On Tue, 7 Mar 2017, Julien Grall wrote: > Hi Stefano, > > On 03/06/2017 08:01 PM, Stefano Stabellini wrote: > > Sync the ring.h file with upstream Xen, to introduce the new ring macros. > > They will be used by the Xen transport for 9pfs. > > > > Signed-off-by: Stefano Stabellini <stefano@aporeto.com> > > CC: konrad.wilk@oracle.com > > CC: boris.ostrovsky@oracle.com > > CC: jgross@suse.com > > > > --- > > NB: The new macros have not been committed to Xen yet. Do not apply this > > patch until they do. > > --- > > --- > > include/xen/interface/io/ring.h | 131 > > ++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 131 insertions(+) > > > > diff --git a/include/xen/interface/io/ring.h > > b/include/xen/interface/io/ring.h > > index 21f4fbd..e16aa92 100644 > > --- a/include/xen/interface/io/ring.h > > +++ b/include/xen/interface/io/ring.h > > [...] > > > +#define XEN_FLEX_RING_SIZE(order) > > \ > > + (1UL << (order + PAGE_SHIFT - 1)) > > This will need to be XEN_PAGE_SHIFT in order to works with 64K kernel. Good point! I had it right at the beginning but I lost the change in one of the updates from xen.git.
[toc] | [prev] | [next] | [standalone]
| From | Julien Grall <julien.grall@arm.com> |
|---|---|
| Date | 2017-03-08 12:50 +0100 |
| Subject | Re: [Xen-devel] [PATCH 1/7] xen: import new ring macros in ring.h |
| Message-ID | <tiIgp-5ZP-1@gated-at.bofh.it> |
| In reply to | #1594745 |
Hi Stefano, On 08/03/17 00:12, Stefano Stabellini wrote: > On Tue, 7 Mar 2017, Julien Grall wrote: >> Hi Stefano, >> >> On 03/06/2017 08:01 PM, Stefano Stabellini wrote: >>> Sync the ring.h file with upstream Xen, to introduce the new ring macros. >>> They will be used by the Xen transport for 9pfs. >>> >>> Signed-off-by: Stefano Stabellini <stefano@aporeto.com> >>> CC: konrad.wilk@oracle.com >>> CC: boris.ostrovsky@oracle.com >>> CC: jgross@suse.com >>> >>> --- >>> NB: The new macros have not been committed to Xen yet. Do not apply this >>> patch until they do. >>> --- >>> --- >>> include/xen/interface/io/ring.h | 131 >>> ++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 131 insertions(+) >>> >>> diff --git a/include/xen/interface/io/ring.h >>> b/include/xen/interface/io/ring.h >>> index 21f4fbd..e16aa92 100644 >>> --- a/include/xen/interface/io/ring.h >>> +++ b/include/xen/interface/io/ring.h >> >> [...] >> >>> +#define XEN_FLEX_RING_SIZE(order) >>> \ >>> + (1UL << (order + PAGE_SHIFT - 1)) >> >> This will need to be XEN_PAGE_SHIFT in order to works with 64K kernel. > > Good point! I had it right at the beginning but I lost the change in one > of the updates from xen.git. It is probably worth to consider introducing XEN_PAGE_SIZE in the hypervisor code because we are likely to miss the change when the headers will be re-sync in the future. Cheers, -- Julien Grall
[toc] | [prev] | [next] | [standalone]
| From | Roger Pau Monné <roger.pau@citrix.com> |
|---|---|
| Date | 2017-03-07 17:50 +0100 |
| Subject | Re: [Xen-devel] [PATCH 0/7] Xen transport for 9pfs frontend driver |
| Message-ID | <tiqtc-1LM-11@gated-at.bofh.it> |
| In reply to | #1593682 |
On Mon, Mar 06, 2017 at 12:00:41PM -0800, Stefano Stabellini wrote: > Hi all, > > This patch series implements a new transport for 9pfs, aimed at Xen > systems. > > The transport is based on a traditional Xen frontend and backend drivers > pair. This patch series implements the frontend, which typically runs in > a regular unprivileged guest. > > I'll follow up with another series that implements the backend in > userspace in QEMU, which typically runs in Dom0 (but could also run in > a another guest). > > The frontend complies to the Xen transport for 9pfs specification > version 1, available here: > > http://xenbits.xen.org/gitweb/?p=xen.git;a=blob_plain;f=docs/misc/9pfs.markdown;hb=HEAD Kind of tangential to this series, but maybe it would make sense to implement this transport in a fuse based 9pfs driver? I see there are already several fuse-9pfs implementations around. Something for a GSoC/Outreach project? Roger.
[toc] | [prev] | [next] | [standalone]
| From | Stefano Stabellini <sstabellini@kernel.org> |
|---|---|
| Date | 2017-03-07 19:30 +0100 |
| Subject | Re: [Xen-devel] [PATCH 0/7] Xen transport for 9pfs frontend driver |
| Message-ID | <tis1Y-2YE-25@gated-at.bofh.it> |
| In reply to | #1594419 |
[Multipart message — attachments visible in raw view] — view raw
On Tue, 7 Mar 2017, Roger Pau Monné wrote: > On Mon, Mar 06, 2017 at 12:00:41PM -0800, Stefano Stabellini wrote: > > Hi all, > > > > This patch series implements a new transport for 9pfs, aimed at Xen > > systems. > > > > The transport is based on a traditional Xen frontend and backend drivers > > pair. This patch series implements the frontend, which typically runs in > > a regular unprivileged guest. > > > > I'll follow up with another series that implements the backend in > > userspace in QEMU, which typically runs in Dom0 (but could also run in > > a another guest). > > > > The frontend complies to the Xen transport for 9pfs specification > > version 1, available here: > > > > http://xenbits.xen.org/gitweb/?p=xen.git;a=blob_plain;f=docs/misc/9pfs.markdown;hb=HEAD > > Kind of tangential to this series, but maybe it would make sense to implement > this transport in a fuse based 9pfs driver? I see there are already several > fuse-9pfs implementations around. Something for a GSoC/Outreach project? Sure. Additionally, with open source frontends and backends already available, it should be easier to code. I am happy to co-mentor the project with you, if you feel like it.
[toc] | [prev] | [next] | [standalone]
| From | Roger Pau Monné <roger.pau@citrix.com> |
|---|---|
| Date | 2017-03-09 04:10 +0100 |
| Subject | Re: [Xen-devel] [PATCH 0/7] Xen transport for 9pfs frontend driver |
| Message-ID | <tiWCK-7py-1@gated-at.bofh.it> |
| In reply to | #1594502 |
On Tue, Mar 07, 2017 at 10:27:05AM -0800, Stefano Stabellini wrote: > On Tue, 7 Mar 2017, Roger Pau Monné wrote: > > On Mon, Mar 06, 2017 at 12:00:41PM -0800, Stefano Stabellini wrote: > > > Hi all, > > > > > > This patch series implements a new transport for 9pfs, aimed at Xen > > > systems. > > > > > > The transport is based on a traditional Xen frontend and backend drivers > > > pair. This patch series implements the frontend, which typically runs in > > > a regular unprivileged guest. > > > > > > I'll follow up with another series that implements the backend in > > > userspace in QEMU, which typically runs in Dom0 (but could also run in > > > a another guest). > > > > > > The frontend complies to the Xen transport for 9pfs specification > > > version 1, available here: > > > > > > http://xenbits.xen.org/gitweb/?p=xen.git;a=blob_plain;f=docs/misc/9pfs.markdown;hb=HEAD > > > > Kind of tangential to this series, but maybe it would make sense to implement > > this transport in a fuse based 9pfs driver? I see there are already several > > fuse-9pfs implementations around. Something for a GSoC/Outreach project? > > Sure. Additionally, with open source frontends and backends already > available, it should be easier to code. I am happy to co-mentor the > project with you, if you feel like it. I don't mind co-mentoring it, so far I haven't got lucky with any of my other GSoC projects, but I don't know anything about 9pfs or fuse :). This also has the difficulty that neither you not me is a member of any of the 9pfs-fuse projects, so it might be hard to get the changes upstream. Roger.
[toc] | [prev] | [next] | [standalone]
| From | Stefano Stabellini <sstabellini@kernel.org> |
|---|---|
| Date | 2017-03-13 23:40 +0100 |
| Subject | Re: [Xen-devel] [PATCH 0/7] Xen transport for 9pfs frontend driver |
| Message-ID | <tkGNb-7uk-15@gated-at.bofh.it> |
| In reply to | #1595649 |
[Multipart message — attachments visible in raw view] — view raw
On Thu, 9 Mar 2017, Roger Pau Monné wrote: > On Tue, Mar 07, 2017 at 10:27:05AM -0800, Stefano Stabellini wrote: > > On Tue, 7 Mar 2017, Roger Pau Monné wrote: > > > On Mon, Mar 06, 2017 at 12:00:41PM -0800, Stefano Stabellini wrote: > > > > Hi all, > > > > > > > > This patch series implements a new transport for 9pfs, aimed at Xen > > > > systems. > > > > > > > > The transport is based on a traditional Xen frontend and backend drivers > > > > pair. This patch series implements the frontend, which typically runs in > > > > a regular unprivileged guest. > > > > > > > > I'll follow up with another series that implements the backend in > > > > userspace in QEMU, which typically runs in Dom0 (but could also run in > > > > a another guest). > > > > > > > > The frontend complies to the Xen transport for 9pfs specification > > > > version 1, available here: > > > > > > > > http://xenbits.xen.org/gitweb/?p=xen.git;a=blob_plain;f=docs/misc/9pfs.markdown;hb=HEAD > > > > > > Kind of tangential to this series, but maybe it would make sense to implement > > > this transport in a fuse based 9pfs driver? I see there are already several > > > fuse-9pfs implementations around. Something for a GSoC/Outreach project? > > > > Sure. Additionally, with open source frontends and backends already > > available, it should be easier to code. I am happy to co-mentor the > > project with you, if you feel like it. > > I don't mind co-mentoring it, so far I haven't got lucky with any of my other > GSoC projects, but I don't know anything about 9pfs or fuse :). > > This also has the difficulty that neither you not me is a member of any of the > 9pfs-fuse projects, so it might be hard to get the changes upstream. Good point. It would be best if one of the mentors was already engaged with the 9pfs-fuse community.
[toc] | [prev] | [standalone]
Page 2 of 2 — ← Prev page 1 [2]
Back to top | Article view | linux.kernel
csiph-web