[PATCH 0/4] Drivers: hv: vmbus: bugfix and improvements for message handling

[PATCH 0/4] Drivers: hv: vmbus: bugfix and improvements for message handling

[Vitaly Kuznetsov]

First patch of the series addresses a potentially important issue,
second patch tries to make crash path more stable, the rest is just
a cleanup.

Please review.

Vitaly Kuznetsov (4):
  Drivers: hv: vmbus: don't loose HVMSG_TIMER_EXPIRED messages
  Drivers: hv: vmbus: avoid wait_for_completion() on crash
  Drivers: hv: vmbus: remove code duplication in message handling
  Drivers: hv: vmbus: avoid unneeded compiler optimizations in
    vmbus_wait_for_unload()

 drivers/hv/channel_mgmt.c | 16 +++------
 drivers/hv/connection.c   |  2 +-
 drivers/hv/hyperv_vmbus.h | 26 ++++++++++++++-
 drivers/hv/vmbus_drv.c    | 82 +++++++++++++----------------------------------
 4 files changed, 52 insertions(+), 74 deletions(-)

-- 
2.5.0

[PATCH 1/4] Drivers: hv: vmbus: don't loose HVMSG_TIMER_EXPIRED messages

[Vitaly Kuznetsov]

We must handle HVMSG_TIMER_EXPIRED messages in the interrupt context
and we offload all the rest to vmbus_on_msg_dpc() tasklet. This functions
loops to see if there are new messages pending. In case we'll ever see
HVMSG_TIMER_EXPIRED message there we're going to lose it as we can't
handle it from there. Avoid looping in vmbus_on_msg_dpc(), we're OK
with handling one message per interrupt.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/hv/vmbus_drv.c | 68 ++++++++++++++++++++++++--------------------------
 1 file changed, 33 insertions(+), 35 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 328e4c3..895eeed 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -716,51 +716,49 @@ static void vmbus_on_msg_dpc(unsigned long data)
 	struct vmbus_channel_message_table_entry *entry;
 	struct onmessage_work_context *ctx;
 
-	while (1) {
-		if (msg->header.message_type == HVMSG_NONE)
-			/* no msg */
-			break;
+	if (msg->header.message_type == HVMSG_NONE)
+		/* no msg */
+		return;
 
-		hdr = (struct vmbus_channel_message_header *)msg->u.payload;
+	hdr = (struct vmbus_channel_message_header *)msg->u.payload;
 
-		if (hdr->msgtype >= CHANNELMSG_COUNT) {
-			WARN_ONCE(1, "unknown msgtype=%d\n", hdr->msgtype);
-			goto msg_handled;
-		}
+	if (hdr->msgtype >= CHANNELMSG_COUNT) {
+		WARN_ONCE(1, "unknown msgtype=%d\n", hdr->msgtype);
+		goto msg_handled;
+	}
 
-		entry = &channel_message_table[hdr->msgtype];
-		if (entry->handler_type	== VMHT_BLOCKING) {
-			ctx = kmalloc(sizeof(*ctx), GFP_ATOMIC);
-			if (ctx == NULL)
-				continue;
+	entry = &channel_message_table[hdr->msgtype];
+	if (entry->handler_type	== VMHT_BLOCKING) {
+		ctx = kmalloc(sizeof(*ctx), GFP_ATOMIC);
+		if (ctx == NULL)
+			return;
 
-			INIT_WORK(&ctx->work, vmbus_onmessage_work);
-			memcpy(&ctx->msg, msg, sizeof(*msg));
+		INIT_WORK(&ctx->work, vmbus_onmessage_work);
+		memcpy(&ctx->msg, msg, sizeof(*msg));
 
-			queue_work(vmbus_connection.work_queue, &ctx->work);
-		} else
-			entry->message_handler(hdr);
+		queue_work(vmbus_connection.work_queue, &ctx->work);
+	} else
+		entry->message_handler(hdr);
 
 msg_handled:
-		msg->header.message_type = HVMSG_NONE;
+	msg->header.message_type = HVMSG_NONE;
+
+	/*
+	 * Make sure the write to MessageType (ie set to
+	 * HVMSG_NONE) happens before we read the
+	 * MessagePending and EOMing. Otherwise, the EOMing
+	 * will not deliver any more messages since there is
+	 * no empty slot
+	 */
+	mb();
 
+	if (msg->header.message_flags.msg_pending) {
 		/*
-		 * Make sure the write to MessageType (ie set to
-		 * HVMSG_NONE) happens before we read the
-		 * MessagePending and EOMing. Otherwise, the EOMing
-		 * will not deliver any more messages since there is
-		 * no empty slot
+		 * This will cause message queue rescan to
+		 * possibly deliver another msg from the
+		 * hypervisor
 		 */
-		mb();
-
-		if (msg->header.message_flags.msg_pending) {
-			/*
-			 * This will cause message queue rescan to
-			 * possibly deliver another msg from the
-			 * hypervisor
-			 */
-			wrmsrl(HV_X64_MSR_EOM, 0);
-		}
+		wrmsrl(HV_X64_MSR_EOM, 0);
 	}
 }
 
-- 
2.5.0

[PATCH 2/4] Drivers: hv: vmbus: avoid wait_for_completion() on crash

[Vitaly Kuznetsov]

wait_for_completion() may sleep, it enables interrupts and this
is something we really want to avoid on crashes because interrupt
handlers can cause other crashes. Switch to the recently introduced
vmbus_wait_for_unload() doing busy wait instead.

Reported-by: Radim Krcmar <rkrcmar@redhat.com>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/hv/channel_mgmt.c | 4 ++--
 drivers/hv/connection.c   | 2 +-
 drivers/hv/hyperv_vmbus.h | 2 +-
 drivers/hv/vmbus_drv.c    | 4 ++--
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 763d0c1..bd60207 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -556,7 +556,7 @@ static void vmbus_unload_response(struct vmbus_channel_message_header *hdr)
 	complete(&vmbus_connection.unload_event);
 }
 
-void vmbus_initiate_unload(void)
+void vmbus_initiate_unload(bool crash)
 {
 	struct vmbus_channel_message_header hdr;
 
@@ -573,7 +573,7 @@ void vmbus_initiate_unload(void)
 	 * vmbus_initiate_unload() is also called on crash and the crash can be
 	 * happening in an interrupt context, where scheduling is impossible.
 	 */
-	if (!in_interrupt())
+	if (!crash)
 		wait_for_completion(&vmbus_connection.unload_event);
 	else
 		vmbus_wait_for_unload();
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index 3dc5a9c..fb63d30 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -236,7 +236,7 @@ void vmbus_disconnect(void)
 	/*
 	 * First send the unload request to the host.
 	 */
-	vmbus_initiate_unload();
+	vmbus_initiate_unload(false);
 
 	if (vmbus_connection.work_queue) {
 		drain_workqueue(vmbus_connection.work_queue);
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 4ebc796..e6160a0 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -665,7 +665,7 @@ void hv_vss_onchannelcallback(void *);
 int hv_fcopy_init(struct hv_util_service *);
 void hv_fcopy_deinit(void);
 void hv_fcopy_onchannelcallback(void *);
-void vmbus_initiate_unload(void);
+void vmbus_initiate_unload(bool crash);
 
 static inline void hv_poll_channel(struct vmbus_channel *channel,
 				   void (*cb)(void *))
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 895eeed..08f98a1 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1266,7 +1266,7 @@ static void hv_kexec_handler(void)
 	int cpu;
 
 	hv_synic_clockevents_cleanup();
-	vmbus_initiate_unload();
+	vmbus_initiate_unload(false);
 	for_each_online_cpu(cpu)
 		smp_call_function_single(cpu, hv_synic_cleanup, NULL, 1);
 	hv_cleanup();
@@ -1274,7 +1274,7 @@ static void hv_kexec_handler(void)
 
 static void hv_crash_handler(struct pt_regs *regs)
 {
-	vmbus_initiate_unload();
+	vmbus_initiate_unload(true);
 	/*
 	 * In crash handler we can't schedule synic cleanup for all CPUs,
 	 * doing the cleanup for current CPU only. This should be sufficient
-- 
2.5.0

[PATCH 3/4] Drivers: hv: vmbus: remove code duplication in message handling

[Vitaly Kuznetsov]

We have 3 functions dealing with messages and they all implement
the same logic to finalize reads, move it to vmbus_signal_eom().

Suggested-by: Radim Krcmar <rkrcmar@redhat.com>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/hv/channel_mgmt.c | 10 +---------
 drivers/hv/hyperv_vmbus.h | 24 ++++++++++++++++++++++++
 drivers/hv/vmbus_drv.c    | 40 ++--------------------------------------
 3 files changed, 27 insertions(+), 47 deletions(-)

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index bd60207..1e43967 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -529,15 +529,7 @@ static void vmbus_wait_for_unload(void)
 		if (hdr->msgtype == CHANNELMSG_UNLOAD_RESPONSE)
 			unloaded = true;
 
-		msg->header.message_type = HVMSG_NONE;
-		/*
-		 * header.message_type needs to be written before we do
-		 * wrmsrl() below.
-		 */
-		mb();
-
-		if (msg->header.message_flags.msg_pending)
-			wrmsrl(HV_X64_MSR_EOM, 0);
+		vmbus_signal_eom(msg);
 
 		if (unloaded)
 			break;
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index e6160a0..86049a3 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -626,6 +626,30 @@ struct vmbus_channel_message_table_entry {
 extern struct vmbus_channel_message_table_entry
 	channel_message_table[CHANNELMSG_COUNT];
 
+/* Free the message slot and signal end-of-message if required */
+static inline void vmbus_signal_eom(struct hv_message *msg)
+{
+	msg->header.message_type = HVMSG_NONE;
+
+	/*
+	 * Make sure the write to MessageType (ie set to
+	 * HVMSG_NONE) happens before we read the
+	 * MessagePending and EOMing. Otherwise, the EOMing
+	 * will not deliver any more messages since there is
+	 * no empty slot
+	 */
+	mb();
+
+	if (msg->header.message_flags.msg_pending) {
+		/*
+		 * This will cause message queue rescan to
+		 * possibly deliver another msg from the
+		 * hypervisor
+		 */
+		wrmsrl(HV_X64_MSR_EOM, 0);
+	}
+}
+
 /* General vmbus interface */
 
 struct hv_device *vmbus_device_create(const uuid_le *type,
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 08f98a1..c9204c0 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -685,25 +685,7 @@ static void hv_process_timer_expiration(struct hv_message *msg, int cpu)
 	if (dev->event_handler)
 		dev->event_handler(dev);
 
-	msg->header.message_type = HVMSG_NONE;
-
-	/*
-	 * Make sure the write to MessageType (ie set to
-	 * HVMSG_NONE) happens before we read the
-	 * MessagePending and EOMing. Otherwise, the EOMing
-	 * will not deliver any more messages since there is
-	 * no empty slot
-	 */
-	mb();
-
-	if (msg->header.message_flags.msg_pending) {
-		/*
-		 * This will cause message queue rescan to
-		 * possibly deliver another msg from the
-		 * hypervisor
-		 */
-		wrmsrl(HV_X64_MSR_EOM, 0);
-	}
+	vmbus_signal_eom(msg);
 }
 
 static void vmbus_on_msg_dpc(unsigned long data)
@@ -741,25 +723,7 @@ static void vmbus_on_msg_dpc(unsigned long data)
 		entry->message_handler(hdr);
 
 msg_handled:
-	msg->header.message_type = HVMSG_NONE;
-
-	/*
-	 * Make sure the write to MessageType (ie set to
-	 * HVMSG_NONE) happens before we read the
-	 * MessagePending and EOMing. Otherwise, the EOMing
-	 * will not deliver any more messages since there is
-	 * no empty slot
-	 */
-	mb();
-
-	if (msg->header.message_flags.msg_pending) {
-		/*
-		 * This will cause message queue rescan to
-		 * possibly deliver another msg from the
-		 * hypervisor
-		 */
-		wrmsrl(HV_X64_MSR_EOM, 0);
-	}
+	vmbus_signal_eom(msg);
 }
 
 static void vmbus_isr(void)
-- 
2.5.0

[PATCH 4/4] Drivers: hv: vmbus: avoid unneeded compiler optimizations in vmbus_wait_for_unload()

[Vitaly Kuznetsov]

Message header is modified by the hypervisor and we read it in a loop,
we need to prevent compilers from optimizing accesses. There are no such
optimizations at this moment, this is just a future proof.

Suggested-by: Radim Krcmar <rkrcmar@redhat.com>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/hv/channel_mgmt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 1e43967..b10e8f74 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -520,7 +520,7 @@ static void vmbus_wait_for_unload(void)
 	bool unloaded = false;
 
 	while (1) {
-		if (msg->header.message_type == HVMSG_NONE) {
+		if (READ_ONCE(msg->header.message_type) == HVMSG_NONE) {
 			mdelay(10);
 			continue;
 		}
-- 
2.5.0

Re: [PATCH 1/4] Drivers: hv: vmbus: don't loose HVMSG_TIMER_EXPIRED messages

[Radim Krcmar]

2016-02-12 16:42+0100, Vitaly Kuznetsov:
> We must handle HVMSG_TIMER_EXPIRED messages in the interrupt context
> and we offload all the rest to vmbus_on_msg_dpc() tasklet. This functions
> loops to see if there are new messages pending. In case we'll ever see
> HVMSG_TIMER_EXPIRED message there we're going to lose it as we can't
> handle it from there. Avoid looping in vmbus_on_msg_dpc(), we're OK
> with handling one message per interrupt.

We could loop as long as the message type is handleable in the tasklet,
which might increase performance, but the code nicer this way;

Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> @@ -716,51 +716,49 @@ static void vmbus_on_msg_dpc(unsigned long data)
>  	struct vmbus_channel_message_table_entry *entry;
>  	struct onmessage_work_context *ctx;
>  
> -	while (1) {
> -		if (msg->header.message_type == HVMSG_NONE)
> -			/* no msg */
> -			break;
> +	if (msg->header.message_type == HVMSG_NONE)

(This condition should never be true now.)

> +		/* no msg */
> +		return;

Btw. what is/are the other HVMSG_* that we get?

Re: [PATCH 2/4] Drivers: hv: vmbus: avoid wait_for_completion() on crash

[Radim Krcmar]

2016-02-12 16:42+0100, Vitaly Kuznetsov:
> wait_for_completion() may sleep, it enables interrupts and this
> is something we really want to avoid on crashes because interrupt
> handlers can cause other crashes. Switch to the recently introduced
> vmbus_wait_for_unload() doing busy wait instead.

(Too bad we fix crashers, so there is nothing to test on. :])

> Reported-by: Radim Krcmar <rkrcmar@redhat.com>

Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  drivers/hv/channel_mgmt.c | 4 ++--
>  drivers/hv/connection.c   | 2 +-
>  drivers/hv/hyperv_vmbus.h | 2 +-
>  drivers/hv/vmbus_drv.c    | 4 ++--
>  4 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index 763d0c1..bd60207 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -556,7 +556,7 @@ static void vmbus_unload_response(struct vmbus_channel_message_header *hdr)
>  	complete(&vmbus_connection.unload_event);
>  }
>  
> -void vmbus_initiate_unload(void)
> +void vmbus_initiate_unload(bool crash)
>  {
>  	struct vmbus_channel_message_header hdr;
>  
> @@ -573,7 +573,7 @@ void vmbus_initiate_unload(void)
>  	 * vmbus_initiate_unload() is also called on crash and the crash can be
>  	 * happening in an interrupt context, where scheduling is impossible.
>  	 */
> -	if (!in_interrupt())
> +	if (!crash)
>  		wait_for_completion(&vmbus_connection.unload_event);
>  	else
>  		vmbus_wait_for_unload();
> diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
> index 3dc5a9c..fb63d30 100644
> --- a/drivers/hv/connection.c
> +++ b/drivers/hv/connection.c
> @@ -236,7 +236,7 @@ void vmbus_disconnect(void)
>  	/*
>  	 * First send the unload request to the host.
>  	 */
> -	vmbus_initiate_unload();
> +	vmbus_initiate_unload(false);
>  
>  	if (vmbus_connection.work_queue) {
>  		drain_workqueue(vmbus_connection.work_queue);
> diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> index 4ebc796..e6160a0 100644
> --- a/drivers/hv/hyperv_vmbus.h
> +++ b/drivers/hv/hyperv_vmbus.h
> @@ -665,7 +665,7 @@ void hv_vss_onchannelcallback(void *);
>  int hv_fcopy_init(struct hv_util_service *);
>  void hv_fcopy_deinit(void);
>  void hv_fcopy_onchannelcallback(void *);
> -void vmbus_initiate_unload(void);
> +void vmbus_initiate_unload(bool crash);
>  
>  static inline void hv_poll_channel(struct vmbus_channel *channel,
>  				   void (*cb)(void *))
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 895eeed..08f98a1 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1266,7 +1266,7 @@ static void hv_kexec_handler(void)
>  	int cpu;
>  
>  	hv_synic_clockevents_cleanup();
> -	vmbus_initiate_unload();
> +	vmbus_initiate_unload(false);
>  	for_each_online_cpu(cpu)
>  		smp_call_function_single(cpu, hv_synic_cleanup, NULL, 1);
>  	hv_cleanup();
> @@ -1274,7 +1274,7 @@ static void hv_kexec_handler(void)
>  
>  static void hv_crash_handler(struct pt_regs *regs)
>  {
> -	vmbus_initiate_unload();
> +	vmbus_initiate_unload(true);
>  	/*
>  	 * In crash handler we can't schedule synic cleanup for all CPUs,
>  	 * doing the cleanup for current CPU only. This should be sufficient
> -- 
> 2.5.0
> 

Re: [PATCH 3/4] Drivers: hv: vmbus: remove code duplication in message handling

[Radim Krcmar]

2016-02-12 16:42+0100, Vitaly Kuznetsov:
> We have 3 functions dealing with messages and they all implement
> the same logic to finalize reads, move it to vmbus_signal_eom().
> 
> Suggested-by: Radim Krcmar <rkrcmar@redhat.com>

Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

Re: [PATCH 4/4] Drivers: hv: vmbus: avoid unneeded compiler optimizations in vmbus_wait_for_unload()

[Radim Krcmar]

2016-02-12 16:42+0100, Vitaly Kuznetsov:
> Message header is modified by the hypervisor and we read it in a loop,
> we need to prevent compilers from optimizing accesses. There are no such
> optimizations at this moment, this is just a future proof.
> 
> Suggested-by: Radim Krcmar <rkrcmar@redhat.com>

Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>