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

Groups > linux.kernel > #1220211 > unrolled thread

[PATCH 0/4] hv: utils: propagate state to interrupt thread

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] hv: utils: propagate state to interrupt thread Olaf Hering <olaf@aepfle.de> - 2015-09-07 16:30 +0200
    [PATCH 1/4] hv: add helpers to handle hv_util device state Olaf Hering <olaf@aepfle.de> - 2015-09-07 16:30 +0200
    [PATCH 4/4] hv: vss: use wrapper to propate state Olaf Hering <olaf@aepfle.de> - 2015-09-07 16:30 +0200
    [PATCH 2/4] hv: fcopy: use wrapper to propate state Olaf Hering <olaf@aepfle.de> - 2015-09-07 16:40 +0200

#1220211 — [PATCH 0/4] hv: utils: propagate state to interrupt thread

The Copy-VMFile cmdlet on the host may fail because the guest fcopy
driver state machine gets out of sync. This happens because the ->state
and ->context variables are accessed by the main thread and from
interrupt context. If an interrupt happens between fcopy_respond_to_host
and hv_poll_channel in fcopy_write, then hv_fcopy_onchannelcallback
called from that interrupt sees still state HVUTIL_USERSPACE_RECV. It
updates the context, but fcopy_write will not notice that update and
hv_poll_channel gets called with an empty context.  As a result
hv_fcopy_daemon gets no more data. After a timeout Copy-VMFile fails
with timeout.

In my initial testing for a fix I put a "mb()" after the last .state
change in fcopy_write. But this series implementes read/write memory
barriers as needed. Let me know if this is overdoing things.

Olaf

Olaf Hering (4):
  hv: add helpers to handle hv_util device state
  hv: fcopy: use wrapper to propate state
  hv: kvp: use wrapper to propate state
  hv: vss: use wrapper to propate state

 drivers/hv/hv_fcopy.c     | 36 ++++++++++++++++++++----------------
 drivers/hv/hv_kvp.c       | 38 +++++++++++++++++++++-----------------
 drivers/hv/hv_snapshot.c  | 37 ++++++++++++++++++++-----------------
 drivers/hv/hyperv_vmbus.h | 12 ++++++++++++
 4 files changed, 73 insertions(+), 50 deletions(-)

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

#1220212 — [PATCH 1/4] hv: add helpers to handle hv_util device state

The callbacks in kvp, vss and fcopy code are called the main thread and
also from interrupt context. If a state change is done by the main
thread it is not immediately seen by the interrupt. As a result the
state machine gets out of sync.

Force propagation of state changes via get/set helpers with a memory barrier.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 drivers/hv/hyperv_vmbus.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 3d70e36..6c03925 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -780,4 +780,16 @@ enum hvutil_device_state {
 	HVUTIL_DEVICE_DYING,     /* driver unload is in progress */
 };
 
+static inline void hvutil_device_set_state(enum hvutil_device_state *p, enum hvutil_device_state s)
+{
+	*p = s;
+	wmb();
+}
+
+static inline enum hvutil_device_state hvutil_device_get_state(enum hvutil_device_state *p)
+{
+       rmb();
+       return *p;
+}
+
 #endif /* _HYPERV_VMBUS_H */
--
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]

#1220214 — [PATCH 4/4] hv: vss: use wrapper to propate state

The .state is used by several threads of execution.
Propagate the state to make changes visible. Also propagate context
change in vss_on_msg.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 drivers/hv/hv_snapshot.c | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c
index 815405f..f3cb822 100644
--- a/drivers/hv/hv_snapshot.c
+++ b/drivers/hv/hv_snapshot.c
@@ -48,7 +48,7 @@
  */
 
 static struct {
-	int state;   /* hvutil_device_state */
+	enum hvutil_device_state state;
 	int recv_len; /* number of bytes received. */
 	struct vmbus_channel *recv_channel; /* chn we got the request */
 	u64 recv_req_id; /* request ID. */
@@ -64,6 +64,9 @@ static void vss_respond_to_host(int error);
  */
 static int dm_reg_value;
 
+#define vss_get_state() hvutil_device_get_state(&vss_transaction.state)
+#define vss_set_state(s) hvutil_device_set_state(&vss_transaction.state, s)
+
 static const char vss_devname[] = "vmbus/hv_vss";
 static __u8 *recv_buffer;
 static struct hvutil_transport *hvt;
@@ -87,8 +90,8 @@ static void vss_timeout_func(struct work_struct *dummy)
 	vss_respond_to_host(HV_E_FAIL);
 
 	/* Transaction is finished, reset the state. */
-	if (vss_transaction.state > HVUTIL_READY)
-		vss_transaction.state = HVUTIL_READY;
+	if (vss_get_state() > HVUTIL_READY)
+		vss_set_state(HVUTIL_READY);
 
 	hv_poll_channel(vss_transaction.vss_context,
 			hv_vss_onchannelcallback);
@@ -112,7 +115,7 @@ static int vss_handle_handshake(struct hv_vss_msg *vss_msg)
 	default:
 		return -EINVAL;
 	}
-	vss_transaction.state = HVUTIL_READY;
+	vss_set_state(HVUTIL_READY);
 	pr_debug("VSS: userspace daemon ver. %d registered\n", dm_reg_value);
 	return 0;
 }
@@ -130,15 +133,15 @@ static int vss_on_msg(void *msg, int len)
 		 * Don't process registration messages if we're in the middle
 		 * of a transaction processing.
 		 */
-		if (vss_transaction.state > HVUTIL_READY)
+		if (vss_get_state() > HVUTIL_READY)
 			return -EINVAL;
 		return vss_handle_handshake(vss_msg);
-	} else if (vss_transaction.state == HVUTIL_USERSPACE_REQ) {
-		vss_transaction.state = HVUTIL_USERSPACE_RECV;
+	} else if (vss_get_state() == HVUTIL_USERSPACE_REQ) {
+		vss_set_state(HVUTIL_USERSPACE_RECV);
 		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;
+			vss_set_state(HVUTIL_READY);
 			hv_poll_channel(vss_transaction.vss_context,
 					hv_vss_onchannelcallback);
 		}
@@ -158,7 +161,7 @@ static void vss_send_op(struct work_struct *dummy)
 	struct hv_vss_msg *vss_msg;
 
 	/* The transaction state is wrong. */
-	if (vss_transaction.state != HVUTIL_HOSTMSG_RECEIVED)
+	if (vss_get_state() != HVUTIL_HOSTMSG_RECEIVED)
 		return;
 
 	vss_msg = kzalloc(sizeof(*vss_msg), GFP_KERNEL);
@@ -167,13 +170,13 @@ static void vss_send_op(struct work_struct *dummy)
 
 	vss_msg->vss_hdr.operation = op;
 
-	vss_transaction.state = HVUTIL_USERSPACE_REQ;
+	vss_set_state(HVUTIL_USERSPACE_REQ);
 	rc = hvutil_transport_send(hvt, vss_msg, sizeof(*vss_msg));
 	if (rc) {
 		pr_warn("VSS: failed to communicate to the daemon: %d\n", rc);
 		if (cancel_delayed_work_sync(&vss_timeout_work)) {
 			vss_respond_to_host(HV_E_FAIL);
-			vss_transaction.state = HVUTIL_READY;
+			vss_set_state(HVUTIL_READY);
 		}
 	}
 
@@ -238,7 +241,7 @@ void hv_vss_onchannelcallback(void *context)
 	struct icmsg_hdr *icmsghdrp;
 	struct icmsg_negotiate *negop = NULL;
 
-	if (vss_transaction.state > HVUTIL_READY) {
+	if (vss_get_state() > HVUTIL_READY) {
 		/*
 		 * We will defer processing this callback once
 		 * the current transaction is complete.
@@ -288,12 +291,12 @@ void hv_vss_onchannelcallback(void *context)
 				 */
 			case VSS_OP_FREEZE:
 			case VSS_OP_THAW:
-				if (vss_transaction.state < HVUTIL_READY) {
+				if (vss_get_state() < HVUTIL_READY) {
 					/* Userspace is not registered yet */
 					vss_respond_to_host(HV_E_FAIL);
 					return;
 				}
-				vss_transaction.state = HVUTIL_HOSTMSG_RECEIVED;
+				vss_set_state(HVUTIL_HOSTMSG_RECEIVED);
 				schedule_work(&vss_send_op_work);
 				schedule_delayed_work(&vss_timeout_work,
 						      VSS_USERSPACE_TIMEOUT);
@@ -332,7 +335,7 @@ static void vss_on_reset(void)
 {
 	if (cancel_delayed_work_sync(&vss_timeout_work))
 		vss_respond_to_host(HV_E_FAIL);
-	vss_transaction.state = HVUTIL_DEVICE_INIT;
+	vss_set_state(HVUTIL_DEVICE_INIT);
 }
 
 int
@@ -346,7 +349,7 @@ hv_vss_init(struct hv_util_service *srv)
 	 * Defer processing channel callbacks until the daemon
 	 * has registered.
 	 */
-	vss_transaction.state = HVUTIL_DEVICE_INIT;
+	vss_set_state(HVUTIL_DEVICE_INIT);
 
 	hvt = hvutil_transport_init(vss_devname, CN_VSS_IDX, CN_VSS_VAL,
 				    vss_on_msg, vss_on_reset);
@@ -358,7 +361,7 @@ hv_vss_init(struct hv_util_service *srv)
 
 void hv_vss_deinit(void)
 {
-	vss_transaction.state = HVUTIL_DEVICE_DYING;
+	vss_set_state(HVUTIL_DEVICE_DYING);
 	cancel_delayed_work_sync(&vss_timeout_work);
 	cancel_work_sync(&vss_send_op_work);
 	hvutil_transport_destroy(hvt);
--
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]

#1220236 — [PATCH 2/4] hv: fcopy: use wrapper to propate state

The .state is used by several threads of execution.
Propagate the state to make changes visible. Also propagate context
change in hv_fcopy_onchannelcallback.
Without this change fcopy may hang at random points.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 drivers/hv/hv_fcopy.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/drivers/hv/hv_fcopy.c b/drivers/hv/hv_fcopy.c
index db4b887..47d9c34 100644
--- a/drivers/hv/hv_fcopy.c
+++ b/drivers/hv/hv_fcopy.c
@@ -46,7 +46,7 @@
  */
 
 static struct {
-	int state;   /* hvutil_device_state */
+	enum hvutil_device_state state;
 	int recv_len; /* number of bytes received. */
 	struct hv_fcopy_hdr  *fcopy_msg; /* current message */
 	struct vmbus_channel *recv_channel; /* chn we got the request */
@@ -67,6 +67,9 @@ static struct hvutil_transport *hvt;
  */
 static int dm_reg_value;
 
+#define fcopy_get_state() hvutil_device_get_state(&fcopy_transaction.state)
+#define fcopy_set_state(s) hvutil_device_set_state(&fcopy_transaction.state, s)
+
 static void fcopy_timeout_func(struct work_struct *dummy)
 {
 	/*
@@ -76,8 +79,8 @@ static void fcopy_timeout_func(struct work_struct *dummy)
 	fcopy_respond_to_host(HV_E_FAIL);
 
 	/* Transaction is finished, reset the state. */
-	if (fcopy_transaction.state > HVUTIL_READY)
-		fcopy_transaction.state = HVUTIL_READY;
+	if (fcopy_get_state() > HVUTIL_READY)
+		fcopy_set_state(HVUTIL_READY);
 
 	hv_poll_channel(fcopy_transaction.fcopy_context,
 			hv_fcopy_onchannelcallback);
@@ -108,7 +111,7 @@ static int fcopy_handle_handshake(u32 version)
 		return -EINVAL;
 	}
 	pr_debug("FCP: userspace daemon ver. %d registered\n", version);
-	fcopy_transaction.state = HVUTIL_READY;
+	fcopy_set_state(HVUTIL_READY);
 	hv_poll_channel(fcopy_transaction.fcopy_context,
 			hv_fcopy_onchannelcallback);
 	return 0;
@@ -162,13 +165,13 @@ static void fcopy_send_data(struct work_struct *dummy)
 		break;
 	}
 
-	fcopy_transaction.state = HVUTIL_USERSPACE_REQ;
+	fcopy_set_state(HVUTIL_USERSPACE_REQ);
 	rc = hvutil_transport_send(hvt, out_src, out_len);
 	if (rc) {
 		pr_debug("FCP: failed to communicate to the daemon: %d\n", rc);
 		if (cancel_delayed_work_sync(&fcopy_timeout_work)) {
 			fcopy_respond_to_host(HV_E_FAIL);
-			fcopy_transaction.state = HVUTIL_READY;
+			fcopy_set_state(HVUTIL_READY);
 		}
 	}
 	kfree(smsg_out);
@@ -227,12 +230,13 @@ void hv_fcopy_onchannelcallback(void *context)
 	int util_fw_version;
 	int fcopy_srv_version;
 
-	if (fcopy_transaction.state > HVUTIL_READY) {
+	if (fcopy_get_state() > HVUTIL_READY) {
 		/*
 		 * We will defer processing this callback once
 		 * the current transaction is complete.
 		 */
 		fcopy_transaction.fcopy_context = context;
+		wmb();
 		return;
 	}
 	fcopy_transaction.fcopy_context = NULL;
@@ -264,12 +268,12 @@ void hv_fcopy_onchannelcallback(void *context)
 		fcopy_transaction.recv_req_id = requestid;
 		fcopy_transaction.fcopy_msg = fcopy_msg;
 
-		if (fcopy_transaction.state < HVUTIL_READY) {
+		if (fcopy_get_state() < HVUTIL_READY) {
 			/* Userspace is not registered yet */
 			fcopy_respond_to_host(HV_E_FAIL);
 			return;
 		}
-		fcopy_transaction.state = HVUTIL_HOSTMSG_RECEIVED;
+		fcopy_set_state(HVUTIL_HOSTMSG_RECEIVED);
 
 		/*
 		 * Send the information to the user-level daemon.
@@ -291,10 +295,10 @@ static int fcopy_on_msg(void *msg, int len)
 	if (len != sizeof(int))
 		return -EINVAL;
 
-	if (fcopy_transaction.state == HVUTIL_DEVICE_INIT)
+	if (fcopy_get_state() == HVUTIL_DEVICE_INIT)
 		return fcopy_handle_handshake(*val);
 
-	if (fcopy_transaction.state != HVUTIL_USERSPACE_REQ)
+	if (fcopy_get_state() != HVUTIL_USERSPACE_REQ)
 		return -EINVAL;
 
 	/*
@@ -302,9 +306,9 @@ static int fcopy_on_msg(void *msg, int len)
 	 * to the host. But first, cancel the timeout.
 	 */
 	if (cancel_delayed_work_sync(&fcopy_timeout_work)) {
-		fcopy_transaction.state = HVUTIL_USERSPACE_RECV;
+		fcopy_set_state(HVUTIL_USERSPACE_RECV);
 		fcopy_respond_to_host(*val);
-		fcopy_transaction.state = HVUTIL_READY;
+		fcopy_set_state(HVUTIL_READY);
 		hv_poll_channel(fcopy_transaction.fcopy_context,
 				hv_fcopy_onchannelcallback);
 	}
@@ -317,7 +321,7 @@ static void fcopy_on_reset(void)
 	/*
 	 * The daemon has exited; reset the state.
 	 */
-	fcopy_transaction.state = HVUTIL_DEVICE_INIT;
+	fcopy_set_state(HVUTIL_DEVICE_INIT);
 
 	if (cancel_delayed_work_sync(&fcopy_timeout_work))
 		fcopy_respond_to_host(HV_E_FAIL);
@@ -333,7 +337,7 @@ int hv_fcopy_init(struct hv_util_service *srv)
 	 * Defer processing channel callbacks until the daemon
 	 * has registered.
 	 */
-	fcopy_transaction.state = HVUTIL_DEVICE_INIT;
+	fcopy_set_state(HVUTIL_DEVICE_INIT);
 
 	hvt = hvutil_transport_init(fcopy_devname, 0, 0,
 				    fcopy_on_msg, fcopy_on_reset);
@@ -345,7 +349,7 @@ int hv_fcopy_init(struct hv_util_service *srv)
 
 void hv_fcopy_deinit(void)
 {
-	fcopy_transaction.state = HVUTIL_DEVICE_DYING;
+	fcopy_set_state(HVUTIL_DEVICE_DYING);
 	cancel_delayed_work_sync(&fcopy_timeout_work);
 	hvutil_transport_destroy(hvt);
 }
--
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