Drivers: hv: vmbus: handle various crash scenarios
PATCH 1/1 Drivers: hv: vmbus: handle various crash scenarios
Kdump keeps biting. Turns out CHANNELMSG_UNLOAD_RESPONSE is always delivered to CPU0 regardless of what CPU we're sending CHANNELMSG_UNLOAD from. vmbus_wait_for_unload() doesn't account for the fact that in case we're crashing on some other CPU and CPU0 is still alive and operational CHANNELMSG_UNLOAD_RESPONSE will be delivered there completing vmbus_connection.unload_event, our wait on the current CPU will never end.
Do the following:
1) Check for completion_done() in the loop. In case interrupt handler is still alive we'll get the confirmation we need.
2) Always read CPU0's message page as CHANNELMSG_UNLOAD_RESPONSE will be delivered there. We can race with still-alive interrupt handler doing the same but we don't care as we're checking completion_done() now.
3) Cleanup message pages on all CPUs. This is required (at least for the current CPU as we're clearing CPU0 messages now but we may want to bring up additional CPUs on crash) as new messages won't be delivered till we consume what's pending. On boot we'll place message pages somewhere else and we won't be able to read stale messages.
--- drivers/hv/channel_mgmt.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-)
drivers/hv/channel_mgmt.c +25 -5
@@ -512,14 +512,26 @@ static void init_vp_index(struct vmbus_channel *channel, const uuid_le *type_guistatic void vmbus_wait_for_unload (void ) { -int cpu =smp_processor_id (); -void *page_addr = hv_context.synic_message_page[cpu]; +int cpu; +void *page_addr = hv_context.synic_message_page[0 ];struct hv_message *msg = (struct hv_message *)page_addr +VMBUS_MESSAGE_SINT ;struct vmbus_channel_message_header *hdr;bool unloaded = false; -while (1 ) { +/* +* CHANNELMSG_UNLOAD_RESPONSE is always delivered to CPU0. When we're +* crashing on a different CPU let's hope that IRQ handler on CPU0 is +* still functional and vmbus_unload_response() will complete +* vmbus_connection.unload_event. If not, the last thing we can do is +* read message page for CPU0 regardless of what CPU we're on. +*/ +while (!unloaded) { +if (completion_done (&vmbus_connection.unload_event)) { + unloaded = true; +break ; + } +if (READ_ONCE (msg->header.message_type) ==HVMSG_NONE ) {mdelay (10 );continue ;
@@ -530,9 +542,17 @@ static void vmbus_wait_for_unload(void) unloaded = true;vmbus_signal_eom (msg); + } -if (unloaded) -break ; +/* +* We're crashing and already got the UNLOAD_RESPONSE, cleanup all +* maybe-pending messages on all CPUs to be able to receive new +* messages after we reconnect. +*/ +for_each_online_cpu (cpu) { + page_addr = hv_context.synic_message_page[cpu]; + msg = (struct hv_message *)page_addr +VMBUS_MESSAGE_SINT ; + msg->header.message_type =HVMSG_NONE ; } }
Signature · 1 line
2.5.0
2016-03-18 13:33+0100, Vitaly Kuznetsov:
Quoted 7 lines
Kdump keeps biting. Turns out
CHANNELMSG_UNLOAD_RESPONSEis always delivered to CPU0 regardless of what CPU we're sendingCHANNELMSG_UNLOADfrom.vmbus_wait_for_unload()doesn't account for the fact that in case we're crashing on some other CPU and CPU0 is still alive and operationalCHANNELMSG_UNLOAD_RESPONSEwill be delivered there completing vmbus_connection.unload_event, our wait on the current CPU will never end.
(Any chance of learning about this behavior from the spec?)
Quoted 7 lines
Do the following:
1) Check forcompletion_done()in the loop. In case interrupt handler is still alive we'll get the confirmation we need.2) Always read CPU0's message page as
CHANNELMSG_UNLOAD_RESPONSEwill be delivered there. We can race with still-alive interrupt handler doing the same but we don't care as we're checkingcompletion_done()now.
(Yeah, seems better than hv_setup_vmbus_irq(NULL) or other hacks.)
Quoted 5 lines
3) Cleanup message pages on all CPUs. This is required (at least for the current CPU as we're clearing CPU0 messages now but we may want to bring up additional CPUs on crash) as new messages won't be delivered till we consume what's pending. On boot we'll place message pages somewhere else and we won't be able to read stale messages.
What if HV already set the pending message bit on current message, do we get any guarantees that clearing once after UNLOAD_RESPONSE is enough?
Quoted 2 lines
Signed-off-by Vitaly Kuznetsov <[email protected]>---
I had a question about NULL below. (Parenthesised rants aren't related to r-b tag. ;)
Quoted 19 lines
drivers/hv/channel_mgmt.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-)drivers/hv/channel_mgmt.c +2
-2@@ -512,14 +512,26 @@ static void init_vp_index(struct vmbus_channel *channel, const uuid_le *type_guistatic void vmbus_wait_for_unload (void ) { -int cpu =smp_processor_id (); -void *page_addr = hv_context.synic_message_page[cpu]; +int cpu; +void *page_addr = hv_context.synic_message_page[0 ];struct hv_message *msg = (struct hv_message *)page_addr +VMBUS_MESSAGE_SINT ;struct vmbus_channel_message_header *hdr;bool unloaded = false;
drivers/hv/channel_mgmt.c static void init_vp_index(struct vmbus_channel *channel, const uuid_le *type_gui · line 529
Quoted 10 lines
drivers/hv/channel_mgmt.c +8
-1-while (1 ) { +/* +* CHANNELMSG_UNLOAD_RESPONSE is always delivered to CPU0. When we're +* crashing on a different CPU let's hope that IRQ handler on CPU0 is +* still functional and vmbus_unload_response() will complete +* vmbus_connection.unload_event. If not, the last thing we can do is +* read message page for CPU0 regardless of what CPU we're on. +*/ +while (!unloaded) {
(I'd feel a bit safer if this was bounded by some timeout, but all
scenarios where this would make a difference are unplausible ... queue_work() not working while the rest is fine is the best one.)
drivers/hv/channel_mgmt.c static void init_vp_index(struct vmbus_channel *channel, const uuid_le *type_gui · line 531
Quoted 2 lines
drivers/hv/channel_mgmt.c +2
-0+if (completion_done (&vmbus_connection.unload_event)) { + unloaded = true;
(No need to set unloaded when you break.)
drivers/hv/channel_mgmt.c static void vmbus_wait_for_unload(void)
Quoted 7 lines
drivers/hv/channel_mgmt.c +3
-0+break ; + } +if (READ_ONCE (msg->header.message_type) ==HVMSG_NONE ) {mdelay (10 );continue ;@@ -530,9 +542,17 @@ static void vmbus_wait_for_unload(void)
(I'm not a huge fan of the unloaded variable; what about remembering the
header/msgtype here ...
drivers/hv/channel_mgmt.c static void vmbus_wait_for_unload(void) · line 544
Quoted 3 lines
drivers/hv/channel_mgmt.c +0
-0unloaded = true;vmbus_signal_eom (msg);
and checking its value here?)
drivers/hv/channel_mgmt.c static void vmbus_wait_for_unload(void) · line 553
Quoted 11 lines
drivers/hv/channel_mgmt.c +8
-2+ } -if (unloaded) -break ; +/* +* We're crashing and already got the UNLOAD_RESPONSE, cleanup all +* maybe-pending messages on all CPUs to be able to receive new +* messages after we reconnect. +*/ +for_each_online_cpu (cpu) { + page_addr = hv_context.synic_message_page[cpu];
Can't this be NULL?
drivers/hv/channel_mgmt.c static void vmbus_wait_for_unload(void) · line 556
Quoted 3 lines
drivers/hv/channel_mgmt.c +2
-0+ msg = (struct hv_message *)page_addr +VMBUS_MESSAGE_SINT ; + msg->header.message_type =HVMSG_NONE ; }
(And, this block belongs to a separate function. ;])
Quoted 27 lines · Radim Krcmar
2016-03-18 13:33+0100, Vitaly Kuznetsov:
Quoted 7 lines
Kdump keeps biting. Turns out
CHANNELMSG_UNLOAD_RESPONSEis always delivered to CPU0 regardless of what CPU we're sendingCHANNELMSG_UNLOADfrom.vmbus_wait_for_unload()doesn't account for the fact that in case we're crashing on some other CPU and CPU0 is still alive and operationalCHANNELMSG_UNLOAD_RESPONSEwill be delivered there completing vmbus_connection.unload_event, our wait on the current CPU will never end.(Any chance of learning about this behavior from the spec?)
Quoted 7 lines
Do the following:
1) Check forcompletion_done()in the loop. In case interrupt handler is still alive we'll get the confirmation we need.2) Always read CPU0's message page as
CHANNELMSG_UNLOAD_RESPONSEwill be delivered there. We can race with still-alive interrupt handler doing the same but we don't care as we're checkingcompletion_done()now.(Yeah, seems better than
hv_setup_vmbus_irq(NULL)or other hacks.)Quoted 5 lines
3) Cleanup message pages on all CPUs. This is required (at least for the current CPU as we're clearing CPU0 messages now but we may want to bring up additional CPUs on crash) as new messages won't be delivered till we consume what's pending. On boot we'll place message pages somewhere else and we won't be able to read stale messages.
Quoted 3 lines
What if HV already set the pending message bit on current message, do we get any guarantees that clearing once after
UNLOAD_RESPONSEis enough?
I think so but I'd like to get a confirmation from K.Y./Alex/Haiyang.
Quoted 57 lines
Quoted 3 lines
Signed-off-by Vitaly Kuznetsov <[email protected]>---
I had a question about NULL below. (Parenthesised rants aren't related to r-b tag. ;)
Quoted 19 lines
drivers/hv/channel_mgmt.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-)drivers/hv/channel_mgmt.c +2
-2@@ -512,14 +512,26 @@ static void init_vp_index(struct vmbus_channel *channel, const uuid_le *type_guistatic void vmbus_wait_for_unload (void ) { -int cpu =smp_processor_id (); -void *page_addr = hv_context.synic_message_page[cpu]; +int cpu; +void *page_addr = hv_context.synic_message_page[0 ];struct hv_message *msg = (struct hv_message *)page_addr +VMBUS_MESSAGE_SINT ;struct vmbus_channel_message_header *hdr;bool unloaded = false;drivers/hv/channel_mgmt.c static void init_vp_index(struct vmbus_channel *channel, const uuid_le *type_gui · line 529
Quoted 10 lines
drivers/hv/channel_mgmt.c +8
-1-while (1 ) { +/* +* CHANNELMSG_UNLOAD_RESPONSE is always delivered to CPU0. When we're +* crashing on a different CPU let's hope that IRQ handler on CPU0 is +* still functional and vmbus_unload_response() will complete +* vmbus_connection.unload_event. If not, the last thing we can do is +* read message page for CPU0 regardless of what CPU we're on. +*/ +while (!unloaded) {(I'd feel a bit safer if this was bounded by some timeout, but all
scenarios where this would make a difference are unplausible ...queue_work()not working while the rest is fine is the best one.)drivers/hv/channel_mgmt.c static void init_vp_index(struct vmbus_channel *channel, const uuid_le *type_gui · line 531
Quoted 2 lines
drivers/hv/channel_mgmt.c +2
-0+if (completion_done (&vmbus_connection.unload_event)) { + unloaded = true;(No need to set unloaded when you break.)
drivers/hv/channel_mgmt.c static void vmbus_wait_for_unload(void)
Quoted 7 lines
drivers/hv/channel_mgmt.c +3
-0+break ; + } +if (READ_ONCE (msg->header.message_type) ==HVMSG_NONE ) {mdelay (10 );continue ;@@ -530,9 +542,17 @@ static void vmbus_wait_for_unload(void)(I'm not a huge fan of the unloaded variable; what about remembering the
header/msgtype here ...
Quoted 6 lines
drivers/hv/channel_mgmt.c static void vmbus_wait_for_unload(void) · line 544
Quoted 3 lines
drivers/hv/channel_mgmt.c +0
-0unloaded = true;vmbus_signal_eom (msg);and checking its value here?)
Sure, but we'll have to use a variable for that ... why would it be better than 'unloaded'?
Quoted 13 lines
drivers/hv/channel_mgmt.c static void vmbus_wait_for_unload(void) · line 553
Quoted 11 lines
drivers/hv/channel_mgmt.c +8
-2+ } -if (unloaded) -break ; +/* +* We're crashing and already got the UNLOAD_RESPONSE, cleanup all +* maybe-pending messages on all CPUs to be able to receive new +* messages after we reconnect. +*/ +for_each_online_cpu (cpu) { + page_addr = hv_context.synic_message_page[cpu];Can't this be NULL?
It can't, we allocate it from hv_synic_alloc() (and we don't support cpu onlining/offlining on WS2012R2+).
Quoted 6 lines
drivers/hv/channel_mgmt.c static void vmbus_wait_for_unload(void) · line 556
Quoted 4 lines
drivers/hv/channel_mgmt.c +2
-0+ msg = (struct hv_message *)page_addr +VMBUS_MESSAGE_SINT ; + msg->header.message_type =HVMSG_NONE ; }(And, this block belongs to a separate function. ;])
I thought about moving it to hv_crash_handler() but then I decided to leave it here as the need for this fixup is rather an artifact of how we recieve the message. But I'm flexible here)
Signature · 1 line
Vitaly
Radim Krcmar
Reviewed-by2016-03-18 16:53+0100, Vitaly Kuznetsov:
Quoted 9 lines
Quoted 8 lines · Radim Krcmar
2016-03-18 13:33+0100, Vitaly Kuznetsov:
Review static void vmbus_wait_for_unload(void)
Quoted 1 line
(quoted diff) +0
-0@@ -530,9 +542,17 @@ static void vmbus_wait_for_unload(void)(I'm not a huge fan of the unloaded variable; what about remembering the
header/msgtype here ...Quoted 2 lines
drivers/hv/channel_mgmt.c +0
-0unloaded = true;
Quoted 7 lines
Quoted 4 lines
drivers/hv/channel_mgmt.c static void vmbus_wait_for_unload(void) · line 544
Quoted 1 line
drivers/hv/channel_mgmt.c +0
-0vmbus_signal_eom (msg);and checking its value here?)
Sure, but we'll have to use a variable for that ... why would it be better than 'unloaded'?
It's easier to understand IMO,
x = mem | x = mem
if *x == sth | z = *x
u = true |
eoi() | eoi()
if u | if z == sth
break | breakAnd you can replace msg with the new variable,
z = *mem
eoi()
if z == sth
break
Quoted 4 lines
Quoted 1 line
Can't this be NULL?
It can't, we allocate it from
hv_synic_alloc()(and we don't support cpu onlining/offlining on WS2012R2+).
Thanks.
Quoted 9 lines
Quoted 5 lines
drivers/hv/channel_mgmt.c static void vmbus_wait_for_unload(void) · line 556
Quoted 3 lines
drivers/hv/channel_mgmt.c +2
-0+ msg = (struct hv_message *)page_addr +VMBUS_MESSAGE_SINT ; + msg->header.message_type =HVMSG_NONE ; }(And, this block belongs to a separate function. ;])
I thought about moving it to
hv_crash_handler()but then I decided to leave it here as the need for this fixup is rather an artifact of how we recieve the message. But I'm flexible here)
Ok, clearing all VCPUs made me think that it would be generally useful.
Quoted 12 lines
-----Original Message-----
From: Vitaly Kuznetsov [mailto:[email protected]]
Sent: Friday, March 18, 2016 5:33 AM
To: [email protected]
Cc: [email protected]; KY Srinivasan <[email protected]>; Haiyang Zhang <[email protected]>; Alex Ng (LIS) <[email protected]>; Radim Krcmar <[email protected]>; Cathy Avery <[email protected]>
Subject: [PATCH] Drivers: hv: vmbus: handle various crash scenarios
Quoted 8 lines
Kdump keeps biting. Turns out
CHANNELMSG_UNLOAD_RESPONSEis always delivered to CPU0 regardless of what CPU we're sendingCHANNELMSG_UNLOAD
from.vmbus_wait_for_unload()doesn't account for the fact that in case we're crashing on some other CPU and CPU0 is still alive and operationalCHANNELMSG_UNLOAD_RESPONSEwill be delivered there completing vmbus_connection.unload_event, our wait on the current CPU will never end.
What was the host you were testing on?
K. Y
Quoted 85 lines
Do the following:
1) Check forcompletion_done()in the loop. In case interrupt handler is still alive we'll get the confirmation we need.2) Always read CPU0's message page as
CHANNELMSG_UNLOAD_RESPONSEwill be
delivered there. We can race with still-alive interrupt handler doing the same but we don't care as we're checkingcompletion_done()now.3) Cleanup message pages on all CPUs. This is required (at least for the current CPU as we're clearing CPU0 messages now but we may want to
bring
up additional CPUs on crash) as new messages won't be delivered till we consume what's pending. On boot we'll place message pages somewhere
else
and we won't be able to read stale messages.Signed-off-by Vitaly Kuznetsov <[email protected]>--- drivers/hv/channel_mgmt.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-)drivers/hv/channel_mgmt.c +4
-3@@ -512,14 +512,26 @@ static void init_vp_index(struct vmbus_channel *channel, const uuid_le *type_guistatic void vmbus_wait_for_unload (void ) { -int cpu =smp_processor_id (); -void *page_addr = hv_context.synic_message_page[cpu]; +int cpu; +void *page_addr = hv_context.synic_message_page[0 ];struct hv_message *msg = (struct hv_message *)page_addr +VMBUS_MESSAGE_SINT ;struct vmbus_channel_message_header *hdr;bool unloaded = false; -while (1 ) { +/* +* CHANNELMSG_UNLOAD_RESPONSE is always delivered to CPU0. When we're
drivers/hv/channel_mgmt.c +3
-0+* crashing on a different CPU let's hope that IRQ handler on CPU0 is +* still functional and vmbus_unload_response() will complete +* vmbus_connection.unload_event. If not, the last thing we can do is
drivers/hv/channel_mgmt.c +8
-0+* read message page for CPU0 regardless of what CPU we're on. +*/ +while (!unloaded) { +if (completion_done (&vmbus_connection.unload_event)) { + unloaded = true; +break ; + } + if (READ_ONCE(msg->header.message_type) ==
HVMSG_NONE) {drivers/hv/channel_mgmt.c +3
-2mdelay (10 );continue ;@@ -530,9 +542,17 @@ static void vmbus_wait_for_unload(void) unloaded = true;vmbus_signal_eom (msg); + } -if (unloaded) -break ; +/* +* We're crashing and already got the UNLOAD_RESPONSE, cleanup all
drivers/hv/channel_mgmt.c +6
-0+* maybe-pending messages on all CPUs to be able to receive new +* messages after we reconnect. +*/ +for_each_online_cpu (cpu) { + page_addr = hv_context.synic_message_page[cpu]; + msg = (struct hv_message *)page_addr +
VMBUS_MESSAGE_SINT;drivers/hv/channel_mgmt.c +1
-1+ msg->header.message_type =HVMSG_NONE ; } } --2.5.0
Quoted 19 lines · KY Srinivasan
Quoted 10 lines
-----Original Message-----
From: Vitaly Kuznetsov [mailto:[email protected]]
Sent: Friday, March 18, 2016 5:33 AM
To: [email protected]
Cc: [email protected]; KY Srinivasan <[email protected]>; Haiyang Zhang <[email protected]>; Alex Ng (LIS) <[email protected]>; Radim Krcmar <[email protected]>; Cathy Avery <[email protected]>
Subject: [PATCH] Drivers: hv: vmbus: handle various crash scenariosQuoted 8 lines
Kdump keeps biting. Turns out
CHANNELMSG_UNLOAD_RESPONSEis always delivered to CPU0 regardless of what CPU we're sendingCHANNELMSG_UNLOAD
from.vmbus_wait_for_unload()doesn't account for the fact that in case we're crashing on some other CPU and CPU0 is still alive and operationalCHANNELMSG_UNLOAD_RESPONSEwill be delivered there completing vmbus_connection.unload_event, our wait on the current CPU will never end.
Quoted 2 lines
What was the host you were testing on?
I was testing on both 2012R2 and 2016TP4. The bug is easily reproducible by forcing crash on a secondary CPU, e.g.:
# cat crash.sh #! /bin/sh echo c > /proc/sysrq-trigger
# taskset -c 1 ./crash.sh
Quoted 85 lines
Quoted 85 lines
Do the following:
1) Check forcompletion_done()in the loop. In case interrupt handler is still alive we'll get the confirmation we need.2) Always read CPU0's message page as
CHANNELMSG_UNLOAD_RESPONSEwill be
delivered there. We can race with still-alive interrupt handler doing the same but we don't care as we're checkingcompletion_done()now.3) Cleanup message pages on all CPUs. This is required (at least for the current CPU as we're clearing CPU0 messages now but we may want to
bring
up additional CPUs on crash) as new messages won't be delivered till we consume what's pending. On boot we'll place message pages somewhere
else
and we won't be able to read stale messages.Signed-off-by Vitaly Kuznetsov <[email protected]>--- drivers/hv/channel_mgmt.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-)drivers/hv/channel_mgmt.c +4
-3@@ -512,14 +512,26 @@ static void init_vp_index(struct vmbus_channel *channel, const uuid_le *type_guistatic void vmbus_wait_for_unload (void ) { -int cpu =smp_processor_id (); -void *page_addr = hv_context.synic_message_page[cpu]; +int cpu; +void *page_addr = hv_context.synic_message_page[0 ];struct hv_message *msg = (struct hv_message *)page_addr +VMBUS_MESSAGE_SINT ;struct vmbus_channel_message_header *hdr;bool unloaded = false; -while (1 ) { +/* +* CHANNELMSG_UNLOAD_RESPONSE is always delivered to CPU0. When we're
drivers/hv/channel_mgmt.c +3
-0+* crashing on a different CPU let's hope that IRQ handler on CPU0 is +* still functional and vmbus_unload_response() will complete +* vmbus_connection.unload_event. If not, the last thing we can do is
drivers/hv/channel_mgmt.c +8
-0+* read message page for CPU0 regardless of what CPU we're on. +*/ +while (!unloaded) { +if (completion_done (&vmbus_connection.unload_event)) { + unloaded = true; +break ; + } + if (READ_ONCE(msg->header.message_type) ==
HVMSG_NONE) {drivers/hv/channel_mgmt.c +3
-2mdelay (10 );continue ;@@ -530,9 +542,17 @@ static void vmbus_wait_for_unload(void) unloaded = true;vmbus_signal_eom (msg); + } -if (unloaded) -break ; +/* +* We're crashing and already got the UNLOAD_RESPONSE, cleanup all
drivers/hv/channel_mgmt.c +6
-0+* maybe-pending messages on all CPUs to be able to receive new +* messages after we reconnect. +*/ +for_each_online_cpu (cpu) { + page_addr = hv_context.synic_message_page[cpu]; + msg = (struct hv_message *)page_addr +
VMBUS_MESSAGE_SINT;drivers/hv/channel_mgmt.c +1
-1+ msg->header.message_type =HVMSG_NONE ; } } --2.5.0
Signature · 1 line
Vitaly
Quoted 33 lines
-----Original Message-----
From: Vitaly Kuznetsov [mailto:[email protected]]
Sent: Monday, March 21, 2016 12:52 AM
To: KY Srinivasan <[email protected]>
Cc: [email protected]; [email protected]; Haiyang Zhang <[email protected]>; Alex Ng (LIS) <[email protected]>; Radim Krcmar <[email protected]>; Cathy Avery <[email protected]>
Subject: Re: [PATCH] Drivers: hv: vmbus: handle various crash scenariosQuoted 11 lines · KY Srinivasan
Quoted 11 lines
-----Original Message-----
From: Vitaly Kuznetsov [mailto:[email protected]]
Sent: Friday, March 18, 2016 5:33 AM
To: [email protected]
Cc: [email protected]; KY Srinivasan <[email protected]>; Haiyang Zhang <[email protected]>; Alex Ng (LIS) <[email protected]>; Radim Krcmar <[email protected]>; Cathy Avery <[email protected]>
Subject: [PATCH] Drivers: hv: vmbus: handle various crash scenariosKdump keeps biting. Turns out
CHANNELMSG_UNLOAD_RESPONSEisalways
Quoted 8 lines
Quoted 7 lines
delivered to CPU0 regardless of what CPU we're sending
CHANNELMSG_UNLOAD
from.vmbus_wait_for_unload()doesn't account for the fact that in case we're crashing on some other CPU and CPU0 is still alive and operationalCHANNELMSG_UNLOAD_RESPONSEwill be delivered there completing vmbus_connection.unload_event, our wait on the current CPU will never end.
Quoted 5 lines
Quoted 2 lines
What was the host you were testing on?
I was testing on both 2012R2 and 2016TP4. The bug is easily reproducible by forcing crash on a secondary CPU, e.g.:
Prior to 2012R2, all messages would be delivered on CPU0 and this includes CHANNELMSG_UNLOAD_RESPONSE. For this reason we don't support kexec on pre-2012 R2 hosts. On 2012. From 2012 R2 on, all vmbus messages (responses) will be delivered on the CPU that we initially set up - look at the code in vmbus_negotiate_version(). So on post 2012 R2 hosts, the response to CHANNELMSG_UNLOAD_RESPONSE will be delivered on the CPU where we initiate the contact with the host - CHANNELMSG_INITIATE_CONTACT message. So, maybe we can stash away the CPU on which we made the initial contact and poll the state on that CPU to make forward progress in the case of crash.
Regards,
K. Y
Quoted 34 lines · KY Srinivasan
Quoted 34 lines
-----Original Message-----
From: Vitaly Kuznetsov [mailto:[email protected]]
Sent: Monday, March 21, 2016 12:52 AM
To: KY Srinivasan <[email protected]>
Cc: [email protected]; [email protected]; Haiyang Zhang <[email protected]>; Alex Ng (LIS) <[email protected]>; Radim Krcmar <[email protected]>; Cathy Avery <[email protected]>
Subject: Re: [PATCH] Drivers: hv: vmbus: handle various crash scenariosQuoted 11 lines · KY Srinivasan
Quoted 11 lines
-----Original Message-----
From: Vitaly Kuznetsov [mailto:[email protected]]
Sent: Friday, March 18, 2016 5:33 AM
To: [email protected]
Cc: [email protected]; KY Srinivasan <[email protected]>; Haiyang Zhang <[email protected]>; Alex Ng (LIS) <[email protected]>; Radim Krcmar <[email protected]>; Cathy Avery <[email protected]>
Subject: [PATCH] Drivers: hv: vmbus: handle various crash scenariosKdump keeps biting. Turns out
CHANNELMSG_UNLOAD_RESPONSEisalways
Quoted 10 lines
Quoted 7 lines
delivered to CPU0 regardless of what CPU we're sending
CHANNELMSG_UNLOAD
from.vmbus_wait_for_unload()doesn't account for the fact that in case we're crashing on some other CPU and CPU0 is still alive and operationalCHANNELMSG_UNLOAD_RESPONSEwill be delivered there completing vmbus_connection.unload_event, our wait on the current CPU will never end.What was the host you were testing on?
Quoted 7 lines
Quoted 2 lines
I was testing on both 2012R2 and 2016TP4. The bug is easily reproducible by forcing crash on a secondary CPU, e.g.:
Prior to 2012R2, all messages would be delivered on CPU0 and this includes
CHANNELMSG_UNLOAD_RESPONSE. For this reason we don't support kexec on pre-2012 R2 hosts. On 2012. From 2012 R2 on, all vmbus messages (responses) will be delivered on the CPU that we initially set up - look at the code invmbus_negotiate_version().
Ok, missed that. In that case we need to remember which CPU it was -- I'll add this in v2.
Quoted 4 lines
So on post 2012 R2 hosts, the response to
CHANNELMSG_UNLOAD_RESPONSE
will be delivered on the CPU where we initiate the contact with the host -CHANNELMSG_INITIATE_CONTACTmessage. So, maybe we can stash away the CPU on which we made the initial contact and poll the state on that CPU to make forward progress in the case of crash.
Yes, we can't have any expectation about other CPUs on crash as they can be in any state (crashing also, hanging on some mutex/spinlock/...,) so we need to use current CPU only. I'll fix and resend.
Thanks!
Signature · 1 line
Vitaly
Quoted 37 lines · KY Srinivasan
Quoted 31 lines
-----Original Message-----
From: Vitaly Kuznetsov [mailto:[email protected]]
Sent: Monday, March 21, 2016 12:52 AM
To: KY Srinivasan <[email protected]>
Cc: [email protected]; [email protected]; Haiyang Zhang <[email protected]>; Alex Ng (LIS) <[email protected]>; Radim Krcmar <[email protected]>; Cathy Avery <[email protected]>
Subject: Re: [PATCH] Drivers: hv: vmbus: handle various crash scenariosQuoted 11 lines · KY Srinivasan
Quoted 11 lines
-----Original Message-----
From: Vitaly Kuznetsov [mailto:[email protected]]
Sent: Friday, March 18, 2016 5:33 AM
To: [email protected]
Cc: [email protected]; KY Srinivasan <[email protected]>; Haiyang Zhang <[email protected]>; Alex Ng (LIS) <[email protected]>; Radim Krcmar <[email protected]>; Cathy Avery <[email protected]>
Subject: [PATCH] Drivers: hv: vmbus: handle various crash scenariosKdump keeps biting. Turns out
CHANNELMSG_UNLOAD_RESPONSEisalways
Quoted 8 lines
Quoted 7 lines
delivered to CPU0 regardless of what CPU we're sending
CHANNELMSG_UNLOAD
from.vmbus_wait_for_unload()doesn't account for the fact that in case we're crashing on some other CPU and CPU0 is still alive and operationalCHANNELMSG_UNLOAD_RESPONSEwill be delivered there completing vmbus_connection.unload_event, our wait on the current CPU will never end.Quoted 5 lines
Quoted 2 lines
What was the host you were testing on?
I was testing on both 2012R2 and 2016TP4. The bug is easily reproducible by forcing crash on a secondary CPU, e.g.:
Quoted 6 lines
Prior to 2012R2, all messages would be delivered on CPU0 and this includes
CHANNELMSG_UNLOAD_RESPONSE. For this reason we don't support kexec on pre-2012 R2 hosts. On 2012. From 2012 R2 on, all vmbus messages (responses) will be delivered on the CPU that we initially set up - look at the code invmbus_negotiate_version(). So on post 2012 R2 hosts, the response toCHANNELMSG_UNLOAD_RESPONSEwill be delivered on the CPU where we initiate the contact with the
host -CHANNELMSG_INITIATE_CONTACTmessage.
Unfortunatelly there is a descrepancy between WS2012R2 and WS2016TP4. On WS2012R2 what you're saying is true and all messages including CHANNELMSG_UNLOAD_RESPONSE are delivered to the CPU we used for initial contact. On WS2016TP4 CHANNELMSG_UNLOAD_RESPONSE seems to be a special case and it is always delivered to CPU0, no matter which CPU we used for initial contact. This can be a host bug. You can use the attached patch to see the issue.
For now I can suggest we check message pages for all CPUs from vmbus_wait_for_unload(). We can race with other CPUs again but we don't care as we're checking for completion_done() in the loop as well. I'll try this approach.
Signature · 1 line
Vitaly
Quoted 1 line
From
27170c1bb8f21f7b20c1716c1df65e4812b421f8Mon Sep 17 00:00:00 2001
From: Vitaly Kuznetsov <[email protected]>
Date: Thu, 17 Mar 2016 14:41:07 +0100
Subject: [PATCH] Drivers: hv: vmbus: handle various crash scenarios
Kdump keeps biting. Turns out CHANNELMSG_UNLOAD_RESPONSE is always delivered to the cpu which was used to initiate contact regardless of what CPU we're sending CHANNELMSG_UNLOAD from. vmbus_wait_for_unload() doesn't account for the fact that in case we're crashing on some other CPU and the CPU which was used to initate contact is still alive and operational CHANNELMSG_UNLOAD_RESPONSE will be delivered there completing vmbus_connection.unload_event, our wait on the current CPU will never end.
Do the following:
1) Remember the CPU we used to initiate contact in vmbus_connection.
1) Check for completion_done() in the loop. In case interrupt handler is still alive we'll get the confirmation we need.
2) Always read the init_cpu's message page as CHANNELMSG_UNLOAD_RESPONSE will be delivered there. We can race with still-alive interrupt handler doing the same but we don't care as we're checking completion_done() now.
3) Cleanup message pages on all CPUs. This is required (at least for the current CPU as we're clearing some other CPU's messages now but we may want to bring up additional CPUs on crash) as new messages won't be delivered till we consume what's pending. On boot we'll place message pages somewhere else and we won't be able to read stale messages.
---
Changes since v1:
- Use init_cpu instead of CPU0 [K. Y. Srinivasan]
- Style changes in vmbus_wait_for_unload [Radim Krcmar]
--- drivers/hv/channel_mgmt.c | 39 ++++++++++++++++++++++++++++++++------- drivers/hv/connection.c | 10 +++++++--- drivers/hv/hyperv_vmbus.h | 3 +++ drivers/hv/vmbus_drv.c | 1 + 4 files changed, 43 insertions(+), 10 deletions(-)
drivers/hv/channel_mgmt.c +32 -7
@@ -597,28 +597,53 @@ static void init_vp_index(struct vmbus_channel *channel, u16 dev_type)static void vmbus_wait_for_unload (void ) { -int cpu =smp_processor_id (); -void *page_addr = hv_context.synic_message_page[cpu]; +int cpu; +void *page_addr = + hv_context.synic_message_page[vmbus_connection.init_cpu];struct hv_message *msg = (struct hv_message *)page_addr +VMBUS_MESSAGE_SINT ;struct vmbus_channel_message_header *hdr; -bool unloaded = false; +enum vmbus_channel_message_type msgtype; + +printk ("vmbus_wait_for_unload: %d (%d)\n" , vmbus_connection.init_cpu,smp_processor_id ()); +/* +* CHANNELMSG_UNLOAD_RESPONSE is always delivered to the CPU which was +* used to initate contact (see vmbus_negotiate_version()). When we're +* crashing on a different CPU let's hope that IRQ handler on that CPU +* is still functional and vmbus_unload_response() will complete +* vmbus_connection.unload_event. If not, the last thing we can do is +* read message page for that CPU regardless of what CPU we're on. +*/ while (1 ) { +if (completion_done (&vmbus_connection.unload_event)) +break ; +if (READ_ONCE (msg->header.message_type) ==HVMSG_NONE ) {mdelay (10 );continue ; } hdr = (struct vmbus_channel_message_header *)msg->u.payload; -if (hdr->msgtype ==CHANNELMSG_UNLOAD_RESPONSE ) - unloaded = true; - + msgtype = hdr->msgtype;vmbus_signal_eom (msg); -if (unloaded) +if (msgtype ==CHANNELMSG_UNLOAD_RESPONSE )break ; } + +/* +* We're crashing and already got the UNLOAD_RESPONSE, cleanup all +* maybe-pending messages on all CPUs to be able to receive new +* messages after we reconnect. +*/ +for_each_online_cpu (cpu) { + page_addr = hv_context.synic_message_page[cpu]; + msg = (struct hv_message *)page_addr +VMBUS_MESSAGE_SINT ; + msg->header.message_type =HVMSG_NONE ; + } + +printk ("vmbus_wait_for_unload done: %d (%d)\n" , vmbus_connection.init_cpu,smp_processor_id ()); }/*
drivers/hv/connection.c +7 -3
@@ -70,7 +70,7 @@ static __u32 vmbus_get_next_version(__u32 current_version)static int vmbus_negotiate_version (struct vmbus_channel_msginfo *msginfo,__u32 version) { -int ret =0 ; +int ret =0 , cpu =smp_processor_id ();struct vmbus_channel_initiate_contact *msg;unsigned long flags;
@@ -91,12 +91,16 @@ static int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo,* For post win8 hosts, we support receiving channel messagges on * all the CPUs. This is needed for kexec to work correctly where * the CPU attempting to connect may not be CPU 0. +* We need to remember the CPU we use here as in case of unload +* CHANNELMSG_UNLOAD_RESPONSE will be delivered to this CPU. */ if (version >=VERSION_WIN8_1 ) { - msg->target_vcpu = hv_context.vp_index[get_cpu ()]; -put_cpu (); +printk ("vmbus_negotiate_version: %d %d\n" , cpu, hv_context.vp_index[cpu]); + msg->target_vcpu = hv_context.vp_index[cpu]; + vmbus_connection.init_cpu = cpu; }else { msg->target_vcpu =0 ; + vmbus_connection.init_cpu =0 ; }/*
drivers/hv/hyperv_vmbus.h +3 -0
@@ -563,6 +563,9 @@ struct vmbus_connection {atomic_t next_gpadl_handle; +/* CPU which was used to initiate contact */ +int init_cpu; +struct completion unload_event;/* * Represents channel interrupts. Each bit position represents a
drivers/hv/vmbus_drv.c +1 -0
@@ -732,6 +732,7 @@ void vmbus_on_msg_dpc(unsigned long data) goto msg_handled; } +printk ("vmbus_on_msg_dpc: %d on %d\n" , hdr->msgtype, cpu); entry = &channel_message_table[hdr->msgtype];if (entry->handler_type ==VMHT_BLOCKING ) { ctx =kmalloc (sizeof (*ctx),GFP_ATOMIC );
Signature · 1 line
2.5.5
Quoted 68 lines
-----Original Message-----
From: Vitaly Kuznetsov [mailto:[email protected]]
Sent: Tuesday, March 22, 2016 7:01 AM
To: KY Srinivasan <[email protected]>
Cc: [email protected]; [email protected]; Haiyang Zhang <[email protected]>; Alex Ng (LIS) <[email protected]>; Radim Krcmar <[email protected]>; Cathy Avery <[email protected]>
Subject: Re: [PATCH] Drivers: hv: vmbus: handle various crash scenariosQuoted 6 lines · KY Srinivasan
Quoted 6 lines
-----Original Message-----
From: Vitaly Kuznetsov [mailto:[email protected]]
Sent: Monday, March 21, 2016 12:52 AM
To: KY Srinivasan <[email protected]>
Cc: [email protected]; [email protected]; Haiyang Zhang <[email protected]>; Alex Ng (LIS)Quoted 1 line
Quoted 1 line
Radim Krcmar <[email protected]>; Cathy Avery
Quoted 11 lines
Quoted 11 lines
Subject: Re: [PATCH] Drivers: hv: vmbus: handle various crash scenarios
Quoted 7 lines · KY Srinivasan
Quoted 7 lines
-----Original Message-----
From: Vitaly Kuznetsov [mailto:[email protected]]
Sent: Friday, March 18, 2016 5:33 AM
To: [email protected]
Cc: [email protected]; KY Srinivasan <[email protected]>; Haiyang Zhang <[email protected]>; Alex Ng (LIS) <[email protected]>; Radim Krcmar <[email protected]>;Cathy
Quoted 8 lines
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
Avery <[email protected]>
Subject: [PATCH] Drivers: hv: vmbus: handle various crash scenariosKdump keeps biting. Turns out
CHANNELMSG_UNLOAD_RESPONSEisalways
Quoted 3 lines
Quoted 3 lines
delivered to CPU0 regardless of what CPU we're sending
CHANNELMSG_UNLOAD
from.vmbus_wait_for_unload()doesn't account for the fact that incase
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
we're crashing on some other CPU and CPU0 is still alive and
operational
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
CHANNELMSG_UNLOAD_RESPONSEwill be delivered therecompleting
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
vmbus_connection.unload_event, our wait on the current CPU will
never
Quoted 9 lines
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
end.
What was the host you were testing on?
I was testing on both 2012R2 and 2016TP4. The bug is easily reproducible by forcing crash on a secondary CPU, e.g.:
Prior to 2012R2, all messages would be delivered on CPU0 and this includes
CHANNELMSG_UNLOAD_RESPONSE.Quoted 1 line
For this reason we don't support kexec on pre-2012 R2 hosts. On 2012.
From 2012 R2 on, all vmbus
Quoted 1 line
messages (responses) will be delivered on the CPU that we initially set up -
look at the code in
Quoted 1 line
vmbus_negotiate_version(). So on post 2012 R2 hosts, the response to
CHANNELMSG_UNLOAD_RESPONSEQuoted 2 lines
will be delivered on the CPU where we initiate the contact with the host -
CHANNELMSG_INITIATE_CONTACTmessage.
Quoted 10 lines
Unfortunatelly there is a descrepancy between WS2012R2 and WS2016TP4. On
WS2012R2 what you're saying is true and all messages includingCHANNELMSG_UNLOAD_RESPONSEare delivered to the CPU we used for initial
contact. On WS2016TP4CHANNELMSG_UNLOAD_RESPONSEseems to be a special
case and it is always delivered to CPU0, no matter which CPU we used for initial contact. This can be a host bug. You can use the attached patch to see the issue.
This looks like a host bug and I will try to get is addressed before ws2016 ships.
Quoted 5 lines
For now I can suggest we check message pages for all CPUs from
vmbus_wait_for_unload(). We can race with other CPUs again but we don't care as we're checking forcompletion_done()in the loop as well. I'll try this approach.
Thank you.
K. Y
Quoted 3 lines
--
Vitaly