Groups > linux.kernel > #1225576 > unrolled thread

[PATCH 0/5] Drivers: hv: Miscellaneous fixes

Started by	"K. Y. Srinivasan" <kys@microsoft.com>
First post	2015-09-16 01:10 +0200
Last post	2015-09-16 02:20 +0200
Articles	19 — 5 participants

Back to article view | Back to linux.kernel

  [PATCH 0/5] Drivers: hv: Miscellaneous fixes  "K. Y. Srinivasan" <kys@microsoft.com> - 2015-09-16 01:10 +0200
    [PATCH 1/5] Drivers: hv: vmbus: fix init_vp_index() for reloading hv_netvsc "K. Y. Srinivasan" <kys@microsoft.com> - 2015-09-16 01:20 +0200
      [PATCH 4/5] hv: kvp: use wrappers to propaigate state "K. Y. Srinivasan" <kys@microsoft.com> - 2015-09-16 01:20 +0200
        Re: [PATCH 4/5] hv: kvp: use wrappers to propaigate state Greg KH <gregkh@linuxfoundation.org> - 2015-09-21 07:30 +0200
          Re: [PATCH 4/5] hv: kvp: use wrappers to propaigate state Olaf Hering <olaf@aepfle.de> - 2015-09-21 12:20 +0200
            RE: [PATCH 4/5] hv: kvp: use wrappers to propaigate state KY Srinivasan <kys@microsoft.com> - 2015-09-21 18:40 +0200
          RE: [PATCH 4/5] hv: kvp: use wrappers to propaigate state KY Srinivasan <kys@microsoft.com> - 2015-09-21 18:20 +0200
      [PATCH 5/5] hv: vss: use wrappers to propagate state "K. Y. Srinivasan" <kys@microsoft.com> - 2015-09-16 01:20 +0200
      [PATCH 2/5] hv: add helpers to handle hv_util device state "K. Y. Srinivasan" <kys@microsoft.com> - 2015-09-16 01:20 +0200
        Re: [PATCH 2/5] hv: add helpers to handle hv_util device state Greg KH <gregkh@linuxfoundation.org> - 2015-09-21 07:30 +0200
          Re: [PATCH 2/5] hv: add helpers to handle hv_util device state Olaf Hering <olaf@aepfle.de> - 2015-09-21 12:30 +0200
            Re: [PATCH 2/5] hv: add helpers to handle hv_util device state Vitaly Kuznetsov <vkuznets@redhat.com> - 2015-09-21 13:30 +0200
              Re: [PATCH 2/5] hv: add helpers to handle hv_util device state Olaf Hering <olaf@aepfle.de> - 2015-09-21 14:20 +0200
                Re: [PATCH 2/5] hv: add helpers to handle hv_util device state Vitaly Kuznetsov <vkuznets@redhat.com> - 2015-09-21 15:40 +0200
                RE: [PATCH 2/5] hv: add helpers to handle hv_util device state KY Srinivasan <kys@microsoft.com> - 2015-09-21 18:50 +0200
            RE: [PATCH 2/5] hv: add helpers to handle hv_util device state KY Srinivasan <kys@microsoft.com> - 2015-09-21 18:40 +0200
              Re: [PATCH 2/5] hv: add helpers to handle hv_util device state Greg KH <gregkh@linuxfoundation.org> - 2015-09-21 18:50 +0200
                RE: [PATCH 2/5] hv: add helpers to handle hv_util device state KY Srinivasan <kys@microsoft.com> - 2015-09-21 19:10 +0200
      [PATCH 3/5] hv: fcopy: use wrappers to propagate state "K. Y. Srinivasan" <kys@microsoft.com> - 2015-09-16 02:20 +0200

#1225576 — [PATCH 0/5] Drivers: hv: Miscellaneous fixes

From	"K. Y. Srinivasan" <kys@microsoft.com>
Date	2015-09-16 01:10 +0200
Subject	[PATCH 0/5] Drivers: hv: Miscellaneous fixes
Message-ID	<q97wl-3i4-17@gated-at.bofh.it>

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.


Dexuan Cui (1):
  Drivers: hv: vmbus: fix init_vp_index() for reloading hv_netvsc

Olaf Hering (4):
  hv: add helpers to handle hv_util device state
  hv: fcopy: use wrappers to propagate state
  hv: kvp: use wrappers to propaigate state
  hv: vss: use wrappers to propagate state

 drivers/hv/channel_mgmt.c |   17 +++++++++++++++++
 drivers/hv/hv_fcopy.c     |   36 ++++++++++++++++++++----------------
 drivers/hv/hv_kvp.c       |   39 +++++++++++++++++++++------------------
 drivers/hv/hv_snapshot.c  |   37 ++++++++++++++++++++-----------------
 drivers/hv/hyperv_vmbus.h |   14 ++++++++++++++
 5 files changed, 92 insertions(+), 51 deletions(-)

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

#1225578 — [PATCH 1/5] Drivers: hv: vmbus: fix init_vp_index() for reloading hv_netvsc

From	"K. Y. Srinivasan" <kys@microsoft.com>
Date	2015-09-16 01:20 +0200
Subject	[PATCH 1/5] Drivers: hv: vmbus: fix init_vp_index() for reloading hv_netvsc
Message-ID	<q97G1-3tj-5@gated-at.bofh.it>
In reply to	#1225576

From: Dexuan Cui <decui@microsoft.com>

This fixes the recent commit 3b71107d73b16074afa7658f3f0fcf837aabfe24:
Drivers: hv: vmbus: Further improve CPU affiliation logic

Without the fix, reloading hv_netvsc hangs the guest.

Signed-off-by: Dexuan Cui <decui@microsoft.com>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/hv/channel_mgmt.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 3ab4753..8a4105c 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -204,6 +204,8 @@ void hv_process_channel_removal(struct vmbus_channel *channel, u32 relid)
 		spin_lock_irqsave(&vmbus_connection.channel_lock, flags);
 		list_del(&channel->listentry);
 		spin_unlock_irqrestore(&vmbus_connection.channel_lock, flags);
+
+		primary_channel = channel;
 	} else {
 		primary_channel = channel->primary_channel;
 		spin_lock_irqsave(&primary_channel->lock, flags);
@@ -211,6 +213,14 @@ void hv_process_channel_removal(struct vmbus_channel *channel, u32 relid)
 		primary_channel->num_sc--;
 		spin_unlock_irqrestore(&primary_channel->lock, flags);
 	}
+
+	/*
+	 * We need to free the bit for init_vp_index() to work in the case
+	 * of sub-channel, when we reload drivers like hv_netvsc.
+	 */
+	cpumask_clear_cpu(channel->target_cpu,
+			  &primary_channel->alloced_cpus_in_node);
+
 	free_channel(channel);
 }
 
@@ -457,6 +467,13 @@ static void init_vp_index(struct vmbus_channel *channel, const uuid_le *type_gui
 			continue;
 		}
 
+		/*
+		 * NOTE: in the case of sub-channel, we clear the sub-channel
+		 * related bit(s) in primary->alloced_cpus_in_node in
+		 * hv_process_channel_removal(), so when we reload drivers
+		 * like hv_netvsc in SMP guest, here we're able to re-allocate
+		 * bit from primary->alloced_cpus_in_node.
+		 */
 		if (!cpumask_test_cpu(cur_cpu,
 				&primary->alloced_cpus_in_node)) {
 			cpumask_set_cpu(cur_cpu,
-- 
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] | [prev] | [next] | [standalone]

#1225579 — [PATCH 4/5] hv: kvp: use wrappers to propaigate state

From	"K. Y. Srinivasan" <kys@microsoft.com>
Date	2015-09-16 01:20 +0200
Subject	[PATCH 4/5] hv: kvp: use wrappers to propaigate state
Message-ID	<q97G2-3tj-7@gated-at.bofh.it>
In reply to	#1225578

From: Olaf Hering <olaf@aepfle.de>

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

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/hv/hv_kvp.c |   39 +++++++++++++++++++++------------------
 1 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
index 74c38a9..778d353 100644
--- a/drivers/hv/hv_kvp.c
+++ b/drivers/hv/hv_kvp.c
@@ -61,7 +61,7 @@
  */
 
 static struct {
-	int state;   /* hvutil_device_state */
+	enum hvutil_device_state state;
 	int recv_len; /* number of bytes received. */
 	struct hv_kvp_msg  *kvp_msg; /* current message */
 	struct vmbus_channel *recv_channel; /* chn we got the request */
@@ -74,6 +74,9 @@ static struct {
  */
 static int dm_reg_value;
 
+#define kvp_get_state() hvutil_device_get_state(&kvp_transaction.state)
+#define kvp_set_state(s) hvutil_device_set_state(&kvp_transaction.state, s)
+
 static void kvp_send_key(struct work_struct *dummy);
 
 
@@ -122,8 +125,8 @@ 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;
+	if (kvp_get_state() > HVUTIL_READY)
+		kvp_set_state(HVUTIL_READY);
 
 	hv_poll_channel(kvp_transaction.kvp_context,
 			hv_kvp_onchannelcallback);
@@ -153,7 +156,7 @@ static int kvp_handle_handshake(struct hv_kvp_msg *msg)
 	pr_debug("KVP: userspace daemon ver. %d registered\n",
 		 KVP_OP_REGISTER);
 	kvp_register(dm_reg_value);
-	kvp_transaction.state = HVUTIL_READY;
+	kvp_set_state(HVUTIL_READY);
 
 	return 0;
 }
@@ -177,15 +180,14 @@ static int kvp_on_msg(void *msg, int len)
 	 * with the daemon; handle that first.
 	 */
 
-	if (kvp_transaction.state < HVUTIL_READY) {
+	if (kvp_get_state() < HVUTIL_READY)
 		return kvp_handle_handshake(message);
-	}
 
 	/* We didn't send anything to userspace so the reply is spurious */
-	if (kvp_transaction.state < HVUTIL_USERSPACE_REQ)
+	if (kvp_get_state() < HVUTIL_USERSPACE_REQ)
 		return -EINVAL;
 
-	kvp_transaction.state = HVUTIL_USERSPACE_RECV;
+	kvp_set_state(HVUTIL_USERSPACE_RECV);
 
 	/*
 	 * Based on the version of the daemon, we propagate errors from the
@@ -218,7 +220,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;
+		kvp_set_state(HVUTIL_READY);
 		hv_poll_channel(kvp_transaction.kvp_context,
 				hv_kvp_onchannelcallback);
 	}
@@ -349,7 +351,7 @@ kvp_send_key(struct work_struct *dummy)
 	int rc;
 
 	/* The transaction state is wrong. */
-	if (kvp_transaction.state != HVUTIL_HOSTMSG_RECEIVED)
+	if (kvp_get_state() != HVUTIL_HOSTMSG_RECEIVED)
 		return;
 
 	message = kzalloc(sizeof(*message), GFP_KERNEL);
@@ -442,13 +444,13 @@ kvp_send_key(struct work_struct *dummy)
 			break;
 	}
 
-	kvp_transaction.state = HVUTIL_USERSPACE_REQ;
+	kvp_set_state(HVUTIL_USERSPACE_REQ);
 	rc = hvutil_transport_send(hvt, message, sizeof(*message));
 	if (rc) {
 		pr_debug("KVP: failed to communicate to the daemon: %d\n", rc);
 		if (cancel_delayed_work_sync(&kvp_timeout_work)) {
 			kvp_respond_to_host(message, HV_E_FAIL);
-			kvp_transaction.state = HVUTIL_READY;
+			kvp_set_state(HVUTIL_READY);
 		}
 	}
 
@@ -596,12 +598,13 @@ void hv_kvp_onchannelcallback(void *context)
 	int util_fw_version;
 	int kvp_srv_version;
 
-	if (kvp_transaction.state > HVUTIL_READY) {
+	if (kvp_get_state() > HVUTIL_READY) {
 		/*
 		 * We will defer processing this callback once
 		 * the current transaction is complete.
 		 */
 		kvp_transaction.kvp_context = context;
+		wmb();
 		return;
 	}
 	kvp_transaction.kvp_context = NULL;
@@ -651,12 +654,12 @@ void hv_kvp_onchannelcallback(void *context)
 			kvp_transaction.recv_req_id = requestid;
 			kvp_transaction.kvp_msg = kvp_msg;
 
-			if (kvp_transaction.state < HVUTIL_READY) {
+			if (kvp_get_state() < HVUTIL_READY) {
 				/* Userspace is not registered yet */
 				kvp_respond_to_host(NULL, HV_E_FAIL);
 				return;
 			}
-			kvp_transaction.state = HVUTIL_HOSTMSG_RECEIVED;
+			kvp_set_state(HVUTIL_HOSTMSG_RECEIVED);
 
 			/*
 			 * Get the information from the
@@ -688,7 +691,7 @@ static void kvp_on_reset(void)
 {
 	if (cancel_delayed_work_sync(&kvp_timeout_work))
 		kvp_respond_to_host(NULL, HV_E_FAIL);
-	kvp_transaction.state = HVUTIL_DEVICE_INIT;
+	kvp_set_state(HVUTIL_DEVICE_INIT);
 }
 
 int
@@ -702,7 +705,7 @@ hv_kvp_init(struct hv_util_service *srv)
 	 * Defer processing channel callbacks until the daemon
 	 * has registered.
 	 */
-	kvp_transaction.state = HVUTIL_DEVICE_INIT;
+	kvp_set_state(HVUTIL_DEVICE_INIT);
 
 	hvt = hvutil_transport_init(kvp_devname, CN_KVP_IDX, CN_KVP_VAL,
 				    kvp_on_msg, kvp_on_reset);
@@ -714,7 +717,7 @@ hv_kvp_init(struct hv_util_service *srv)
 
 void hv_kvp_deinit(void)
 {
-	kvp_transaction.state = HVUTIL_DEVICE_DYING;
+	kvp_set_state(HVUTIL_DEVICE_DYING);
 	cancel_delayed_work_sync(&kvp_timeout_work);
 	cancel_work_sync(&kvp_sendkey_work);
 	hvutil_transport_destroy(hvt);
-- 
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] | [prev] | [next] | [standalone]

#1229043 — Re: [PATCH 4/5] hv: kvp: use wrappers to propaigate state

From	Greg KH <gregkh@linuxfoundation.org>
Date	2015-09-21 07:30 +0200
Subject	Re: [PATCH 4/5] hv: kvp: use wrappers to propaigate state
Message-ID	<qb1PQ-5Cg-13@gated-at.bofh.it>
In reply to	#1225579

On Tue, Sep 15, 2015 at 05:37:53PM -0700, K. Y. Srinivasan wrote:
> From: Olaf Hering <olaf@aepfle.de>
> 
> The "state" is used by several threads of execution.
> Propagate the state to make changes visible. Also propagate context
> change in kvp_on_msg.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> ---
>  drivers/hv/hv_kvp.c |   39 +++++++++++++++++++++------------------
>  1 files changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
> index 74c38a9..778d353 100644
> --- a/drivers/hv/hv_kvp.c
> +++ b/drivers/hv/hv_kvp.c
> @@ -61,7 +61,7 @@
>   */
>  
>  static struct {
> -	int state;   /* hvutil_device_state */
> +	enum hvutil_device_state state;
>  	int recv_len; /* number of bytes received. */
>  	struct hv_kvp_msg  *kvp_msg; /* current message */
>  	struct vmbus_channel *recv_channel; /* chn we got the request */
> @@ -74,6 +74,9 @@ static struct {
>   */
>  static int dm_reg_value;
>  
> +#define kvp_get_state() hvutil_device_get_state(&kvp_transaction.state)
> +#define kvp_set_state(s) hvutil_device_set_state(&kvp_transaction.state, s)
> +
>  static void kvp_send_key(struct work_struct *dummy);
>  
>  
> @@ -122,8 +125,8 @@ 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;
> +	if (kvp_get_state() > HVUTIL_READY)
> +		kvp_set_state(HVUTIL_READY);
>  

And what if the state changed the line after this?  Oops, your code is
hosed.  See, you need a lock, do this correctly.

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

#1229185 — Re: [PATCH 4/5] hv: kvp: use wrappers to propaigate state

From	Olaf Hering <olaf@aepfle.de>
Date	2015-09-21 12:20 +0200
Subject	Re: [PATCH 4/5] hv: kvp: use wrappers to propaigate state
Message-ID	<qb6mu-3FW-25@gated-at.bofh.it>
In reply to	#1229043

On Sun, Sep 20, Greg KH wrote:

> On Tue, Sep 15, 2015 at 05:37:53PM -0700, K. Y. Srinivasan wrote:
> > @@ -122,8 +125,8 @@ static void kvp_timeout_func(struct work_struct *dummy)

> > +	if (kvp_get_state() > HVUTIL_READY)
> > +		kvp_set_state(HVUTIL_READY);

> And what if the state changed the line after this?  Oops, your code is
> hosed.  See, you need a lock, do this correctly.

KY,  can this happen?

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]

#1229547 — RE: [PATCH 4/5] hv: kvp: use wrappers to propaigate state

From	KY Srinivasan <kys@microsoft.com>
Date	2015-09-21 18:40 +0200
Subject	RE: [PATCH 4/5] hv: kvp: use wrappers to propaigate state
Message-ID	<qbcie-3Gm-17@gated-at.bofh.it>
In reply to	#1229185

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogT2xhZiBIZXJpbmcgW21h
aWx0bzpvbGFmQGFlcGZsZS5kZV0NCj4gU2VudDogTW9uZGF5LCBTZXB0ZW1iZXIgMjEsIDIwMTUg
MzoxOCBBTQ0KPiBUbzogS1kgU3Jpbml2YXNhbiA8a3lzQG1pY3Jvc29mdC5jb20+OyBHcmVnIEtI
DQo+IDxncmVna2hAbGludXhmb3VuZGF0aW9uLm9yZz4NCj4gQ2M6IGxpbnV4LWtlcm5lbEB2Z2Vy
Lmtlcm5lbC5vcmc7IGRldmVsQGxpbnV4ZHJpdmVycHJvamVjdC5vcmc7DQo+IGFwd0BjYW5vbmlj
YWwuY29tOyB2a3V6bmV0c0ByZWRoYXQuY29tOyBqYXNvd2FuZ0ByZWRoYXQuY29tDQo+IFN1Ympl
Y3Q6IFJlOiBbUEFUQ0ggNC81XSBodjoga3ZwOiB1c2Ugd3JhcHBlcnMgdG8gcHJvcGFpZ2F0ZSBz
dGF0ZQ0KPiANCj4gT24gU3VuLCBTZXAgMjAsIEdyZWcgS0ggd3JvdGU6DQo+IA0KPiA+IE9uIFR1
ZSwgU2VwIDE1LCAyMDE1IGF0IDA1OjM3OjUzUE0gLTA3MDAsIEsuIFkuIFNyaW5pdmFzYW4gd3Jv
dGU6DQo+ID4gPiBAQCAtMTIyLDggKzEyNSw4IEBAIHN0YXRpYyB2b2lkIGt2cF90aW1lb3V0X2Z1
bmMoc3RydWN0IHdvcmtfc3RydWN0DQo+ICpkdW1teSkNCj4gDQo+ID4gPiArCWlmIChrdnBfZ2V0
X3N0YXRlKCkgPiBIVlVUSUxfUkVBRFkpDQo+ID4gPiArCQlrdnBfc2V0X3N0YXRlKEhWVVRJTF9S
RUFEWSk7DQo+IA0KPiA+IEFuZCB3aGF0IGlmIHRoZSBzdGF0ZSBjaGFuZ2VkIHRoZSBsaW5lIGFm
dGVyIHRoaXM/ICBPb3BzLCB5b3VyIGNvZGUgaXMNCj4gPiBob3NlZC4gIFNlZSwgeW91IG5lZWQg
YSBsb2NrLCBkbyB0aGlzIGNvcnJlY3RseS4NCj4gDQo+IEtZLCAgY2FuIHRoaXMgaGFwcGVuPw0K
DQpObyAtIHRoZSBzdGF0ZSBoYXMgdG8gYmUgPiBIVlVUSUxfUkVBRFkuIEFsc28sIGlmIHRoZSB0
aW1lb3V0IGZpcmVzLA0KaXQgd2lucyBzaW5jZSBvdGhlciBjb250ZXh0cyB3aWxsIGF0dGVtcHQg
dG8gZmlyc3QgY2FuY2VsIHRoZSB0aW1lb3V0IGJlZm9yZQ0KcHJvY2VlZGluZyBmdXJ0aGVyLg0K
DQpLLiBZIA0KPiANCj4gT2xhZg0K
--
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]

#1229525 — RE: [PATCH 4/5] hv: kvp: use wrappers to propaigate state

From	KY Srinivasan <kys@microsoft.com>
Date	2015-09-21 18:20 +0200
Subject	RE: [PATCH 4/5] hv: kvp: use wrappers to propaigate state
Message-ID	<qbbYS-3k6-7@gated-at.bofh.it>
In reply to	#1229043


> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Sunday, September 20, 2015 10:26 PM
> To: KY Srinivasan <kys@microsoft.com>
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> olaf@aepfle.de; apw@canonical.com; vkuznets@redhat.com;
> jasowang@redhat.com
> Subject: Re: [PATCH 4/5] hv: kvp: use wrappers to propaigate state
> 
> On Tue, Sep 15, 2015 at 05:37:53PM -0700, K. Y. Srinivasan wrote:
> > From: Olaf Hering <olaf@aepfle.de>
> >
> > The "state" is used by several threads of execution.
> > Propagate the state to make changes visible. Also propagate context
> > change in kvp_on_msg.
> >
> > Signed-off-by: Olaf Hering <olaf@aepfle.de>
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > ---
> >  drivers/hv/hv_kvp.c |   39 +++++++++++++++++++++------------------
> >  1 files changed, 21 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
> > index 74c38a9..778d353 100644
> > --- a/drivers/hv/hv_kvp.c
> > +++ b/drivers/hv/hv_kvp.c
> > @@ -61,7 +61,7 @@
> >   */
> >
> >  static struct {
> > -	int state;   /* hvutil_device_state */
> > +	enum hvutil_device_state state;
> >  	int recv_len; /* number of bytes received. */
> >  	struct hv_kvp_msg  *kvp_msg; /* current message */
> >  	struct vmbus_channel *recv_channel; /* chn we got the request */
> > @@ -74,6 +74,9 @@ static struct {
> >   */
> >  static int dm_reg_value;
> >
> > +#define kvp_get_state()
> hvutil_device_get_state(&kvp_transaction.state)
> > +#define kvp_set_state(s)
> hvutil_device_set_state(&kvp_transaction.state, s)
> > +
> >  static void kvp_send_key(struct work_struct *dummy);
> >
> >
> > @@ -122,8 +125,8 @@ 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;
> > +	if (kvp_get_state() > HVUTIL_READY)
> > +		kvp_set_state(HVUTIL_READY);
> >
> 
> And what if the state changed the line after this?  Oops, your code is
> hosed.  See, you need a lock, do this correctly.

This code path is an exception path - request has already been sent to the guest user space and we
are protecting against the guest user space not responding in a reasonable time. Consequently,
the state here has to be >  HVUTIL_READY (we should probably ASSERT this here). When the timeout
triggers, this will be the only code responding back to the host. So there is no issue here and 
I don't think you need a lock here.

The channels for the util driver are all bound to CPU 0. Given this, the simpler solution maybe to 
ensure that we execute all of the various contexts on CPU 0 and have implicit locking.

Regards,

K. Y   











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

#1225580 — [PATCH 5/5] hv: vss: use wrappers to propagate state

From	"K. Y. Srinivasan" <kys@microsoft.com>
Date	2015-09-16 01:20 +0200
Subject	[PATCH 5/5] hv: vss: use wrappers to propagate state
Message-ID	<q97G2-3tj-15@gated-at.bofh.it>
In reply to	#1225578

From: Olaf Hering <olaf@aepfle.de>

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>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/hv/hv_snapshot.c |   37 ++++++++++++++++++++-----------------
 1 files 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);
-- 
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] | [prev] | [next] | [standalone]

#1225581 — [PATCH 2/5] hv: add helpers to handle hv_util device state

From	"K. Y. Srinivasan" <kys@microsoft.com>
Date	2015-09-16 01:20 +0200
Subject	[PATCH 2/5] hv: add helpers to handle hv_util device state
Message-ID	<q97G2-3tj-19@gated-at.bofh.it>
In reply to	#1225578

From: Olaf Hering <olaf@aepfle.de>

The callbacks in kvp, vss and fcopy code are called both from the main thread
as well as 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>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/hv/hyperv_vmbus.h |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 4b1eb6d..dee5798 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -780,4 +780,18 @@ 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 */
-- 
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] | [prev] | [next] | [standalone]

#1229041 — Re: [PATCH 2/5] hv: add helpers to handle hv_util device state

From	Greg KH <gregkh@linuxfoundation.org>
Date	2015-09-21 07:30 +0200
Subject	Re: [PATCH 2/5] hv: add helpers to handle hv_util device state
Message-ID	<qb1PQ-5Cg-7@gated-at.bofh.it>
In reply to	#1225581

On Tue, Sep 15, 2015 at 05:37:51PM -0700, K. Y. Srinivasan wrote:
> From: Olaf Hering <olaf@aepfle.de>
> 
> The callbacks in kvp, vss and fcopy code are called both from the main thread
> as well as 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>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> ---
>  drivers/hv/hyperv_vmbus.h |   14 ++++++++++++++
>  1 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> index 4b1eb6d..dee5798 100644
> --- a/drivers/hv/hyperv_vmbus.h
> +++ b/drivers/hv/hyperv_vmbus.h
> @@ -780,4 +780,18 @@ 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 */

This is crazy.  If you need to know the state of something (pun
intended) then you had better be using a lock, and not relying on a
random pointer to contain a random value and be able to do something
based on that.

This shows the code is broken, don't paper over things by throwing in
random read/write barriers, that is a HUGE flag that something bad is
happening here.

Just use a lock, that's what it is there for.

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

#1229194 — Re: [PATCH 2/5] hv: add helpers to handle hv_util device state

From	Olaf Hering <olaf@aepfle.de>
Date	2015-09-21 12:30 +0200
Subject	Re: [PATCH 2/5] hv: add helpers to handle hv_util device state
Message-ID	<qb6wa-3Rr-9@gated-at.bofh.it>
In reply to	#1229041

On Sun, Sep 20, Greg KH wrote:

> Just use a lock, that's what it is there for.

How would that help? It might help because it enforces ordering. But
that requires that all three utils get refactored to deal with the
introduced locking. I will let KY comment on this.

The issue I see with fcopy is that after or while fcopy_respond_to_host
runs an interrupt triggers which also calls into
hv_fcopy_onchannelcallback.  It was most likely caused by a logic change
in "recent" vmbus updates because this did not happen before. At least,
the fcopy hang was not seen earler. Maybe the bug did just not trigger
up to now for other reasons...

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]

#1229224 — Re: [PATCH 2/5] hv: add helpers to handle hv_util device state

From	Vitaly Kuznetsov <vkuznets@redhat.com>
Date	2015-09-21 13:30 +0200
Subject	Re: [PATCH 2/5] hv: add helpers to handle hv_util device state
Message-ID	<qb7sd-5bY-1@gated-at.bofh.it>
In reply to	#1229194

Olaf Hering <olaf@aepfle.de> writes:

> On Sun, Sep 20, Greg KH wrote:
>
>> Just use a lock, that's what it is there for.
>
> How would that help? It might help because it enforces ordering. But
> that requires that all three utils get refactored to deal with the
> introduced locking. I will let KY comment on this.
>
> The issue I see with fcopy is that after or while fcopy_respond_to_host
> runs an interrupt triggers which also calls into
> hv_fcopy_onchannelcallback.  It was most likely caused by a logic change
> in "recent" vmbus updates because this did not happen before. At least,
> the fcopy hang was not seen earler. Maybe the bug did just not trigger
> up to now for other reasons...

I'd like to see a trace from the hang, it is not obvious to me how it
happened and what caused it. (or if you have such hang scenario in your
head, can you please reveal it?) AFAICS barriers you introduced don't
give you guarantees in an SMP environment.

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

#1229244 — Re: [PATCH 2/5] hv: add helpers to handle hv_util device state

From	Olaf Hering <olaf@aepfle.de>
Date	2015-09-21 14:20 +0200
Subject	Re: [PATCH 2/5] hv: add helpers to handle hv_util device state
Message-ID	<qb8eB-6lp-5@gated-at.bofh.it>
In reply to	#1229224

On Mon, Sep 21, Vitaly Kuznetsov wrote:

> I'd like to see a trace from the hang, it is not obvious to me how it
> happened and what caused it. (or if you have such hang scenario in your
> head, can you please reveal it?)

There is no trace. I think fcopy_respond_to_host notifies the host,
which in turn triggers an interrupt right away which is processed while
fcopy_on_msg is executing somewhere between the return from
fcopy_respond_to_host and the call into hv_fcopy_onchannelcallback.

> AFAICS barriers you introduced don't give you guarantees in an SMP environment.

Happens to work on x86, and for this purpose. I will see how to add
locking around access to  state and context.

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]

#1229327 — Re: [PATCH 2/5] hv: add helpers to handle hv_util device state

From	Vitaly Kuznetsov <vkuznets@redhat.com>
Date	2015-09-21 15:40 +0200
Subject	Re: [PATCH 2/5] hv: add helpers to handle hv_util device state
Message-ID	<qb9u3-83M-45@gated-at.bofh.it>
In reply to	#1229244

Olaf Hering <olaf@aepfle.de> writes:

> On Mon, Sep 21, Vitaly Kuznetsov wrote:
>
>> I'd like to see a trace from the hang, it is not obvious to me how it
>> happened and what caused it. (or if you have such hang scenario in your
>> head, can you please reveal it?)
>
> There is no trace. I think fcopy_respond_to_host notifies the host,
> which in turn triggers an interrupt right away which is processed while
> fcopy_on_msg is executing somewhere between the return from
> fcopy_respond_to_host and the call into hv_fcopy_onchannelcallback.
>

I think it is fcopy_transaction.fcopy_context which gets out of sync.

When we're done processing some request we have the following code:

		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);

If interrupt happens after we did fcopy_respond_to_host()
fcopy_transaction.state will still be HVUTIL_USERSPACE_RECV or even its
previous HVUTIL_USERSPACE_REQ but it's OK as we have the following in
hv_fcopy_onchannelcallback()

	if (fcopy_transaction.state > HVUTIL_READY) {
		/*
		 * We will defer processing this callback once
		 * the current transaction is complete.
		 */
		fcopy_transaction.fcopy_context = context;
		return;
	}

And we're supposed to process the work with hv_poll_channel(). The
problem is (I guess) that fcopy_transaction.fcopy_context gets out of
sync and it still has its previous value (possibly NULL). We call
hv_poll_channel() with NULL and everything gets stuck as we'll never
process the request.

AFAICS proper locking is requred here (and probably in all three
drivers), we need to protect not only .state but the whole transaction.

[...]

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

#1229561 — RE: [PATCH 2/5] hv: add helpers to handle hv_util device state

From	KY Srinivasan <kys@microsoft.com>
Date	2015-09-21 18:50 +0200
Subject	RE: [PATCH 2/5] hv: add helpers to handle hv_util device state
Message-ID	<qbcrV-3RU-37@gated-at.bofh.it>
In reply to	#1229244

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogT2xhZiBIZXJpbmcgW21h
aWx0bzpvbGFmQGFlcGZsZS5kZV0NCj4gU2VudDogTW9uZGF5LCBTZXB0ZW1iZXIgMjEsIDIwMTUg
NToxNyBBTQ0KPiBUbzogVml0YWx5IEt1em5ldHNvdiA8dmt1em5ldHNAcmVkaGF0LmNvbT4NCj4g
Q2M6IEtZIFNyaW5pdmFzYW4gPGt5c0BtaWNyb3NvZnQuY29tPjsgR3JlZyBLSA0KPiA8Z3JlZ2to
QGxpbnV4Zm91bmRhdGlvbi5vcmc+OyBsaW51eC1rZXJuZWxAdmdlci5rZXJuZWwub3JnOw0KPiBk
ZXZlbEBsaW51eGRyaXZlcnByb2plY3Qub3JnOyBhcHdAY2Fub25pY2FsLmNvbTsgamFzb3dhbmdA
cmVkaGF0LmNvbQ0KPiBTdWJqZWN0OiBSZTogW1BBVENIIDIvNV0gaHY6IGFkZCBoZWxwZXJzIHRv
IGhhbmRsZSBodl91dGlsIGRldmljZSBzdGF0ZQ0KPiANCj4gT24gTW9uLCBTZXAgMjEsIFZpdGFs
eSBLdXpuZXRzb3Ygd3JvdGU6DQo+IA0KPiA+IEknZCBsaWtlIHRvIHNlZSBhIHRyYWNlIGZyb20g
dGhlIGhhbmcsIGl0IGlzIG5vdCBvYnZpb3VzIHRvIG1lIGhvdyBpdA0KPiA+IGhhcHBlbmVkIGFu
ZCB3aGF0IGNhdXNlZCBpdC4gKG9yIGlmIHlvdSBoYXZlIHN1Y2ggaGFuZyBzY2VuYXJpbyBpbiB5
b3VyDQo+ID4gaGVhZCwgY2FuIHlvdSBwbGVhc2UgcmV2ZWFsIGl0PykNCj4gDQo+IFRoZXJlIGlz
IG5vIHRyYWNlLiBJIHRoaW5rIGZjb3B5X3Jlc3BvbmRfdG9faG9zdCBub3RpZmllcyB0aGUgaG9z
dCwNCj4gd2hpY2ggaW4gdHVybiB0cmlnZ2VycyBhbiBpbnRlcnJ1cHQgcmlnaHQgYXdheSB3aGlj
aCBpcyBwcm9jZXNzZWQgd2hpbGUNCj4gZmNvcHlfb25fbXNnIGlzIGV4ZWN1dGluZyBzb21ld2hl
cmUgYmV0d2VlbiB0aGUgcmV0dXJuIGZyb20NCj4gZmNvcHlfcmVzcG9uZF90b19ob3N0IGFuZCB0
aGUgY2FsbCBpbnRvIGh2X2Zjb3B5X29uY2hhbm5lbGNhbGxiYWNrLg0KPiANCj4gPiBBRkFJQ1Mg
YmFycmllcnMgeW91IGludHJvZHVjZWQgZG9uJ3QgZ2l2ZSB5b3UgZ3VhcmFudGVlcyBpbiBhbiBT
TVANCj4gZW52aXJvbm1lbnQuDQo+IA0KPiBIYXBwZW5zIHRvIHdvcmsgb24geDg2LCBhbmQgZm9y
IHRoaXMgcHVycG9zZS4gSSB3aWxsIHNlZSBob3cgdG8gYWRkDQo+IGxvY2tpbmcgYXJvdW5kIGFj
Y2VzcyB0byAgc3RhdGUgYW5kIGNvbnRleHQuDQoNCkFsbCBhY3Rpdml0eSBzdGFydHMgd2l0aCB0
aGUgaW50ZXJydXB0IGhhbmRsZXIgLSB0aGlzIGlzIHRoZSBzdGFydCBvZiBlYWNoIG5ldw0KdHJh
bnNhY3Rpb24uIEdpdmVuIHRoYXQgd2UgaGF2ZSBvbmx5IG9uZSBvdXRzdGFuZGluZyB0cmFuc2Fj
dGlvbiBhdCBhIHRpbWUNCndlIGhhdmUgbmF0dXJhbGx5IHNlcmlhbGl6ZWQgdGhlIG9wZXJhdGlv
bnMuDQoNCklmIHdlIGZvcmNlIGFsbCBhY3Rpdml0eSBvbnRvIHRoZSBjb3JyZWN0IENQVSAodGhl
IENQVSB0aGUgY2hhbm5lbCBpcyBib3VuZCB0bykgDQooY3VycmVudGx5IHdlIGZvcmNlIHBvbGxp
bmcgdG8gb2NjdXIgb24gdGhlIGNvcnJlY3QgQ1BVKSB3ZSBzaG91bGQgYmUgZmluZS4NCg0KUmVn
YXJkcywNCg0KSy4gWSANCj4gDQo+IE9sYWYNCg==
--
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]

#1229544 — RE: [PATCH 2/5] hv: add helpers to handle hv_util device state

From	KY Srinivasan <kys@microsoft.com>
Date	2015-09-21 18:40 +0200
Subject	RE: [PATCH 2/5] hv: add helpers to handle hv_util device state
Message-ID	<qbcid-3Gm-3@gated-at.bofh.it>
In reply to	#1229194

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogT2xhZiBIZXJpbmcgW21h
aWx0bzpvbGFmQGFlcGZsZS5kZV0NCj4gU2VudDogTW9uZGF5LCBTZXB0ZW1iZXIgMjEsIDIwMTUg
MzoyNiBBTQ0KPiBUbzogS1kgU3Jpbml2YXNhbiA8a3lzQG1pY3Jvc29mdC5jb20+OyBHcmVnIEtI
DQo+IDxncmVna2hAbGludXhmb3VuZGF0aW9uLm9yZz4NCj4gQ2M6IGxpbnV4LWtlcm5lbEB2Z2Vy
Lmtlcm5lbC5vcmc7IGRldmVsQGxpbnV4ZHJpdmVycHJvamVjdC5vcmc7DQo+IGFwd0BjYW5vbmlj
YWwuY29tOyB2a3V6bmV0c0ByZWRoYXQuY29tOyBqYXNvd2FuZ0ByZWRoYXQuY29tDQo+IFN1Ympl
Y3Q6IFJlOiBbUEFUQ0ggMi81XSBodjogYWRkIGhlbHBlcnMgdG8gaGFuZGxlIGh2X3V0aWwgZGV2
aWNlIHN0YXRlDQo+IA0KPiBPbiBTdW4sIFNlcCAyMCwgR3JlZyBLSCB3cm90ZToNCj4gDQo+ID4g
SnVzdCB1c2UgYSBsb2NrLCB0aGF0J3Mgd2hhdCBpdCBpcyB0aGVyZSBmb3IuDQo+IA0KPiBIb3cg
d291bGQgdGhhdCBoZWxwPyBJdCBtaWdodCBoZWxwIGJlY2F1c2UgaXQgZW5mb3JjZXMgb3JkZXJp
bmcuIEJ1dA0KPiB0aGF0IHJlcXVpcmVzIHRoYXQgYWxsIHRocmVlIHV0aWxzIGdldCByZWZhY3Rv
cmVkIHRvIGRlYWwgd2l0aCB0aGUNCj4gaW50cm9kdWNlZCBsb2NraW5nLiBJIHdpbGwgbGV0IEtZ
IGNvbW1lbnQgb24gdGhpcy4NCj4gDQo+IFRoZSBpc3N1ZSBJIHNlZSB3aXRoIGZjb3B5IGlzIHRo
YXQgYWZ0ZXIgb3Igd2hpbGUgZmNvcHlfcmVzcG9uZF90b19ob3N0DQo+IHJ1bnMgYW4gaW50ZXJy
dXB0IHRyaWdnZXJzIHdoaWNoIGFsc28gY2FsbHMgaW50bw0KPiBodl9mY29weV9vbmNoYW5uZWxj
YWxsYmFjay4gIEl0IHdhcyBtb3N0IGxpa2VseSBjYXVzZWQgYnkgYSBsb2dpYyBjaGFuZ2UNCj4g
aW4gInJlY2VudCIgdm1idXMgdXBkYXRlcyBiZWNhdXNlIHRoaXMgZGlkIG5vdCBoYXBwZW4gYmVm
b3JlLiBBdCBsZWFzdCwNCj4gdGhlIGZjb3B5IGhhbmcgd2FzIG5vdCBzZWVuIGVhcmxlci4gTWF5
YmUgdGhlIGJ1ZyBkaWQganVzdCBub3QgdHJpZ2dlcg0KPiB1cCB0byBub3cgZm9yIG90aGVyIHJl
YXNvbnMuLi4NCg0KQWxsIHV0aWwgY2hhbm5lbHMgYXJlIGJvdW5kIHRvIENQVSAwLiBKdXN0IGZv
cmNpbmcgYWxsIGFjdGl2aXR5IG9uIENQVSAwIG1heSBiZSB0aGUgDQpzaW1wbGVzdCBzb2x1dGlv
biBoZXJlLiBCZXNpZGVzLCB0aGVzZSBhcmUgbm90IHBlcmZvcm1hbmNlIGNyaXRpY2FsIHNlcnZp
Y2VzIGFueXdheS4NCg0KVGhlIHByb2JsZW0geW91IG1heSBoYXZlIHJ1biBpbnRvIGNvdWxkIGJl
IHJlbGF0ZWQgdG8gdGhlIGZhY3QgdGhhdCB3ZSBjb3VsZCBwb3RlbnRpYWxseQ0KcnVuIHRoZSBw
b2xsaW5nIGZ1bmN0aW9uIG9uIGEgQ1BVIG90aGVyIHRoYW4gQ1BVIDAuDQoNClJlZ2FyZHMsDQoN
CksuIFkNCj4gDQo+IE9sYWYNCg==
--
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]

#1229563 — Re: [PATCH 2/5] hv: add helpers to handle hv_util device state

From	Greg KH <gregkh@linuxfoundation.org>
Date	2015-09-21 18:50 +0200
Subject	Re: [PATCH 2/5] hv: add helpers to handle hv_util device state
Message-ID	<qbcrV-3RU-41@gated-at.bofh.it>
In reply to	#1229544

On Mon, Sep 21, 2015 at 04:34:56PM +0000, KY Srinivasan wrote:
> 
> 
> > -----Original Message-----
> > From: Olaf Hering [mailto:olaf@aepfle.de]
> > Sent: Monday, September 21, 2015 3:26 AM
> > To: KY Srinivasan <kys@microsoft.com>; Greg KH
> > <gregkh@linuxfoundation.org>
> > Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> > apw@canonical.com; vkuznets@redhat.com; jasowang@redhat.com
> > Subject: Re: [PATCH 2/5] hv: add helpers to handle hv_util device state
> > 
> > On Sun, Sep 20, Greg KH wrote:
> > 
> > > Just use a lock, that's what it is there for.
> > 
> > How would that help? It might help because it enforces ordering. But
> > that requires that all three utils get refactored to deal with the
> > introduced locking. I will let KY comment on this.
> > 
> > The issue I see with fcopy is that after or while fcopy_respond_to_host
> > runs an interrupt triggers which also calls into
> > hv_fcopy_onchannelcallback.  It was most likely caused by a logic change
> > in "recent" vmbus updates because this did not happen before. At least,
> > the fcopy hang was not seen earler. Maybe the bug did just not trigger
> > up to now for other reasons...
> 
> All util channels are bound to CPU 0. Just forcing all activity on CPU 0 may be the 
> simplest solution here. Besides, these are not performance critical services anyway.
> 
> The problem you may have run into could be related to the fact that we could potentially
> run the polling function on a CPU other than CPU 0.

Again, this sounds like a locking issue, you have multiple
threads/processes accessing the same data.  Even if you bind it all to
one cpu, this shows a real design problem.

Use a lock to fix this properly.  That way, when you stop using only one
CPU, the code will "just work", and if you are really only on one CPU
today, there will not be any lock contention.

thanks,

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

#1229577 — RE: [PATCH 2/5] hv: add helpers to handle hv_util device state

From	KY Srinivasan <kys@microsoft.com>
Date	2015-09-21 19:10 +0200
Subject	RE: [PATCH 2/5] hv: add helpers to handle hv_util device state
Message-ID	<qbcLg-4tN-23@gated-at.bofh.it>
In reply to	#1229563


> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Monday, September 21, 2015 9:44 AM
> To: KY Srinivasan <kys@microsoft.com>
> Cc: Olaf Hering <olaf@aepfle.de>; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; apw@canonical.com; vkuznets@redhat.com;
> jasowang@redhat.com
> Subject: Re: [PATCH 2/5] hv: add helpers to handle hv_util device state
> 
> On Mon, Sep 21, 2015 at 04:34:56PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Olaf Hering [mailto:olaf@aepfle.de]
> > > Sent: Monday, September 21, 2015 3:26 AM
> > > To: KY Srinivasan <kys@microsoft.com>; Greg KH
> > > <gregkh@linuxfoundation.org>
> > > Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> > > apw@canonical.com; vkuznets@redhat.com; jasowang@redhat.com
> > > Subject: Re: [PATCH 2/5] hv: add helpers to handle hv_util device state
> > >
> > > On Sun, Sep 20, Greg KH wrote:
> > >
> > > > Just use a lock, that's what it is there for.
> > >
> > > How would that help? It might help because it enforces ordering. But
> > > that requires that all three utils get refactored to deal with the
> > > introduced locking. I will let KY comment on this.
> > >
> > > The issue I see with fcopy is that after or while fcopy_respond_to_host
> > > runs an interrupt triggers which also calls into
> > > hv_fcopy_onchannelcallback.  It was most likely caused by a logic change
> > > in "recent" vmbus updates because this did not happen before. At least,
> > > the fcopy hang was not seen earler. Maybe the bug did just not trigger
> > > up to now for other reasons...
> >
> > All util channels are bound to CPU 0. Just forcing all activity on CPU 0 may be
> the
> > simplest solution here. Besides, these are not performance critical services
> anyway.
> >
> > The problem you may have run into could be related to the fact that we
> could potentially
> > run the polling function on a CPU other than CPU 0.
> 
> Again, this sounds like a locking issue, you have multiple
> threads/processes accessing the same data.  Even if you bind it all to
> one cpu, this shows a real design problem.
> 
> Use a lock to fix this properly.  That way, when you stop using only one
> CPU, the code will "just work", and if you are really only on one CPU
> today, there will not be any lock contention.
> 
> thanks,

Thanks Greg; will do.

Regards,

K. Y
> 
> greg k-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]

#1225606 — [PATCH 3/5] hv: fcopy: use wrappers to propagate state

From	"K. Y. Srinivasan" <kys@microsoft.com>
Date	2015-09-16 02:20 +0200
Subject	[PATCH 3/5] hv: fcopy: use wrappers to propagate state
Message-ID	<q98C5-4ND-1@gated-at.bofh.it>
In reply to	#1225578

From: Olaf Hering <olaf@aepfle.de>

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>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/hv/hv_fcopy.c |   36 ++++++++++++++++++++----------------
 1 files 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);
 }
-- 
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] | [prev] | [standalone]

csiph-web

[PATCH 0/5] Drivers: hv: Miscellaneous fixes

Contents

#1225576 — [PATCH 0/5] Drivers: hv: Miscellaneous fixes

#1225578 — [PATCH 1/5] Drivers: hv: vmbus: fix init_vp_index() for reloading hv_netvsc

#1225579 — [PATCH 4/5] hv: kvp: use wrappers to propaigate state

#1229043 — Re: [PATCH 4/5] hv: kvp: use wrappers to propaigate state

#1229185 — Re: [PATCH 4/5] hv: kvp: use wrappers to propaigate state

#1229547 — RE: [PATCH 4/5] hv: kvp: use wrappers to propaigate state

#1229525 — RE: [PATCH 4/5] hv: kvp: use wrappers to propaigate state

#1225580 — [PATCH 5/5] hv: vss: use wrappers to propagate state

#1225581 — [PATCH 2/5] hv: add helpers to handle hv_util device state

#1229041 — Re: [PATCH 2/5] hv: add helpers to handle hv_util device state

#1229194 — Re: [PATCH 2/5] hv: add helpers to handle hv_util device state

#1229224 — Re: [PATCH 2/5] hv: add helpers to handle hv_util device state

#1229244 — Re: [PATCH 2/5] hv: add helpers to handle hv_util device state

#1229327 — Re: [PATCH 2/5] hv: add helpers to handle hv_util device state

#1229561 — RE: [PATCH 2/5] hv: add helpers to handle hv_util device state

#1229544 — RE: [PATCH 2/5] hv: add helpers to handle hv_util device state

#1229563 — Re: [PATCH 2/5] hv: add helpers to handle hv_util device state

#1229577 — RE: [PATCH 2/5] hv: add helpers to handle hv_util device state

#1225606 — [PATCH 3/5] hv: fcopy: use wrappers to propagate state