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

Groups > linux.kernel > #1696538 > unrolled thread

[PATCH v2 00/13] introduce the Xen PV Calls frontend

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 00/13] introduce the Xen PV Calls frontend Stefano Stabellini <sstabellini@kernel.org> - 2017-07-25 23:30 +0200
    [PATCH v2 13/13] xen: introduce a Kconfig option to enable the pvcalls frontend Stefano Stabellini <sstabellini@kernel.org> - 2017-07-25 23:30 +0200
    [PATCH v2 10/13] xen/pvcalls: implement poll command Stefano Stabellini <sstabellini@kernel.org> - 2017-07-25 23:30 +0200
      Re: [PATCH v2 10/13] xen/pvcalls: implement poll command Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-07-27 01:30 +0200
        Re: [PATCH v2 10/13] xen/pvcalls: implement poll command Stefano Stabellini <sstabellini@kernel.org> - 2017-07-27 02:30 +0200
    [PATCH v2 06/13] xen/pvcalls: implement listen command Stefano Stabellini <sstabellini@kernel.org> - 2017-07-25 23:30 +0200
    [PATCH v2 07/13] xen/pvcalls: implement accept command Stefano Stabellini <sstabellini@kernel.org> - 2017-07-25 23:30 +0200
      Re: [PATCH v2 07/13] xen/pvcalls: implement accept command Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-07-26 20:00 +0200
        Re: [PATCH v2 07/13] xen/pvcalls: implement accept command Stefano Stabellini <sstabellini@kernel.org> - 2017-07-27 01:30 +0200
    [PATCH v2 01/13] xen/pvcalls: introduce the pvcalls xenbus frontend Stefano Stabellini <sstabellini@kernel.org> - 2017-07-25 23:30 +0200
      [PATCH v2 12/13] xen/pvcalls: implement frontend disconnect Stefano Stabellini <sstabellini@kernel.org> - 2017-07-25 23:30 +0200
      [PATCH v2 05/13] xen/pvcalls: implement bind command Stefano Stabellini <sstabellini@kernel.org> - 2017-07-25 23:30 +0200
        Re: [PATCH v2 05/13] xen/pvcalls: implement bind command Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-07-26 17:00 +0200
          Re: [PATCH v2 05/13] xen/pvcalls: implement bind command Stefano Stabellini <sstabellini@kernel.org> - 2017-07-27 02:00 +0200
            Re: [PATCH v2 05/13] xen/pvcalls: implement bind command Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-07-27 16:50 +0200
      [PATCH v2 04/13] xen/pvcalls: implement connect command Stefano Stabellini <sstabellini@kernel.org> - 2017-07-25 23:30 +0200
        Re: [PATCH v2 04/13] xen/pvcalls: implement connect command Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-07-26 17:00 +0200
          Re: [PATCH v2 04/13] xen/pvcalls: implement connect command Stefano Stabellini <sstabellini@kernel.org> - 2017-07-27 01:30 +0200
      [PATCH v2 09/13] xen/pvcalls: implement recvmsg Stefano Stabellini <sstabellini@kernel.org> - 2017-07-25 23:30 +0200
        Re: [PATCH v2 09/13] xen/pvcalls: implement recvmsg Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-07-26 23:30 +0200
          Re: [Xen-devel] [PATCH v2 09/13] xen/pvcalls: implement recvmsg Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-07-26 23:40 +0200
            Re: [Xen-devel] [PATCH v2 09/13] xen/pvcalls: implement recvmsg Stefano Stabellini <sstabellini@kernel.org> - 2017-07-27 02:10 +0200
              Re: [Xen-devel] [PATCH v2 09/13] xen/pvcalls: implement recvmsg Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-07-27 17:00 +0200
      [PATCH v2 11/13] xen/pvcalls: implement release command Stefano Stabellini <sstabellini@kernel.org> - 2017-07-25 23:30 +0200
        Re: [PATCH v2 11/13] xen/pvcalls: implement release command Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-07-27 20:40 +0200
      [PATCH v2 02/13] xen/pvcalls: connect to the backend Stefano Stabellini <sstabellini@kernel.org> - 2017-07-25 23:30 +0200
        Re: [PATCH v2 02/13] xen/pvcalls: connect to the backend Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-07-26 15:40 +0200
          Re: [PATCH v2 02/13] xen/pvcalls: connect to the backend Stefano Stabellini <sstabellini@kernel.org> - 2017-07-27 02:30 +0200
            Re: [PATCH v2 02/13] xen/pvcalls: connect to the backend Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-07-27 17:10 +0200
      [PATCH v2 03/13] xen/pvcalls: implement socket command and handle events Stefano Stabellini <sstabellini@kernel.org> - 2017-07-25 23:30 +0200
        Re: [PATCH v2 03/13] xen/pvcalls: implement socket command and handle  events Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-07-26 16:30 +0200
          Re: [PATCH v2 03/13] xen/pvcalls: implement socket command and handle  events Stefano Stabellini <sstabellini@kernel.org> - 2017-07-27 01:20 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1697576 — Re: [Xen-devel] [PATCH v2 09/13] xen/pvcalls: implement recvmsg

>> +	while (!(flags & MSG_DONTWAIT) && !pvcalls_front_read_todo(map)) {
>> +		if (count < PVCALLS_FRONT_MAX_SPIN)
>> +			count++;
>> +		else
>> +			wait_event_interruptible(map->active.inflight_conn_req,
>> +						 pvcalls_front_read_todo(map));
>> +	}
> Should we be using PVCALLS_FRONT_MAX_SPIN here? In sendmsg it is
> counting non-sleeping iterations but here we are sleeping so
> PVCALLS_FRONT_MAX_SPIN (5000) may take a while.
>
> In fact, what shouldn't this waiting be a function of MSG_DONTWAIT

err, which it already is. But the question still stands (except for
MSG_DONTWAIT).

-boris

> and/or socket's O_NONBLOCK?

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

#1697638 — Re: [Xen-devel] [PATCH v2 09/13] xen/pvcalls: implement recvmsg

On Wed, 26 Jul 2017, Boris Ostrovsky wrote:
> >> +			count++;
> >> +		else
> >> +			wait_event_interruptible(map->active.inflight_conn_req,
> >> +						 pvcalls_front_read_todo(map));
> >> +	}
> > Should we be using PVCALLS_FRONT_MAX_SPIN here? In sendmsg it is
> > counting non-sleeping iterations but here we are sleeping so
> > PVCALLS_FRONT_MAX_SPIN (5000) may take a while.
> >
> > In fact, what shouldn't this waiting be a function of MSG_DONTWAIT
> 
> err, which it already is. But the question still stands (except for
> MSG_DONTWAIT).

The code (admittedly unintuitive) is busy-looping (non-sleeping) for
5000 iterations *before* attempting to sleep. So in that regard, recvmsg
and sendmsg use PVCALLS_FRONT_MAX_SPIN in the same way: only for
non-sleeping iterations.

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

#1698069 — Re: [Xen-devel] [PATCH v2 09/13] xen/pvcalls: implement recvmsg

On 07/26/2017 08:08 PM, Stefano Stabellini wrote:
> On Wed, 26 Jul 2017, Boris Ostrovsky wrote:
>>>> +			count++;
>>>> +		else
>>>> +			wait_event_interruptible(map->active.inflight_conn_req,
>>>> +						 pvcalls_front_read_todo(map));
>>>> +	}
>>> Should we be using PVCALLS_FRONT_MAX_SPIN here? In sendmsg it is
>>> counting non-sleeping iterations but here we are sleeping so
>>> PVCALLS_FRONT_MAX_SPIN (5000) may take a while.
>>>
>>> In fact, what shouldn't this waiting be a function of MSG_DONTWAIT
>> err, which it already is. But the question still stands (except for
>> MSG_DONTWAIT).
> The code (admittedly unintuitive) is busy-looping (non-sleeping) for
> 5000 iterations *before* attempting to sleep. So in that regard, recvmsg
> and sendmsg use PVCALLS_FRONT_MAX_SPIN in the same way: only for
> non-sleeping iterations.
>

OK.

Why not go directly into wait_event_interruptible()? I see you write in
the commit message

If not enough data is available on the ring, rather than returning
immediately or sleep-waiting, spin for up to 5000 cycles. This small
optimization turns out to improve performance and latency significantly.


Is this because of scheduling latency? I think this should be mentioned not just in the commit message but also as a comment in the code.

(I also think it's not "not enough data" but rather "no data"?)



-boris

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

#1696561 — [PATCH v2 11/13] xen/pvcalls: implement release command

Send PVCALLS_RELEASE to the backend and wait for a reply. Take both
in_mutex and out_mutex to avoid concurrent accesses. Then, free the
socket.

Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: boris.ostrovsky@oracle.com
CC: jgross@suse.com
---
 drivers/xen/pvcalls-front.c | 85 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/xen/pvcalls-front.h |  1 +
 2 files changed, 86 insertions(+)

diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c
index 833b717..5a4040e 100644
--- a/drivers/xen/pvcalls-front.c
+++ b/drivers/xen/pvcalls-front.c
@@ -184,6 +184,23 @@ static irqreturn_t pvcalls_front_conn_handler(int irq, void *sock_map)
 	return IRQ_HANDLED;
 }
 
+static void pvcalls_front_free_map(struct pvcalls_bedata *bedata,
+				   struct sock_mapping *map)
+{
+	int i;
+
+	spin_lock(&bedata->pvcallss_lock);
+	if (!list_empty(&map->list))
+		list_del_init(&map->list);
+	spin_unlock(&bedata->pvcallss_lock);
+
+	for (i = 0; i < (1 << map->active.ring->ring_order); i++)
+		gnttab_end_foreign_access(map->active.ring->ref[i], 0, 0);
+	gnttab_end_foreign_access(map->active.ref, 0, 0);
+	free_page((unsigned long)map->active.ring);
+	unbind_from_irqhandler(map->active.irq, map);
+}
+
 int pvcalls_front_socket(struct socket *sock)
 {
 	struct pvcalls_bedata *bedata;
@@ -819,6 +836,74 @@ unsigned int pvcalls_front_poll(struct file *file, struct socket *sock,
 		return pvcalls_front_poll_passive(file, bedata, map, wait);
 }
 
+int pvcalls_front_release(struct socket *sock)
+{
+	struct pvcalls_bedata *bedata;
+	struct sock_mapping *map;
+	int req_id, notify;
+	struct xen_pvcalls_request *req;
+
+	if (!pvcalls_front_dev)
+		return -EIO;
+	bedata = dev_get_drvdata(&pvcalls_front_dev->dev);
+	if (!bedata)
+		return -EIO;
+
+	if (sock->sk == NULL)
+		return 0;
+
+	map = (struct sock_mapping *) READ_ONCE(sock->sk->sk_send_head);
+	if (map == NULL)
+		return 0;
+
+	spin_lock(&bedata->pvcallss_lock);
+	req_id = bedata->ring.req_prod_pvt & (RING_SIZE(&bedata->ring) - 1);
+	if (RING_FULL(&bedata->ring) ||
+	    READ_ONCE(bedata->rsp[req_id].req_id) != PVCALLS_INVALID_ID) {
+		spin_unlock(&bedata->pvcallss_lock);
+		return -EAGAIN;
+	}
+	WRITE_ONCE(sock->sk->sk_send_head, NULL);
+
+	req = RING_GET_REQUEST(&bedata->ring, req_id);
+	req->req_id = req_id;
+	req->cmd = PVCALLS_RELEASE;
+	req->u.release.id = (uint64_t)sock;
+
+	bedata->ring.req_prod_pvt++;
+	RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(&bedata->ring, notify);
+	spin_unlock(&bedata->pvcallss_lock);
+	if (notify)
+		notify_remote_via_irq(bedata->irq);
+
+	wait_event(bedata->inflight_req,
+		READ_ONCE(bedata->rsp[req_id].req_id) == req_id);
+
+	if (map->active_socket) {
+		/* 
+		 * Set in_error and wake up inflight_conn_req to force
+		 * recvmsg waiters to exit.
+		 */
+		map->active.ring->in_error = -EBADF;
+		wake_up_interruptible(&map->active.inflight_conn_req);
+
+		mutex_lock(&map->active.in_mutex);
+		mutex_lock(&map->active.out_mutex);
+		pvcalls_front_free_map(bedata, map);
+		mutex_unlock(&map->active.out_mutex);
+		mutex_unlock(&map->active.in_mutex);
+		kfree(map);
+	} else {
+		spin_lock(&bedata->pvcallss_lock);
+		list_del_init(&map->list);
+		kfree(map);
+		spin_unlock(&bedata->pvcallss_lock);
+	}
+	WRITE_ONCE(bedata->rsp[req_id].req_id, PVCALLS_INVALID_ID);
+
+	return 0;
+}
+
 static const struct xenbus_device_id pvcalls_front_ids[] = {
 	{ "pvcalls" },
 	{ "" }
diff --git a/drivers/xen/pvcalls-front.h b/drivers/xen/pvcalls-front.h
index 25e05b8..3332978 100644
--- a/drivers/xen/pvcalls-front.h
+++ b/drivers/xen/pvcalls-front.h
@@ -23,5 +23,6 @@ int pvcalls_front_recvmsg(struct socket *sock,
 unsigned int pvcalls_front_poll(struct file *file,
 				struct socket *sock,
 				poll_table *wait);
+int pvcalls_front_release(struct socket *sock);
 
 #endif
-- 
1.9.1

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

#1698239 — Re: [PATCH v2 11/13] xen/pvcalls: implement release command

> +int pvcalls_front_release(struct socket *sock)
> +{
> +	struct pvcalls_bedata *bedata;
> +	struct sock_mapping *map;
> +	int req_id, notify;
> +	struct xen_pvcalls_request *req;
> +
> +	if (!pvcalls_front_dev)
> +		return -EIO;
> +	bedata = dev_get_drvdata(&pvcalls_front_dev->dev);
> +	if (!bedata)
> +		return -EIO;

Some (all?) other ops don't check bedata validity. Should they all do?

> +
> +	if (sock->sk == NULL)
> +		return 0;
> +
> +	map = (struct sock_mapping *) READ_ONCE(sock->sk->sk_send_head);
> +	if (map == NULL)
> +		return 0;
> +
> +	spin_lock(&bedata->pvcallss_lock);
> +	req_id = bedata->ring.req_prod_pvt & (RING_SIZE(&bedata->ring) - 1);
> +	if (RING_FULL(&bedata->ring) ||
> +	    READ_ONCE(bedata->rsp[req_id].req_id) != PVCALLS_INVALID_ID) {
> +		spin_unlock(&bedata->pvcallss_lock);
> +		return -EAGAIN;
> +	}
> +	WRITE_ONCE(sock->sk->sk_send_head, NULL);
> +
> +	req = RING_GET_REQUEST(&bedata->ring, req_id);
> +	req->req_id = req_id;
> +	req->cmd = PVCALLS_RELEASE;
> +	req->u.release.id = (uint64_t)sock;
> +
> +	bedata->ring.req_prod_pvt++;
> +	RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(&bedata->ring, notify);
> +	spin_unlock(&bedata->pvcallss_lock);
> +	if (notify)
> +		notify_remote_via_irq(bedata->irq);
> +
> +	wait_event(bedata->inflight_req,
> +		READ_ONCE(bedata->rsp[req_id].req_id) == req_id);
> +
> +	if (map->active_socket) {
> +		/* 
> +		 * Set in_error and wake up inflight_conn_req to force
> +		 * recvmsg waiters to exit.
> +		 */
> +		map->active.ring->in_error = -EBADF;
> +		wake_up_interruptible(&map->active.inflight_conn_req);
> +
> +		mutex_lock(&map->active.in_mutex);
> +		mutex_lock(&map->active.out_mutex);
> +		pvcalls_front_free_map(bedata, map);
> +		mutex_unlock(&map->active.out_mutex);
> +		mutex_unlock(&map->active.in_mutex);
> +		kfree(map);

Since you are locking here I assume you expect that someone else might
also be trying to lock the map. But you are freeing it immediately after
unlocking. Wouldn't that mean that whoever is trying to grab the lock
might then dereference freed memory?


-boris

> +	} else {
> +		spin_lock(&bedata->pvcallss_lock);
> +		list_del_init(&map->list);
> +		kfree(map);
> +		spin_unlock(&bedata->pvcallss_lock);
> +	}
> +	WRITE_ONCE(bedata->rsp[req_id].req_id, PVCALLS_INVALID_ID);
> +
> +	return 0;
> +}
> +
>  static const struct xenbus_device_id pvcalls_front_ids[] = {
>  	{ "pvcalls" },
>  	{ "" }
> diff --git a/drivers/xen/pvcalls-front.h b/drivers/xen/pvcalls-front.h
> index 25e05b8..3332978 100644
> --- a/drivers/xen/pvcalls-front.h
> +++ b/drivers/xen/pvcalls-front.h
> @@ -23,5 +23,6 @@ int pvcalls_front_recvmsg(struct socket *sock,
>  unsigned int pvcalls_front_poll(struct file *file,
>  				struct socket *sock,
>  				poll_table *wait);
> +int pvcalls_front_release(struct socket *sock);
>  
>  #endif

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

#1696562 — [PATCH v2 02/13] xen/pvcalls: connect to the backend

Implement the probe function for the pvcalls frontend. Read the
supported versions, max-page-order and function-calls nodes from
xenstore.

Introduce a data structure named pvcalls_bedata. It contains pointers to
the command ring, the event channel, a list of active sockets and a list
of passive sockets. Lists accesses are protected by a spin_lock.

Introduce a waitqueue to allow waiting for a response on commands sent
to the backend.

Introduce an array of struct xen_pvcalls_response to store commands
responses.

Only one frontend<->backend connection is supported at any given time
for a guest. Store the active frontend device to a static pointer.

Introduce a stub functions for the event handler.

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

diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c
index a8d38c2..5e0b265 100644
--- a/drivers/xen/pvcalls-front.c
+++ b/drivers/xen/pvcalls-front.c
@@ -20,6 +20,29 @@
 #include <xen/xenbus.h>
 #include <xen/interface/io/pvcalls.h>
 
+#define PVCALLS_INVALID_ID (UINT_MAX)
+#define RING_ORDER XENBUS_MAX_RING_GRANT_ORDER
+#define PVCALLS_NR_REQ_PER_RING __CONST_RING_SIZE(xen_pvcalls, XEN_PAGE_SIZE)
+
+struct pvcalls_bedata {
+	struct xen_pvcalls_front_ring ring;
+	grant_ref_t ref;
+	int irq;
+
+	struct list_head socket_mappings;
+	struct list_head socketpass_mappings;
+	spinlock_t pvcallss_lock;
+
+	wait_queue_head_t inflight_req;
+	struct xen_pvcalls_response rsp[PVCALLS_NR_REQ_PER_RING];
+};
+struct xenbus_device *pvcalls_front_dev;
+
+static irqreturn_t pvcalls_front_event_handler(int irq, void *dev_id)
+{
+	return IRQ_HANDLED;
+}
+
 static const struct xenbus_device_id pvcalls_front_ids[] = {
 	{ "pvcalls" },
 	{ "" }
@@ -33,12 +56,142 @@ static int pvcalls_front_remove(struct xenbus_device *dev)
 static int pvcalls_front_probe(struct xenbus_device *dev,
 			  const struct xenbus_device_id *id)
 {
+	int ret = -EFAULT, evtchn, ref = -1, i;
+	unsigned int max_page_order, function_calls, len;
+	char *versions;
+	grant_ref_t gref_head = 0;
+	struct xenbus_transaction xbt;
+	struct pvcalls_bedata *bedata = NULL;
+	struct xen_pvcalls_sring *sring;
+
+	if (pvcalls_front_dev != NULL) {
+		dev_err(&dev->dev, "only one PV Calls connection supported\n");
+		return -EINVAL;
+	}
+
+	versions = xenbus_read(XBT_NIL, dev->otherend, "versions", &len);
+	if (!len)
+		return -EINVAL;
+	if (strcmp(versions, "1")) {
+		kfree(versions);
+		return -EINVAL;
+	}
+	kfree(versions);
+	ret = xenbus_scanf(XBT_NIL, dev->otherend,
+			   "max-page-order", "%u", &max_page_order);
+	if (ret <= 0)
+		return -ENODEV;
+	if (max_page_order < RING_ORDER)
+		return -ENODEV;
+	ret = xenbus_scanf(XBT_NIL, dev->otherend,
+			   "function-calls", "%u", &function_calls);
+	if (ret <= 0 || function_calls != 1)
+		return -ENODEV;
+	pr_info("%s max-page-order is %u\n", __func__, max_page_order);
+
+	bedata = kzalloc(sizeof(struct pvcalls_bedata), GFP_KERNEL);
+	if (!bedata)
+		return -ENOMEM;
+
+	init_waitqueue_head(&bedata->inflight_req);
+	for (i = 0; i < PVCALLS_NR_REQ_PER_RING; i++)
+		bedata->rsp[i].req_id = PVCALLS_INVALID_ID;
+
+	sring = (struct xen_pvcalls_sring *) __get_free_page(GFP_KERNEL |
+							     __GFP_ZERO);
+	if (!sring)
+		goto error;
+	SHARED_RING_INIT(sring);
+	FRONT_RING_INIT(&bedata->ring, sring, XEN_PAGE_SIZE);
+
+	ret = xenbus_alloc_evtchn(dev, &evtchn);
+	if (ret)
+		goto error;
+
+	bedata->irq = bind_evtchn_to_irqhandler(evtchn,
+						pvcalls_front_event_handler,
+						0, "pvcalls-frontend", dev);
+	if (bedata->irq < 0) {
+		ret = bedata->irq;
+		goto error;
+	}
+
+	ret = gnttab_alloc_grant_references(1, &gref_head);
+	if (ret < 0)
+		goto error;
+	bedata->ref = ref = gnttab_claim_grant_reference(&gref_head);
+	if (ref < 0)
+		goto error;
+	gnttab_grant_foreign_access_ref(ref, dev->otherend_id,
+					virt_to_gfn((void *)sring), 0);
+
+ again:
+	ret = xenbus_transaction_start(&xbt);
+	if (ret) {
+		xenbus_dev_fatal(dev, ret, "starting transaction");
+		goto error;
+	}
+	ret = xenbus_printf(xbt, dev->nodename, "version", "%u", 1);
+	if (ret)
+		goto error_xenbus;
+	ret = xenbus_printf(xbt, dev->nodename, "ring-ref", "%d", ref);
+	if (ret)
+		goto error_xenbus;
+	ret = xenbus_printf(xbt, dev->nodename, "port", "%u",
+			    evtchn);
+	if (ret)
+		goto error_xenbus;
+	ret = xenbus_transaction_end(xbt, 0);
+	if (ret) {
+		if (ret == -EAGAIN)
+			goto again;
+		xenbus_dev_fatal(dev, ret, "completing transaction");
+		goto error;
+	}
+
+	INIT_LIST_HEAD(&bedata->socket_mappings);
+	INIT_LIST_HEAD(&bedata->socketpass_mappings);
+	spin_lock_init(&bedata->pvcallss_lock);
+	dev_set_drvdata(&dev->dev, bedata);
+	pvcalls_front_dev = dev;
+	xenbus_switch_state(dev, XenbusStateInitialised);
+
 	return 0;
+
+ error_xenbus:
+	xenbus_transaction_end(xbt, 1);
+	xenbus_dev_fatal(dev, ret, "writing xenstore");
+ error:
+	pvcalls_front_remove(dev);
+	return ret;
 }
 
 static void pvcalls_front_changed(struct xenbus_device *dev,
 			    enum xenbus_state backend_state)
 {
+	switch (backend_state) {
+	case XenbusStateReconfiguring:
+	case XenbusStateReconfigured:
+	case XenbusStateInitialising:
+	case XenbusStateInitialised:
+	case XenbusStateUnknown:
+		break;
+
+	case XenbusStateInitWait:
+		break;
+
+	case XenbusStateConnected:
+		xenbus_switch_state(dev, XenbusStateConnected);
+		break;
+
+	case XenbusStateClosed:
+		if (dev->state == XenbusStateClosed)
+			break;
+		/* Missed the backend's CLOSING state -- fallthrough */
+	case XenbusStateClosing:
+		xenbus_frontend_closed(dev);
+		break;
+	}
 }
 
 static struct xenbus_driver pvcalls_front_driver = {
-- 
1.9.1

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

#1697109 — Re: [PATCH v2 02/13] xen/pvcalls: connect to the backend

On 7/25/2017 5:21 PM, Stefano Stabellini wrote:
> Implement the probe function for the pvcalls frontend. Read the
> supported versions, max-page-order and function-calls nodes from
> xenstore.
>
> Introduce a data structure named pvcalls_bedata. It contains pointers to
> the command ring, the event channel, a list of active sockets and a list
> of passive sockets. Lists accesses are protected by a spin_lock.
>
> Introduce a waitqueue to allow waiting for a response on commands sent
> to the backend.
>
> Introduce an array of struct xen_pvcalls_response to store commands
> responses.
>
> Only one frontend<->backend connection is supported at any given time
> for a guest. Store the active frontend device to a static pointer.
>
> Introduce a stub functions for the event handler.
>
> Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> CC: boris.ostrovsky@oracle.com
> CC: jgross@suse.com
> ---
>   drivers/xen/pvcalls-front.c | 153 ++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 153 insertions(+)
>
> diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c
> index a8d38c2..5e0b265 100644
> --- a/drivers/xen/pvcalls-front.c
> +++ b/drivers/xen/pvcalls-front.c
> @@ -20,6 +20,29 @@
>   #include <xen/xenbus.h>
>   #include <xen/interface/io/pvcalls.h>
>   
> +#define PVCALLS_INVALID_ID (UINT_MAX)

Unnecessary parentheses

> +#define RING_ORDER XENBUS_MAX_RING_GRANT_ORDER

PVCALLS_RING_ORDER?

> +#define PVCALLS_NR_REQ_PER_RING __CONST_RING_SIZE(xen_pvcalls, XEN_PAGE_SIZE)
> +
> +struct pvcalls_bedata {
> +	struct xen_pvcalls_front_ring ring;
> +	grant_ref_t ref;
> +	int irq;
> +
> +	struct list_head socket_mappings;
> +	struct list_head socketpass_mappings;
> +	spinlock_t pvcallss_lock;
> +
> +	wait_queue_head_t inflight_req;
> +	struct xen_pvcalls_response rsp[PVCALLS_NR_REQ_PER_RING];
> +};
> +struct xenbus_device *pvcalls_front_dev;

static

> +
> +static irqreturn_t pvcalls_front_event_handler(int irq, void *dev_id)
> +{
> +	return IRQ_HANDLED;
> +}
> +
>   static const struct xenbus_device_id pvcalls_front_ids[] = {
>   	{ "pvcalls" },
>   	{ "" }
> @@ -33,12 +56,142 @@ static int pvcalls_front_remove(struct xenbus_device *dev)
>   static int pvcalls_front_probe(struct xenbus_device *dev,
>   			  const struct xenbus_device_id *id)
>   {
> +	int ret = -EFAULT, evtchn, ref = -1, i;
> +	unsigned int max_page_order, function_calls, len;
> +	char *versions;
> +	grant_ref_t gref_head = 0;
> +	struct xenbus_transaction xbt;
> +	struct pvcalls_bedata *bedata = NULL;
> +	struct xen_pvcalls_sring *sring;
> +
> +	if (pvcalls_front_dev != NULL) {
> +		dev_err(&dev->dev, "only one PV Calls connection supported\n");
> +		return -EINVAL;
> +	}
> +
> +	versions = xenbus_read(XBT_NIL, dev->otherend, "versions", &len);
> +	if (!len)
> +		return -EINVAL;
> +	if (strcmp(versions, "1")) {
> +		kfree(versions);
> +		return -EINVAL;
> +	}
> +	kfree(versions);
> +	ret = xenbus_scanf(XBT_NIL, dev->otherend,
> +			   "max-page-order", "%u", &max_page_order);
> +	if (ret <= 0)
> +		return -ENODEV;
> +	if (max_page_order < RING_ORDER)
> +		return -ENODEV;
> +	ret = xenbus_scanf(XBT_NIL, dev->otherend,
> +			   "function-calls", "%u", &function_calls);
> +	if (ret <= 0 || function_calls != 1)
> +		return -ENODEV;
> +	pr_info("%s max-page-order is %u\n", __func__, max_page_order);
> +
> +	bedata = kzalloc(sizeof(struct pvcalls_bedata), GFP_KERNEL);
> +	if (!bedata)
> +		return -ENOMEM;
> +
> +	init_waitqueue_head(&bedata->inflight_req);
> +	for (i = 0; i < PVCALLS_NR_REQ_PER_RING; i++)
> +		bedata->rsp[i].req_id = PVCALLS_INVALID_ID;
> +
> +	sring = (struct xen_pvcalls_sring *) __get_free_page(GFP_KERNEL |
> +							     __GFP_ZERO);
> +	if (!sring)
> +		goto error;
> +	SHARED_RING_INIT(sring);
> +	FRONT_RING_INIT(&bedata->ring, sring, XEN_PAGE_SIZE);
> +
> +	ret = xenbus_alloc_evtchn(dev, &evtchn);
> +	if (ret)
> +		goto error;
> +
> +	bedata->irq = bind_evtchn_to_irqhandler(evtchn,
> +						pvcalls_front_event_handler,
> +						0, "pvcalls-frontend", dev);
> +	if (bedata->irq < 0) {
> +		ret = bedata->irq;
> +		goto error;
> +	}
> +
> +	ret = gnttab_alloc_grant_references(1, &gref_head);
> +	if (ret < 0)
> +		goto error;
> +	bedata->ref = ref = gnttab_claim_grant_reference(&gref_head);

Is ref really needed?

> +	if (ref < 0)
> +		goto error;
> +	gnttab_grant_foreign_access_ref(ref, dev->otherend_id,
> +					virt_to_gfn((void *)sring), 0);
> +
> + again:
> +	ret = xenbus_transaction_start(&xbt);
> +	if (ret) {
> +		xenbus_dev_fatal(dev, ret, "starting transaction");
> +		goto error;
> +	}
> +	ret = xenbus_printf(xbt, dev->nodename, "version", "%u", 1);
> +	if (ret)
> +		goto error_xenbus;
> +	ret = xenbus_printf(xbt, dev->nodename, "ring-ref", "%d", ref);
> +	if (ret)
> +		goto error_xenbus;
> +	ret = xenbus_printf(xbt, dev->nodename, "port", "%u",
> +			    evtchn);
> +	if (ret)
> +		goto error_xenbus;
> +	ret = xenbus_transaction_end(xbt, 0);
> +	if (ret) {
> +		if (ret == -EAGAIN)
> +			goto again;
> +		xenbus_dev_fatal(dev, ret, "completing transaction");
> +		goto error;
> +	}
> +
> +	INIT_LIST_HEAD(&bedata->socket_mappings);
> +	INIT_LIST_HEAD(&bedata->socketpass_mappings);
> +	spin_lock_init(&bedata->pvcallss_lock);
> +	dev_set_drvdata(&dev->dev, bedata);
> +	pvcalls_front_dev = dev;
> +	xenbus_switch_state(dev, XenbusStateInitialised);
> +
>   	return 0;
> +
> + error_xenbus:
> +	xenbus_transaction_end(xbt, 1);
> +	xenbus_dev_fatal(dev, ret, "writing xenstore");
> + error:
> +	pvcalls_front_remove(dev);

I think patch 12 (where you implement cleanup) could be moved before 
this one.

I also think you are leaking bedata on error paths.

-boris
> +	return ret;
>   }
>   
>   static void pvcalls_front_changed(struct xenbus_device *dev,
>   			    enum xenbus_state backend_state)
>   {
> +	switch (backend_state) {
> +	case XenbusStateReconfiguring:
> +	case XenbusStateReconfigured:
> +	case XenbusStateInitialising:
> +	case XenbusStateInitialised:
> +	case XenbusStateUnknown:
> +		break;
> +
> +	case XenbusStateInitWait:
> +		break;
> +
> +	case XenbusStateConnected:
> +		xenbus_switch_state(dev, XenbusStateConnected);
> +		break;
> +
> +	case XenbusStateClosed:
> +		if (dev->state == XenbusStateClosed)
> +			break;
> +		/* Missed the backend's CLOSING state -- fallthrough */
> +	case XenbusStateClosing:
> +		xenbus_frontend_closed(dev);
> +		break;
> +	}
>   }
>   
>   static struct xenbus_driver pvcalls_front_driver = {

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

#1697641 — Re: [PATCH v2 02/13] xen/pvcalls: connect to the backend

On Wed, 26 Jul 2017, Boris Ostrovsky wrote:
> On 7/25/2017 5:21 PM, Stefano Stabellini wrote:
> > Implement the probe function for the pvcalls frontend. Read the
> > supported versions, max-page-order and function-calls nodes from
> > xenstore.
> > 
> > Introduce a data structure named pvcalls_bedata. It contains pointers to
> > the command ring, the event channel, a list of active sockets and a list
> > of passive sockets. Lists accesses are protected by a spin_lock.
> > 
> > Introduce a waitqueue to allow waiting for a response on commands sent
> > to the backend.
> > 
> > Introduce an array of struct xen_pvcalls_response to store commands
> > responses.
> > 
> > Only one frontend<->backend connection is supported at any given time
> > for a guest. Store the active frontend device to a static pointer.
> > 
> > Introduce a stub functions for the event handler.
> > 
> > Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> > CC: boris.ostrovsky@oracle.com
> > CC: jgross@suse.com
> > ---
> >   drivers/xen/pvcalls-front.c | 153
> > ++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 153 insertions(+)
> > 
> > diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c
> > index a8d38c2..5e0b265 100644
> > --- a/drivers/xen/pvcalls-front.c
> > +++ b/drivers/xen/pvcalls-front.c
> > @@ -20,6 +20,29 @@
> >   #include <xen/xenbus.h>
> >   #include <xen/interface/io/pvcalls.h>
> >   +#define PVCALLS_INVALID_ID (UINT_MAX)
> 
> Unnecessary parentheses

OK


> > +#define RING_ORDER XENBUS_MAX_RING_GRANT_ORDER
> 
> PVCALLS_RING_ORDER?

Sure


> > +#define PVCALLS_NR_REQ_PER_RING __CONST_RING_SIZE(xen_pvcalls,
> > XEN_PAGE_SIZE)
> > +
> > +struct pvcalls_bedata {
> > +	struct xen_pvcalls_front_ring ring;
> > +	grant_ref_t ref;
> > +	int irq;
> > +
> > +	struct list_head socket_mappings;
> > +	struct list_head socketpass_mappings;
> > +	spinlock_t pvcallss_lock;
> > +
> > +	wait_queue_head_t inflight_req;
> > +	struct xen_pvcalls_response rsp[PVCALLS_NR_REQ_PER_RING];
> > +};
> > +struct xenbus_device *pvcalls_front_dev;
> 
> static

good point, I'll fix


> > +
> > +static irqreturn_t pvcalls_front_event_handler(int irq, void *dev_id)
> > +{
> > +	return IRQ_HANDLED;
> > +}
> > +
> >   static const struct xenbus_device_id pvcalls_front_ids[] = {
> >   	{ "pvcalls" },
> >   	{ "" }
> > @@ -33,12 +56,142 @@ static int pvcalls_front_remove(struct xenbus_device
> > *dev)
> >   static int pvcalls_front_probe(struct xenbus_device *dev,
> >   			  const struct xenbus_device_id *id)
> >   {
> > +	int ret = -EFAULT, evtchn, ref = -1, i;
> > +	unsigned int max_page_order, function_calls, len;
> > +	char *versions;
> > +	grant_ref_t gref_head = 0;
> > +	struct xenbus_transaction xbt;
> > +	struct pvcalls_bedata *bedata = NULL;
> > +	struct xen_pvcalls_sring *sring;
> > +
> > +	if (pvcalls_front_dev != NULL) {
> > +		dev_err(&dev->dev, "only one PV Calls connection
> > supported\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	versions = xenbus_read(XBT_NIL, dev->otherend, "versions", &len);
> > +	if (!len)
> > +		return -EINVAL;
> > +	if (strcmp(versions, "1")) {
> > +		kfree(versions);
> > +		return -EINVAL;
> > +	}
> > +	kfree(versions);
> > +	ret = xenbus_scanf(XBT_NIL, dev->otherend,
> > +			   "max-page-order", "%u", &max_page_order);
> > +	if (ret <= 0)
> > +		return -ENODEV;
> > +	if (max_page_order < RING_ORDER)
> > +		return -ENODEV;
> > +	ret = xenbus_scanf(XBT_NIL, dev->otherend,
> > +			   "function-calls", "%u", &function_calls);
> > +	if (ret <= 0 || function_calls != 1)
> > +		return -ENODEV;
> > +	pr_info("%s max-page-order is %u\n", __func__, max_page_order);
> > +
> > +	bedata = kzalloc(sizeof(struct pvcalls_bedata), GFP_KERNEL);
> > +	if (!bedata)
> > +		return -ENOMEM;
> > +
> > +	init_waitqueue_head(&bedata->inflight_req);
> > +	for (i = 0; i < PVCALLS_NR_REQ_PER_RING; i++)
> > +		bedata->rsp[i].req_id = PVCALLS_INVALID_ID;
> > +
> > +	sring = (struct xen_pvcalls_sring *) __get_free_page(GFP_KERNEL |
> > +							     __GFP_ZERO);
> > +	if (!sring)
> > +		goto error;
> > +	SHARED_RING_INIT(sring);
> > +	FRONT_RING_INIT(&bedata->ring, sring, XEN_PAGE_SIZE);
> > +
> > +	ret = xenbus_alloc_evtchn(dev, &evtchn);
> > +	if (ret)
> > +		goto error;
> > +
> > +	bedata->irq = bind_evtchn_to_irqhandler(evtchn,
> > +						pvcalls_front_event_handler,
> > +						0, "pvcalls-frontend", dev);
> > +	if (bedata->irq < 0) {
> > +		ret = bedata->irq;
> > +		goto error;
> > +	}
> > +
> > +	ret = gnttab_alloc_grant_references(1, &gref_head);
> > +	if (ret < 0)
> > +		goto error;
> > +	bedata->ref = ref = gnttab_claim_grant_reference(&gref_head);
> 
> Is ref really needed?

No, I'll remove it


> > +	if (ref < 0)
> > +		goto error;
> > +	gnttab_grant_foreign_access_ref(ref, dev->otherend_id,
> > +					virt_to_gfn((void *)sring), 0);
> > +
> > + again:
> > +	ret = xenbus_transaction_start(&xbt);
> > +	if (ret) {
> > +		xenbus_dev_fatal(dev, ret, "starting transaction");
> > +		goto error;
> > +	}
> > +	ret = xenbus_printf(xbt, dev->nodename, "version", "%u", 1);
> > +	if (ret)
> > +		goto error_xenbus;
> > +	ret = xenbus_printf(xbt, dev->nodename, "ring-ref", "%d", ref);
> > +	if (ret)
> > +		goto error_xenbus;
> > +	ret = xenbus_printf(xbt, dev->nodename, "port", "%u",
> > +			    evtchn);
> > +	if (ret)
> > +		goto error_xenbus;
> > +	ret = xenbus_transaction_end(xbt, 0);
> > +	if (ret) {
> > +		if (ret == -EAGAIN)
> > +			goto again;
> > +		xenbus_dev_fatal(dev, ret, "completing transaction");
> > +		goto error;
> > +	}
> > +
> > +	INIT_LIST_HEAD(&bedata->socket_mappings);
> > +	INIT_LIST_HEAD(&bedata->socketpass_mappings);
> > +	spin_lock_init(&bedata->pvcallss_lock);
> > +	dev_set_drvdata(&dev->dev, bedata);
> > +	pvcalls_front_dev = dev;
> > +	xenbus_switch_state(dev, XenbusStateInitialised);
> > +
> >   	return 0;
> > +
> > + error_xenbus:
> > +	xenbus_transaction_end(xbt, 1);
> > +	xenbus_dev_fatal(dev, ret, "writing xenstore");
> > + error:
> > +	pvcalls_front_remove(dev);
> 
> I think patch 12 (where you implement cleanup) could be moved before this one.

I'll move the patch


> I also think you are leaking bedata on error paths.

bedata is freed by pvcalls_front_remove (kfree(bedata)), why do you say
so?


> > +	return ret;
> >   }
> >     static void pvcalls_front_changed(struct xenbus_device *dev,
> >   			    enum xenbus_state backend_state)
> >   {
> > +	switch (backend_state) {
> > +	case XenbusStateReconfiguring:
> > +	case XenbusStateReconfigured:
> > +	case XenbusStateInitialising:
> > +	case XenbusStateInitialised:
> > +	case XenbusStateUnknown:
> > +		break;
> > +
> > +	case XenbusStateInitWait:
> > +		break;
> > +
> > +	case XenbusStateConnected:
> > +		xenbus_switch_state(dev, XenbusStateConnected);
> > +		break;
> > +
> > +	case XenbusStateClosed:
> > +		if (dev->state == XenbusStateClosed)
> > +			break;
> > +		/* Missed the backend's CLOSING state -- fallthrough */
> > +	case XenbusStateClosing:
> > +		xenbus_frontend_closed(dev);
> > +		break;
> > +	}
> >   }
> >     static struct xenbus_driver pvcalls_front_driver = {

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

#1698078 — Re: [PATCH v2 02/13] xen/pvcalls: connect to the backend

>>>   static int pvcalls_front_probe(struct xenbus_device *dev,
>>>   			  const struct xenbus_device_id *id)
>>>   {
>>> +	int ret = -EFAULT, evtchn, ref = -1, i;
>>> +	unsigned int max_page_order, function_calls, len;
>>> +	char *versions;
>>> +	grant_ref_t gref_head = 0;
>>> +	struct xenbus_transaction xbt;
>>> +	struct pvcalls_bedata *bedata = NULL;
>>> +	struct xen_pvcalls_sring *sring;
>>> +
>>> +	if (pvcalls_front_dev != NULL) {
>>> +		dev_err(&dev->dev, "only one PV Calls connection
>>> supported\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	versions = xenbus_read(XBT_NIL, dev->otherend, "versions", &len);
>>> +	if (!len)
>>> +		return -EINVAL;
>>> +	if (strcmp(versions, "1")) {
>>> +		kfree(versions);
>>> +		return -EINVAL;
>>> +	}
>>> +	kfree(versions);
>>> +	ret = xenbus_scanf(XBT_NIL, dev->otherend,
>>> +			   "max-page-order", "%u", &max_page_order);
>>> +	if (ret <= 0)
>>> +		return -ENODEV;
>>> +	if (max_page_order < RING_ORDER)
>>> +		return -ENODEV;
>>> +	ret = xenbus_scanf(XBT_NIL, dev->otherend,
>>> +			   "function-calls", "%u", &function_calls);
>>> +	if (ret <= 0 || function_calls != 1)
>>> +		return -ENODEV;
>>> +	pr_info("%s max-page-order is %u\n", __func__, max_page_order);
>>> +
>>> +	bedata = kzalloc(sizeof(struct pvcalls_bedata), GFP_KERNEL);
>>> +	if (!bedata)
>>> +		return -ENOMEM;
>>> +
>>> +	init_waitqueue_head(&bedata->inflight_req);
>>> +	for (i = 0; i < PVCALLS_NR_REQ_PER_RING; i++)
>>> +		bedata->rsp[i].req_id = PVCALLS_INVALID_ID;
>>> +
>>> +	sring = (struct xen_pvcalls_sring *) __get_free_page(GFP_KERNEL |
>>> +							     __GFP_ZERO);
>>> +	if (!sring)
>>> +		goto error;
>>> +	SHARED_RING_INIT(sring);
>>> +	FRONT_RING_INIT(&bedata->ring, sring, XEN_PAGE_SIZE);
>>> +
>>> +	ret = xenbus_alloc_evtchn(dev, &evtchn);
>>> +	if (ret)
>>> +		goto error;
>>> +
>>> +	bedata->irq = bind_evtchn_to_irqhandler(evtchn,
>>> +						pvcalls_front_event_handler,
>>> +						0, "pvcalls-frontend", dev);
>>> +	if (bedata->irq < 0) {
>>> +		ret = bedata->irq;
>>> +		goto error;
>>> +	}
>>> +
>>> +	ret = gnttab_alloc_grant_references(1, &gref_head);
>>> +	if (ret < 0)
>>> +		goto error;
>>> +	bedata->ref = ref = gnttab_claim_grant_reference(&gref_head);
>> Is ref really needed?
> No, I'll remove it
>
>
>>> +	if (ref < 0)
>>> +		goto error;
>>> +	gnttab_grant_foreign_access_ref(ref, dev->otherend_id,
>>> +					virt_to_gfn((void *)sring), 0);
>>> +
>>> + again:
>>> +	ret = xenbus_transaction_start(&xbt);
>>> +	if (ret) {
>>> +		xenbus_dev_fatal(dev, ret, "starting transaction");
>>> +		goto error;
>>> +	}
>>> +	ret = xenbus_printf(xbt, dev->nodename, "version", "%u", 1);
>>> +	if (ret)
>>> +		goto error_xenbus;
>>> +	ret = xenbus_printf(xbt, dev->nodename, "ring-ref", "%d", ref);
>>> +	if (ret)
>>> +		goto error_xenbus;
>>> +	ret = xenbus_printf(xbt, dev->nodename, "port", "%u",
>>> +			    evtchn);
>>> +	if (ret)
>>> +		goto error_xenbus;
>>> +	ret = xenbus_transaction_end(xbt, 0);
>>> +	if (ret) {
>>> +		if (ret == -EAGAIN)
>>> +			goto again;
>>> +		xenbus_dev_fatal(dev, ret, "completing transaction");
>>> +		goto error;
>>> +	}
>>> +
>>> +	INIT_LIST_HEAD(&bedata->socket_mappings);
>>> +	INIT_LIST_HEAD(&bedata->socketpass_mappings);
>>> +	spin_lock_init(&bedata->pvcallss_lock);
>>> +	dev_set_drvdata(&dev->dev, bedata);
>>> +	pvcalls_front_dev = dev;
>>> +	xenbus_switch_state(dev, XenbusStateInitialised);
>>> +
>>>   	return 0;
>>> +
>>> + error_xenbus:
>>> +	xenbus_transaction_end(xbt, 1);
>>> +	xenbus_dev_fatal(dev, ret, "writing xenstore");
>>> + error:
>>> +	pvcalls_front_remove(dev);
>> I think patch 12 (where you implement cleanup) could be moved before this one.
> I'll move the patch
>
>
>> I also think you are leaking bedata on error paths.
> bedata is freed by pvcalls_front_remove (kfree(bedata)), why do you say
> so?

bedata there is read from dev_get_drvdata() and here you assign drvdata
at the very end.

Come think of it, pvcalls_front_remove() should probably first check
whether bedata is valid. Or drvdata should be assigned right away in
this routine, before any 'got error/error_xenbus'.

-boris

>
>
>>> +	return ret;
>>>   }
>>>     

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

#1696566 — [PATCH v2 03/13] xen/pvcalls: implement socket command and handle events

Send a PVCALLS_SOCKET command to the backend, use the masked
req_prod_pvt as req_id. This way, req_id is guaranteed to be between 0
and PVCALLS_NR_REQ_PER_RING. We already have a slot in the rsp array
ready for the response, and there cannot be two outstanding responses
with the same req_id.

Wait for the response by waiting on the inflight_req waitqueue and
check for the req_id field in rsp[req_id]. Use atomic accesses to
read the field. Once a response is received, clear the corresponding rsp
slot by setting req_id to PVCALLS_INVALID_ID. Note that
PVCALLS_INVALID_ID is invalid only from the frontend point of view. It
is not part of the PVCalls protocol.

pvcalls_front_event_handler is in charge of copying responses from the
ring to the appropriate rsp slot. It is done by copying the body of the
response first, then by copying req_id atomically. After the copies,
wake up anybody waiting on waitqueue.

pvcallss_lock protects accesses to the ring.

Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: boris.ostrovsky@oracle.com
CC: jgross@suse.com
---
 drivers/xen/pvcalls-front.c | 94 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/xen/pvcalls-front.h |  8 ++++
 2 files changed, 102 insertions(+)
 create mode 100644 drivers/xen/pvcalls-front.h

diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c
index 5e0b265..d1dbcf1 100644
--- a/drivers/xen/pvcalls-front.c
+++ b/drivers/xen/pvcalls-front.c
@@ -20,6 +20,8 @@
 #include <xen/xenbus.h>
 #include <xen/interface/io/pvcalls.h>
 
+#include "pvcalls-front.h"
+
 #define PVCALLS_INVALID_ID (UINT_MAX)
 #define RING_ORDER XENBUS_MAX_RING_GRANT_ORDER
 #define PVCALLS_NR_REQ_PER_RING __CONST_RING_SIZE(xen_pvcalls, XEN_PAGE_SIZE)
@@ -40,9 +42,101 @@ struct pvcalls_bedata {
 
 static irqreturn_t pvcalls_front_event_handler(int irq, void *dev_id)
 {
+	struct xenbus_device *dev = dev_id;
+	struct pvcalls_bedata *bedata;
+	struct xen_pvcalls_response *rsp;
+	uint8_t *src, *dst;
+	int req_id = 0, more = 0, done = 0;
+
+	if (dev == NULL)
+		return IRQ_HANDLED;
+
+	bedata = dev_get_drvdata(&dev->dev);
+	if (bedata == NULL)
+		return IRQ_HANDLED;
+
+again:
+	while (RING_HAS_UNCONSUMED_RESPONSES(&bedata->ring)) {
+		rsp = RING_GET_RESPONSE(&bedata->ring, bedata->ring.rsp_cons);
+
+		req_id = rsp->req_id;
+		src = (uint8_t *)&bedata->rsp[req_id];
+		src += sizeof(rsp->req_id);
+		dst = (uint8_t *)rsp;
+		dst += sizeof(rsp->req_id);
+		memcpy(dst, src, sizeof(*rsp) - sizeof(rsp->req_id));
+		/*
+		 * First copy the rest of the data, then req_id. It is
+		 * paired with the barrier when accessing bedata->rsp.
+		 */
+		smp_wmb();
+		WRITE_ONCE(bedata->rsp[req_id].req_id, rsp->req_id);
+
+		done = 1;
+		bedata->ring.rsp_cons++;
+	}
+
+	RING_FINAL_CHECK_FOR_RESPONSES(&bedata->ring, more);
+	if (more)
+		goto again;
+	if (done)
+		wake_up(&bedata->inflight_req);
 	return IRQ_HANDLED;
 }
 
+int pvcalls_front_socket(struct socket *sock)
+{
+	struct pvcalls_bedata *bedata;
+	struct xen_pvcalls_request *req;
+	int notify, req_id, ret;
+
+	if (!pvcalls_front_dev)
+		return -EACCES;
+	/*
+	 * PVCalls only supports domain AF_INET,
+	 * type SOCK_STREAM and protocol 0 sockets for now.
+	 *
+	 * Check socket type here, AF_INET and protocol checks are done
+	 * by the caller.
+	 */
+	if (sock->type != SOCK_STREAM)
+	    return -ENOTSUPP;
+
+	bedata = dev_get_drvdata(&pvcalls_front_dev->dev);
+
+	spin_lock(&bedata->pvcallss_lock);
+	req_id = bedata->ring.req_prod_pvt & (RING_SIZE(&bedata->ring) - 1);
+	if (RING_FULL(&bedata->ring) ||
+	    READ_ONCE(bedata->rsp[req_id].req_id) != PVCALLS_INVALID_ID) {
+		spin_unlock(&bedata->pvcallss_lock);
+		return -EAGAIN;
+	}
+	req = RING_GET_REQUEST(&bedata->ring, req_id);
+	req->req_id = req_id;
+	req->cmd = PVCALLS_SOCKET;
+	req->u.socket.id = (uint64_t) sock;
+	req->u.socket.domain = AF_INET;
+	req->u.socket.type = SOCK_STREAM;
+	req->u.socket.protocol = 0;
+
+	bedata->ring.req_prod_pvt++;
+	RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(&bedata->ring, notify);
+	spin_unlock(&bedata->pvcallss_lock);
+	if (notify)
+		notify_remote_via_irq(bedata->irq);
+
+	if (wait_event_interruptible(bedata->inflight_req,
+		READ_ONCE(bedata->rsp[req_id].req_id) == req_id) != 0)
+		return -EINTR;
+
+	ret = bedata->rsp[req_id].ret;
+	/* read ret, then set this rsp slot to be reused */
+	smp_mb();
+	WRITE_ONCE(bedata->rsp[req_id].req_id, PVCALLS_INVALID_ID);
+
+	return ret;
+}
+
 static const struct xenbus_device_id pvcalls_front_ids[] = {
 	{ "pvcalls" },
 	{ "" }
diff --git a/drivers/xen/pvcalls-front.h b/drivers/xen/pvcalls-front.h
new file mode 100644
index 0000000..b7dabed
--- /dev/null
+++ b/drivers/xen/pvcalls-front.h
@@ -0,0 +1,8 @@
+#ifndef __PVCALLS_FRONT_H__
+#define __PVCALLS_FRONT_H__
+
+#include <linux/net.h>
+
+int pvcalls_front_socket(struct socket *sock);
+
+#endif
-- 
1.9.1

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

#1697184 — Re: [PATCH v2 03/13] xen/pvcalls: implement socket command and handle events

On 7/25/2017 5:22 PM, Stefano Stabellini wrote:
> Send a PVCALLS_SOCKET command to the backend, use the masked
> req_prod_pvt as req_id. This way, req_id is guaranteed to be between 0
> and PVCALLS_NR_REQ_PER_RING. We already have a slot in the rsp array
> ready for the response, and there cannot be two outstanding responses
> with the same req_id.
>
> Wait for the response by waiting on the inflight_req waitqueue and
> check for the req_id field in rsp[req_id]. Use atomic accesses to
> read the field. Once a response is received, clear the corresponding rsp
> slot by setting req_id to PVCALLS_INVALID_ID. Note that
> PVCALLS_INVALID_ID is invalid only from the frontend point of view. It
> is not part of the PVCalls protocol.
>
> pvcalls_front_event_handler is in charge of copying responses from the
> ring to the appropriate rsp slot. It is done by copying the body of the
> response first, then by copying req_id atomically. After the copies,
> wake up anybody waiting on waitqueue.
>
> pvcallss_lock protects accesses to the ring.
>
> Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> CC: boris.ostrovsky@oracle.com
> CC: jgross@suse.com
> ---
>   drivers/xen/pvcalls-front.c | 94 +++++++++++++++++++++++++++++++++++++++++++++
>   drivers/xen/pvcalls-front.h |  8 ++++
>   2 files changed, 102 insertions(+)
>   create mode 100644 drivers/xen/pvcalls-front.h
>
> diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c
> index 5e0b265..d1dbcf1 100644
> --- a/drivers/xen/pvcalls-front.c
> +++ b/drivers/xen/pvcalls-front.c
> @@ -20,6 +20,8 @@
>   #include <xen/xenbus.h>
>   #include <xen/interface/io/pvcalls.h>
>   
> +#include "pvcalls-front.h"
> +
>   #define PVCALLS_INVALID_ID (UINT_MAX)
>   #define RING_ORDER XENBUS_MAX_RING_GRANT_ORDER
>   #define PVCALLS_NR_REQ_PER_RING __CONST_RING_SIZE(xen_pvcalls, XEN_PAGE_SIZE)
> @@ -40,9 +42,101 @@ struct pvcalls_bedata {
>   
>   static irqreturn_t pvcalls_front_event_handler(int irq, void *dev_id)
>   {
> +	struct xenbus_device *dev = dev_id;
> +	struct pvcalls_bedata *bedata;
> +	struct xen_pvcalls_response *rsp;
> +	uint8_t *src, *dst;
> +	int req_id = 0, more = 0, done = 0;
> +
> +	if (dev == NULL)
> +		return IRQ_HANDLED;
> +
> +	bedata = dev_get_drvdata(&dev->dev);
> +	if (bedata == NULL)
> +		return IRQ_HANDLED;
> +
> +again:
> +	while (RING_HAS_UNCONSUMED_RESPONSES(&bedata->ring)) {
> +		rsp = RING_GET_RESPONSE(&bedata->ring, bedata->ring.rsp_cons);
> +
> +		req_id = rsp->req_id;
> +		src = (uint8_t *)&bedata->rsp[req_id];
> +		src += sizeof(rsp->req_id);
> +		dst = (uint8_t *)rsp;
> +		dst += sizeof(rsp->req_id);

These two lines can be combined (both src and dst)

> +		memcpy(dst, src, sizeof(*rsp) - sizeof(rsp->req_id));
> +		/*
> +		 * First copy the rest of the data, then req_id. It is
> +		 * paired with the barrier when accessing bedata->rsp.
> +		 */
> +		smp_wmb();
> +		WRITE_ONCE(bedata->rsp[req_id].req_id, rsp->req_id);
> +
> +		done = 1;
> +		bedata->ring.rsp_cons++;
> +	}
> +
> +	RING_FINAL_CHECK_FOR_RESPONSES(&bedata->ring, more);
> +	if (more)
> +		goto again;
> +	if (done)
> +		wake_up(&bedata->inflight_req);
>   	return IRQ_HANDLED;
>   }
>   
> +int pvcalls_front_socket(struct socket *sock)
> +{
> +	struct pvcalls_bedata *bedata;
> +	struct xen_pvcalls_request *req;
> +	int notify, req_id, ret;
> +
> +	if (!pvcalls_front_dev)
> +		return -EACCES;
> +	/*
> +	 * PVCalls only supports domain AF_INET,
> +	 * type SOCK_STREAM and protocol 0 sockets for now.
> +	 *
> +	 * Check socket type here, AF_INET and protocol checks are done
> +	 * by the caller.
> +	 */
> +	if (sock->type != SOCK_STREAM)
> +	    return -ENOTSUPP;
> +
> +	bedata = dev_get_drvdata(&pvcalls_front_dev->dev);
> +
> +	spin_lock(&bedata->pvcallss_lock);
> +	req_id = bedata->ring.req_prod_pvt & (RING_SIZE(&bedata->ring) - 1);
> +	if (RING_FULL(&bedata->ring) ||
> +	    READ_ONCE(bedata->rsp[req_id].req_id) != PVCALLS_INVALID_ID) {
> +		spin_unlock(&bedata->pvcallss_lock);
> +		return -EAGAIN;
> +	}
> +	req = RING_GET_REQUEST(&bedata->ring, req_id);
> +	req->req_id = req_id;
> +	req->cmd = PVCALLS_SOCKET;
> +	req->u.socket.id = (uint64_t) sock;
> +	req->u.socket.domain = AF_INET;
> +	req->u.socket.type = SOCK_STREAM;
> +	req->u.socket.protocol = 0;
> +
> +	bedata->ring.req_prod_pvt++;
> +	RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(&bedata->ring, notify);
> +	spin_unlock(&bedata->pvcallss_lock);
> +	if (notify)
> +		notify_remote_via_irq(bedata->irq);
> +
> +	if (wait_event_interruptible(bedata->inflight_req,
> +		READ_ONCE(bedata->rsp[req_id].req_id) == req_id) != 0)

"!= 0" can be dropped

-boris

> +		return -EINTR;
> +
> +	ret = bedata->rsp[req_id].ret;
> +	/* read ret, then set this rsp slot to be reused */
> +	smp_mb();
> +	WRITE_ONCE(bedata->rsp[req_id].req_id, PVCALLS_INVALID_ID);
> +
> +	return ret;
> +}
> +
>   static const struct xenbus_device_id pvcalls_front_ids[] = {
>   	{ "pvcalls" },
>   	{ "" }
> diff --git a/drivers/xen/pvcalls-front.h b/drivers/xen/pvcalls-front.h
> new file mode 100644
> index 0000000..b7dabed
> --- /dev/null
> +++ b/drivers/xen/pvcalls-front.h
> @@ -0,0 +1,8 @@
> +#ifndef __PVCALLS_FRONT_H__
> +#define __PVCALLS_FRONT_H__
> +
> +#include <linux/net.h>
> +
> +int pvcalls_front_socket(struct socket *sock);
> +
> +#endif

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

#1697621 — Re: [PATCH v2 03/13] xen/pvcalls: implement socket command and handle events

On Wed, 26 Jul 2017, Boris Ostrovsky wrote:
> On 7/25/2017 5:22 PM, Stefano Stabellini wrote:
> > Send a PVCALLS_SOCKET command to the backend, use the masked
> > req_prod_pvt as req_id. This way, req_id is guaranteed to be between 0
> > and PVCALLS_NR_REQ_PER_RING. We already have a slot in the rsp array
> > ready for the response, and there cannot be two outstanding responses
> > with the same req_id.
> > 
> > Wait for the response by waiting on the inflight_req waitqueue and
> > check for the req_id field in rsp[req_id]. Use atomic accesses to
> > read the field. Once a response is received, clear the corresponding rsp
> > slot by setting req_id to PVCALLS_INVALID_ID. Note that
> > PVCALLS_INVALID_ID is invalid only from the frontend point of view. It
> > is not part of the PVCalls protocol.
> > 
> > pvcalls_front_event_handler is in charge of copying responses from the
> > ring to the appropriate rsp slot. It is done by copying the body of the
> > response first, then by copying req_id atomically. After the copies,
> > wake up anybody waiting on waitqueue.
> > 
> > pvcallss_lock protects accesses to the ring.
> > 
> > Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> > CC: boris.ostrovsky@oracle.com
> > CC: jgross@suse.com
> > ---
> >   drivers/xen/pvcalls-front.c | 94
> > +++++++++++++++++++++++++++++++++++++++++++++
> >   drivers/xen/pvcalls-front.h |  8 ++++
> >   2 files changed, 102 insertions(+)
> >   create mode 100644 drivers/xen/pvcalls-front.h
> > 
> > diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c
> > index 5e0b265..d1dbcf1 100644
> > --- a/drivers/xen/pvcalls-front.c
> > +++ b/drivers/xen/pvcalls-front.c
> > @@ -20,6 +20,8 @@
> >   #include <xen/xenbus.h>
> >   #include <xen/interface/io/pvcalls.h>
> >   +#include "pvcalls-front.h"
> > +
> >   #define PVCALLS_INVALID_ID (UINT_MAX)
> >   #define RING_ORDER XENBUS_MAX_RING_GRANT_ORDER
> >   #define PVCALLS_NR_REQ_PER_RING __CONST_RING_SIZE(xen_pvcalls,
> > XEN_PAGE_SIZE)
> > @@ -40,9 +42,101 @@ struct pvcalls_bedata {
> >     static irqreturn_t pvcalls_front_event_handler(int irq, void *dev_id)
> >   {
> > +	struct xenbus_device *dev = dev_id;
> > +	struct pvcalls_bedata *bedata;
> > +	struct xen_pvcalls_response *rsp;
> > +	uint8_t *src, *dst;
> > +	int req_id = 0, more = 0, done = 0;
> > +
> > +	if (dev == NULL)
> > +		return IRQ_HANDLED;
> > +
> > +	bedata = dev_get_drvdata(&dev->dev);
> > +	if (bedata == NULL)
> > +		return IRQ_HANDLED;
> > +
> > +again:
> > +	while (RING_HAS_UNCONSUMED_RESPONSES(&bedata->ring)) {
> > +		rsp = RING_GET_RESPONSE(&bedata->ring, bedata->ring.rsp_cons);
> > +
> > +		req_id = rsp->req_id;
> > +		src = (uint8_t *)&bedata->rsp[req_id];
> > +		src += sizeof(rsp->req_id);
> > +		dst = (uint8_t *)rsp;
> > +		dst += sizeof(rsp->req_id);
> 
> These two lines can be combined (both src and dst)

I'll do that


> > +		memcpy(dst, src, sizeof(*rsp) - sizeof(rsp->req_id));
> > +		/*
> > +		 * First copy the rest of the data, then req_id. It is
> > +		 * paired with the barrier when accessing bedata->rsp.
> > +		 */
> > +		smp_wmb();
> > +		WRITE_ONCE(bedata->rsp[req_id].req_id, rsp->req_id);
> > +
> > +		done = 1;
> > +		bedata->ring.rsp_cons++;
> > +	}
> > +
> > +	RING_FINAL_CHECK_FOR_RESPONSES(&bedata->ring, more);
> > +	if (more)
> > +		goto again;
> > +	if (done)
> > +		wake_up(&bedata->inflight_req);
> >   	return IRQ_HANDLED;
> >   }
> >   +int pvcalls_front_socket(struct socket *sock)
> > +{
> > +	struct pvcalls_bedata *bedata;
> > +	struct xen_pvcalls_request *req;
> > +	int notify, req_id, ret;
> > +
> > +	if (!pvcalls_front_dev)
> > +		return -EACCES;
> > +	/*
> > +	 * PVCalls only supports domain AF_INET,
> > +	 * type SOCK_STREAM and protocol 0 sockets for now.
> > +	 *
> > +	 * Check socket type here, AF_INET and protocol checks are done
> > +	 * by the caller.
> > +	 */
> > +	if (sock->type != SOCK_STREAM)
> > +	    return -ENOTSUPP;
> > +
> > +	bedata = dev_get_drvdata(&pvcalls_front_dev->dev);
> > +
> > +	spin_lock(&bedata->pvcallss_lock);
> > +	req_id = bedata->ring.req_prod_pvt & (RING_SIZE(&bedata->ring) - 1);
> > +	if (RING_FULL(&bedata->ring) ||
> > +	    READ_ONCE(bedata->rsp[req_id].req_id) != PVCALLS_INVALID_ID) {
> > +		spin_unlock(&bedata->pvcallss_lock);
> > +		return -EAGAIN;
> > +	}
> > +	req = RING_GET_REQUEST(&bedata->ring, req_id);
> > +	req->req_id = req_id;
> > +	req->cmd = PVCALLS_SOCKET;
> > +	req->u.socket.id = (uint64_t) sock;
> > +	req->u.socket.domain = AF_INET;
> > +	req->u.socket.type = SOCK_STREAM;
> > +	req->u.socket.protocol = 0;
> > +
> > +	bedata->ring.req_prod_pvt++;
> > +	RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(&bedata->ring, notify);
> > +	spin_unlock(&bedata->pvcallss_lock);
> > +	if (notify)
> > +		notify_remote_via_irq(bedata->irq);
> > +
> > +	if (wait_event_interruptible(bedata->inflight_req,
> > +		READ_ONCE(bedata->rsp[req_id].req_id) == req_id) != 0)
> 
> "!= 0" can be dropped

OK


> > +		return -EINTR;
> > +
> > +	ret = bedata->rsp[req_id].ret;
> > +	/* read ret, then set this rsp slot to be reused */
> > +	smp_mb();
> > +	WRITE_ONCE(bedata->rsp[req_id].req_id, PVCALLS_INVALID_ID);
> > +
> > +	return ret;
> > +}
> > +
> >   static const struct xenbus_device_id pvcalls_front_ids[] = {
> >   	{ "pvcalls" },
> >   	{ "" }
> > diff --git a/drivers/xen/pvcalls-front.h b/drivers/xen/pvcalls-front.h
> > new file mode 100644
> > index 0000000..b7dabed
> > --- /dev/null
> > +++ b/drivers/xen/pvcalls-front.h
> > @@ -0,0 +1,8 @@
> > +#ifndef __PVCALLS_FRONT_H__
> > +#define __PVCALLS_FRONT_H__
> > +
> > +#include <linux/net.h>
> > +
> > +int pvcalls_front_socket(struct socket *sock);
> > +
> > +#endif
> 

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web