| Message ID | 1438086930-19483-1-git-send-email-decui@microsoft.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Dexuan Cui <decui@microsoft.com> Date: Tue, 28 Jul 2015 05:35:30 -0700 > In the SMP guest case, when the per-channel callback hvsock_events() is > running on virtual CPU A, if the guest tries to close the connection on > virtual CPU B: we invoke vmbus_close() -> vmbus_close_internal(), > then we can have trouble: on B, vmbus_close_internal() will send IPI > reset_channel_cb() to A, trying to set channel->onchannel_callbackto NULL; > on A, if the IPI handler happens between > "if (channel->onchannel_callback != NULL)" and invoking > channel->onchannel_callback, we'll invoke a function pointer of NULL. > > This is why the patch is necessary. > > Signed-off-by: Dexuan Cui <decui@microsoft.com> Sorry, I do not accept that you must use conditional locking and/or IRQ disabling. Boil it down to what is necessary for the least common denominator, and use that unconditionally. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> From: David Miller > Sent: Thursday, July 30, 2015 6:28 > > From: Dexuan Cui <decui@microsoft.com> > > Date: Tue, 28 Jul 2015 05:35:30 -0700 > > > > In the SMP guest case, when the per-channel callback hvsock_events() is > > running on virtual CPU A, if the guest tries to close the connection on > > virtual CPU B: we invoke vmbus_close() -> vmbus_close_internal(), > > then we can have trouble: on B, vmbus_close_internal() will send IPI > > reset_channel_cb() to A, trying to set channel->onchannel_callbackto NULL; > > on A, if the IPI handler happens between > > "if (channel->onchannel_callback != NULL)" and invoking > > channel->onchannel_callback, we'll invoke a function pointer of NULL. > > > > This is why the patch is necessary. > > > Sorry, I do not accept that you must use conditional locking and/or > IRQ disabling. > > Boil it down to what is necessary for the least common denominator, > and use that unconditionally. Hi David, Thanks for the comment! I agree with you it's not clean to use conditional IRQ disabling. Here I didn't use unconditionally IRQ disabling because the Hyper-V netvsc and storvsc driver's vmbus event callbacks (i.e. netvsc_channel_cb() and storvsc_on_channel_callback()) may take relatively long time (e.g., netvsc can operate at a speed of 10Gb) and I think it's bad to disable IRQ for long time when the callbacks are running in a tasklet context, e.g., the Hyper-V timer can be affected: see vmbus_isr() -> hv_process_timer_expiration(). To resolve the race condition between vmbus_close_internal() and process_chn_event() in SMP case, now I propose a new method: we can serialize the 2 paths by adding tasklet_disable(hv_context.event_dpc[channel->target_cpu]) and tasklet_enable(...) in vmbus_close_internal(). In this way, we need the least change and we can drop this patch. Please let me know your opinion. Thanks, -- Dexuan -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: devel [mailto:driverdev-devel-bounces@linuxdriverproject.org] On Behalf > Of Dexuan Cui > Sent: Thursday, July 30, 2015 18:18 > To: David Miller <davem@davemloft.net>; KY Srinivasan <kys@microsoft.com> > Cc: olaf@aepfle.de; gregkh@linuxfoundation.org; jasowang@redhat.com; > driverdev-devel@linuxdriverproject.org; linux-kernel@vger.kernel.org; > stephen@networkplumber.org; stefanha@redhat.com; netdev@vger.kernel.org; > apw@canonical.com; pebolle@tiscali.nl; dan.carpenter@oracle.com > Subject: RE: [PATCH V4 7/7] Drivers: hv: vmbus: disable local interrupt when > hvsock's callback is running > > > From: David Miller > > Sent: Thursday, July 30, 2015 6:28 > > > From: Dexuan Cui <decui@microsoft.com> > > > Date: Tue, 28 Jul 2015 05:35:30 -0700 > > > > > > In the SMP guest case, when the per-channel callback hvsock_events() is > > > running on virtual CPU A, if the guest tries to close the connection on > > > virtual CPU B: we invoke vmbus_close() -> vmbus_close_internal(), > > > then we can have trouble: on B, vmbus_close_internal() will send IPI > > > reset_channel_cb() to A, trying to set channel->onchannel_callbackto NULL; > > > on A, if the IPI handler happens between > > > "if (channel->onchannel_callback != NULL)" and invoking > > > channel->onchannel_callback, we'll invoke a function pointer of NULL. > > > > > > This is why the patch is necessary. > > > > > Sorry, I do not accept that you must use conditional locking and/or > > IRQ disabling. > > > > Boil it down to what is necessary for the least common denominator, > > and use that unconditionally. > > Hi David, > Thanks for the comment! > > I agree with you it's not clean to use conditional IRQ disabling. > > Here I didn't use unconditionally IRQ disabling because the Hyper-V netvsc > and storvsc driver's vmbus event callbacks (i.e. netvsc_channel_cb() and > storvsc_on_channel_callback()) may take relatively long time (e.g., netvsc can > operate at a speed of 10Gb) and I think it's bad to disable IRQ for long time > when the callbacks are running in a tasklet context, e.g., the Hyper-V timer > can be affected: see vmbus_isr() -> hv_process_timer_expiration(). > > To resolve the race condition between vmbus_close_internal() and > process_chn_event() in SMP case, now I propose a new method: > > we can serialize the 2 paths by adding > tasklet_disable(hv_context.event_dpc[channel->target_cpu]) and > tasklet_enable(...) in vmbus_close_internal(). > > In this way, we need the least change and we can drop this patch. > > Please let me know your opinion. > > -- Dexuan Hi David, KY and all, May I know your opinion about my idea of adding tasklet_disable/enbable() in vmbus_close_internal() and dropping this patch? Thanks, -- Dexuan -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Dexuan Cui > Sent: Wednesday, August 5, 2015 9:44 PM > To: David Miller <davem@davemloft.net>; KY Srinivasan > <kys@microsoft.com> > Cc: olaf@aepfle.de; gregkh@linuxfoundation.org; jasowang@redhat.com; > driverdev-devel@linuxdriverproject.org; linux-kernel@vger.kernel.org; > stephen@networkplumber.org; stefanha@redhat.com; > netdev@vger.kernel.org; apw@canonical.com; pebolle@tiscali.nl; > dan.carpenter@oracle.com > Subject: RE: [PATCH V4 7/7] Drivers: hv: vmbus: disable local interrupt when > hvsock's callback is running > > > -----Original Message----- > > From: devel [mailto:driverdev-devel-bounces@linuxdriverproject.org] On > Behalf > > Of Dexuan Cui > > Sent: Thursday, July 30, 2015 18:18 > > To: David Miller <davem@davemloft.net>; KY Srinivasan > <kys@microsoft.com> > > Cc: olaf@aepfle.de; gregkh@linuxfoundation.org; jasowang@redhat.com; > > driverdev-devel@linuxdriverproject.org; linux-kernel@vger.kernel.org; > > stephen@networkplumber.org; stefanha@redhat.com; > netdev@vger.kernel.org; > > apw@canonical.com; pebolle@tiscali.nl; dan.carpenter@oracle.com > > Subject: RE: [PATCH V4 7/7] Drivers: hv: vmbus: disable local interrupt > when > > hvsock's callback is running > > > > > From: David Miller > > > Sent: Thursday, July 30, 2015 6:28 > > > > From: Dexuan Cui <decui@microsoft.com> > > > > Date: Tue, 28 Jul 2015 05:35:30 -0700 > > > > > > > > In the SMP guest case, when the per-channel callback hvsock_events() > is > > > > running on virtual CPU A, if the guest tries to close the connection on > > > > virtual CPU B: we invoke vmbus_close() -> vmbus_close_internal(), > > > > then we can have trouble: on B, vmbus_close_internal() will send IPI > > > > reset_channel_cb() to A, trying to set channel->onchannel_callbackto > NULL; > > > > on A, if the IPI handler happens between > > > > "if (channel->onchannel_callback != NULL)" and invoking > > > > channel->onchannel_callback, we'll invoke a function pointer of NULL. > > > > > > > > This is why the patch is necessary. > > > > > > > Sorry, I do not accept that you must use conditional locking and/or > > > IRQ disabling. > > > > > > Boil it down to what is necessary for the least common denominator, > > > and use that unconditionally. > > > > Hi David, > > Thanks for the comment! > > > > I agree with you it's not clean to use conditional IRQ disabling. > > > > Here I didn't use unconditionally IRQ disabling because the Hyper-V netvsc > > and storvsc driver's vmbus event callbacks (i.e. netvsc_channel_cb() and > > storvsc_on_channel_callback()) may take relatively long time (e.g., netvsc > can > > operate at a speed of 10Gb) and I think it's bad to disable IRQ for long time > > when the callbacks are running in a tasklet context, e.g., the Hyper-V timer > > can be affected: see vmbus_isr() -> hv_process_timer_expiration(). > > > > To resolve the race condition between vmbus_close_internal() and > > process_chn_event() in SMP case, now I propose a new method: > > > > we can serialize the 2 paths by adding > > tasklet_disable(hv_context.event_dpc[channel->target_cpu]) and > > tasklet_enable(...) in vmbus_close_internal(). > > > > In this way, we need the least change and we can drop this patch. > > > > Please let me know your opinion. > > > > -- Dexuan > > Hi David, KY and all, > > May I know your opinion about my idea of adding tasklet_disable/enbable() > in vmbus_close_internal() and dropping this patch? Sorry for the delayed response; I think this is a reasonable solution. Send me the patch. Regards, K. Y > > Thanks, > -- Dexuan -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> From: KY Srinivasan > Sent: Friday, August 7, 2015 1:50 > To: Dexuan Cui <decui@microsoft.com>; David Miller <davem@davemloft.net> > Cc: olaf@aepfle.de; gregkh@linuxfoundation.org; jasowang@redhat.com; > driverdev-devel@linuxdriverproject.org; linux-kernel@vger.kernel.org; > stephen@networkplumber.org; stefanha@redhat.com; netdev@vger.kernel.org; > apw@canonical.com; pebolle@tiscali.nl; dan.carpenter@oracle.com > Subject: RE: [PATCH V4 7/7] Drivers: hv: vmbus: disable local interrupt when > hvsock's callback is running > > From: Dexuan Cui > > Sent: Wednesday, August 5, 2015 9:44 PM > > To: David Miller <davem@davemloft.net>; KY Srinivasan > > <kys@microsoft.com> > > Cc: olaf@aepfle.de; gregkh@linuxfoundation.org; jasowang@redhat.com; > > driverdev-devel@linuxdriverproject.org; linux-kernel@vger.kernel.org; > > stephen@networkplumber.org; stefanha@redhat.com; > > netdev@vger.kernel.org; apw@canonical.com; pebolle@tiscali.nl; > > dan.carpenter@oracle.com > > Subject: RE: [PATCH V4 7/7] Drivers: hv: vmbus: disable local interrupt when > > hvsock's callback is running > > > > > From: devel [mailto:driverdev-devel-bounces@linuxdriverproject.org] On > > Behalf > > > Of Dexuan Cui > > > Sent: Thursday, July 30, 2015 18:18 > > > To: David Miller <davem@davemloft.net>; KY Srinivasan > > <kys@microsoft.com> > > > Cc: olaf@aepfle.de; gregkh@linuxfoundation.org; jasowang@redhat.com; > > > driverdev-devel@linuxdriverproject.org; linux-kernel@vger.kernel.org; > > > stephen@networkplumber.org; stefanha@redhat.com; > > netdev@vger.kernel.org; > > > apw@canonical.com; pebolle@tiscali.nl; dan.carpenter@oracle.com > > > Subject: RE: [PATCH V4 7/7] Drivers: hv: vmbus: disable local interrupt > > when > > > hvsock's callback is running > > > > > > > From: David Miller > > > > Sent: Thursday, July 30, 2015 6:28 > > > > > From: Dexuan Cui <decui@microsoft.com> > > > > > Date: Tue, 28 Jul 2015 05:35:30 -0700 > > > > > > > > > > In the SMP guest case, when the per-channel callback hvsock_events() > > is > > > > > running on virtual CPU A, if the guest tries to close the connection on > > > > > virtual CPU B: we invoke vmbus_close() -> vmbus_close_internal(), > > > > > then we can have trouble: on B, vmbus_close_internal() will send IPI > > > > > reset_channel_cb() to A, trying to set channel->onchannel_callbackto > > NULL; > > > > > on A, if the IPI handler happens between > > > > > "if (channel->onchannel_callback != NULL)" and invoking > > > > > channel->onchannel_callback, we'll invoke a function pointer of NULL. > > > > > > > > > > This is why the patch is necessary. > > > > > > > > > Sorry, I do not accept that you must use conditional locking and/or > > > > IRQ disabling. > > > > > > > > Boil it down to what is necessary for the least common denominator, > > > > and use that unconditionally. > > > > > > Hi David, > > > Thanks for the comment! > > > > > > I agree with you it's not clean to use conditional IRQ disabling. > > > > > > Here I didn't use unconditionally IRQ disabling because the Hyper-V netvsc > > > and storvsc driver's vmbus event callbacks (i.e. netvsc_channel_cb() and > > > storvsc_on_channel_callback()) may take relatively long time (e.g., netvsc > > can > > > operate at a speed of 10Gb) and I think it's bad to disable IRQ for long time > > > when the callbacks are running in a tasklet context, e.g., the Hyper-V timer > > > can be affected: see vmbus_isr() -> hv_process_timer_expiration(). > > > > > > To resolve the race condition between vmbus_close_internal() and > > > process_chn_event() in SMP case, now I propose a new method: > > > > > > we can serialize the 2 paths by adding > > > tasklet_disable(hv_context.event_dpc[channel->target_cpu]) and > > > tasklet_enable(...) in vmbus_close_internal(). > > > > > > In this way, we need the least change and we can drop this patch. > > > > > > Please let me know your opinion. > > > > > > -- Dexuan > > > > Hi David, KY and all, > > > > May I know your opinion about my idea of adding tasklet_disable/enbable() > > in vmbus_close_internal() and dropping this patch? > > Sorry for the delayed response; I think this is a reasonable solution. Send me the > patch. > > Regards, > > K. Y OK. Will do. Thanks, -- Dexuan -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c index 4fc2e88..4766fd8 100644 --- a/drivers/hv/connection.c +++ b/drivers/hv/connection.c @@ -319,6 +319,9 @@ static void process_chn_event(u32 relid) void *arg; bool read_state; u32 bytes_to_read; + bool is_hvsock = false; + + local_irq_disable(); /* * Find the channel based on this relid and invokes the @@ -327,7 +330,11 @@ static void process_chn_event(u32 relid) channel = pcpu_relid2channel(relid); if (!channel) - return; + goto out; + + is_hvsock = is_hvsock_channel(channel); + if (!is_hvsock) + local_irq_enable(); /* * A channel once created is persistent even when there @@ -363,6 +370,12 @@ static void process_chn_event(u32 relid) bytes_to_read = 0; } while (read_state && (bytes_to_read != 0)); } + + /* local_irq_enable() is alredy invoked above */ + if (!is_hvsock) + return; +out: + local_irq_enable(); } /*
In the SMP guest case, when the per-channel callback hvsock_events() is running on virtual CPU A, if the guest tries to close the connection on virtual CPU B: we invoke vmbus_close() -> vmbus_close_internal(), then we can have trouble: on B, vmbus_close_internal() will send IPI reset_channel_cb() to A, trying to set channel->onchannel_callbackto NULL; on A, if the IPI handler happens between "if (channel->onchannel_callback != NULL)" and invoking channel->onchannel_callback, we'll invoke a function pointer of NULL. This is why the patch is necessary. Signed-off-by: Dexuan Cui <decui@microsoft.com> --- drivers/hv/connection.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)