* [PATCH 1/4] Drivers: hv: vmbus: don't loose HVMSG_TIMER_EXPIRED messages
2016-02-12 15:42 [PATCH 0/4] Drivers: hv: vmbus: bugfix and improvements for message handling Vitaly Kuznetsov
@ 2016-02-12 15:42 ` Vitaly Kuznetsov
2016-02-15 20:50 ` Radim Krcmar
2016-02-12 15:42 ` [PATCH 2/4] Drivers: hv: vmbus: avoid wait_for_completion() on crash Vitaly Kuznetsov
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Vitaly Kuznetsov @ 2016-02-12 15:42 UTC (permalink / raw)
To: devel
Cc: linux-kernel, K. Y. Srinivasan, Haiyang Zhang, Radim Krcmar, Cathy Avery
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/4] Drivers: hv: vmbus: don't loose HVMSG_TIMER_EXPIRED messages
2016-02-12 15:42 ` [PATCH 1/4] Drivers: hv: vmbus: don't loose HVMSG_TIMER_EXPIRED messages Vitaly Kuznetsov
@ 2016-02-15 20:50 ` Radim Krcmar
0 siblings, 0 replies; 9+ messages in thread
From: Radim Krcmar @ 2016-02-15 20:50 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: devel, linux-kernel, K. Y. Srinivasan, Haiyang Zhang, Cathy Avery
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?
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/4] Drivers: hv: vmbus: avoid wait_for_completion() on crash
2016-02-12 15:42 [PATCH 0/4] Drivers: hv: vmbus: bugfix and improvements for message handling Vitaly Kuznetsov
2016-02-12 15:42 ` [PATCH 1/4] Drivers: hv: vmbus: don't loose HVMSG_TIMER_EXPIRED messages Vitaly Kuznetsov
@ 2016-02-12 15:42 ` Vitaly Kuznetsov
2016-02-15 21:00 ` Radim Krcmar
2016-02-12 15:42 ` [PATCH 3/4] Drivers: hv: vmbus: remove code duplication in message handling Vitaly Kuznetsov
2016-02-12 15:42 ` [PATCH 4/4] Drivers: hv: vmbus: avoid unneeded compiler optimizations in vmbus_wait_for_unload() Vitaly Kuznetsov
3 siblings, 1 reply; 9+ messages in thread
From: Vitaly Kuznetsov @ 2016-02-12 15:42 UTC (permalink / raw)
To: devel
Cc: linux-kernel, K. Y. Srinivasan, Haiyang Zhang, Radim Krcmar, Cathy Avery
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 2/4] Drivers: hv: vmbus: avoid wait_for_completion() on crash
2016-02-12 15:42 ` [PATCH 2/4] Drivers: hv: vmbus: avoid wait_for_completion() on crash Vitaly Kuznetsov
@ 2016-02-15 21:00 ` Radim Krcmar
0 siblings, 0 replies; 9+ messages in thread
From: Radim Krcmar @ 2016-02-15 21:00 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: devel, linux-kernel, K. Y. Srinivasan, Haiyang Zhang, Cathy Avery
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/4] Drivers: hv: vmbus: remove code duplication in message handling
2016-02-12 15:42 [PATCH 0/4] Drivers: hv: vmbus: bugfix and improvements for message handling Vitaly Kuznetsov
2016-02-12 15:42 ` [PATCH 1/4] Drivers: hv: vmbus: don't loose HVMSG_TIMER_EXPIRED messages Vitaly Kuznetsov
2016-02-12 15:42 ` [PATCH 2/4] Drivers: hv: vmbus: avoid wait_for_completion() on crash Vitaly Kuznetsov
@ 2016-02-12 15:42 ` Vitaly Kuznetsov
2016-02-15 21:01 ` Radim Krcmar
2016-02-12 15:42 ` [PATCH 4/4] Drivers: hv: vmbus: avoid unneeded compiler optimizations in vmbus_wait_for_unload() Vitaly Kuznetsov
3 siblings, 1 reply; 9+ messages in thread
From: Vitaly Kuznetsov @ 2016-02-12 15:42 UTC (permalink / raw)
To: devel
Cc: linux-kernel, K. Y. Srinivasan, Haiyang Zhang, Radim Krcmar, Cathy Avery
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
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 4/4] Drivers: hv: vmbus: avoid unneeded compiler optimizations in vmbus_wait_for_unload()
2016-02-12 15:42 [PATCH 0/4] Drivers: hv: vmbus: bugfix and improvements for message handling Vitaly Kuznetsov
` (2 preceding siblings ...)
2016-02-12 15:42 ` [PATCH 3/4] Drivers: hv: vmbus: remove code duplication in message handling Vitaly Kuznetsov
@ 2016-02-12 15:42 ` Vitaly Kuznetsov
2016-02-15 21:12 ` Radim Krcmar
3 siblings, 1 reply; 9+ messages in thread
From: Vitaly Kuznetsov @ 2016-02-12 15:42 UTC (permalink / raw)
To: devel
Cc: linux-kernel, K. Y. Srinivasan, Haiyang Zhang, Radim Krcmar, Cathy Avery
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
^ permalink raw reply [flat|nested] 9+ messages in thread