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

Groups > linux.kernel > #1241898 > unrolled thread

[PATCH 02/10] Drivers: hv: utils: run polling callback always in interrupt context

Back to article view | Back to linux.kernel

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

Contents

  [PATCH 02/10] Drivers: hv: utils: run polling callback always in interrupt context "K. Y. Srinivasan" <kys@microsoft.com> - 2015-10-08 02:40 +0200
    Re: [PATCH 02/10] Drivers: hv: utils: run polling callback always in interrupt context Vitaly Kuznetsov <vkuznets@redhat.com> - 2015-10-08 15:30 +0200
      Re: [PATCH 02/10] Drivers: hv: utils: run polling callback always in  interrupt context Olaf Hering <olaf@aepfle.de> - 2015-10-08 15:40 +0200
        Re: [PATCH 02/10] Drivers: hv: utils: run polling callback always in interrupt context Vitaly Kuznetsov <vkuznets@redhat.com> - 2015-10-08 16:00 +0200
          RE: [PATCH 02/10] Drivers: hv: utils: run polling callback always in  interrupt context KY Srinivasan <kys@microsoft.com> - 2015-10-08 17:00 +0200
            Re: [PATCH 02/10] Drivers: hv: utils: run polling callback always in  interrupt context Olaf Hering <olaf@aepfle.de> - 2015-10-09 09:10 +0200
              Re: [PATCH 02/10] Drivers: hv: utils: run polling callback always in interrupt context Vitaly Kuznetsov <vkuznets@redhat.com> - 2015-10-09 12:20 +0200
                Re: [PATCH 02/10] Drivers: hv: utils: run polling callback always in  interrupt context Olaf Hering <olaf@aepfle.de> - 2015-10-09 13:30 +0200
                  RE: [PATCH 02/10] Drivers: hv: utils: run polling callback always in  interrupt context KY Srinivasan <kys@microsoft.com> - 2015-10-12 08:10 +0200
              Re: [PATCH 02/10] Drivers: hv: utils: run polling callback always in  interrupt context Olaf Hering <olaf@aepfle.de> - 2015-10-13 11:50 +0200
                RE: [PATCH 02/10] Drivers: hv: utils: run polling callback always in  interrupt context KY Srinivasan <kys@microsoft.com> - 2015-10-13 23:40 +0200

#1241898 — [PATCH 02/10] Drivers: hv: utils: run polling callback always in interrupt context

From: Olaf Hering <olaf@aepfle.de>

All channel interrupts are bound to specific VCPUs in the guest
at the point channel is created. While currently, we invoke the
polling function on the correct CPU (the CPU to which the channel
is bound to) in some cases we may run the polling function in
a non-interrupt context. This  potentially can cause an issue as the
polling function can be interrupted by the channel callback function.
Fix the issue by running the polling function on the appropriate CPU
at interrupt level. Additional details of the issue being addressed by
this patch are given below:

Currently hv_fcopy_onchannelcallback is called from interrupts and also
via the ->write function of hv_utils. Since the used global variables to
maintain state are not thread safe the state can get out of sync.
This affects the variable state as well as the channel inbound buffer.

As suggested by KY adjust hv_poll_channel to always run the given
callback on the cpu which the channel is bound to. This avoids the need
for locking because all the util services are single threaded and only
one transaction is active at any given point in time.

Additionally, remove the context variable, they will always be the same as
recv_channel.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/hv/hv_fcopy.c     |   37 +++++++++++++------------------------
 drivers/hv/hv_kvp.c       |   28 ++++++++++------------------
 drivers/hv/hv_snapshot.c  |   29 +++++++++++------------------
 drivers/hv/hyperv_vmbus.h |    6 +-----
 4 files changed, 35 insertions(+), 65 deletions(-)

diff --git a/drivers/hv/hv_fcopy.c b/drivers/hv/hv_fcopy.c
index bbdec50..4eab465 100644
--- a/drivers/hv/hv_fcopy.c
+++ b/drivers/hv/hv_fcopy.c
@@ -51,7 +51,6 @@ static struct {
 	struct hv_fcopy_hdr  *fcopy_msg; /* current message */
 	struct vmbus_channel *recv_channel; /* chn we got the request */
 	u64 recv_req_id; /* request ID. */
-	void *fcopy_context; /* for the channel callback */
 } fcopy_transaction;
 
 static void fcopy_respond_to_host(int error);
@@ -67,6 +66,13 @@ static struct hvutil_transport *hvt;
  */
 static int dm_reg_value;
 
+static void fcopy_poll_wrapper(void *channel)
+{
+	/* Transaction is finished, reset the state here to avoid races. */
+	fcopy_transaction.state = HVUTIL_READY;
+	hv_fcopy_onchannelcallback(channel);
+}
+
 static void fcopy_timeout_func(struct work_struct *dummy)
 {
 	/*
@@ -74,13 +80,7 @@ static void fcopy_timeout_func(struct work_struct *dummy)
 	 * process the pending transaction.
 	 */
 	fcopy_respond_to_host(HV_E_FAIL);
-
-	/* Transaction is finished, reset the state. */
-	if (fcopy_transaction.state > HVUTIL_READY)
-		fcopy_transaction.state = HVUTIL_READY;
-
-	hv_poll_channel(fcopy_transaction.fcopy_context,
-			hv_fcopy_onchannelcallback);
+	hv_poll_channel(fcopy_transaction.recv_channel, fcopy_poll_wrapper);
 }
 
 static int fcopy_handle_handshake(u32 version)
@@ -108,9 +108,9 @@ static int fcopy_handle_handshake(u32 version)
 		return -EINVAL;
 	}
 	pr_debug("FCP: userspace daemon ver. %d registered\n", version);
+	/* Forward state for hv_fcopy_onchannelcallback */
 	fcopy_transaction.state = HVUTIL_READY;
-	hv_poll_channel(fcopy_transaction.fcopy_context,
-			hv_fcopy_onchannelcallback);
+	hv_poll_channel(fcopy_transaction.recv_channel, fcopy_poll_wrapper);
 	return 0;
 }
 
@@ -227,15 +227,8 @@ void hv_fcopy_onchannelcallback(void *context)
 	int util_fw_version;
 	int fcopy_srv_version;
 
-	if (fcopy_transaction.state > HVUTIL_READY) {
-		/*
-		 * We will defer processing this callback once
-		 * the current transaction is complete.
-		 */
-		fcopy_transaction.fcopy_context = context;
+	if (fcopy_transaction.state > HVUTIL_READY)
 		return;
-	}
-	fcopy_transaction.fcopy_context = NULL;
 
 	vmbus_recvpacket(channel, recv_buffer, PAGE_SIZE * 2, &recvlen,
 			 &requestid);
@@ -295,9 +288,6 @@ static int fcopy_on_msg(void *msg, int len)
 	if (fcopy_transaction.state == HVUTIL_DEVICE_INIT)
 		return fcopy_handle_handshake(*val);
 
-	if (fcopy_transaction.state != HVUTIL_USERSPACE_REQ)
-		return -EINVAL;
-
 	/*
 	 * Complete the transaction by forwarding the result
 	 * to the host. But first, cancel the timeout.
@@ -305,9 +295,8 @@ static int fcopy_on_msg(void *msg, int len)
 	if (cancel_delayed_work_sync(&fcopy_timeout_work)) {
 		fcopy_transaction.state = HVUTIL_USERSPACE_RECV;
 		fcopy_respond_to_host(*val);
-		fcopy_transaction.state = HVUTIL_READY;
-		hv_poll_channel(fcopy_transaction.fcopy_context,
-				hv_fcopy_onchannelcallback);
+		hv_poll_channel(fcopy_transaction.recv_channel,
+				fcopy_poll_wrapper);
 	}
 
 	return 0;
diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
index e6aa33a..2a3420c 100644
--- a/drivers/hv/hv_kvp.c
+++ b/drivers/hv/hv_kvp.c
@@ -66,7 +66,6 @@ static struct {
 	struct hv_kvp_msg  *kvp_msg; /* current message */
 	struct vmbus_channel *recv_channel; /* chn we got the request */
 	u64 recv_req_id; /* request ID. */
-	void *kvp_context; /* for the channel callback */
 } kvp_transaction;
 
 /*
@@ -94,6 +93,13 @@ static struct hvutil_transport *hvt;
  */
 #define HV_DRV_VERSION           "3.1"
 
+static void kvp_poll_wrapper(void *channel)
+{
+	/* Transaction is finished, reset the state here to avoid races. */
+	kvp_transaction.state = HVUTIL_READY;
+	hv_kvp_onchannelcallback(channel);
+}
+
 static void
 kvp_register(int reg_value)
 {
@@ -121,12 +127,7 @@ static void kvp_timeout_func(struct work_struct *dummy)
 	 */
 	kvp_respond_to_host(NULL, HV_E_FAIL);
 
-	/* Transaction is finished, reset the state. */
-	if (kvp_transaction.state > HVUTIL_READY)
-		kvp_transaction.state = HVUTIL_READY;
-
-	hv_poll_channel(kvp_transaction.kvp_context,
-			hv_kvp_onchannelcallback);
+	hv_poll_channel(kvp_transaction.recv_channel, kvp_poll_wrapper);
 }
 
 static int kvp_handle_handshake(struct hv_kvp_msg *msg)
@@ -218,9 +219,7 @@ static int kvp_on_msg(void *msg, int len)
 	 */
 	if (cancel_delayed_work_sync(&kvp_timeout_work)) {
 		kvp_respond_to_host(message, error);
-		kvp_transaction.state = HVUTIL_READY;
-		hv_poll_channel(kvp_transaction.kvp_context,
-				hv_kvp_onchannelcallback);
+		hv_poll_channel(kvp_transaction.recv_channel, kvp_poll_wrapper);
 	}
 
 	return 0;
@@ -596,15 +595,8 @@ void hv_kvp_onchannelcallback(void *context)
 	int util_fw_version;
 	int kvp_srv_version;
 
-	if (kvp_transaction.state > HVUTIL_READY) {
-		/*
-		 * We will defer processing this callback once
-		 * the current transaction is complete.
-		 */
-		kvp_transaction.kvp_context = context;
+	if (kvp_transaction.state > HVUTIL_READY)
 		return;
-	}
-	kvp_transaction.kvp_context = NULL;
 
 	vmbus_recvpacket(channel, recv_buffer, PAGE_SIZE * 4, &recvlen,
 			 &requestid);
diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c
index 815405f..a548ae4 100644
--- a/drivers/hv/hv_snapshot.c
+++ b/drivers/hv/hv_snapshot.c
@@ -53,7 +53,6 @@ static struct {
 	struct vmbus_channel *recv_channel; /* chn we got the request */
 	u64 recv_req_id; /* request ID. */
 	struct hv_vss_msg  *msg; /* current message */
-	void *vss_context; /* for the channel callback */
 } vss_transaction;
 
 
@@ -74,6 +73,13 @@ static void vss_timeout_func(struct work_struct *dummy);
 static DECLARE_DELAYED_WORK(vss_timeout_work, vss_timeout_func);
 static DECLARE_WORK(vss_send_op_work, vss_send_op);
 
+static void vss_poll_wrapper(void *channel)
+{
+	/* Transaction is finished, reset the state here to avoid races. */
+	vss_transaction.state = HVUTIL_READY;
+	hv_vss_onchannelcallback(channel);
+}
+
 /*
  * Callback when data is received from user mode.
  */
@@ -86,12 +92,7 @@ static void vss_timeout_func(struct work_struct *dummy)
 	pr_warn("VSS: timeout waiting for daemon to reply\n");
 	vss_respond_to_host(HV_E_FAIL);
 
-	/* Transaction is finished, reset the state. */
-	if (vss_transaction.state > HVUTIL_READY)
-		vss_transaction.state = HVUTIL_READY;
-
-	hv_poll_channel(vss_transaction.vss_context,
-			hv_vss_onchannelcallback);
+	hv_poll_channel(vss_transaction.recv_channel, vss_poll_wrapper);
 }
 
 static int vss_handle_handshake(struct hv_vss_msg *vss_msg)
@@ -138,9 +139,8 @@ static int vss_on_msg(void *msg, int len)
 		if (cancel_delayed_work_sync(&vss_timeout_work)) {
 			vss_respond_to_host(vss_msg->error);
 			/* Transaction is finished, reset the state. */
-			vss_transaction.state = HVUTIL_READY;
-			hv_poll_channel(vss_transaction.vss_context,
-					hv_vss_onchannelcallback);
+			hv_poll_channel(vss_transaction.recv_channel,
+					vss_poll_wrapper);
 		}
 	} else {
 		/* This is a spurious call! */
@@ -238,15 +238,8 @@ void hv_vss_onchannelcallback(void *context)
 	struct icmsg_hdr *icmsghdrp;
 	struct icmsg_negotiate *negop = NULL;
 
-	if (vss_transaction.state > HVUTIL_READY) {
-		/*
-		 * We will defer processing this callback once
-		 * the current transaction is complete.
-		 */
-		vss_transaction.vss_context = context;
+	if (vss_transaction.state > HVUTIL_READY)
 		return;
-	}
-	vss_transaction.vss_context = NULL;
 
 	vmbus_recvpacket(channel, recv_buffer, PAGE_SIZE * 2, &recvlen,
 			 &requestid);
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index f26599b..40c0c855 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -769,11 +769,7 @@ static inline void hv_poll_channel(struct vmbus_channel *channel,
 	if (!channel)
 		return;
 
-	if (channel->target_cpu != smp_processor_id())
-		smp_call_function_single(channel->target_cpu,
-					 cb, channel, true);
-	else
-		cb(channel);
+	smp_call_function_single(channel->target_cpu, cb, channel, true);
 }
 
 enum hvutil_device_state {
-- 
1.7.4.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1242342

"K. Y. Srinivasan" <kys@microsoft.com> writes:

> From: Olaf Hering <olaf@aepfle.de>
>
> All channel interrupts are bound to specific VCPUs in the guest
> at the point channel is created. While currently, we invoke the
> polling function on the correct CPU (the CPU to which the channel
> is bound to) in some cases we may run the polling function in
> a non-interrupt context. This  potentially can cause an issue as the
> polling function can be interrupted by the channel callback function.
> Fix the issue by running the polling function on the appropriate CPU
> at interrupt level. Additional details of the issue being addressed by
> this patch are given below:
>
> Currently hv_fcopy_onchannelcallback is called from interrupts and also
> via the ->write function of hv_utils. Since the used global variables to
> maintain state are not thread safe the state can get out of sync.
> This affects the variable state as well as the channel inbound buffer.
>
> As suggested by KY adjust hv_poll_channel to always run the given
> callback on the cpu which the channel is bound to. This avoids the need
> for locking because all the util services are single threaded and only
> one transaction is active at any given point in time.
>
> Additionally, remove the context variable, they will always be the same as
> recv_channel.
>
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> ---
>  drivers/hv/hv_fcopy.c     |   37 +++++++++++++------------------------
>  drivers/hv/hv_kvp.c       |   28 ++++++++++------------------
>  drivers/hv/hv_snapshot.c  |   29 +++++++++++------------------
>  drivers/hv/hyperv_vmbus.h |    6 +-----
>  4 files changed, 35 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/hv/hv_fcopy.c b/drivers/hv/hv_fcopy.c
> index bbdec50..4eab465 100644
> --- a/drivers/hv/hv_fcopy.c
> +++ b/drivers/hv/hv_fcopy.c
> @@ -51,7 +51,6 @@ static struct {
>  	struct hv_fcopy_hdr  *fcopy_msg; /* current message */
>  	struct vmbus_channel *recv_channel; /* chn we got the request */
>  	u64 recv_req_id; /* request ID. */
> -	void *fcopy_context; /* for the channel callback */
>  } fcopy_transaction;
>
>  static void fcopy_respond_to_host(int error);
> @@ -67,6 +66,13 @@ static struct hvutil_transport *hvt;
>   */
>  static int dm_reg_value;
>
> +static void fcopy_poll_wrapper(void *channel)
> +{
> +	/* Transaction is finished, reset the state here to avoid races. */
> +	fcopy_transaction.state = HVUTIL_READY;
> +	hv_fcopy_onchannelcallback(channel);
> +}
> +
>  static void fcopy_timeout_func(struct work_struct *dummy)
>  {
>  	/*
> @@ -74,13 +80,7 @@ static void fcopy_timeout_func(struct work_struct *dummy)
>  	 * process the pending transaction.
>  	 */
>  	fcopy_respond_to_host(HV_E_FAIL);
> -
> -	/* Transaction is finished, reset the state. */
> -	if (fcopy_transaction.state > HVUTIL_READY)
> -		fcopy_transaction.state = HVUTIL_READY;
> -
> -	hv_poll_channel(fcopy_transaction.fcopy_context,
> -			hv_fcopy_onchannelcallback);
> +	hv_poll_channel(fcopy_transaction.recv_channel, fcopy_poll_wrapper);
>  }
>
>  static int fcopy_handle_handshake(u32 version)
> @@ -108,9 +108,9 @@ static int fcopy_handle_handshake(u32 version)
>  		return -EINVAL;
>  	}
>  	pr_debug("FCP: userspace daemon ver. %d registered\n", version);
> +	/* Forward state for hv_fcopy_onchannelcallback */
>  	fcopy_transaction.state = HVUTIL_READY;
> -	hv_poll_channel(fcopy_transaction.fcopy_context,
> -			hv_fcopy_onchannelcallback);
> +	hv_poll_channel(fcopy_transaction.recv_channel, fcopy_poll_wrapper);
>  	return 0;
>  }
>
> @@ -227,15 +227,8 @@ void hv_fcopy_onchannelcallback(void *context)
>  	int util_fw_version;
>  	int fcopy_srv_version;
>
> -	if (fcopy_transaction.state > HVUTIL_READY) {
> -		/*
> -		 * We will defer processing this callback once
> -		 * the current transaction is complete.
> -		 */
> -		fcopy_transaction.fcopy_context = context;
> +	if (fcopy_transaction.state > HVUTIL_READY)
>  		return;
> -	}
> -	fcopy_transaction.fcopy_context = NULL;
>
>  	vmbus_recvpacket(channel, recv_buffer, PAGE_SIZE * 2, &recvlen,
>  			 &requestid);
> @@ -295,9 +288,6 @@ static int fcopy_on_msg(void *msg, int len)
>  	if (fcopy_transaction.state == HVUTIL_DEVICE_INIT)
>  		return fcopy_handle_handshake(*val);
>
> -	if (fcopy_transaction.state != HVUTIL_USERSPACE_REQ)
> -		return -EINVAL;
> -

This particular change seems unrelated and I'm unsure it's safe to
remove this check. It is meant to protect against daemon screwing the
protocol and writing to the device when it wasn't requested for an
action. It is correct to propagate -EINVAL in this case. Or am I missing
something and the check is redundant now?

Thanks,

[...]

-- 
  Vitaly
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1242362 — Re: [PATCH 02/10] Drivers: hv: utils: run polling callback always in interrupt context

On Thu, Oct 08, Vitaly Kuznetsov wrote:

> > @@ -295,9 +288,6 @@ static int fcopy_on_msg(void *msg, int len)
> >  	if (fcopy_transaction.state == HVUTIL_DEVICE_INIT)
> >  		return fcopy_handle_handshake(*val);
> >
> > -	if (fcopy_transaction.state != HVUTIL_USERSPACE_REQ)
> > -		return -EINVAL;
> > -
> 
> This particular change seems unrelated and I'm unsure it's safe to
> remove this check. It is meant to protect against daemon screwing the
> protocol and writing to the device when it wasn't requested for an
> action. It is correct to propagate -EINVAL in this case. Or am I missing
> something and the check is redundant now?

What can happen if there is an odd write request? If there is a timeout
scheduled some return value will be sent to the host. Then the state is
set to RESET and eventually vmbus_recvpacket will receive something.
That something will be processed and passed to the daemon.

If there was no timeout scheduled the write will just return.

Olaf
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1242380

Olaf Hering <olaf@aepfle.de> writes:

> On Thu, Oct 08, Vitaly Kuznetsov wrote:
>
>> > @@ -295,9 +288,6 @@ static int fcopy_on_msg(void *msg, int len)
>> >  	if (fcopy_transaction.state == HVUTIL_DEVICE_INIT)
>> >  		return fcopy_handle_handshake(*val);
>> >
>> > -	if (fcopy_transaction.state != HVUTIL_USERSPACE_REQ)
>> > -		return -EINVAL;
>> > -
>> 
>> This particular change seems unrelated and I'm unsure it's safe to
>> remove this check. It is meant to protect against daemon screwing the
>> protocol and writing to the device when it wasn't requested for an
>> action. It is correct to propagate -EINVAL in this case. Or am I missing
>> something and the check is redundant now?
>
> What can happen if there is an odd write request?

I think we don't want to propagate misbehaving daemon's data to the
host -- let's cut it here. E.g. imagine there is no communication going
on and daemon starts writing something to the device. In case we remove
the check we'll be doing fcopy_respond_to_host() for each daemon's write
flooding the host.

> If there is a timeout
> scheduled some return value will be sent to the host. Then the state is
> set to RESET and eventually vmbus_recvpacket will receive something.
> That something will be processed and passed to the daemon.
>
> If there was no timeout scheduled the write will just return.

yes, but after doing fcopy_respond_to_host(). I'd suggest we leave the
check in place, better safe than sorry.

-- 
  Vitaly
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1242474 — RE: [PATCH 02/10] Drivers: hv: utils: run polling callback always in interrupt context

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Thursday, October 8, 2015 6:53 AM
> To: Olaf Hering <olaf@aepfle.de>
> Cc: KY Srinivasan <kys@microsoft.com>; gregkh@linuxfoundation.org; linux-
> kernel@vger.kernel.org; devel@linuxdriverproject.org; apw@canonical.com;
> jasowang@redhat.com
> Subject: Re: [PATCH 02/10] Drivers: hv: utils: run polling callback always in
> interrupt context
> 
> Olaf Hering <olaf@aepfle.de> writes:
> 
> > On Thu, Oct 08, Vitaly Kuznetsov wrote:
> >
> >> > @@ -295,9 +288,6 @@ static int fcopy_on_msg(void *msg, int len)
> >> >  	if (fcopy_transaction.state == HVUTIL_DEVICE_INIT)
> >> >  		return fcopy_handle_handshake(*val);
> >> >
> >> > -	if (fcopy_transaction.state != HVUTIL_USERSPACE_REQ)
> >> > -		return -EINVAL;
> >> > -
> >>
> >> This particular change seems unrelated and I'm unsure it's safe to
> >> remove this check. It is meant to protect against daemon screwing the
> >> protocol and writing to the device when it wasn't requested for an
> >> action. It is correct to propagate -EINVAL in this case. Or am I missing
> >> something and the check is redundant now?
> >
> > What can happen if there is an odd write request?
> 
> I think we don't want to propagate misbehaving daemon's data to the
> host -- let's cut it here. E.g. imagine there is no communication going
> on and daemon starts writing something to the device. In case we remove
> the check we'll be doing fcopy_respond_to_host() for each daemon's write
> flooding the host.
> 
> > If there is a timeout
> > scheduled some return value will be sent to the host. Then the state is
> > set to RESET and eventually vmbus_recvpacket will receive something.
> > That something will be processed and passed to the daemon.
> >
> > If there was no timeout scheduled the write will just return.
> 
> yes, but after doing fcopy_respond_to_host(). I'd suggest we leave the
> check in place, better safe than sorry.

Agreed; Olaf, if it is ok with you, I can fix it up and send.

Regards,

K. Y
> 
> --
>   Vitaly
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1243073 — Re: [PATCH 02/10] Drivers: hv: utils: run polling callback always in interrupt context

On Thu, Oct 08, KY Srinivasan wrote:

> > yes, but after doing fcopy_respond_to_host(). I'd suggest we leave the
> > check in place, better safe than sorry.
> 
> Agreed; Olaf, if it is ok with you, I can fix it up and send.

I will retest with this part reverted. I think without two code paths
entering hv_fcopy_callback it should be ok to leave this check in.

Olaf
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1243219

Olaf Hering <olaf@aepfle.de> writes:

> On Thu, Oct 08, KY Srinivasan wrote:
>
>> > yes, but after doing fcopy_respond_to_host(). I'd suggest we leave the
>> > check in place, better safe than sorry.
>> 
>> Agreed; Olaf, if it is ok with you, I can fix it up and send.
>
> I will retest with this part reverted. I think without two code paths
> entering hv_fcopy_callback it should be ok to leave this check in.

I think hv_fcopy_callback() is not involved here: we call fcopy_on_msg()
every time userspace daemon writes to the device and it is not anyhow
synchronized with host-guest communication. 

-- 
  Vitaly
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1243262 — Re: [PATCH 02/10] Drivers: hv: utils: run polling callback always in interrupt context

On Fri, Oct 09, Vitaly Kuznetsov wrote:

> Olaf Hering <olaf@aepfle.de> writes:
> 
> > On Thu, Oct 08, KY Srinivasan wrote:
> >
> >> > yes, but after doing fcopy_respond_to_host(). I'd suggest we leave the
> >> > check in place, better safe than sorry.
> >> 
> >> Agreed; Olaf, if it is ok with you, I can fix it up and send.
> >
> > I will retest with this part reverted. I think without two code paths
> > entering hv_fcopy_callback it should be ok to leave this check in.
> 
> I think hv_fcopy_callback() is not involved here: we call fcopy_on_msg()
> every time userspace daemon writes to the device and it is not anyhow
> synchronized with host-guest communication. 

An earlier variant of this patch used locks around the vmbus_recvpacket
and the result was used to decide which thread of execution notifies the
daemon. I think if the interrupt ran earlier than the daemon did the
write then the state expected in fcopy_on_msg would obviously be wrong.
As a result the daemon will just terminate with EFAULT. With the check
removed it would proceed, and either not chancel the timeout or
vmbus_recvpacket reads nothing.

But now that it is single threaded the state in fcopy_on_msg should be
as expected. As said, will retest. Either later today or on Monday.

Olaf
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1244403 — RE: [PATCH 02/10] Drivers: hv: utils: run polling callback always in interrupt context

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogT2xhZiBIZXJpbmcgW21h
aWx0bzpvbGFmQGFlcGZsZS5kZV0NCj4gU2VudDogRnJpZGF5LCBPY3RvYmVyIDksIDIwMTUgNDoy
OSBBTQ0KPiBUbzogVml0YWx5IEt1em5ldHNvdiA8dmt1em5ldHNAcmVkaGF0LmNvbT4NCj4gQ2M6
IEtZIFNyaW5pdmFzYW4gPGt5c0BtaWNyb3NvZnQuY29tPjsgZ3JlZ2toQGxpbnV4Zm91bmRhdGlv
bi5vcmc7IGxpbnV4LQ0KPiBrZXJuZWxAdmdlci5rZXJuZWwub3JnOyBkZXZlbEBsaW51eGRyaXZl
cnByb2plY3Qub3JnOyBhcHdAY2Fub25pY2FsLmNvbTsNCj4gamFzb3dhbmdAcmVkaGF0LmNvbQ0K
PiBTdWJqZWN0OiBSZTogW1BBVENIIDAyLzEwXSBEcml2ZXJzOiBodjogdXRpbHM6IHJ1biBwb2xs
aW5nIGNhbGxiYWNrIGFsd2F5cyBpbg0KPiBpbnRlcnJ1cHQgY29udGV4dA0KPiANCj4gT24gRnJp
LCBPY3QgMDksIFZpdGFseSBLdXpuZXRzb3Ygd3JvdGU6DQo+IA0KPiA+IE9sYWYgSGVyaW5nIDxv
bGFmQGFlcGZsZS5kZT4gd3JpdGVzOg0KPiA+DQo+ID4gPiBPbiBUaHUsIE9jdCAwOCwgS1kgU3Jp
bml2YXNhbiB3cm90ZToNCj4gPiA+DQo+ID4gPj4gPiB5ZXMsIGJ1dCBhZnRlciBkb2luZyBmY29w
eV9yZXNwb25kX3RvX2hvc3QoKS4gSSdkIHN1Z2dlc3Qgd2UgbGVhdmUNCj4gdGhlDQo+ID4gPj4g
PiBjaGVjayBpbiBwbGFjZSwgYmV0dGVyIHNhZmUgdGhhbiBzb3JyeS4NCj4gPiA+Pg0KPiA+ID4+
IEFncmVlZDsgT2xhZiwgaWYgaXQgaXMgb2sgd2l0aCB5b3UsIEkgY2FuIGZpeCBpdCB1cCBhbmQg
c2VuZC4NCj4gPiA+DQo+ID4gPiBJIHdpbGwgcmV0ZXN0IHdpdGggdGhpcyBwYXJ0IHJldmVydGVk
LiBJIHRoaW5rIHdpdGhvdXQgdHdvIGNvZGUgcGF0aHMNCj4gPiA+IGVudGVyaW5nIGh2X2Zjb3B5
X2NhbGxiYWNrIGl0IHNob3VsZCBiZSBvayB0byBsZWF2ZSB0aGlzIGNoZWNrIGluLg0KPiA+DQo+
ID4gSSB0aGluayBodl9mY29weV9jYWxsYmFjaygpIGlzIG5vdCBpbnZvbHZlZCBoZXJlOiB3ZSBj
YWxsIGZjb3B5X29uX21zZygpDQo+ID4gZXZlcnkgdGltZSB1c2Vyc3BhY2UgZGFlbW9uIHdyaXRl
cyB0byB0aGUgZGV2aWNlIGFuZCBpdCBpcyBub3QgYW55aG93DQo+ID4gc3luY2hyb25pemVkIHdp
dGggaG9zdC1ndWVzdCBjb21tdW5pY2F0aW9uLg0KPiANCj4gQW4gZWFybGllciB2YXJpYW50IG9m
IHRoaXMgcGF0Y2ggdXNlZCBsb2NrcyBhcm91bmQgdGhlIHZtYnVzX3JlY3ZwYWNrZXQNCj4gYW5k
IHRoZSByZXN1bHQgd2FzIHVzZWQgdG8gZGVjaWRlIHdoaWNoIHRocmVhZCBvZiBleGVjdXRpb24g
bm90aWZpZXMgdGhlDQo+IGRhZW1vbi4gSSB0aGluayBpZiB0aGUgaW50ZXJydXB0IHJhbiBlYXJs
aWVyIHRoYW4gdGhlIGRhZW1vbiBkaWQgdGhlDQo+IHdyaXRlIHRoZW4gdGhlIHN0YXRlIGV4cGVj
dGVkIGluIGZjb3B5X29uX21zZyB3b3VsZCBvYnZpb3VzbHkgYmUgd3JvbmcuDQo+IEFzIGEgcmVz
dWx0IHRoZSBkYWVtb24gd2lsbCBqdXN0IHRlcm1pbmF0ZSB3aXRoIEVGQVVMVC4gV2l0aCB0aGUg
Y2hlY2sNCj4gcmVtb3ZlZCBpdCB3b3VsZCBwcm9jZWVkLCBhbmQgZWl0aGVyIG5vdCBjaGFuY2Vs
IHRoZSB0aW1lb3V0IG9yDQo+IHZtYnVzX3JlY3ZwYWNrZXQgcmVhZHMgbm90aGluZy4NCj4gDQo+
IEJ1dCBub3cgdGhhdCBpdCBpcyBzaW5nbGUgdGhyZWFkZWQgdGhlIHN0YXRlIGluIGZjb3B5X29u
X21zZyBzaG91bGQgYmUNCj4gYXMgZXhwZWN0ZWQuIEFzIHNhaWQsIHdpbGwgcmV0ZXN0LiBFaXRo
ZXIgbGF0ZXIgdG9kYXkgb3Igb24gTW9uZGF5Lg0KDQpUaGUgb25seSBjYXNlIEkgY2FuIHNlZSBp
cyBpZiB0aGUgZGFlbW9uIGRvZXMgbm90IHJlc3BvbmQgaW4gYSB0aW1lbHkgZmFzaGlvbi4NCklu
IHRoaXMgY2FzZSwgd2Ugd291bGQgdGltZW91dCBhbmQgdGVybWluYXRlIHRoZSB0cmFuc2FjdGlv
biBwcmVtYXR1cmVseS4NCkluIHRoaXMgY2FzZSB3aGVuIHRoZSBkYWVtb24gdWx0aW1hdGVseSBy
ZXNwb25kcywgd2Ugd291bGQgdmlldyB0aGF0IHJlc3BvbnNlIGFzDQphbiBlcnJvciBhbmQgcmV0
dXJuIEVJTlZBTCBhbmQgSSB0aGluayBpbiB0aGlzIGNhc2UgaXQgaXMgZmluZSB0byB0ZXJtaW5h
dGUNCnRoZSBkYWVtb247IGVzcGVjaWFsbHkgbm93IHRoYXQgd2UgYXJlIGdvaW5nIHRvIGluY3Jl
YXNlIHRoZSB0aW1lb3V0IHZhbHVlDQp0byAzMCBzZWNvbmRzLg0KDQpMZXQgbWUga25vdyB0aGUg
cmVzdWx0cyBvZiB5b3VyIHRlc3RpbmcuIEkgd2lsbCByZXBvc3QgdGhlIHBhdGNoZXMgYWZ0ZXIg
SSBoZWFyIGZyb20NCnlvdS4NCg0KUmVnYXJkcywNCg0KSy4gWQ0KDQogDQo=
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1245518 — Re: [PATCH 02/10] Drivers: hv: utils: run polling callback always in interrupt context

On Fri, Oct 09, Olaf Hering wrote:

> On Thu, Oct 08, KY Srinivasan wrote:
> 
> > > yes, but after doing fcopy_respond_to_host(). I'd suggest we leave the
> > > check in place, better safe than sorry.
> > 
> > Agreed; Olaf, if it is ok with you, I can fix it up and send.
> 
> I will retest with this part reverted. I think without two code paths
> entering hv_fcopy_callback it should be ok to leave this check in.

Today I restored the "fcopy_transaction.state != HVUTIL_USERSPACE_REQ"
check and its working fine.

Olaf
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1246181 — RE: [PATCH 02/10] Drivers: hv: utils: run polling callback always in interrupt context

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogT2xhZiBIZXJpbmcgW21h
aWx0bzpvbGFmQGFlcGZsZS5kZV0NCj4gU2VudDogVHVlc2RheSwgT2N0b2JlciAxMywgMjAxNSAy
OjQ3IEFNDQo+IFRvOiBLWSBTcmluaXZhc2FuIDxreXNAbWljcm9zb2Z0LmNvbT4NCj4gQ2M6IFZp
dGFseSBLdXpuZXRzb3YgPHZrdXpuZXRzQHJlZGhhdC5jb20+OyBncmVna2hAbGludXhmb3VuZGF0
aW9uLm9yZzsNCj4gbGludXgta2VybmVsQHZnZXIua2VybmVsLm9yZzsgZGV2ZWxAbGludXhkcml2
ZXJwcm9qZWN0Lm9yZzsNCj4gYXB3QGNhbm9uaWNhbC5jb207IGphc293YW5nQHJlZGhhdC5jb20N
Cj4gU3ViamVjdDogUmU6IFtQQVRDSCAwMi8xMF0gRHJpdmVyczogaHY6IHV0aWxzOiBydW4gcG9s
bGluZyBjYWxsYmFjayBhbHdheXMgaW4NCj4gaW50ZXJydXB0IGNvbnRleHQNCj4gDQo+IE9uIEZy
aSwgT2N0IDA5LCBPbGFmIEhlcmluZyB3cm90ZToNCj4gDQo+ID4gT24gVGh1LCBPY3QgMDgsIEtZ
IFNyaW5pdmFzYW4gd3JvdGU6DQo+ID4NCj4gPiA+ID4geWVzLCBidXQgYWZ0ZXIgZG9pbmcgZmNv
cHlfcmVzcG9uZF90b19ob3N0KCkuIEknZCBzdWdnZXN0IHdlIGxlYXZlIHRoZQ0KPiA+ID4gPiBj
aGVjayBpbiBwbGFjZSwgYmV0dGVyIHNhZmUgdGhhbiBzb3JyeS4NCj4gPiA+DQo+ID4gPiBBZ3Jl
ZWQ7IE9sYWYsIGlmIGl0IGlzIG9rIHdpdGggeW91LCBJIGNhbiBmaXggaXQgdXAgYW5kIHNlbmQu
DQo+ID4NCj4gPiBJIHdpbGwgcmV0ZXN0IHdpdGggdGhpcyBwYXJ0IHJldmVydGVkLiBJIHRoaW5r
IHdpdGhvdXQgdHdvIGNvZGUgcGF0aHMNCj4gPiBlbnRlcmluZyBodl9mY29weV9jYWxsYmFjayBp
dCBzaG91bGQgYmUgb2sgdG8gbGVhdmUgdGhpcyBjaGVjayBpbi4NCj4gDQo+IFRvZGF5IEkgcmVz
dG9yZWQgdGhlICJmY29weV90cmFuc2FjdGlvbi5zdGF0ZSAhPSBIVlVUSUxfVVNFUlNQQUNFX1JF
USINCj4gY2hlY2sgYW5kIGl0cyB3b3JraW5nIGZpbmUuDQoNClRoYW5rcyBPbGFmLiBJIHdpbGwg
cmVwb3N0IHRoZSBwYXRjaGVzIHdpdGggdGhlIHVwZGF0ZS4NCg0KSy4gWQ0KPiANCj4gT2xhZg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web