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
              Re: [PATCH v2 05/13] xen/pvcalls: implement bind command Stefano Stabellini <sstabellini@kernel.org> - 2017-08-01 00:20 +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
                Re: [Xen-devel] [PATCH v2 09/13] xen/pvcalls: implement recvmsg Stefano Stabellini <sstabellini@kernel.org> - 2017-08-01 00:30 +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
          Re: [PATCH v2 11/13] xen/pvcalls: implement release command Stefano Stabellini <sstabellini@kernel.org> - 2017-08-01 00:40 +0200
            Re: [PATCH v2 11/13] xen/pvcalls: implement release command Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-08-01 17:30 +0200
              Re: [PATCH v2 11/13] xen/pvcalls: implement release command Juergen Gross <jgross@suse.com> - 2017-08-01 17:40 +0200
                Re: [PATCH v2 11/13] xen/pvcalls: implement release command Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-08-01 18:10 +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
              Re: [PATCH v2 02/13] xen/pvcalls: connect to the backend Stefano Stabellini <sstabellini@kernel.org> - 2017-08-01 00:00 +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]

#1697571 — Re: [PATCH v2 09/13] xen/pvcalls: implement recvmsg

On 07/25/2017 05:22 PM, Stefano Stabellini wrote:
> Implement recvmsg by copying data from the "in" ring. If not enough data
> is available and the recvmsg call is blocking, then wait on the
> inflight_conn_req waitqueue. Take the active socket in_mutex so that
> only one function can access the ring at any given time.
>
> 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.
>
> Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> CC: boris.ostrovsky@oracle.com
> CC: jgross@suse.com
> ---
>  drivers/xen/pvcalls-front.c | 106 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/xen/pvcalls-front.h |   4 ++
>  2 files changed, 110 insertions(+)
>
> diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c
> index d8ed280..b4ca569 100644
> --- a/drivers/xen/pvcalls-front.c
> +++ b/drivers/xen/pvcalls-front.c
> @@ -96,6 +96,20 @@ static int pvcalls_front_write_todo(struct sock_mapping *map)
>  	return size - pvcalls_queued(prod, cons, size);
>  }
>  
> +static bool pvcalls_front_read_todo(struct sock_mapping *map)
> +{
> +	struct pvcalls_data_intf *intf = map->active.ring;
> +	RING_IDX cons, prod;
> +	int32_t error;
> +
> +	cons = intf->in_cons;
> +	prod = intf->in_prod;
> +	error = intf->in_error;
> +	return (error != 0 ||
> +		pvcalls_queued(prod, cons,
> +			       XEN_FLEX_RING_SIZE(intf->ring_order))) != 0;
> +}
> +
>  static irqreturn_t pvcalls_front_event_handler(int irq, void *dev_id)
>  {
>  	struct xenbus_device *dev = dev_id;
> @@ -418,6 +432,98 @@ int pvcalls_front_sendmsg(struct socket *sock, struct msghdr *msg,
>  	return tot_sent;
>  }
>  
> +static int __read_ring(struct pvcalls_data_intf *intf,
> +		       struct pvcalls_data *data,
> +		       struct iov_iter *msg_iter,
> +		       size_t len, int flags)
> +{
> +	RING_IDX cons, prod, size, masked_prod, masked_cons;
> +	RING_IDX array_size = XEN_FLEX_RING_SIZE(intf->ring_order);
> +	int32_t error;
> +
> +	cons = intf->in_cons;
> +	prod = intf->in_prod;
> +	error = intf->in_error;
> +	/* get pointers before reading from the ring */
> +	virt_rmb();
> +	if (error < 0)
> +		return error;
> +
> +	size = pvcalls_queued(prod, cons, array_size);
> +	masked_prod = pvcalls_mask(prod, array_size);
> +	masked_cons = pvcalls_mask(cons, array_size);
> +
> +	if (size == 0)
> +		return 0;
> +
> +	if (len > size)
> +		len = size;
> +
> +	if (masked_prod > masked_cons) {
> +		copy_to_iter(data->in + masked_cons, len, msg_iter);
> +	} else {
> +		if (len > (array_size - masked_cons)) {
> +			copy_to_iter(data->in + masked_cons,
> +				     array_size - masked_cons, msg_iter);
> +			copy_to_iter(data->in,
> +				     len - (array_size - masked_cons),
> +				     msg_iter);
> +		} else {
> +			copy_to_iter(data->in + masked_cons, len, msg_iter);
> +		}
> +	}
> +	/* read data from the ring before increasing the index */
> +	virt_mb();
> +	if (!(flags & MSG_PEEK))
> +		intf->in_cons += len;
> +
> +	return len;
> +}
> +
> +int pvcalls_front_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> +		     int flags)
> +{
> +	struct pvcalls_bedata *bedata;
> +	int ret = -EAGAIN;
> +	struct sock_mapping *map;
> +	int count = 0;
> +
> +	if (!pvcalls_front_dev)
> +		return -ENOTCONN;
> +	bedata = dev_get_drvdata(&pvcalls_front_dev->dev);
> +
> +	map = (struct sock_mapping *) READ_ONCE(sock->sk->sk_send_head);
> +	if (!map)
> +		return -ENOTSOCK;
> +
> +	if (flags & (MSG_CMSG_CLOEXEC|MSG_ERRQUEUE|MSG_OOB|MSG_TRUNC))
> +		return -EOPNOTSUPP;
> +
> +	mutex_lock(&map->active.in_mutex);
> +	if (len > XEN_FLEX_RING_SIZE(map->active.ring->ring_order))
> +		len = XEN_FLEX_RING_SIZE(map->active.ring->ring_order);
> +
> +	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
and/or socket's O_NONBLOCK?

-boris


> +	ret = __read_ring(map->active.ring, &map->active.data,
> +			  &msg->msg_iter, len, flags);
> +
> +	if (ret > 0)
> +		notify_remote_via_irq(map->active.irq);
> +	if (ret == 0)
> +		ret = -EAGAIN;
> +	if (ret == -ENOTCONN)
> +		ret = 0;
> +
> +	mutex_unlock(&map->active.in_mutex);
> +	return ret;
> +}
> +
>  int pvcalls_front_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
>  {
>  	struct pvcalls_bedata *bedata;
> diff --git a/drivers/xen/pvcalls-front.h b/drivers/xen/pvcalls-front.h
> index d937c24..de24041 100644
> --- a/drivers/xen/pvcalls-front.h
> +++ b/drivers/xen/pvcalls-front.h
> @@ -16,5 +16,9 @@ int pvcalls_front_accept(struct socket *sock,
>  int pvcalls_front_sendmsg(struct socket *sock,
>  			  struct msghdr *msg,
>  			  size_t len);
> +int pvcalls_front_recvmsg(struct socket *sock,
> +			  struct msghdr *msg,
> +			  size_t len,
> +			  int flags);
>  
>  #endif

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

#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]

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

On Thu, 27 Jul 2017, Boris Ostrovsky wrote:
> 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.

It tries to mitigate scheduling latencies on both ends (dom0 and domU)
when the ring buffer is the bottleneck (high bandwidth connections). But
to be honest with you, it's mostly beneficial in the sendmsg case,
because for recvmsg we also introduce a busy-wait in regular
circumstances, when no data is actually available. I confirmed this
statement with a quick iperf test. I'll remove the spin from recvmsg and
keep it in sendmsg.


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

you are right 

[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]

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

On Thu, 27 Jul 2017, Boris Ostrovsky wrote:
> > +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?

No, I don't think they should: dev_set_drvdata is called in the probe
function (pvcalls_front_probe). I'll remove it.


> > +
> > +	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?

The lock is to make sure there are no recvmsg or sendmsg in progress. We
are sure that no newer sendmsg or recvmsg are waiting for
pvcalls_front_release to release the lock because before send a message
to the backend we set sk_send_head to NULL.


> > +	} 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]

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

On 07/31/2017 06:34 PM, Stefano Stabellini wrote:
> On Thu, 27 Jul 2017, Boris Ostrovsky wrote:
>>> +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?
> No, I don't think they should: dev_set_drvdata is called in the probe
> function (pvcalls_front_probe). I'll remove it.
>
>
>>> +
>>> +	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?
> The lock is to make sure there are no recvmsg or sendmsg in progress. We
> are sure that no newer sendmsg or recvmsg are waiting for
> pvcalls_front_release to release the lock because before send a message
> to the backend we set sk_send_head to NULL.

Is there a chance that whoever is potentially calling send/rcvmsg has
checked that sk_send_head is non-NULL but hasn't grabbed the lock yet?

Freeing a structure containing a lock right after releasing the lock
looks weird (to me). Is there any other way to synchronize with
sender/receiver? Any other lock?

BTW, I also noticed that in rcvmsg you are calling
wait_event_interruptible() while holding the lock. Have you tested with
CONFIG_DEBUG_ATOMIC_SLEEP? (or maybe it's some other config  option that
would complain about those sorts of thing)

-boris

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

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

On 01/08/17 17:23, Boris Ostrovsky wrote:
> On 07/31/2017 06:34 PM, Stefano Stabellini wrote:
>> On Thu, 27 Jul 2017, Boris Ostrovsky wrote:
>>>> +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?
>> No, I don't think they should: dev_set_drvdata is called in the probe
>> function (pvcalls_front_probe). I'll remove it.
>>
>>
>>>> +
>>>> +	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?
>> The lock is to make sure there are no recvmsg or sendmsg in progress. We
>> are sure that no newer sendmsg or recvmsg are waiting for
>> pvcalls_front_release to release the lock because before send a message
>> to the backend we set sk_send_head to NULL.
> 
> Is there a chance that whoever is potentially calling send/rcvmsg has
> checked that sk_send_head is non-NULL but hasn't grabbed the lock yet?
> 
> Freeing a structure containing a lock right after releasing the lock
> looks weird (to me). Is there any other way to synchronize with
> sender/receiver? Any other lock?

Right. This looks fishy. Either you don't need the locks or you can't
just free the area right after releasing the lock.

> BTW, I also noticed that in rcvmsg you are calling
> wait_event_interruptible() while holding the lock. Have you tested with
> CONFIG_DEBUG_ATOMIC_SLEEP? (or maybe it's some other config  option that
> would complain about those sorts of thing)

I believe sleeping while holding a mutex is allowed. Sleeping in
spinlocked paths is bad.

BTW: You are looking for CONFIG_DEBUG_MUTEXES (see
Documentation/locking/mutex-design.txt ).


Juergen

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

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

>> BTW, I also noticed that in rcvmsg you are calling
>> wait_event_interruptible() while holding the lock. Have you tested with
>> CONFIG_DEBUG_ATOMIC_SLEEP? (or maybe it's some other config  option that
>> would complain about those sorts of thing)
> I believe sleeping while holding a mutex is allowed. Sleeping in
> spinlocked paths is bad.

Oh, right. I was thinking about spinlocks. Sorry.

-boris

>
> BTW: You are looking for CONFIG_DEBUG_MUTEXES (see
> Documentation/locking/mutex-design.txt ).
>
>
> Juergen

[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]

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

On Thu, 27 Jul 2017, Boris Ostrovsky wrote:
> >>>   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'.

Yes, I'll do that

[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