Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1642026 > unrolled thread

[PATCH 01/18] xen: introduce the pvcalls interface header

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 01/18] xen: introduce the pvcalls interface header Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
    [PATCH 14/18] xen/pvcalls: disconnect and module_exit Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
    [PATCH 02/18] xen/pvcalls: introduce the pvcalls xenbus backend Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
    [PATCH 15/18] xen/pvcalls: introduce the ioworker Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
    [PATCH 09/18] xen/pvcalls: implement bind command Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
    [PATCH 03/18] xen/pvcalls: initialize the module and register the xenbus backend Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
      Re: [PATCH 03/18] xen/pvcalls: initialize the module and register the  xenbus backend Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-05-16 03:50 +0200
        Re: [PATCH 03/18] xen/pvcalls: initialize the module and register  the xenbus backend Stefano Stabellini <sstabellini@kernel.org> - 2017-05-16 22:10 +0200
          Re: [PATCH 03/18] xen/pvcalls: initialize the module and register  the xenbus backend Stefano Stabellini <sstabellini@kernel.org> - 2017-05-16 22:30 +0200
      Re: [PATCH 03/18] xen/pvcalls: initialize the module and register the  xenbus backend Juergen Gross <jgross@suse.com> - 2017-05-16 08:50 +0200
        Re: [PATCH 03/18] xen/pvcalls: initialize the module and register  the xenbus backend Stefano Stabellini <sstabellini@kernel.org> - 2017-05-16 22:00 +0200
          Re: [PATCH 03/18] xen/pvcalls: initialize the module and register the  xenbus backend Juergen Gross <jgross@suse.com> - 2017-05-17 07:30 +0200
            Re: [PATCH 03/18] xen/pvcalls: initialize the module and register  the xenbus backend Stefano Stabellini <sstabellini@kernel.org> - 2017-05-18 23:20 +0200
              Re: [PATCH 03/18] xen/pvcalls: initialize the module and register  the xenbus backend Stefano Stabellini <sstabellini@kernel.org> - 2017-05-20 00:40 +0200
    [PATCH 10/18] xen/pvcalls: implement listen command Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
    [PATCH 13/18] xen/pvcalls: implement release command Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
    [PATCH 06/18] xen/pvcalls: handle commands from the frontend Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
      Re: [PATCH 06/18] xen/pvcalls: handle commands from the frontend Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-05-16 04:20 +0200
        Re: [PATCH 06/18] xen/pvcalls: handle commands from the frontend Stefano Stabellini <sstabellini@kernel.org> - 2017-05-16 23:00 +0200
    [PATCH 07/18] xen/pvcalls: implement socket command Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
      Re: [PATCH 07/18] xen/pvcalls: implement socket command Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-05-16 04:30 +0200
        Re: [PATCH 07/18] xen/pvcalls: implement socket command Stefano Stabellini <sstabellini@kernel.org> - 2017-05-16 22:50 +0200
    [PATCH 12/18] xen/pvcalls: implement poll command Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
    [PATCH 16/18] xen/pvcalls: implement read Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
    [PATCH 08/18] xen/pvcalls: implement connect command Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
      Re: [PATCH 08/18] xen/pvcalls: implement connect command Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-05-16 04:50 +0200
        Re: [PATCH 08/18] xen/pvcalls: implement connect command Stefano Stabellini <sstabellini@kernel.org> - 2017-05-16 23:10 +0200
          Re: [PATCH 08/18] xen/pvcalls: implement connect command Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-05-17 00:00 +0200
            Re: [PATCH 08/18] xen/pvcalls: implement connect command Stefano Stabellini <sstabellini@kernel.org> - 2017-05-18 21:20 +0200
              Re: [PATCH 08/18] xen/pvcalls: implement connect command Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-05-18 22:30 +0200
    [PATCH 11/18] xen/pvcalls: implement accept command Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
    [PATCH 05/18] xen/pvcalls: connect to a frontend Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:50 +0200
      Re: [PATCH 05/18] xen/pvcalls: connect to a frontend Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-05-16 04:10 +0200
        Re: [PATCH 05/18] xen/pvcalls: connect to a frontend Stefano Stabellini <sstabellini@kernel.org> - 2017-05-16 22:30 +0200
          Re: [PATCH 05/18] xen/pvcalls: connect to a frontend Stefano Stabellini <sstabellini@kernel.org> - 2017-05-16 22:40 +0200
    [PATCH 04/18] xen/pvcalls: xenbus state handling Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:50 +0200
      Re: [PATCH 04/18] xen/pvcalls: xenbus state handling Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-05-16 03:50 +0200
        Re: [PATCH 04/18] xen/pvcalls: xenbus state handling Stefano Stabellini <sstabellini@kernel.org> - 2017-05-16 22:20 +0200

Page 1 of 2  [1] 2  Next page →

#1642026 — [PATCH 01/18] xen: introduce the pvcalls interface header

Introduce the C header file which defines the PV Calls interface. It is
imported from xen/include/public/io/pvcalls.h.

Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: konrad.wilk@oracle.com
CC: boris.ostrovsky@oracle.com
CC: jgross@suse.com
---
 include/xen/interface/io/pvcalls.h | 117 +++++++++++++++++++++++++++++++++++++
 1 file changed, 117 insertions(+)
 create mode 100644 include/xen/interface/io/pvcalls.h

diff --git a/include/xen/interface/io/pvcalls.h b/include/xen/interface/io/pvcalls.h
new file mode 100644
index 0000000..c438c1b
--- /dev/null
+++ b/include/xen/interface/io/pvcalls.h
@@ -0,0 +1,117 @@
+#ifndef __XEN_PUBLIC_IO_XEN_PVCALLS_H__
+#define __XEN_PUBLIC_IO_XEN_PVCALLS_H__
+
+#include <linux/net.h>
+#include "xen/interface/io/ring.h"
+
+/*
+ * See docs/misc/pvcalls.markdown in xen.git for the full specification:
+ * https://xenbits.xen.org/docs/unstable/misc/pvcalls.html
+ */
+struct pvcalls_data_intf {
+    RING_IDX in_cons, in_prod, in_error;
+
+    uint8_t pad1[52];
+
+    RING_IDX out_cons, out_prod, out_error;
+
+    uint8_t pad2[52];
+
+    RING_IDX ring_order;
+    grant_ref_t ref[];
+};
+DEFINE_XEN_FLEX_RING(pvcalls);
+
+#define PVCALLS_SOCKET         0
+#define PVCALLS_CONNECT        1
+#define PVCALLS_RELEASE        2
+#define PVCALLS_BIND           3
+#define PVCALLS_LISTEN         4
+#define PVCALLS_ACCEPT         5
+#define PVCALLS_POLL           6
+
+struct xen_pvcalls_request {
+    uint32_t req_id; /* private to guest, echoed in response */
+    uint32_t cmd;    /* command to execute */
+    union {
+        struct xen_pvcalls_socket {
+            uint64_t id;
+            uint32_t domain;
+            uint32_t type;
+            uint32_t protocol;
+        } socket;
+        struct xen_pvcalls_connect {
+            uint64_t id;
+            uint8_t addr[28];
+            uint32_t len;
+            uint32_t flags;
+            grant_ref_t ref;
+            uint32_t evtchn;
+        } connect;
+        struct xen_pvcalls_release {
+            uint64_t id;
+            uint8_t reuse;
+        } release;
+        struct xen_pvcalls_bind {
+            uint64_t id;
+            uint8_t addr[28];
+            uint32_t len;
+        } bind;
+        struct xen_pvcalls_listen {
+            uint64_t id;
+            uint32_t backlog;
+        } listen;
+        struct xen_pvcalls_accept {
+            uint64_t id;
+            uint64_t id_new;
+            grant_ref_t ref;
+            uint32_t evtchn;
+        } accept;
+        struct xen_pvcalls_poll {
+            uint64_t id;
+        } poll;
+        /* dummy member to force sizeof(struct xen_pvcalls_request)
+         * to match across archs */
+        struct xen_pvcalls_dummy {
+            uint8_t dummy[56];
+        } dummy;
+    } u;
+};
+
+struct xen_pvcalls_response {
+    uint32_t req_id;
+    uint32_t cmd;
+    int32_t ret;
+    uint32_t pad;
+    union {
+        struct _xen_pvcalls_socket {
+            uint64_t id;
+        } socket;
+        struct _xen_pvcalls_connect {
+            uint64_t id;
+        } connect;
+        struct _xen_pvcalls_release {
+            uint64_t id;
+        } release;
+        struct _xen_pvcalls_bind {
+            uint64_t id;
+        } bind;
+        struct _xen_pvcalls_listen {
+            uint64_t id;
+        } listen;
+        struct _xen_pvcalls_accept {
+            uint64_t id;
+        } accept;
+        struct _xen_pvcalls_poll {
+            uint64_t id;
+        } poll;
+        struct _xen_pvcalls_dummy {
+            uint8_t dummy[8];
+        } dummy;
+    } u;
+};
+
+DEFINE_RING_TYPES(xen_pvcalls, struct xen_pvcalls_request,
+                  struct xen_pvcalls_response);
+
+#endif
-- 
1.9.1

[toc] | [next] | [standalone]

#1642027 — [PATCH 14/18] xen/pvcalls: disconnect and module_exit

Implement backend_disconnect. Call pvcalls_back_release_active on active
sockets and pvcalls_back_release_passive on passive sockets.

Implement module_exit by calling backend_disconnect on frontend
connections.

Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: boris.ostrovsky@oracle.com
CC: jgross@suse.com
---
 drivers/xen/pvcalls-back.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
index 22c6426..0daa90a 100644
--- a/drivers/xen/pvcalls-back.c
+++ b/drivers/xen/pvcalls-back.c
@@ -855,6 +855,35 @@ static int backend_connect(struct xenbus_device *dev)
 
 static int backend_disconnect(struct xenbus_device *dev)
 {
+	struct pvcalls_back_priv *priv;
+	struct sock_mapping *map, *n;
+	struct sockpass_mapping *mappass;
+	struct radix_tree_iter iter;
+	void **slot;
+
+
+	priv = dev_get_drvdata(&dev->dev);
+
+	list_for_each_entry_safe(map, n, &priv->socket_mappings, list) {
+		pvcalls_back_release_active(dev, priv, map);
+	}
+	radix_tree_for_each_slot(slot, &priv->socketpass_mappings, &iter, 0) {
+		mappass = radix_tree_deref_slot(slot);
+		if (!mappass || radix_tree_exception(mappass)) {
+			if (radix_tree_deref_retry(mappass)) {
+				slot = radix_tree_iter_retry(&iter);
+				continue;
+			}
+		} else
+			pvcalls_back_release_passive(dev, priv, mappass);
+	}
+	xenbus_unmap_ring_vfree(dev, (void *)priv->sring);
+	unbind_from_irqhandler(priv->irq, dev);
+	list_del(&priv->list);
+	destroy_workqueue(priv->wq);
+	kfree(priv);
+	dev_set_drvdata(&dev->dev, NULL);
+
 	return 0;
 }
 
@@ -1056,3 +1085,22 @@ static int __init pvcalls_back_init(void)
 	return -ENOMEM;
 }
 module_init(pvcalls_back_init);
+
+static void __exit pvcalls_back_fin(void)
+{
+	struct pvcalls_back_priv *priv, *npriv;
+
+	down_write(&pvcalls_back_global.privs_lock);
+	list_for_each_entry_safe(priv, npriv, &pvcalls_back_global.privs,
+				 list) {
+		backend_disconnect(priv->dev);
+	}
+	up_write(&pvcalls_back_global.privs_lock);
+
+	xenbus_unregister_driver(&pvcalls_back_driver);
+	destroy_workqueue(pvcalls_back_global.wq);
+	kfree(pvcalls_back_global.ioworkers);
+	memset(&pvcalls_back_global, 0, sizeof(pvcalls_back_global));
+}
+
+module_exit(pvcalls_back_fin);
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1642028 — [PATCH 02/18] xen/pvcalls: introduce the pvcalls xenbus backend

Introduce a xenbus backend for the pvcalls protocol, as defined by
https://xenbits.xen.org/docs/unstable/misc/pvcalls.html.

This patch only adds the stubs, the code will be added by the following
patches.

Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: boris.ostrovsky@oracle.com
CC: jgross@suse.com
---
 drivers/xen/pvcalls-back.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)
 create mode 100644 drivers/xen/pvcalls-back.c

diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
new file mode 100644
index 0000000..2dbf7d8
--- /dev/null
+++ b/drivers/xen/pvcalls-back.c
@@ -0,0 +1,61 @@
+/*
+ * (c) 2017 Stefano Stabellini <stefano@aporeto.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/kthread.h>
+#include <linux/list.h>
+#include <linux/radix-tree.h>
+#include <linux/module.h>
+#include <linux/rwsem.h>
+#include <linux/wait.h>
+
+#include <xen/events.h>
+#include <xen/grant_table.h>
+#include <xen/xen.h>
+#include <xen/xenbus.h>
+#include <xen/interface/io/pvcalls.h>
+
+static int pvcalls_back_probe(struct xenbus_device *dev,
+			      const struct xenbus_device_id *id)
+{
+	return 0;
+}
+
+static void pvcalls_back_changed(struct xenbus_device *dev,
+				 enum xenbus_state frontend_state)
+{
+}
+
+static int pvcalls_back_remove(struct xenbus_device *dev)
+{
+	return 0;
+}
+
+static int pvcalls_back_uevent(struct xenbus_device *xdev,
+			       struct kobj_uevent_env *env)
+{
+	return 0;
+}
+
+static const struct xenbus_device_id pvcalls_back_ids[] = {
+	{ "pvcalls" },
+	{ "" }
+};
+
+static struct xenbus_driver pvcalls_back_driver = {
+	.ids = pvcalls_back_ids,
+	.probe = pvcalls_back_probe,
+	.remove = pvcalls_back_remove,
+	.uevent = pvcalls_back_uevent,
+	.otherend_changed = pvcalls_back_changed,
+};
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1642029 — [PATCH 15/18] xen/pvcalls: introduce the ioworker

We have one ioworker per cpu core. Each ioworker gets assigned active
sockets randomly. Once a socket is assigned to an ioworker, it remains
tied to it until is released.

Each ioworker goes through the list of outstanding read/write requests
by walking a list of struct sock_mapping. Once a request has been dealt
with, the struct sock_mapping is removed from the list.

We use one atomic counter per socket for "read" operations and one
for "write" operations to keep track of the reads/writes to do.

We also use one atomic counter ("io") per ioworker to keep track of how
many outstanding requests we have in total assigned to the ioworker. The
ioworker finishes when there are none.

Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: boris.ostrovsky@oracle.com
CC: jgross@suse.com
---
 drivers/xen/pvcalls-back.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
index 0daa90a..db3e02c 100644
--- a/drivers/xen/pvcalls-back.c
+++ b/drivers/xen/pvcalls-back.c
@@ -99,8 +99,52 @@ struct sockpass_mapping {
 static int pvcalls_back_release_active(struct xenbus_device *dev,
 				       struct pvcalls_back_priv *priv,
 				       struct sock_mapping *map);
+
+static void pvcalls_conn_back_read(unsigned long opaque)
+{
+}
+
+static int pvcalls_conn_back_write(struct sock_mapping *map)
+{
+	return 0;
+}
+
 static void pvcalls_back_ioworker(struct work_struct *work)
 {
+	struct pvcalls_ioworker *ioworker = container_of(work,
+		struct pvcalls_ioworker, register_work);
+	int num = ioworker->num;
+	struct sock_mapping *map, *n;
+	unsigned long flags;
+
+	while (atomic_read(&ioworker->io) > 0) {
+		spin_lock_irqsave(&ioworker->lock, flags);
+		list_for_each_entry_safe(map, n, &ioworker->wqs, queue) {
+			if (map->data_worker != num)
+				continue;
+
+			if (atomic_read(&map->release) > 0) {
+				list_del_init(&map->queue);
+				atomic_set(&map->release, 0);
+				continue;
+			}
+
+			spin_unlock_irqrestore(&ioworker->lock, flags);
+			if (atomic_read(&map->read) > 0)
+				pvcalls_conn_back_read((unsigned long)map);
+			if (atomic_read(&map->write) > 0)
+				pvcalls_conn_back_write(map);
+			spin_lock_irqsave(&ioworker->lock, flags);
+
+			if (atomic_read(&map->read) == 0 &&
+				atomic_read(&map->write) == 0) {
+				list_del_init(&map->queue);
+				atomic_set(&map->release, 0);
+			}
+		}
+		atomic_dec(&ioworker->io);
+		spin_unlock_irqrestore(&ioworker->lock, flags);
+	}
 }
 
 static int pvcalls_back_socket(struct xenbus_device *dev,
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1642030 — [PATCH 09/18] xen/pvcalls: implement bind command

Allocate a socket. Track the allocated passive sockets with a new data
structure named sockpass_mapping. It contains an unbound workqueue to
schedule delayed work for the accept and poll commands. It also has a
reqcopy field to be used to store a copy of a request for delayed work.
Reads/writes to it are protected by a lock (the "copy_lock" spinlock).
Initialize the workqueue in pvcalls_back_bind.

Implement the bind command with inet_bind.

The pass_sk_data_ready event handler will be added later.

Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: boris.ostrovsky@oracle.com
CC: jgross@suse.com
---
 drivers/xen/pvcalls-back.c | 89 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 88 insertions(+), 1 deletion(-)

diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
index 9ac1cf2..ff4634d 100644
--- a/drivers/xen/pvcalls-back.c
+++ b/drivers/xen/pvcalls-back.c
@@ -82,6 +82,18 @@ struct sock_mapping {
 	void (*saved_data_ready)(struct sock *sk);
 };
 
+struct sockpass_mapping {
+	struct list_head list;
+	struct pvcalls_back_priv *priv;
+	struct socket *sock;
+	uint64_t id;
+	struct xen_pvcalls_request reqcopy;
+	spinlock_t copy_lock;
+	struct workqueue_struct *wq;
+	struct work_struct register_work;
+	void (*saved_data_ready)(struct sock *sk);
+};
+
 static irqreturn_t pvcalls_back_conn_event(int irq, void *sock_map);
 static int pvcalls_back_release_active(struct xenbus_device *dev,
 				       struct pvcalls_back_priv *priv,
@@ -249,10 +261,85 @@ static int pvcalls_back_release(struct xenbus_device *dev,
 	return 0;
 }
 
+static void __pvcalls_back_accept(struct work_struct *work)
+{
+}
+
+static void pvcalls_pass_sk_data_ready(struct sock *sock)
+{
+}
+
 static int pvcalls_back_bind(struct xenbus_device *dev,
 			     struct xen_pvcalls_request *req)
 {
-	return 0;
+	struct pvcalls_back_priv *priv;
+	int ret, err;
+	struct socket *sock;
+	struct sockpass_mapping *map = NULL;
+	struct xen_pvcalls_response *rsp;
+
+	if (dev == NULL)
+		return 0;
+	priv = dev_get_drvdata(&dev->dev);
+
+	map = kzalloc(sizeof(*map), GFP_KERNEL);
+	if (map == NULL) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	INIT_WORK(&map->register_work, __pvcalls_back_accept);
+	spin_lock_init(&map->copy_lock);
+	map->wq = alloc_workqueue("pvcalls_wq", WQ_UNBOUND, 1);
+	if (!map->wq) {
+		ret = -ENOMEM;
+		kfree(map);
+		goto out;
+	}
+
+	ret = sock_create(AF_INET, SOCK_STREAM, 0, &sock);
+	if (ret < 0) {
+		destroy_workqueue(map->wq);
+		kfree(map);
+		goto out;
+	}
+
+	ret = inet_bind(sock, (struct sockaddr *)&req->u.bind.addr,
+			req->u.bind.len);
+	if (ret < 0) {
+		destroy_workqueue(map->wq);
+		kfree(map);
+		goto out;
+	}
+
+	map->priv = priv;
+	map->sock = sock;
+	map->id = req->u.bind.id;
+
+	down_write(&priv->pvcallss_lock);
+	err = radix_tree_insert(&priv->socketpass_mappings, map->id,
+				map);
+	up_write(&priv->pvcallss_lock);
+	if (err) {
+		ret = err;
+		destroy_workqueue(map->wq);
+		kfree(map);
+		goto out;
+	}
+
+	lock_sock(sock->sk);
+	map->saved_data_ready = sock->sk->sk_data_ready;
+	sock->sk->sk_user_data = map;
+	sock->sk->sk_data_ready = pvcalls_pass_sk_data_ready;
+	release_sock(sock->sk);
+
+out:
+	rsp = RING_GET_RESPONSE(&priv->ring, priv->ring.rsp_prod_pvt++);
+	rsp->req_id = req->req_id;
+	rsp->cmd = req->cmd;
+	rsp->u.bind.id = req->u.bind.id;
+	rsp->ret = ret;
+	return 1;
 }
 
 static int pvcalls_back_listen(struct xenbus_device *dev,
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1642032 — [PATCH 03/18] xen/pvcalls: initialize the module and register the xenbus backend

The pvcalls backend has one ioworker per cpu: the ioworkers are
implemented as a cpu bound workqueue, and will deal with the actual
socket and data ring reads/writes.

ioworkers are global: we only have one set for all the frontends. They
process requests on their wqs list in order, once they are done with a
request, they'll remove it from the list. A spinlock is used for
protecting the list. Each ioworker is bound to a different cpu to
maximize throughput.

Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: boris.ostrovsky@oracle.com
CC: jgross@suse.com
---
 drivers/xen/pvcalls-back.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
index 2dbf7d8..46a889a 100644
--- a/drivers/xen/pvcalls-back.c
+++ b/drivers/xen/pvcalls-back.c
@@ -25,6 +25,26 @@
 #include <xen/xenbus.h>
 #include <xen/interface/io/pvcalls.h>
 
+struct pvcalls_ioworker {
+	struct work_struct register_work;
+	atomic_t io;
+	struct list_head wqs;
+	spinlock_t lock;
+	int num;
+};
+
+struct pvcalls_back_global {
+	struct pvcalls_ioworker *ioworkers;
+	int nr_ioworkers;
+	struct workqueue_struct *wq;
+	struct list_head privs;
+	struct rw_semaphore privs_lock;
+} pvcalls_back_global;
+
+static void pvcalls_back_ioworker(struct work_struct *work)
+{
+}
+
 static int pvcalls_back_probe(struct xenbus_device *dev,
 			      const struct xenbus_device_id *id)
 {
@@ -59,3 +79,47 @@ static int pvcalls_back_uevent(struct xenbus_device *xdev,
 	.uevent = pvcalls_back_uevent,
 	.otherend_changed = pvcalls_back_changed,
 };
+
+static int __init pvcalls_back_init(void)
+{
+	int ret, i, cpu;
+
+	if (!xen_domain())
+		return -ENODEV;
+
+	ret = xenbus_register_backend(&pvcalls_back_driver);
+	if (ret < 0)
+		return ret;
+
+	init_rwsem(&pvcalls_back_global.privs_lock);
+	INIT_LIST_HEAD(&pvcalls_back_global.privs);
+	pvcalls_back_global.wq = alloc_workqueue("pvcalls_io", 0, 0);
+	if (!pvcalls_back_global.wq)
+		goto error;
+	pvcalls_back_global.nr_ioworkers = num_online_cpus();
+	pvcalls_back_global.ioworkers = kzalloc(
+		sizeof(*pvcalls_back_global.ioworkers) *
+		pvcalls_back_global.nr_ioworkers, GFP_KERNEL);
+	if (!pvcalls_back_global.ioworkers)
+		goto error;
+	i = 0;
+	for_each_online_cpu(cpu) {
+		pvcalls_back_global.ioworkers[i].num = i;
+		atomic_set(&pvcalls_back_global.ioworkers[i].io, 1);
+		spin_lock_init(&pvcalls_back_global.ioworkers[i].lock);
+		INIT_LIST_HEAD(&pvcalls_back_global.ioworkers[i].wqs);
+		INIT_WORK(&pvcalls_back_global.ioworkers[i].register_work,
+			pvcalls_back_ioworker);
+		i++;
+	}
+	return 0;
+
+error:
+	if (pvcalls_back_global.wq)
+		destroy_workqueue(pvcalls_back_global.wq);
+	xenbus_unregister_driver(&pvcalls_back_driver);
+	kfree(pvcalls_back_global.ioworkers);
+	memset(&pvcalls_back_global, 0, sizeof(pvcalls_back_global));
+	return -ENOMEM;
+}
+module_init(pvcalls_back_init);
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1642176 — Re: [PATCH 03/18] xen/pvcalls: initialize the module and register the xenbus backend

On 05/15/2017 04:35 PM, Stefano Stabellini wrote:
> The pvcalls backend has one ioworker per cpu: the ioworkers are
> implemented as a cpu bound workqueue, and will deal with the actual
> socket and data ring reads/writes.
>
> ioworkers are global: we only have one set for all the frontends. They
> process requests on their wqs list in order, once they are done with a
> request, they'll remove it from the list. A spinlock is used for
> protecting the list. Each ioworker is bound to a different cpu to
> maximize throughput.
>
> Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> CC: boris.ostrovsky@oracle.com
> CC: jgross@suse.com
> ---
>  drivers/xen/pvcalls-back.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
>
> diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
> index 2dbf7d8..46a889a 100644
> --- a/drivers/xen/pvcalls-back.c
> +++ b/drivers/xen/pvcalls-back.c
> @@ -25,6 +25,26 @@
>  #include <xen/xenbus.h>
>  #include <xen/interface/io/pvcalls.h>
>
> +struct pvcalls_ioworker {
> +	struct work_struct register_work;
> +	atomic_t io;
> +	struct list_head wqs;
> +	spinlock_t lock;
> +	int num;
> +};
> +
> +struct pvcalls_back_global {
> +	struct pvcalls_ioworker *ioworkers;
> +	int nr_ioworkers;
> +	struct workqueue_struct *wq;
> +	struct list_head privs;
> +	struct rw_semaphore privs_lock;

Is there a reason why these are called "privs"?

And why are you using a rw semaphore --- I only noticed two instances of 
use and both are writes.


> +} pvcalls_back_global;
> +
> +static void pvcalls_back_ioworker(struct work_struct *work)
> +{
> +}
> +
>  static int pvcalls_back_probe(struct xenbus_device *dev,
>  			      const struct xenbus_device_id *id)
>  {
> @@ -59,3 +79,47 @@ static int pvcalls_back_uevent(struct xenbus_device *xdev,
>  	.uevent = pvcalls_back_uevent,
>  	.otherend_changed = pvcalls_back_changed,
>  };
> +
> +static int __init pvcalls_back_init(void)
> +{
> +	int ret, i, cpu;
> +
> +	if (!xen_domain())
> +		return -ENODEV;
> +
> +	ret = xenbus_register_backend(&pvcalls_back_driver);
> +	if (ret < 0)
> +		return ret;
> +
> +	init_rwsem(&pvcalls_back_global.privs_lock);
> +	INIT_LIST_HEAD(&pvcalls_back_global.privs);
> +	pvcalls_back_global.wq = alloc_workqueue("pvcalls_io", 0, 0);
> +	if (!pvcalls_back_global.wq)
> +		goto error;
> +	pvcalls_back_global.nr_ioworkers = num_online_cpus();


Should nr_ioworkers be updated on CPU hot(un)plug?


> +	pvcalls_back_global.ioworkers = kzalloc(
> +		sizeof(*pvcalls_back_global.ioworkers) *
> +		pvcalls_back_global.nr_ioworkers, GFP_KERNEL);
> +	if (!pvcalls_back_global.ioworkers)
> +		goto error;
> +	i = 0;
> +	for_each_online_cpu(cpu) {
> +		pvcalls_back_global.ioworkers[i].num = i;
> +		atomic_set(&pvcalls_back_global.ioworkers[i].io, 1);
> +		spin_lock_init(&pvcalls_back_global.ioworkers[i].lock);
> +		INIT_LIST_HEAD(&pvcalls_back_global.ioworkers[i].wqs);
> +		INIT_WORK(&pvcalls_back_global.ioworkers[i].register_work,
> +			pvcalls_back_ioworker);
> +		i++;
> +	}
> +	return 0;
> +
> +error:
> +	if (pvcalls_back_global.wq)
> +		destroy_workqueue(pvcalls_back_global.wq);
> +	xenbus_unregister_driver(&pvcalls_back_driver);
> +	kfree(pvcalls_back_global.ioworkers);
> +	memset(&pvcalls_back_global, 0, sizeof(pvcalls_back_global));
> +	return -ENOMEM;

This routine could use more newlines. (and in other patches too)

-boris

> +}
> +module_init(pvcalls_back_init);
>

[toc] | [prev] | [next] | [standalone]

#1642792 — Re: [PATCH 03/18] xen/pvcalls: initialize the module and register the xenbus backend

On Mon, 15 May 2017, Boris Ostrovsky wrote:
> On 05/15/2017 04:35 PM, Stefano Stabellini wrote:
> > The pvcalls backend has one ioworker per cpu: the ioworkers are
> > implemented as a cpu bound workqueue, and will deal with the actual
> > socket and data ring reads/writes.
> > 
> > ioworkers are global: we only have one set for all the frontends. They
> > process requests on their wqs list in order, once they are done with a
> > request, they'll remove it from the list. A spinlock is used for
> > protecting the list. Each ioworker is bound to a different cpu to
> > maximize throughput.
> > 
> > Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> > CC: boris.ostrovsky@oracle.com
> > CC: jgross@suse.com
> > ---
> >  drivers/xen/pvcalls-back.c | 64
> > ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 64 insertions(+)
> > 
> > diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
> > index 2dbf7d8..46a889a 100644
> > --- a/drivers/xen/pvcalls-back.c
> > +++ b/drivers/xen/pvcalls-back.c
> > @@ -25,6 +25,26 @@
> >  #include <xen/xenbus.h>
> >  #include <xen/interface/io/pvcalls.h>
> > 
> > +struct pvcalls_ioworker {
> > +	struct work_struct register_work;
> > +	atomic_t io;
> > +	struct list_head wqs;
> > +	spinlock_t lock;
> > +	int num;
> > +};
> > +
> > +struct pvcalls_back_global {
> > +	struct pvcalls_ioworker *ioworkers;
> > +	int nr_ioworkers;
> > +	struct workqueue_struct *wq;
> > +	struct list_head privs;
> > +	struct rw_semaphore privs_lock;
> 
> Is there a reason why these are called "privs"?

I realize it is a silly name :-)
It is called "privs" because it is a list of "priv" where priv is the
private per frontend data structure. I could call it "frontends"?


> And why are you using a rw semaphore --- I only noticed two instances of use
> and both are writes.

Yes, this is wrong, legacy from a previous version of the codebase. A
simple spin_lock should suffice for this use-case.


> > +} pvcalls_back_global;
> > +
> > +static void pvcalls_back_ioworker(struct work_struct *work)
> > +{
> > +}
> > +
> >  static int pvcalls_back_probe(struct xenbus_device *dev,
> >  			      const struct xenbus_device_id *id)
> >  {
> > @@ -59,3 +79,47 @@ static int pvcalls_back_uevent(struct xenbus_device
> > *xdev,
> >  	.uevent = pvcalls_back_uevent,
> >  	.otherend_changed = pvcalls_back_changed,
> >  };
> > +
> > +static int __init pvcalls_back_init(void)
> > +{
> > +	int ret, i, cpu;
> > +
> > +	if (!xen_domain())
> > +		return -ENODEV;
> > +
> > +	ret = xenbus_register_backend(&pvcalls_back_driver);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	init_rwsem(&pvcalls_back_global.privs_lock);
> > +	INIT_LIST_HEAD(&pvcalls_back_global.privs);
> > +	pvcalls_back_global.wq = alloc_workqueue("pvcalls_io", 0, 0);
> > +	if (!pvcalls_back_global.wq)
> > +		goto error;
> > +	pvcalls_back_global.nr_ioworkers = num_online_cpus();
> 
> 
> Should nr_ioworkers be updated on CPU hot(un)plug?

I thought about it, but I don't think it is worth introducing the
complexity to deal with dynamic ioworkers allocations.


 
> > +	pvcalls_back_global.ioworkers = kzalloc(
> > +		sizeof(*pvcalls_back_global.ioworkers) *
> > +		pvcalls_back_global.nr_ioworkers, GFP_KERNEL);
> > +	if (!pvcalls_back_global.ioworkers)
> > +		goto error;
> > +	i = 0;
> > +	for_each_online_cpu(cpu) {
> > +		pvcalls_back_global.ioworkers[i].num = i;
> > +		atomic_set(&pvcalls_back_global.ioworkers[i].io, 1);
> > +		spin_lock_init(&pvcalls_back_global.ioworkers[i].lock);
> > +		INIT_LIST_HEAD(&pvcalls_back_global.ioworkers[i].wqs);
> > +		INIT_WORK(&pvcalls_back_global.ioworkers[i].register_work,
> > +			pvcalls_back_ioworker);
> > +		i++;
> > +	}
> > +	return 0;
> > +
> > +error:
> > +	if (pvcalls_back_global.wq)
> > +		destroy_workqueue(pvcalls_back_global.wq);
> > +	xenbus_unregister_driver(&pvcalls_back_driver);
> > +	kfree(pvcalls_back_global.ioworkers);
> > +	memset(&pvcalls_back_global, 0, sizeof(pvcalls_back_global));
> > +	return -ENOMEM;
> 
> This routine could use more newlines. (and in other patches too)

I'll sprinkle some around


> > +}
> > +module_init(pvcalls_back_init);

[toc] | [prev] | [next] | [standalone]

#1642809 — Re: [PATCH 03/18] xen/pvcalls: initialize the module and register the xenbus backend

On Tue, 16 May 2017, Stefano Stabellini wrote:
> > And why are you using a rw semaphore --- I only noticed two instances of use
> > and both are writes.
> 
> Yes, this is wrong, legacy from a previous version of the codebase. A
> simple spin_lock should suffice for this use-case.

I replied too quickly: it is best as a semaphore because the functions
within the critical regions can cause a reschedule. But there is no need
to use a rw_semaphore, so I'll switch it to a regular semaphore.

[toc] | [prev] | [next] | [standalone]

#1642249 — Re: [PATCH 03/18] xen/pvcalls: initialize the module and register the xenbus backend

On 15/05/17 22:35, Stefano Stabellini wrote:
> The pvcalls backend has one ioworker per cpu: the ioworkers are
> implemented as a cpu bound workqueue, and will deal with the actual
> socket and data ring reads/writes.
> 
> ioworkers are global: we only have one set for all the frontends. They
> process requests on their wqs list in order, once they are done with a
> request, they'll remove it from the list. A spinlock is used for
> protecting the list. Each ioworker is bound to a different cpu to
> maximize throughput.
> 
> Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> CC: boris.ostrovsky@oracle.com
> CC: jgross@suse.com
> ---
>  drivers/xen/pvcalls-back.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
> index 2dbf7d8..46a889a 100644
> --- a/drivers/xen/pvcalls-back.c
> +++ b/drivers/xen/pvcalls-back.c
> @@ -25,6 +25,26 @@
>  #include <xen/xenbus.h>
>  #include <xen/interface/io/pvcalls.h>
>  
> +struct pvcalls_ioworker {
> +	struct work_struct register_work;
> +	atomic_t io;
> +	struct list_head wqs;
> +	spinlock_t lock;
> +	int num;
> +};
> +
> +struct pvcalls_back_global {
> +	struct pvcalls_ioworker *ioworkers;
> +	int nr_ioworkers;
> +	struct workqueue_struct *wq;
> +	struct list_head privs;
> +	struct rw_semaphore privs_lock;
> +} pvcalls_back_global;
> +
> +static void pvcalls_back_ioworker(struct work_struct *work)
> +{
> +}
> +
>  static int pvcalls_back_probe(struct xenbus_device *dev,
>  			      const struct xenbus_device_id *id)
>  {
> @@ -59,3 +79,47 @@ static int pvcalls_back_uevent(struct xenbus_device *xdev,
>  	.uevent = pvcalls_back_uevent,
>  	.otherend_changed = pvcalls_back_changed,
>  };
> +
> +static int __init pvcalls_back_init(void)
> +{
> +	int ret, i, cpu;
> +
> +	if (!xen_domain())
> +		return -ENODEV;
> +
> +	ret = xenbus_register_backend(&pvcalls_back_driver);
> +	if (ret < 0)
> +		return ret;
> +
> +	init_rwsem(&pvcalls_back_global.privs_lock);
> +	INIT_LIST_HEAD(&pvcalls_back_global.privs);
> +	pvcalls_back_global.wq = alloc_workqueue("pvcalls_io", 0, 0);
> +	if (!pvcalls_back_global.wq)
> +		goto error;
> +	pvcalls_back_global.nr_ioworkers = num_online_cpus();

Really? Recently I cam across a system with 640 dom0 cpus. I don't think
we want 640 workers initialized when loading the backend module. I'd
prefer one or a few workers per connected frontend.

> +	pvcalls_back_global.ioworkers = kzalloc(
> +		sizeof(*pvcalls_back_global.ioworkers) *
> +		pvcalls_back_global.nr_ioworkers, GFP_KERNEL);

kcalloc()?

> +	if (!pvcalls_back_global.ioworkers)
> +		goto error;
> +	i = 0;
> +	for_each_online_cpu(cpu) {
> +		pvcalls_back_global.ioworkers[i].num = i;
> +		atomic_set(&pvcalls_back_global.ioworkers[i].io, 1);
> +		spin_lock_init(&pvcalls_back_global.ioworkers[i].lock);
> +		INIT_LIST_HEAD(&pvcalls_back_global.ioworkers[i].wqs);
> +		INIT_WORK(&pvcalls_back_global.ioworkers[i].register_work,
> +			pvcalls_back_ioworker);
> +		i++;
> +	}
> +	return 0;
> +
> +error:
> +	if (pvcalls_back_global.wq)
> +		destroy_workqueue(pvcalls_back_global.wq);
> +	xenbus_unregister_driver(&pvcalls_back_driver);
> +	kfree(pvcalls_back_global.ioworkers);
> +	memset(&pvcalls_back_global, 0, sizeof(pvcalls_back_global));
> +	return -ENOMEM;
> +}
> +module_init(pvcalls_back_init);
> 

Juergen

[toc] | [prev] | [next] | [standalone]

#1642788 — Re: [PATCH 03/18] xen/pvcalls: initialize the module and register the xenbus backend

On Tue, 16 May 2017, Juergen Gross wrote:
> On 15/05/17 22:35, Stefano Stabellini wrote:
> > The pvcalls backend has one ioworker per cpu: the ioworkers are
> > implemented as a cpu bound workqueue, and will deal with the actual
> > socket and data ring reads/writes.
> > 
> > ioworkers are global: we only have one set for all the frontends. They
> > process requests on their wqs list in order, once they are done with a
> > request, they'll remove it from the list. A spinlock is used for
> > protecting the list. Each ioworker is bound to a different cpu to
> > maximize throughput.
> > 
> > Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> > CC: boris.ostrovsky@oracle.com
> > CC: jgross@suse.com
> > ---
> >  drivers/xen/pvcalls-back.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 64 insertions(+)
> > 
> > diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
> > index 2dbf7d8..46a889a 100644
> > --- a/drivers/xen/pvcalls-back.c
> > +++ b/drivers/xen/pvcalls-back.c
> > @@ -25,6 +25,26 @@
> >  #include <xen/xenbus.h>
> >  #include <xen/interface/io/pvcalls.h>
> >  
> > +struct pvcalls_ioworker {
> > +	struct work_struct register_work;
> > +	atomic_t io;
> > +	struct list_head wqs;
> > +	spinlock_t lock;
> > +	int num;
> > +};
> > +
> > +struct pvcalls_back_global {
> > +	struct pvcalls_ioworker *ioworkers;
> > +	int nr_ioworkers;
> > +	struct workqueue_struct *wq;
> > +	struct list_head privs;
> > +	struct rw_semaphore privs_lock;
> > +} pvcalls_back_global;
> > +
> > +static void pvcalls_back_ioworker(struct work_struct *work)
> > +{
> > +}
> > +
> >  static int pvcalls_back_probe(struct xenbus_device *dev,
> >  			      const struct xenbus_device_id *id)
> >  {
> > @@ -59,3 +79,47 @@ static int pvcalls_back_uevent(struct xenbus_device *xdev,
> >  	.uevent = pvcalls_back_uevent,
> >  	.otherend_changed = pvcalls_back_changed,
> >  };
> > +
> > +static int __init pvcalls_back_init(void)
> > +{
> > +	int ret, i, cpu;
> > +
> > +	if (!xen_domain())
> > +		return -ENODEV;
> > +
> > +	ret = xenbus_register_backend(&pvcalls_back_driver);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	init_rwsem(&pvcalls_back_global.privs_lock);
> > +	INIT_LIST_HEAD(&pvcalls_back_global.privs);
> > +	pvcalls_back_global.wq = alloc_workqueue("pvcalls_io", 0, 0);
> > +	if (!pvcalls_back_global.wq)
> > +		goto error;
> > +	pvcalls_back_global.nr_ioworkers = num_online_cpus();
> 
> Really? Recently I cam across a system with 640 dom0 cpus. I don't think
> we want 640 workers initialized when loading the backend module. I'd
> prefer one or a few workers per connected frontend.

I think we want to keep the ioworker allocation to be based on the
number of vcpus: we do not want more ioworkers than vcpus because it is
a waste of resources and leads to worse performance.  Also, given that
they do memcpy's, I also think it is a good idea to bind them to vcpus
(and pin vcpus to pcpus) to get best performance.

However, you have a point there: we need to handle systems with an
extremely large number of Dom0 vcpus. I suggest we introduce an
upper limit for the number of ioworkers. Something like:

#define MAX_IOWORKERS 64
nr_ioworkers = min(MAX_IOWORKERS, num_online_cpus())

MAX_IOWORKERS could be configurable via a command line option.


> > +	pvcalls_back_global.ioworkers = kzalloc(
> > +		sizeof(*pvcalls_back_global.ioworkers) *
> > +		pvcalls_back_global.nr_ioworkers, GFP_KERNEL);
> 
> kcalloc()?

I'll make the change


> > +	if (!pvcalls_back_global.ioworkers)
> > +		goto error;
> > +	i = 0;
> > +	for_each_online_cpu(cpu) {
> > +		pvcalls_back_global.ioworkers[i].num = i;
> > +		atomic_set(&pvcalls_back_global.ioworkers[i].io, 1);
> > +		spin_lock_init(&pvcalls_back_global.ioworkers[i].lock);
> > +		INIT_LIST_HEAD(&pvcalls_back_global.ioworkers[i].wqs);
> > +		INIT_WORK(&pvcalls_back_global.ioworkers[i].register_work,
> > +			pvcalls_back_ioworker);
> > +		i++;
> > +	}
> > +	return 0;
> > +
> > +error:
> > +	if (pvcalls_back_global.wq)
> > +		destroy_workqueue(pvcalls_back_global.wq);
> > +	xenbus_unregister_driver(&pvcalls_back_driver);
> > +	kfree(pvcalls_back_global.ioworkers);
> > +	memset(&pvcalls_back_global, 0, sizeof(pvcalls_back_global));
> > +	return -ENOMEM;
> > +}
> > +module_init(pvcalls_back_init);
> > 
> 
> Juergen
> 

[toc] | [prev] | [next] | [standalone]

#1642982 — Re: [PATCH 03/18] xen/pvcalls: initialize the module and register the xenbus backend

On 16/05/17 21:58, Stefano Stabellini wrote:
> On Tue, 16 May 2017, Juergen Gross wrote:
>> On 15/05/17 22:35, Stefano Stabellini wrote:
>>> The pvcalls backend has one ioworker per cpu: the ioworkers are
>>> implemented as a cpu bound workqueue, and will deal with the actual
>>> socket and data ring reads/writes.
>>>
>>> ioworkers are global: we only have one set for all the frontends. They
>>> process requests on their wqs list in order, once they are done with a
>>> request, they'll remove it from the list. A spinlock is used for
>>> protecting the list. Each ioworker is bound to a different cpu to
>>> maximize throughput.
>>>
>>> Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
>>> CC: boris.ostrovsky@oracle.com
>>> CC: jgross@suse.com
>>> ---
>>>  drivers/xen/pvcalls-back.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 64 insertions(+)
>>>
>>> diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
>>> index 2dbf7d8..46a889a 100644
>>> --- a/drivers/xen/pvcalls-back.c
>>> +++ b/drivers/xen/pvcalls-back.c
>>> @@ -25,6 +25,26 @@
>>>  #include <xen/xenbus.h>
>>>  #include <xen/interface/io/pvcalls.h>
>>>  
>>> +struct pvcalls_ioworker {
>>> +	struct work_struct register_work;
>>> +	atomic_t io;
>>> +	struct list_head wqs;
>>> +	spinlock_t lock;
>>> +	int num;
>>> +};
>>> +
>>> +struct pvcalls_back_global {
>>> +	struct pvcalls_ioworker *ioworkers;
>>> +	int nr_ioworkers;
>>> +	struct workqueue_struct *wq;
>>> +	struct list_head privs;
>>> +	struct rw_semaphore privs_lock;
>>> +} pvcalls_back_global;
>>> +
>>> +static void pvcalls_back_ioworker(struct work_struct *work)
>>> +{
>>> +}
>>> +
>>>  static int pvcalls_back_probe(struct xenbus_device *dev,
>>>  			      const struct xenbus_device_id *id)
>>>  {
>>> @@ -59,3 +79,47 @@ static int pvcalls_back_uevent(struct xenbus_device *xdev,
>>>  	.uevent = pvcalls_back_uevent,
>>>  	.otherend_changed = pvcalls_back_changed,
>>>  };
>>> +
>>> +static int __init pvcalls_back_init(void)
>>> +{
>>> +	int ret, i, cpu;
>>> +
>>> +	if (!xen_domain())
>>> +		return -ENODEV;
>>> +
>>> +	ret = xenbus_register_backend(&pvcalls_back_driver);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	init_rwsem(&pvcalls_back_global.privs_lock);
>>> +	INIT_LIST_HEAD(&pvcalls_back_global.privs);
>>> +	pvcalls_back_global.wq = alloc_workqueue("pvcalls_io", 0, 0);
>>> +	if (!pvcalls_back_global.wq)
>>> +		goto error;
>>> +	pvcalls_back_global.nr_ioworkers = num_online_cpus();
>>
>> Really? Recently I cam across a system with 640 dom0 cpus. I don't think
>> we want 640 workers initialized when loading the backend module. I'd
>> prefer one or a few workers per connected frontend.
> 
> I think we want to keep the ioworker allocation to be based on the
> number of vcpus: we do not want more ioworkers than vcpus because it is
> a waste of resources and leads to worse performance.  Also, given that
> they do memcpy's, I also think it is a good idea to bind them to vcpus
> (and pin vcpus to pcpus) to get best performance.

This will cause a lot of pain for the cpu offline case. Please don't try
to work against the hypervisor scheduler by designing a backend based on
a vcpu pin policy. This might result in best performance for your
special workload, but generally it is a bad idea!

> However, you have a point there: we need to handle systems with an
> extremely large number of Dom0 vcpus. I suggest we introduce an
> upper limit for the number of ioworkers. Something like:
> 
> #define MAX_IOWORKERS 64
> nr_ioworkers = min(MAX_IOWORKERS, num_online_cpus())
> 
> MAX_IOWORKERS could be configurable via a command line option.

Later you are assigning each active socket to exactly one ioworker.
Wouldn't it make more sense to allocate the ioworker when doing
the connect? This would avoid the problem of having only a statistical
distribution, possibly with all sockets on the same ioworker.

Basically you are re-inventing the wheel by using an own workqueue
implementation in each ioworker looping through all assigned sockets.


Juergen

[toc] | [prev] | [next] | [standalone]

#1644926 — Re: [PATCH 03/18] xen/pvcalls: initialize the module and register the xenbus backend

On Wed, 17 May 2017, Juergen Gross wrote:
> On 16/05/17 21:58, Stefano Stabellini wrote:
> > On Tue, 16 May 2017, Juergen Gross wrote:
> >> On 15/05/17 22:35, Stefano Stabellini wrote:
> >>> The pvcalls backend has one ioworker per cpu: the ioworkers are
> >>> implemented as a cpu bound workqueue, and will deal with the actual
> >>> socket and data ring reads/writes.
> >>>
> >>> ioworkers are global: we only have one set for all the frontends. They
> >>> process requests on their wqs list in order, once they are done with a
> >>> request, they'll remove it from the list. A spinlock is used for
> >>> protecting the list. Each ioworker is bound to a different cpu to
> >>> maximize throughput.
> >>>
> >>> Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> >>> CC: boris.ostrovsky@oracle.com
> >>> CC: jgross@suse.com
> >>> ---
> >>>  drivers/xen/pvcalls-back.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++
> >>>  1 file changed, 64 insertions(+)
> >>>
> >>> diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
> >>> index 2dbf7d8..46a889a 100644
> >>> --- a/drivers/xen/pvcalls-back.c
> >>> +++ b/drivers/xen/pvcalls-back.c
> >>> @@ -25,6 +25,26 @@
> >>>  #include <xen/xenbus.h>
> >>>  #include <xen/interface/io/pvcalls.h>
> >>>  
> >>> +struct pvcalls_ioworker {
> >>> +	struct work_struct register_work;
> >>> +	atomic_t io;
> >>> +	struct list_head wqs;
> >>> +	spinlock_t lock;
> >>> +	int num;
> >>> +};
> >>> +
> >>> +struct pvcalls_back_global {
> >>> +	struct pvcalls_ioworker *ioworkers;
> >>> +	int nr_ioworkers;
> >>> +	struct workqueue_struct *wq;
> >>> +	struct list_head privs;
> >>> +	struct rw_semaphore privs_lock;
> >>> +} pvcalls_back_global;
> >>> +
> >>> +static void pvcalls_back_ioworker(struct work_struct *work)
> >>> +{
> >>> +}
> >>> +
> >>>  static int pvcalls_back_probe(struct xenbus_device *dev,
> >>>  			      const struct xenbus_device_id *id)
> >>>  {
> >>> @@ -59,3 +79,47 @@ static int pvcalls_back_uevent(struct xenbus_device *xdev,
> >>>  	.uevent = pvcalls_back_uevent,
> >>>  	.otherend_changed = pvcalls_back_changed,
> >>>  };
> >>> +
> >>> +static int __init pvcalls_back_init(void)
> >>> +{
> >>> +	int ret, i, cpu;
> >>> +
> >>> +	if (!xen_domain())
> >>> +		return -ENODEV;
> >>> +
> >>> +	ret = xenbus_register_backend(&pvcalls_back_driver);
> >>> +	if (ret < 0)
> >>> +		return ret;
> >>> +
> >>> +	init_rwsem(&pvcalls_back_global.privs_lock);
> >>> +	INIT_LIST_HEAD(&pvcalls_back_global.privs);
> >>> +	pvcalls_back_global.wq = alloc_workqueue("pvcalls_io", 0, 0);
> >>> +	if (!pvcalls_back_global.wq)
> >>> +		goto error;
> >>> +	pvcalls_back_global.nr_ioworkers = num_online_cpus();
> >>
> >> Really? Recently I cam across a system with 640 dom0 cpus. I don't think
> >> we want 640 workers initialized when loading the backend module. I'd
> >> prefer one or a few workers per connected frontend.
> > 
> > I think we want to keep the ioworker allocation to be based on the
> > number of vcpus: we do not want more ioworkers than vcpus because it is
> > a waste of resources and leads to worse performance.  Also, given that
> > they do memcpy's, I also think it is a good idea to bind them to vcpus
> > (and pin vcpus to pcpus) to get best performance.
> 
> This will cause a lot of pain for the cpu offline case. Please don't try
> to work against the hypervisor scheduler by designing a backend based on
> a vcpu pin policy. This might result in best performance for your
> special workload, but generally it is a bad idea!

You are right. Of course, vcpu pinning is not a fundamental requirement
for this backend. I wrote about vcpu pinning only to help with the
explanation.

However, pvcalls is a memcpy based protocol and to perform memcpys
efficiently is very important to keep caches hot. The target is to hit
the same cacheline when reading and writing, which makes a huge
difference; it depends on processor and architecture but it is easily
20%. To get caching benefits, we need to do memcpys for the same socket
on the same vcpu (and on the same pcpu as well, that's why I mentioned
vcpu pinning, but we'll trust the Xen scheduler to do the right thing
when there is no contention).

This is why in this backend, regardless of the workqueue
design/allocation we use, I think we have to stick to two basic
principles:

- each socket is bound to one vcpu
- sockets are distributed evenly across vcpus


> > However, you have a point there: we need to handle systems with an
> > extremely large number of Dom0 vcpus. I suggest we introduce an
> > upper limit for the number of ioworkers. Something like:
> > 
> > #define MAX_IOWORKERS 64
> > nr_ioworkers = min(MAX_IOWORKERS, num_online_cpus())
> > 
> > MAX_IOWORKERS could be configurable via a command line option.
> 
> Later you are assigning each active socket to exactly one ioworker.
> Wouldn't it make more sense to allocate the ioworker when doing
> the connect? This would avoid the problem of having only a statistical
> distribution, possibly with all sockets on the same ioworker.
>
> Basically you are re-inventing the wheel by using an own workqueue
> implementation in each ioworker looping through all assigned sockets.

It might be possible to create an ioworker for each socket (instead of
an ioworker for each vcpu) if we wanted to, as long as we bind it to a
vcpu and distribute them evenly across vcpus.

[toc] | [prev] | [next] | [standalone]

#1645989 — Re: [PATCH 03/18] xen/pvcalls: initialize the module and register the xenbus backend

On Thu, 18 May 2017, Stefano Stabellini wrote:
> On Wed, 17 May 2017, Juergen Gross wrote:
> > On 16/05/17 21:58, Stefano Stabellini wrote:
> > > On Tue, 16 May 2017, Juergen Gross wrote:
> > >> On 15/05/17 22:35, Stefano Stabellini wrote:
> > >>> The pvcalls backend has one ioworker per cpu: the ioworkers are
> > >>> implemented as a cpu bound workqueue, and will deal with the actual
> > >>> socket and data ring reads/writes.
> > >>>
> > >>> ioworkers are global: we only have one set for all the frontends. They
> > >>> process requests on their wqs list in order, once they are done with a
> > >>> request, they'll remove it from the list. A spinlock is used for
> > >>> protecting the list. Each ioworker is bound to a different cpu to
> > >>> maximize throughput.
> > >>>
> > >>> Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> > >>> CC: boris.ostrovsky@oracle.com
> > >>> CC: jgross@suse.com
> > >>> ---
> > >>>  drivers/xen/pvcalls-back.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++
> > >>>  1 file changed, 64 insertions(+)
> > >>>
> > >>> diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
> > >>> index 2dbf7d8..46a889a 100644
> > >>> --- a/drivers/xen/pvcalls-back.c
> > >>> +++ b/drivers/xen/pvcalls-back.c
> > >>> @@ -25,6 +25,26 @@
> > >>>  #include <xen/xenbus.h>
> > >>>  #include <xen/interface/io/pvcalls.h>
> > >>>  
> > >>> +struct pvcalls_ioworker {
> > >>> +	struct work_struct register_work;
> > >>> +	atomic_t io;
> > >>> +	struct list_head wqs;
> > >>> +	spinlock_t lock;
> > >>> +	int num;
> > >>> +};
> > >>> +
> > >>> +struct pvcalls_back_global {
> > >>> +	struct pvcalls_ioworker *ioworkers;
> > >>> +	int nr_ioworkers;
> > >>> +	struct workqueue_struct *wq;
> > >>> +	struct list_head privs;
> > >>> +	struct rw_semaphore privs_lock;
> > >>> +} pvcalls_back_global;
> > >>> +
> > >>> +static void pvcalls_back_ioworker(struct work_struct *work)
> > >>> +{
> > >>> +}
> > >>> +
> > >>>  static int pvcalls_back_probe(struct xenbus_device *dev,
> > >>>  			      const struct xenbus_device_id *id)
> > >>>  {
> > >>> @@ -59,3 +79,47 @@ static int pvcalls_back_uevent(struct xenbus_device *xdev,
> > >>>  	.uevent = pvcalls_back_uevent,
> > >>>  	.otherend_changed = pvcalls_back_changed,
> > >>>  };
> > >>> +
> > >>> +static int __init pvcalls_back_init(void)
> > >>> +{
> > >>> +	int ret, i, cpu;
> > >>> +
> > >>> +	if (!xen_domain())
> > >>> +		return -ENODEV;
> > >>> +
> > >>> +	ret = xenbus_register_backend(&pvcalls_back_driver);
> > >>> +	if (ret < 0)
> > >>> +		return ret;
> > >>> +
> > >>> +	init_rwsem(&pvcalls_back_global.privs_lock);
> > >>> +	INIT_LIST_HEAD(&pvcalls_back_global.privs);
> > >>> +	pvcalls_back_global.wq = alloc_workqueue("pvcalls_io", 0, 0);
> > >>> +	if (!pvcalls_back_global.wq)
> > >>> +		goto error;
> > >>> +	pvcalls_back_global.nr_ioworkers = num_online_cpus();
> > >>
> > >> Really? Recently I cam across a system with 640 dom0 cpus. I don't think
> > >> we want 640 workers initialized when loading the backend module. I'd
> > >> prefer one or a few workers per connected frontend.
> > > 
> > > I think we want to keep the ioworker allocation to be based on the
> > > number of vcpus: we do not want more ioworkers than vcpus because it is
> > > a waste of resources and leads to worse performance.  Also, given that
> > > they do memcpy's, I also think it is a good idea to bind them to vcpus
> > > (and pin vcpus to pcpus) to get best performance.
> > 
> > This will cause a lot of pain for the cpu offline case. Please don't try
> > to work against the hypervisor scheduler by designing a backend based on
> > a vcpu pin policy. This might result in best performance for your
> > special workload, but generally it is a bad idea!
> 
> You are right. Of course, vcpu pinning is not a fundamental requirement
> for this backend. I wrote about vcpu pinning only to help with the
> explanation.
> 
> However, pvcalls is a memcpy based protocol and to perform memcpys
> efficiently is very important to keep caches hot. The target is to hit
> the same cacheline when reading and writing, which makes a huge
> difference; it depends on processor and architecture but it is easily
> 20%. To get caching benefits, we need to do memcpys for the same socket
> on the same vcpu (and on the same pcpu as well, that's why I mentioned
> vcpu pinning, but we'll trust the Xen scheduler to do the right thing
> when there is no contention).
> 
> This is why in this backend, regardless of the workqueue
> design/allocation we use, I think we have to stick to two basic
> principles:
> 
> - each socket is bound to one vcpu
> - sockets are distributed evenly across vcpus

[...]

> It might be possible to create an ioworker for each socket (instead of
> an ioworker for each vcpu) if we wanted to, as long as we bind it to a
> vcpu and distribute them evenly across vcpus.

I don't have access anymore to the large machine I used for the
benchmarks a few months back, but even on my current small testbox
I can see a (small) performance penalty when I don't bind ioworkers to
specific vcpus.

However, using an ioworker per socket, rather than per vcpu, makes the
code much smaller and nicer! Also it makes it much easier to change the
policy between binding, or not binding, ioworkers to specific vcpu.

[toc] | [prev] | [next] | [standalone]

#1642034 — [PATCH 10/18] xen/pvcalls: implement listen command

Call inet_listen to implement the listen command.

Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: boris.ostrovsky@oracle.com
CC: jgross@suse.com
---
 drivers/xen/pvcalls-back.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
index ff4634d..a762877 100644
--- a/drivers/xen/pvcalls-back.c
+++ b/drivers/xen/pvcalls-back.c
@@ -345,7 +345,28 @@ static int pvcalls_back_bind(struct xenbus_device *dev,
 static int pvcalls_back_listen(struct xenbus_device *dev,
 			       struct xen_pvcalls_request *req)
 {
-	return 0;
+	struct pvcalls_back_priv *priv;
+	int ret = -EINVAL;
+	struct sockpass_mapping *map;
+	struct xen_pvcalls_response *rsp;
+
+	if (dev == NULL)
+		return 0;
+	priv = dev_get_drvdata(&dev->dev);
+
+	map = radix_tree_lookup(&priv->socketpass_mappings, req->u.listen.id);
+	if (map == NULL)
+		goto out;
+
+	ret = inet_listen(map->sock, req->u.listen.backlog);
+
+out:
+	rsp = RING_GET_RESPONSE(&priv->ring, priv->ring.rsp_prod_pvt++);
+	rsp->req_id = req->req_id;
+	rsp->cmd = req->cmd;
+	rsp->u.listen.id = req->u.listen.id;
+	rsp->ret = ret;
+	return 1;
 }
 
 static int pvcalls_back_accept(struct xenbus_device *dev,
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1642035 — [PATCH 13/18] xen/pvcalls: implement release command

Release both active and passive sockets. For active sockets, make sure
to avoid possible conflicts with the ioworker reading/writing to those
sockets concurrently. Set map->release to let the ioworker know
atomically that the socket will be released soon, then wait until the
ioworker removed the socket from its list.

Unmap indexes pages and data rings.

Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: boris.ostrovsky@oracle.com
CC: jgross@suse.com
---
 drivers/xen/pvcalls-back.c | 94 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 93 insertions(+), 1 deletion(-)

diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
index d5b7412..22c6426 100644
--- a/drivers/xen/pvcalls-back.c
+++ b/drivers/xen/pvcalls-back.c
@@ -253,13 +253,105 @@ static int pvcalls_back_release_active(struct xenbus_device *dev,
 				       struct pvcalls_back_priv *priv,
 				       struct sock_mapping *map)
 {
+	struct pvcalls_ioworker *iow =
+	    &pvcalls_back_global.ioworkers[map->data_worker];
+	unsigned long flags;
+	bool in_loop = false;
+
+
+	disable_irq(map->irq);
+	if (map->sock->sk != NULL) {
+		lock_sock(map->sock->sk);
+		map->sock->sk->sk_user_data = NULL;
+		map->sock->sk->sk_data_ready = map->saved_data_ready;
+		release_sock(map->sock->sk);
+	}
+
+	atomic_set(&map->release, 1);
+
+	/*
+	 * To avoid concurrency problems with ioworker, check if the socket
+	 * has any outstanding io requests. If so, wait until the ioworker
+	 * removes it from the list before proceeding.
+	 */
+	spin_lock_irqsave(&iow->lock, flags);
+	in_loop = !list_empty(&map->queue);
+	spin_unlock_irqrestore(&iow->lock, flags);
+
+	if (in_loop) {
+		atomic_inc(&iow->io);
+		queue_work_on(map->data_worker, pvcalls_back_global.wq,
+			      &iow->register_work);
+		while (atomic_read(&map->release) > 0)
+			cond_resched();
+	}
+
+	down_write(&priv->pvcallss_lock);
+	list_del(&map->list);
+	up_write(&priv->pvcallss_lock);
+
+	xenbus_unmap_ring_vfree(dev, (void *)map->bytes);
+	xenbus_unmap_ring_vfree(dev, (void *)map->ring);
+	unbind_from_irqhandler(map->irq, map);
+
+	sock_release(map->sock);
+	kfree(map);
+
+	return 0;
+}
+
+static int pvcalls_back_release_passive(struct xenbus_device *dev,
+					struct pvcalls_back_priv *priv,
+					struct sockpass_mapping *mappass)
+{
+	if (mappass->sock->sk != NULL) {
+		lock_sock(mappass->sock->sk);
+		mappass->sock->sk->sk_user_data = NULL;
+		mappass->sock->sk->sk_data_ready = mappass->saved_data_ready;
+		release_sock(mappass->sock->sk);
+	}
+	down_write(&priv->pvcallss_lock);
+	radix_tree_delete(&priv->socketpass_mappings, mappass->id);
+	sock_release(mappass->sock);
+	flush_workqueue(mappass->wq);
+	destroy_workqueue(mappass->wq);
+	kfree(mappass);
+	up_write(&priv->pvcallss_lock);
+
 	return 0;
 }
 
 static int pvcalls_back_release(struct xenbus_device *dev,
 				struct xen_pvcalls_request *req)
 {
-	return 0;
+	struct pvcalls_back_priv *priv;
+	struct sock_mapping *map, *n;
+	struct sockpass_mapping *mappass;
+	int ret = 0;
+	struct xen_pvcalls_response *rsp;
+
+	priv = dev_get_drvdata(&dev->dev);
+
+	list_for_each_entry_safe(map, n, &priv->socket_mappings, list) {
+		if (map->id == req->u.release.id) {
+			ret = pvcalls_back_release_active(dev, priv, map);
+			goto out;
+		}
+	}
+	mappass = radix_tree_lookup(&priv->socketpass_mappings,
+				    req->u.release.id);
+	if (mappass != NULL) {
+		ret = pvcalls_back_release_passive(dev, priv, mappass);
+		goto out;
+	}
+
+out:
+	rsp = RING_GET_RESPONSE(&priv->ring, priv->ring.rsp_prod_pvt++);
+	rsp->req_id = req->req_id;
+	rsp->u.release.id = req->u.release.id;
+	rsp->cmd = req->cmd;
+	rsp->ret = ret;
+	return 1;
 }
 
 static void __pvcalls_back_accept(struct work_struct *work)
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1642037 — [PATCH 06/18] xen/pvcalls: handle commands from the frontend

When the other end notifies us that there are commands to be read
(pvcalls_back_event), wake up the backend thread to parse the command.

The command ring works like most other Xen rings, so use the usual
ring macros to read and write to it. The functions implementing the
commands are empty stubs for now.

Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: boris.ostrovsky@oracle.com
CC: jgross@suse.com
---
 drivers/xen/pvcalls-back.c | 115 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 115 insertions(+)

diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
index 876e577..2b2a49a 100644
--- a/drivers/xen/pvcalls-back.c
+++ b/drivers/xen/pvcalls-back.c
@@ -62,12 +62,127 @@ static void pvcalls_back_ioworker(struct work_struct *work)
 {
 }
 
+static int pvcalls_back_socket(struct xenbus_device *dev,
+		struct xen_pvcalls_request *req)
+{
+	return 0;
+}
+
+static int pvcalls_back_connect(struct xenbus_device *dev,
+				struct xen_pvcalls_request *req)
+{
+	return 0;
+}
+
+static int pvcalls_back_release(struct xenbus_device *dev,
+				struct xen_pvcalls_request *req)
+{
+	return 0;
+}
+
+static int pvcalls_back_bind(struct xenbus_device *dev,
+			     struct xen_pvcalls_request *req)
+{
+	return 0;
+}
+
+static int pvcalls_back_listen(struct xenbus_device *dev,
+			       struct xen_pvcalls_request *req)
+{
+	return 0;
+}
+
+static int pvcalls_back_accept(struct xenbus_device *dev,
+			       struct xen_pvcalls_request *req)
+{
+	return 0;
+}
+
+static int pvcalls_back_poll(struct xenbus_device *dev,
+			     struct xen_pvcalls_request *req)
+{
+	return 0;
+}
+
+static int pvcalls_back_handle_cmd(struct xenbus_device *dev,
+				   struct xen_pvcalls_request *req)
+{
+	int ret = 0;
+
+	switch (req->cmd) {
+	case PVCALLS_SOCKET:
+		ret = pvcalls_back_socket(dev, req);
+		break;
+	case PVCALLS_CONNECT:
+		ret = pvcalls_back_connect(dev, req);
+		break;
+	case PVCALLS_RELEASE:
+		ret = pvcalls_back_release(dev, req);
+		break;
+	case PVCALLS_BIND:
+		ret = pvcalls_back_bind(dev, req);
+		break;
+	case PVCALLS_LISTEN:
+		ret = pvcalls_back_listen(dev, req);
+		break;
+	case PVCALLS_ACCEPT:
+		ret = pvcalls_back_accept(dev, req);
+		break;
+	case PVCALLS_POLL:
+		ret = pvcalls_back_poll(dev, req);
+		break;
+	default:
+		ret = -ENOTSUPP;
+		break;
+	}
+	return ret;
+}
+
 static void pvcalls_back_work(struct work_struct *work)
 {
+	struct pvcalls_back_priv *priv = container_of(work,
+		struct pvcalls_back_priv, register_work);
+	int notify, notify_all = 0, more = 1;
+	struct xen_pvcalls_request req;
+	struct xenbus_device *dev = priv->dev;
+
+	atomic_set(&priv->work, 1);
+
+	while (more || !atomic_dec_and_test(&priv->work)) {
+		while (RING_HAS_UNCONSUMED_REQUESTS(&priv->ring)) {
+			RING_COPY_REQUEST(&priv->ring,
+					  priv->ring.req_cons++,
+					  &req);
+
+			if (pvcalls_back_handle_cmd(dev, &req) > 0) {
+				RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(
+					&priv->ring, notify);
+				notify_all += notify;
+			}
+		}
+
+		if (notify_all)
+			notify_remote_via_irq(priv->irq);
+
+		RING_FINAL_CHECK_FOR_REQUESTS(&priv->ring, more);
+	}
 }
 
 static irqreturn_t pvcalls_back_event(int irq, void *dev_id)
 {
+	struct xenbus_device *dev = dev_id;
+	struct pvcalls_back_priv *priv = NULL;
+
+	if (dev == NULL)
+		return IRQ_HANDLED;
+
+	priv = dev_get_drvdata(&dev->dev);
+	if (priv == NULL)
+		return IRQ_HANDLED;
+
+	atomic_inc(&priv->work);
+	queue_work(priv->wq, &priv->register_work);
+
 	return IRQ_HANDLED;
 }
 
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1642185 — Re: [PATCH 06/18] xen/pvcalls: handle commands from the frontend

On 05/15/2017 04:35 PM, Stefano Stabellini wrote:
> When the other end notifies us that there are commands to be read
> (pvcalls_back_event), wake up the backend thread to parse the command.
>
> The command ring works like most other Xen rings, so use the usual
> ring macros to read and write to it. The functions implementing the
> commands are empty stubs for now.
>
> Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> CC: boris.ostrovsky@oracle.com
> CC: jgross@suse.com
> ---
>  drivers/xen/pvcalls-back.c | 115 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 115 insertions(+)
>
> diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
> index 876e577..2b2a49a 100644
> --- a/drivers/xen/pvcalls-back.c
> +++ b/drivers/xen/pvcalls-back.c
> @@ -62,12 +62,127 @@ static void pvcalls_back_ioworker(struct work_struct *work)
>  {
>  }
>
> +static int pvcalls_back_socket(struct xenbus_device *dev,
> +		struct xen_pvcalls_request *req)
> +{
> +	return 0;
> +}
> +
> +static int pvcalls_back_connect(struct xenbus_device *dev,
> +				struct xen_pvcalls_request *req)
> +{
> +	return 0;
> +}
> +
> +static int pvcalls_back_release(struct xenbus_device *dev,
> +				struct xen_pvcalls_request *req)
> +{
> +	return 0;
> +}
> +
> +static int pvcalls_back_bind(struct xenbus_device *dev,
> +			     struct xen_pvcalls_request *req)
> +{
> +	return 0;
> +}
> +
> +static int pvcalls_back_listen(struct xenbus_device *dev,
> +			       struct xen_pvcalls_request *req)
> +{
> +	return 0;
> +}
> +
> +static int pvcalls_back_accept(struct xenbus_device *dev,
> +			       struct xen_pvcalls_request *req)
> +{
> +	return 0;
> +}
> +
> +static int pvcalls_back_poll(struct xenbus_device *dev,
> +			     struct xen_pvcalls_request *req)
> +{
> +	return 0;
> +}
> +
> +static int pvcalls_back_handle_cmd(struct xenbus_device *dev,
> +				   struct xen_pvcalls_request *req)
> +{
> +	int ret = 0;
> +
> +	switch (req->cmd) {
> +	case PVCALLS_SOCKET:
> +		ret = pvcalls_back_socket(dev, req);
> +		break;
> +	case PVCALLS_CONNECT:
> +		ret = pvcalls_back_connect(dev, req);
> +		break;
> +	case PVCALLS_RELEASE:
> +		ret = pvcalls_back_release(dev, req);
> +		break;
> +	case PVCALLS_BIND:
> +		ret = pvcalls_back_bind(dev, req);
> +		break;
> +	case PVCALLS_LISTEN:
> +		ret = pvcalls_back_listen(dev, req);
> +		break;
> +	case PVCALLS_ACCEPT:
> +		ret = pvcalls_back_accept(dev, req);
> +		break;
> +	case PVCALLS_POLL:
> +		ret = pvcalls_back_poll(dev, req);
> +		break;
> +	default:
> +		ret = -ENOTSUPP;
> +		break;
> +	}
> +	return ret;
> +}
> +
>  static void pvcalls_back_work(struct work_struct *work)
>  {
> +	struct pvcalls_back_priv *priv = container_of(work,
> +		struct pvcalls_back_priv, register_work);
> +	int notify, notify_all = 0, more = 1;
> +	struct xen_pvcalls_request req;
> +	struct xenbus_device *dev = priv->dev;
> +
> +	atomic_set(&priv->work, 1);
> +
> +	while (more || !atomic_dec_and_test(&priv->work)) {
> +		while (RING_HAS_UNCONSUMED_REQUESTS(&priv->ring)) {
> +			RING_COPY_REQUEST(&priv->ring,
> +					  priv->ring.req_cons++,
> +					  &req);
> +
> +			if (pvcalls_back_handle_cmd(dev, &req) > 0) {

Can you make handlers make "traditional" returns, i.e. <0 on error and 0 
on success? Or do you really need to distinguish 0 from >0?

> +				RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(
> +					&priv->ring, notify);
> +				notify_all += notify;
> +			}
> +		}
> +
> +		if (notify_all)
> +			notify_remote_via_irq(priv->irq);
> +
> +		RING_FINAL_CHECK_FOR_REQUESTS(&priv->ring, more);
> +	}
>  }
>
>  static irqreturn_t pvcalls_back_event(int irq, void *dev_id)
>  {
> +	struct xenbus_device *dev = dev_id;
> +	struct pvcalls_back_priv *priv = NULL;
> +
> +	if (dev == NULL)
> +		return IRQ_HANDLED;
> +
> +	priv = dev_get_drvdata(&dev->dev);
> +	if (priv == NULL)
> +		return IRQ_HANDLED;

These two aren't errors?

> +
> +	atomic_inc(&priv->work);

Is this really needed? We have a new entry on the ring, so the outer 
loop in pvcalls_back_work() will pick this up (by setting 'more').


-boris

> +	queue_work(priv->wq, &priv->register_work);
> +
>  	return IRQ_HANDLED;
>  }
>
>

[toc] | [prev] | [next] | [standalone]

#1642825 — Re: [PATCH 06/18] xen/pvcalls: handle commands from the frontend

On Mon, 15 May 2017, Boris Ostrovsky wrote:
> On 05/15/2017 04:35 PM, Stefano Stabellini wrote:
> > When the other end notifies us that there are commands to be read
> > (pvcalls_back_event), wake up the backend thread to parse the command.
> > 
> > The command ring works like most other Xen rings, so use the usual
> > ring macros to read and write to it. The functions implementing the
> > commands are empty stubs for now.
> > 
> > Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> > CC: boris.ostrovsky@oracle.com
> > CC: jgross@suse.com
> > ---
> >  drivers/xen/pvcalls-back.c | 115
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 115 insertions(+)
> > 
> > diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
> > index 876e577..2b2a49a 100644
> > --- a/drivers/xen/pvcalls-back.c
> > +++ b/drivers/xen/pvcalls-back.c
> > @@ -62,12 +62,127 @@ static void pvcalls_back_ioworker(struct work_struct
> > *work)
> >  {
> >  }
> > 
> > +static int pvcalls_back_socket(struct xenbus_device *dev,
> > +		struct xen_pvcalls_request *req)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int pvcalls_back_connect(struct xenbus_device *dev,
> > +				struct xen_pvcalls_request *req)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int pvcalls_back_release(struct xenbus_device *dev,
> > +				struct xen_pvcalls_request *req)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int pvcalls_back_bind(struct xenbus_device *dev,
> > +			     struct xen_pvcalls_request *req)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int pvcalls_back_listen(struct xenbus_device *dev,
> > +			       struct xen_pvcalls_request *req)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int pvcalls_back_accept(struct xenbus_device *dev,
> > +			       struct xen_pvcalls_request *req)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int pvcalls_back_poll(struct xenbus_device *dev,
> > +			     struct xen_pvcalls_request *req)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int pvcalls_back_handle_cmd(struct xenbus_device *dev,
> > +				   struct xen_pvcalls_request *req)
> > +{
> > +	int ret = 0;
> > +
> > +	switch (req->cmd) {
> > +	case PVCALLS_SOCKET:
> > +		ret = pvcalls_back_socket(dev, req);
> > +		break;
> > +	case PVCALLS_CONNECT:
> > +		ret = pvcalls_back_connect(dev, req);
> > +		break;
> > +	case PVCALLS_RELEASE:
> > +		ret = pvcalls_back_release(dev, req);
> > +		break;
> > +	case PVCALLS_BIND:
> > +		ret = pvcalls_back_bind(dev, req);
> > +		break;
> > +	case PVCALLS_LISTEN:
> > +		ret = pvcalls_back_listen(dev, req);
> > +		break;
> > +	case PVCALLS_ACCEPT:
> > +		ret = pvcalls_back_accept(dev, req);
> > +		break;
> > +	case PVCALLS_POLL:
> > +		ret = pvcalls_back_poll(dev, req);
> > +		break;
> > +	default:
> > +		ret = -ENOTSUPP;
> > +		break;
> > +	}
> > +	return ret;
> > +}
> > +
> >  static void pvcalls_back_work(struct work_struct *work)
> >  {
> > +	struct pvcalls_back_priv *priv = container_of(work,
> > +		struct pvcalls_back_priv, register_work);
> > +	int notify, notify_all = 0, more = 1;
> > +	struct xen_pvcalls_request req;
> > +	struct xenbus_device *dev = priv->dev;
> > +
> > +	atomic_set(&priv->work, 1);
> > +
> > +	while (more || !atomic_dec_and_test(&priv->work)) {
> > +		while (RING_HAS_UNCONSUMED_REQUESTS(&priv->ring)) {
> > +			RING_COPY_REQUEST(&priv->ring,
> > +					  priv->ring.req_cons++,
> > +					  &req);
> > +
> > +			if (pvcalls_back_handle_cmd(dev, &req) > 0) {
> 
> Can you make handlers make "traditional" returns, i.e. <0 on error and 0 on
> success? Or do you really need to distinguish 0 from >0?

Today < 0 means error, 0 means OK but no notifications required, 1 means
OK with notifications. Given that errors are returned to the other end
using the appropriate response field (we don't do anything with an error
in pvcalls_back_work), I could change this to:

-1: no need for notifications (both errors and regular conditions)
0:  notifications


> > +				RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(
> > +					&priv->ring, notify);
> > +				notify_all += notify;
> > +			}
> > +		}
> > +
> > +		if (notify_all)
> > +			notify_remote_via_irq(priv->irq);
> > +
> > +		RING_FINAL_CHECK_FOR_REQUESTS(&priv->ring, more);
> > +	}
> >  }
> > 
> >  static irqreturn_t pvcalls_back_event(int irq, void *dev_id)
> >  {
> > +	struct xenbus_device *dev = dev_id;
> > +	struct pvcalls_back_priv *priv = NULL;
> > +
> > +	if (dev == NULL)
> > +		return IRQ_HANDLED;
> > +
> > +	priv = dev_get_drvdata(&dev->dev);
> > +	if (priv == NULL)
> > +		return IRQ_HANDLED;
> 
> These two aren't errors?

They are meant to handle spurious event notifications. From the Linux
irq handling subsystem point of view, they are not errors.


> > +
> > +	atomic_inc(&priv->work);
> 
> Is this really needed? We have a new entry on the ring, so the outer loop in
> pvcalls_back_work() will pick this up (by setting 'more').

This is to avoid race conditions. A notification could be delivered
after RING_FINAL_CHECK_FOR_REQUESTS is called, returning more == 0, but
before pvcalls_back_work completes. In that case, without priv->work,
pvcalls_back_work wouldn't be rescheduled because it is still running
and the work would be left undone.


> > +	queue_work(priv->wq, &priv->register_work);
> > +
> >  	return IRQ_HANDLED;
> >  }

[toc] | [prev] | [next] | [standalone]

#1642038 — [PATCH 07/18] xen/pvcalls: implement socket command

Just reply with success to the other end for now. Delay the allocation
of the actual socket to bind and/or connect.

Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: boris.ostrovsky@oracle.com
CC: jgross@suse.com
---
 drivers/xen/pvcalls-back.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
index 2b2a49a..2eae096 100644
--- a/drivers/xen/pvcalls-back.c
+++ b/drivers/xen/pvcalls-back.c
@@ -12,12 +12,17 @@
  * GNU General Public License for more details.
  */
 
+#include <linux/inet.h>
 #include <linux/kthread.h>
 #include <linux/list.h>
 #include <linux/radix-tree.h>
 #include <linux/module.h>
 #include <linux/rwsem.h>
 #include <linux/wait.h>
+#include <net/sock.h>
+#include <net/inet_common.h>
+#include <net/inet_connection_sock.h>
+#include <net/request_sock.h>
 
 #include <xen/events.h>
 #include <xen/grant_table.h>
@@ -65,7 +70,31 @@ static void pvcalls_back_ioworker(struct work_struct *work)
 static int pvcalls_back_socket(struct xenbus_device *dev,
 		struct xen_pvcalls_request *req)
 {
-	return 0;
+	struct pvcalls_back_priv *priv;
+	int ret;
+	struct xen_pvcalls_response *rsp;
+
+	if (dev == NULL)
+		return 0;
+	priv = dev_get_drvdata(&dev->dev);
+
+	if (req->u.socket.domain != AF_INET ||
+	    req->u.socket.type != SOCK_STREAM ||
+	    (req->u.socket.protocol != 0 &&
+	     req->u.socket.protocol != AF_INET))
+		ret = -EAFNOSUPPORT;
+	else
+		ret = 0;
+
+	/* leave the actual socket allocation for later */
+
+	rsp = RING_GET_RESPONSE(&priv->ring, priv->ring.rsp_prod_pvt++);
+	rsp->req_id = req->req_id;
+	rsp->cmd = req->cmd;
+	rsp->u.socket.id = req->u.socket.id;
+	rsp->ret = ret;
+
+	return 1;
 }
 
 static int pvcalls_back_connect(struct xenbus_device *dev,
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web