* [PATCH 0/2] Drivers: hv: vmbus: Miscellaneous fixes @ 2018-11-26 2:28 kys 2018-11-26 2:29 ` [PATCH 1/2] Drivers: hv: vmbus: check the creation_status in vmbus_establish_gpadl() kys 0 siblings, 1 reply; 8+ messages in thread From: kys @ 2018-11-26 2:28 UTC (permalink / raw) To: gregkh, linux-kernel, devel, olaf, apw, jasowang, sthemmin, Michael.H.Kelley, vkuznets Cc: K. Y. Srinivasan From: "K. Y. Srinivasan" <kys@microsoft.com> Miscellaneous fixes. Dexuan Cui (2): Drivers: hv: vmbus: check the creation_status in vmbus_establish_gpadl() Drivers: hv: vmbus: offload the handling of channels to two workqueues drivers/hv/channel.c | 8 ++ drivers/hv/channel_mgmt.c | 188 +++++++++++++++++++++++++------------- drivers/hv/connection.c | 24 ++++- drivers/hv/hyperv_vmbus.h | 7 ++ include/linux/hyperv.h | 7 ++ 5 files changed, 169 insertions(+), 65 deletions(-) -- 2.19.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] Drivers: hv: vmbus: check the creation_status in vmbus_establish_gpadl() 2018-11-26 2:28 [PATCH 0/2] Drivers: hv: vmbus: Miscellaneous fixes kys @ 2018-11-26 2:29 ` kys 2018-11-26 2:29 ` [PATCH 2/2] Drivers: hv: vmbus: offload the handling of channels to two workqueues kys [not found] ` <20181126095254.824D420672@mail.kernel.org> 0 siblings, 2 replies; 8+ messages in thread From: kys @ 2018-11-26 2:29 UTC (permalink / raw) To: gregkh, linux-kernel, devel, olaf, apw, jasowang, sthemmin, Michael.H.Kelley, vkuznets Cc: Dexuan Cui, K . Y . Srinivasan, Haiyang Zhang, stable From: Dexuan Cui <decui@microsoft.com> This is a longstanding issue: if the vmbus upper-layer drivers try to consume too many GPADLs, the host may return with an error 0xC0000044 (STATUS_QUOTA_EXCEEDED), but currently we forget to check the creation_status, and hence we can pass an invalid GPADL handle into the OPEN_CHANNEL message, and get an error code 0xc0000225 in open_info->response.open_result.status, and finally we hang in vmbus_open() -> "goto error_free_info" -> vmbus_teardown_gpadl(). With this patch, we can exit gracefully on STATUS_QUOTA_EXCEEDED. Cc: Stephen Hemminger <sthemmin@microsoft.com> Cc: K. Y. Srinivasan <kys@microsoft.com> Cc: Haiyang Zhang <haiyangz@microsoft.com> Cc: stable@vger.kernel.org Signed-off-by: Dexuan Cui <decui@microsoft.com> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com> --- drivers/hv/channel.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c index f96a77b18bb9..ce0ba2062723 100644 --- a/drivers/hv/channel.c +++ b/drivers/hv/channel.c @@ -516,6 +516,14 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer, } wait_for_completion(&msginfo->waitevent); + if (msginfo->response.gpadl_created.creation_status != 0) { + pr_err("Failed to establish GPADL: err = 0x%x\n", + msginfo->response.gpadl_created.creation_status); + + ret = -EDQUOT; + goto cleanup; + } + if (channel->rescind) { ret = -ENODEV; goto cleanup; -- 2.19.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] Drivers: hv: vmbus: offload the handling of channels to two workqueues 2018-11-26 2:29 ` [PATCH 1/2] Drivers: hv: vmbus: check the creation_status in vmbus_establish_gpadl() kys @ 2018-11-26 2:29 ` kys 2018-11-26 5:23 ` Sasha Levin 2018-11-26 19:35 ` Greg KH [not found] ` <20181126095254.824D420672@mail.kernel.org> 1 sibling, 2 replies; 8+ messages in thread From: kys @ 2018-11-26 2:29 UTC (permalink / raw) To: gregkh, linux-kernel, devel, olaf, apw, jasowang, sthemmin, Michael.H.Kelley, vkuznets Cc: Dexuan Cui, stable, K . Y . Srinivasan, Haiyang Zhang From: Dexuan Cui <decui@microsoft.com> vmbus_process_offer() mustn't call channel->sc_creation_callback() directly for sub-channels, because sc_creation_callback() -> vmbus_open() may never get the host's response to the OPEN_CHANNEL message (the host may rescind a channel at any time, e.g. in the case of hot removing a NIC), and vmbus_onoffer_rescind() may not wake up the vmbus_open() as it's blocked due to a non-zero vmbus_connection.offer_in_progress, and finally we have a deadlock. The above is also true for primary channels, if the related device drivers use sync probing mode by default. And, usually the handling of primary channels and sub-channels can depend on each other, so we should offload them to different workqueues to avoid possible deadlock, e.g. in sync-probing mode, NIC1's netvsc_subchan_work() can race with NIC2's netvsc_probe() -> rtnl_lock(), and causes deadlock: the former gets the rtnl_lock and waits for all the sub-channels to appear, but the latter can't get the rtnl_lock and this blocks the handling of sub-channels. The patch can fix the multiple-NIC deadlock described above for v3.x kernels (e.g. RHEL 7.x) which don't support async-probing of devices, and v4.4, v4.9, v4.14 and v4.18 which support async-probing but don't enable async-probing for Hyper-V drivers (yet). The patch can also fix the hang issue in sub-channel's handling described above for all versions of kernels, including v4.19 and v4.20-rc3. So the patch should be applied to all the existing kernels. Fixes: 8195b1396ec8 ("hv_netvsc: fix deadlock on hotplug") Cc: stable@vger.kernel.org Cc: Stephen Hemminger <sthemmin@microsoft.com> Cc: K. Y. Srinivasan <kys@microsoft.com> Cc: Haiyang Zhang <haiyangz@microsoft.com> Signed-off-by: Dexuan Cui <decui@microsoft.com> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com> --- drivers/hv/channel_mgmt.c | 188 +++++++++++++++++++++++++------------- drivers/hv/connection.c | 24 ++++- drivers/hv/hyperv_vmbus.h | 7 ++ include/linux/hyperv.h | 7 ++ 4 files changed, 161 insertions(+), 65 deletions(-) diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c index 82e673671087..d01689079e9b 100644 --- a/drivers/hv/channel_mgmt.c +++ b/drivers/hv/channel_mgmt.c @@ -434,60 +434,16 @@ void vmbus_free_channels(void) } } -/* - * vmbus_process_offer - Process the offer by creating a channel/device - * associated with this offer - */ -static void vmbus_process_offer(struct vmbus_channel *newchannel) +/* Note: the function can run concurrently for primary/sub channels. */ +static void vmbus_add_channel_work(struct work_struct *work) { - struct vmbus_channel *channel; - bool fnew = true; + struct vmbus_channel *newchannel = + container_of(work, struct vmbus_channel, add_channel_work); + struct vmbus_channel *primary_channel = newchannel->primary_channel; unsigned long flags; u16 dev_type; int ret; - /* Make sure this is a new offer */ - mutex_lock(&vmbus_connection.channel_mutex); - - /* - * Now that we have acquired the channel_mutex, - * we can release the potentially racing rescind thread. - */ - atomic_dec(&vmbus_connection.offer_in_progress); - - list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) { - if (!uuid_le_cmp(channel->offermsg.offer.if_type, - newchannel->offermsg.offer.if_type) && - !uuid_le_cmp(channel->offermsg.offer.if_instance, - newchannel->offermsg.offer.if_instance)) { - fnew = false; - break; - } - } - - if (fnew) - list_add_tail(&newchannel->listentry, - &vmbus_connection.chn_list); - - mutex_unlock(&vmbus_connection.channel_mutex); - - if (!fnew) { - /* - * Check to see if this is a sub-channel. - */ - if (newchannel->offermsg.offer.sub_channel_index != 0) { - /* - * Process the sub-channel. - */ - newchannel->primary_channel = channel; - spin_lock_irqsave(&channel->lock, flags); - list_add_tail(&newchannel->sc_list, &channel->sc_list); - spin_unlock_irqrestore(&channel->lock, flags); - } else { - goto err_free_chan; - } - } - dev_type = hv_get_dev_type(newchannel); init_vp_index(newchannel, dev_type); @@ -505,27 +461,26 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel) /* * This state is used to indicate a successful open * so that when we do close the channel normally, we - * can cleanup properly + * can cleanup properly. */ newchannel->state = CHANNEL_OPEN_STATE; - if (!fnew) { - struct hv_device *dev - = newchannel->primary_channel->device_obj; + if (primary_channel != NULL) { + /* newchannel is a sub-channel. */ + struct hv_device *dev = primary_channel->device_obj; if (vmbus_add_channel_kobj(dev, newchannel)) - goto err_free_chan; + goto err_deq_chan; + + if (primary_channel->sc_creation_callback != NULL) + primary_channel->sc_creation_callback(newchannel); - if (channel->sc_creation_callback != NULL) - channel->sc_creation_callback(newchannel); newchannel->probe_done = true; return; } /* - * Start the process of binding this offer to the driver - * We need to set the DeviceObject field before calling - * vmbus_child_dev_add() + * Start the process of binding the primary channel to the driver */ newchannel->device_obj = vmbus_device_create( &newchannel->offermsg.offer.if_type, @@ -554,13 +509,28 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel) err_deq_chan: mutex_lock(&vmbus_connection.channel_mutex); - list_del(&newchannel->listentry); + + /* + * We need to set the flag, otherwise + * vmbus_onoffer_rescind() can be blocked. + */ + newchannel->probe_done = true; + + if (primary_channel == NULL) { + list_del(&newchannel->listentry); + } else { + spin_lock_irqsave(&primary_channel->lock, flags); + list_del(&newchannel->sc_list); + spin_unlock_irqrestore(&primary_channel->lock, flags); + } + mutex_unlock(&vmbus_connection.channel_mutex); if (newchannel->target_cpu != get_cpu()) { put_cpu(); smp_call_function_single(newchannel->target_cpu, - percpu_channel_deq, newchannel, true); + percpu_channel_deq, + newchannel, true); } else { percpu_channel_deq(newchannel); put_cpu(); @@ -568,14 +538,104 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel) vmbus_release_relid(newchannel->offermsg.child_relid); -err_free_chan: free_channel(newchannel); } +/* + * vmbus_process_offer - Process the offer by creating a channel/device + * associated with this offer + */ +static void vmbus_process_offer(struct vmbus_channel *newchannel) +{ + struct vmbus_channel *channel; + struct workqueue_struct *wq; + unsigned long flags; + bool fnew = true; + + mutex_lock(&vmbus_connection.channel_mutex); + + /* + * Now that we have acquired the channel_mutex, + * we can release the potentially racing rescind thread. + */ + atomic_dec(&vmbus_connection.offer_in_progress); + + list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) { + if (!uuid_le_cmp(channel->offermsg.offer.if_type, + newchannel->offermsg.offer.if_type) && + !uuid_le_cmp(channel->offermsg.offer.if_instance, + newchannel->offermsg.offer.if_instance)) { + fnew = false; + break; + } + } + + if (fnew) + list_add_tail(&newchannel->listentry, + &vmbus_connection.chn_list); + else { + /* + * Check to see if this is a valid sub-channel. + */ + if (newchannel->offermsg.offer.sub_channel_index == 0) { + mutex_unlock(&vmbus_connection.channel_mutex); + /* + * Don't call free_channel(), because newchannel->kobj + * is not initialized yet. + */ + kfree(newchannel); + WARN_ON_ONCE(1); + return; + } + /* + * Process the sub-channel. + */ + newchannel->primary_channel = channel; + spin_lock_irqsave(&channel->lock, flags); + list_add_tail(&newchannel->sc_list, &channel->sc_list); + spin_unlock_irqrestore(&channel->lock, flags); + } + + mutex_unlock(&vmbus_connection.channel_mutex); + + /* + * vmbus_process_offer() mustn't call channel->sc_creation_callback() + * directly for sub-channels, because sc_creation_callback() -> + * vmbus_open() may never get the host's response to the + * OPEN_CHANNEL message (the host may rescind a channel at any time, + * e.g. in the case of hot removing a NIC), and vmbus_onoffer_rescind() + * may not wake up the vmbus_open() as it's blocked due to a non-zero + * vmbus_connection.offer_in_progress, and finally we have a deadlock. + * + * The above is also true for primary channels, if the related device + * drivers use sync probing mode by default. + * + * And, usually the handling of primary channels and sub-channels can + * depend on each other, so we should offload them to different + * workqueues to avoid possible deadlock, e.g. in sync-probing mode, + * NIC1's netvsc_subchan_work() can race with NIC2's netvsc_probe() -> + * rtnl_lock(), and causes deadlock: the former gets the rtnl_lock + * and waits for all the sub-channels to appear, but the latter + * can't get the rtnl_lock and this blocks the handling of + * sub-channels. + */ + INIT_WORK(&newchannel->add_channel_work, vmbus_add_channel_work); + wq = fnew ? vmbus_connection.handle_primary_chan_wq : + vmbus_connection.handle_sub_chan_wq; + queue_work(wq, &newchannel->add_channel_work); +} + /* * We use this state to statically distribute the channel interrupt load. */ static int next_numa_node_id; +/* + * init_vp_index() accesses global variables like next_numa_node_id, and + * it can run concurrently for primary channels and sub-channels: see + * vmbus_process_offer(), so we need the lock to protect the global + * variables. + */ +static DEFINE_SPINLOCK(bind_channel_to_cpu_lock); /* * Starting with Win8, we can statically distribute the incoming @@ -611,6 +671,8 @@ static void init_vp_index(struct vmbus_channel *channel, u16 dev_type) return; } + spin_lock(&bind_channel_to_cpu_lock); + /* * Based on the channel affinity policy, we will assign the NUMA * nodes. @@ -693,6 +755,8 @@ static void init_vp_index(struct vmbus_channel *channel, u16 dev_type) channel->target_cpu = cur_cpu; channel->target_vp = hv_cpu_number_to_vp_number(cur_cpu); + spin_unlock(&bind_channel_to_cpu_lock); + free_cpumask_var(available_mask); } diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c index f4d08c8ac7f8..4fe117b761ce 100644 --- a/drivers/hv/connection.c +++ b/drivers/hv/connection.c @@ -190,6 +190,20 @@ int vmbus_connect(void) goto cleanup; } + vmbus_connection.handle_primary_chan_wq = + create_workqueue("hv_pri_chan"); + if (!vmbus_connection.handle_primary_chan_wq) { + ret = -ENOMEM; + goto cleanup; + } + + vmbus_connection.handle_sub_chan_wq = + create_workqueue("hv_sub_chan"); + if (!vmbus_connection.handle_sub_chan_wq) { + ret = -ENOMEM; + goto cleanup; + } + INIT_LIST_HEAD(&vmbus_connection.chn_msg_list); spin_lock_init(&vmbus_connection.channelmsg_lock); @@ -280,10 +294,14 @@ void vmbus_disconnect(void) */ vmbus_initiate_unload(false); - if (vmbus_connection.work_queue) { - drain_workqueue(vmbus_connection.work_queue); + if (vmbus_connection.handle_sub_chan_wq) + destroy_workqueue(vmbus_connection.handle_sub_chan_wq); + + if (vmbus_connection.handle_primary_chan_wq) + destroy_workqueue(vmbus_connection.handle_primary_chan_wq); + + if (vmbus_connection.work_queue) destroy_workqueue(vmbus_connection.work_queue); - } if (vmbus_connection.int_page) { free_pages((unsigned long)vmbus_connection.int_page, 0); diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h index f17c06a5e74b..313a07f5efa1 100644 --- a/drivers/hv/hyperv_vmbus.h +++ b/drivers/hv/hyperv_vmbus.h @@ -333,7 +333,14 @@ struct vmbus_connection { struct list_head chn_list; struct mutex channel_mutex; + /* + * An offer message is handled first on the work_queue, and then + * is further handled on handle_primary_chan_wq or + * handle_sub_chan_wq. + */ struct workqueue_struct *work_queue; + struct workqueue_struct *handle_primary_chan_wq; + struct workqueue_struct *handle_sub_chan_wq; }; diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h index 07a367f5e22f..f0885cc01db6 100644 --- a/include/linux/hyperv.h +++ b/include/linux/hyperv.h @@ -896,6 +896,13 @@ struct vmbus_channel { bool probe_done; + /* + * We must offload the handling of the primary/sub channels + * from the single-threaded vmbus_connection.work_queue to + * two different workqueue, otherwise we can block + * vmbus_connection.work_queue and hang: see vmbus_process_offer(). + */ + struct work_struct add_channel_work; }; static inline bool is_hvsock_channel(const struct vmbus_channel *c) -- 2.19.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] Drivers: hv: vmbus: offload the handling of channels to two workqueues 2018-11-26 2:29 ` [PATCH 2/2] Drivers: hv: vmbus: offload the handling of channels to two workqueues kys @ 2018-11-26 5:23 ` Sasha Levin 2018-11-26 19:35 ` Greg KH 1 sibling, 0 replies; 8+ messages in thread From: Sasha Levin @ 2018-11-26 5:23 UTC (permalink / raw) To: kys Cc: gregkh, linux-kernel, devel, olaf, apw, jasowang, sthemmin, Michael.H.Kelley, vkuznets, Dexuan Cui, stable, Haiyang Zhang On Mon, Nov 26, 2018 at 02:29:57AM +0000, kys@linuxonhyperv.com wrote: >From: Dexuan Cui <decui@microsoft.com> > >vmbus_process_offer() mustn't call channel->sc_creation_callback() >directly for sub-channels, because sc_creation_callback() -> >vmbus_open() may never get the host's response to the >OPEN_CHANNEL message (the host may rescind a channel at any time, >e.g. in the case of hot removing a NIC), and vmbus_onoffer_rescind() >may not wake up the vmbus_open() as it's blocked due to a non-zero >vmbus_connection.offer_in_progress, and finally we have a deadlock. > >The above is also true for primary channels, if the related device >drivers use sync probing mode by default. > >And, usually the handling of primary channels and sub-channels can >depend on each other, so we should offload them to different >workqueues to avoid possible deadlock, e.g. in sync-probing mode, >NIC1's netvsc_subchan_work() can race with NIC2's netvsc_probe() -> >rtnl_lock(), and causes deadlock: the former gets the rtnl_lock >and waits for all the sub-channels to appear, but the latter >can't get the rtnl_lock and this blocks the handling of sub-channels. > >The patch can fix the multiple-NIC deadlock described above for >v3.x kernels (e.g. RHEL 7.x) which don't support async-probing >of devices, and v4.4, v4.9, v4.14 and v4.18 which support async-probing >but don't enable async-probing for Hyper-V drivers (yet). > >The patch can also fix the hang issue in sub-channel's handling described >above for all versions of kernels, including v4.19 and v4.20-rc3. > >So the patch should be applied to all the existing kernels. > >Fixes: 8195b1396ec8 ("hv_netvsc: fix deadlock on hotplug") >Cc: stable@vger.kernel.org >Cc: Stephen Hemminger <sthemmin@microsoft.com> >Cc: K. Y. Srinivasan <kys@microsoft.com> >Cc: Haiyang Zhang <haiyangz@microsoft.com> >Signed-off-by: Dexuan Cui <decui@microsoft.com> >Signed-off-by: K. Y. Srinivasan <kys@microsoft.com> This patch doesn't apply on next/linus/char-misc; there seems to be a missing patch that touches vmbus_process_offer() which isn't a part of this series. -- Thanks, Sasha ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] Drivers: hv: vmbus: offload the handling of channels to two workqueues 2018-11-26 2:29 ` [PATCH 2/2] Drivers: hv: vmbus: offload the handling of channels to two workqueues kys 2018-11-26 5:23 ` Sasha Levin @ 2018-11-26 19:35 ` Greg KH 2018-11-26 21:12 ` Dexuan Cui 2018-11-27 5:22 ` KY Srinivasan 1 sibling, 2 replies; 8+ messages in thread From: Greg KH @ 2018-11-26 19:35 UTC (permalink / raw) To: kys Cc: linux-kernel, devel, olaf, apw, jasowang, sthemmin, Michael.H.Kelley, vkuznets, Haiyang Zhang, stable On Mon, Nov 26, 2018 at 02:29:57AM +0000, kys@linuxonhyperv.com wrote: > From: Dexuan Cui <decui@microsoft.com> > > vmbus_process_offer() mustn't call channel->sc_creation_callback() > directly for sub-channels, because sc_creation_callback() -> > vmbus_open() may never get the host's response to the > OPEN_CHANNEL message (the host may rescind a channel at any time, > e.g. in the case of hot removing a NIC), and vmbus_onoffer_rescind() > may not wake up the vmbus_open() as it's blocked due to a non-zero > vmbus_connection.offer_in_progress, and finally we have a deadlock. > > The above is also true for primary channels, if the related device > drivers use sync probing mode by default. > > And, usually the handling of primary channels and sub-channels can > depend on each other, so we should offload them to different > workqueues to avoid possible deadlock, e.g. in sync-probing mode, > NIC1's netvsc_subchan_work() can race with NIC2's netvsc_probe() -> > rtnl_lock(), and causes deadlock: the former gets the rtnl_lock > and waits for all the sub-channels to appear, but the latter > can't get the rtnl_lock and this blocks the handling of sub-channels. > > The patch can fix the multiple-NIC deadlock described above for > v3.x kernels (e.g. RHEL 7.x) which don't support async-probing > of devices, and v4.4, v4.9, v4.14 and v4.18 which support async-probing > but don't enable async-probing for Hyper-V drivers (yet). > > The patch can also fix the hang issue in sub-channel's handling described > above for all versions of kernels, including v4.19 and v4.20-rc3. > > So the patch should be applied to all the existing kernels. > > Fixes: 8195b1396ec8 ("hv_netvsc: fix deadlock on hotplug") > Cc: stable@vger.kernel.org > Cc: Stephen Hemminger <sthemmin@microsoft.com> > Cc: K. Y. Srinivasan <kys@microsoft.com> > Cc: Haiyang Zhang <haiyangz@microsoft.com> > Signed-off-by: Dexuan Cui <decui@microsoft.com> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com> > --- > drivers/hv/channel_mgmt.c | 188 +++++++++++++++++++++++++------------- > drivers/hv/connection.c | 24 ++++- > drivers/hv/hyperv_vmbus.h | 7 ++ > include/linux/hyperv.h | 7 ++ > 4 files changed, 161 insertions(+), 65 deletions(-) As Sasha pointed out, this patch does not even apply :( ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH 2/2] Drivers: hv: vmbus: offload the handling of channels to two workqueues 2018-11-26 19:35 ` Greg KH @ 2018-11-26 21:12 ` Dexuan Cui 2018-11-27 5:22 ` KY Srinivasan 1 sibling, 0 replies; 8+ messages in thread From: Dexuan Cui @ 2018-11-26 21:12 UTC (permalink / raw) To: Greg KH, KY Srinivasan Cc: olaf, Stephen Hemminger, jasowang, linux-kernel, stable, Michael Kelley, apw, devel, vkuznets, Haiyang Zhang > From: devel <driverdev-devel-bounces@linuxdriverproject.org> On Behalf Of > Greg KH > Sent: Monday, November 26, 2018 11:35 AM > As Sasha pointed out, this patch does not even apply :( Sorry, I'll rebase to char-misc's char-misc-testing branch, which has had one of the patches, i.e. 4d3c5c69191f ("Drivers: hv: vmbus: Remove the useless API vmbus_get_outgoing_channel()") Thanks, -- Dexuan ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH 2/2] Drivers: hv: vmbus: offload the handling of channels to two workqueues 2018-11-26 19:35 ` Greg KH 2018-11-26 21:12 ` Dexuan Cui @ 2018-11-27 5:22 ` KY Srinivasan 1 sibling, 0 replies; 8+ messages in thread From: KY Srinivasan @ 2018-11-27 5:22 UTC (permalink / raw) To: Greg KH Cc: linux-kernel, devel, olaf, apw, jasowang, Stephen Hemminger, Michael Kelley, vkuznets, Haiyang Zhang, stable > -----Original Message----- > From: Greg KH <gregkh@linuxfoundation.org> > Sent: Monday, November 26, 2018 11:35 AM > To: KY Srinivasan <kys@microsoft.com> > Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org; > olaf@aepfle.de; apw@canonical.com; jasowang@redhat.com; Stephen > Hemminger <sthemmin@microsoft.com>; Michael Kelley > <mikelley@microsoft.com>; vkuznets <vkuznets@redhat.com>; Haiyang > Zhang <haiyangz@microsoft.com>; stable@vger.kernel.org > Subject: Re: [PATCH 2/2] Drivers: hv: vmbus: offload the handling of channels > to two workqueues > > On Mon, Nov 26, 2018 at 02:29:57AM +0000, kys@linuxonhyperv.com wrote: > > From: Dexuan Cui <decui@microsoft.com> > > > > vmbus_process_offer() mustn't call channel->sc_creation_callback() > > directly for sub-channels, because sc_creation_callback() -> > > vmbus_open() may never get the host's response to the > > OPEN_CHANNEL message (the host may rescind a channel at any time, > > e.g. in the case of hot removing a NIC), and vmbus_onoffer_rescind() > > may not wake up the vmbus_open() as it's blocked due to a non-zero > > vmbus_connection.offer_in_progress, and finally we have a deadlock. > > > > The above is also true for primary channels, if the related device > > drivers use sync probing mode by default. > > > > And, usually the handling of primary channels and sub-channels can > > depend on each other, so we should offload them to different > > workqueues to avoid possible deadlock, e.g. in sync-probing mode, > > NIC1's netvsc_subchan_work() can race with NIC2's netvsc_probe() -> > > rtnl_lock(), and causes deadlock: the former gets the rtnl_lock > > and waits for all the sub-channels to appear, but the latter > > can't get the rtnl_lock and this blocks the handling of sub-channels. > > > > The patch can fix the multiple-NIC deadlock described above for > > v3.x kernels (e.g. RHEL 7.x) which don't support async-probing > > of devices, and v4.4, v4.9, v4.14 and v4.18 which support async-probing > > but don't enable async-probing for Hyper-V drivers (yet). > > > > The patch can also fix the hang issue in sub-channel's handling described > > above for all versions of kernels, including v4.19 and v4.20-rc3. > > > > So the patch should be applied to all the existing kernels. > > > > Fixes: 8195b1396ec8 ("hv_netvsc: fix deadlock on hotplug") > > Cc: stable@vger.kernel.org > > Cc: Stephen Hemminger <sthemmin@microsoft.com> > > Cc: K. Y. Srinivasan <kys@microsoft.com> > > Cc: Haiyang Zhang <haiyangz@microsoft.com> > > Signed-off-by: Dexuan Cui <decui@microsoft.com> > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com> > > --- > > drivers/hv/channel_mgmt.c | 188 +++++++++++++++++++++++++--------- > ---- > > drivers/hv/connection.c | 24 ++++- > > drivers/hv/hyperv_vmbus.h | 7 ++ > > include/linux/hyperv.h | 7 ++ > > 4 files changed, 161 insertions(+), 65 deletions(-) > > As Sasha pointed out, this patch does not even apply :( Sorry about that. These patches applied cleanly on my tree (misc-next). This series is to be applied on top of patch 0001-Drivers-hv-vmbus-Remove-the-useless-API-vmbus_get_ou.patch While the patch 0001-Drivers-hv-vmbus-Remove-the-useless-API-vmbus_get_ou.patch has been committed to the char-misc-testing branch, it is not in the misc-linus branch and that is the reason for this problem. Regards, K. Y > ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <20181126095254.824D420672@mail.kernel.org>]
* RE: [PATCH 1/2] Drivers: hv: vmbus: check the creation_status in vmbus_establish_gpadl() [not found] ` <20181126095254.824D420672@mail.kernel.org> @ 2018-11-26 21:02 ` Dexuan Cui 0 siblings, 0 replies; 8+ messages in thread From: Dexuan Cui @ 2018-11-26 21:02 UTC (permalink / raw) To: Sasha Levin, gregkh, linux-kernel Cc: Stephen Hemminger, KY Srinivasan, Haiyang Zhang, stable, stable [-- Attachment #1: Type: text/plain, Size: 730 bytes --] > From: Sasha Levin <sashal@kernel.org> > Sent: Monday, November 26, 2018 1:53 AM > Hi, > > [This is an automated email] > > This commit has been processed because it contains a -stable tag. > The stable tag indicates that it's relevant for the following trees: all > > The bot has tested the following trees: v4.19.4, v4.14.83, v4.9.140, v4.4.164, > v3.18.126. > > v4.19.4: Build OK! > v4.14.83: Build OK! > v4.9.140: Build OK! > v4.4.164: Failed to apply! Possible dependencies: > v3.18.126: Failed to apply! Possible dependencies: > How should we proceed with this patch? > > Sasha Hi Sasha, Please see the attached patch, which is a rebase for both v4.4.164 and v3.18.126. Thanks, --Dexuan [-- Attachment #2: 0001-Drivers-hv-vmbus-check-the-creation_status-in-vmbus_.patch --] [-- Type: application/octet-stream, Size: 1770 bytes --] From 038481c8b771bd22e40cd5d622dc147d486b97c8 Mon Sep 17 00:00:00 2001 From: Dexuan Cui <decui@microsoft.com> Date: Mon, 26 Nov 2018 02:29:56 +0000 Subject: [PATCH] Drivers: hv: vmbus: check the creation_status in vmbus_establish_gpadl() This is a longstanding issue: if the vmbus upper-layer drivers try to consume too many GPADLs, the host may return with an error 0xC0000044 (STATUS_QUOTA_EXCEEDED), but currently we forget to check the creation_status, and hence we can pass an invalid GPADL handle into the OPEN_CHANNEL message, and get an error code 0xc0000225 in open_info->response.open_result.status, and finally we hang in vmbus_open() -> "goto error_free_info" -> vmbus_teardown_gpadl(). With this patch, we can exit gracefully on STATUS_QUOTA_EXCEEDED. Cc: Stephen Hemminger <sthemmin@microsoft.com> Cc: K. Y. Srinivasan <kys@microsoft.com> Cc: Haiyang Zhang <haiyangz@microsoft.com> Cc: stable@vger.kernel.org Signed-off-by: Dexuan Cui <decui@microsoft.com> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- drivers/hv/channel.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c index d037454..b81516c 100644 --- a/drivers/hv/channel.c +++ b/drivers/hv/channel.c @@ -424,6 +424,14 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer, } wait_for_completion(&msginfo->waitevent); + if (msginfo->response.gpadl_created.creation_status != 0) { + pr_err("Failed to establish GPADL: err = 0x%x\n", + msginfo->response.gpadl_created.creation_status); + + ret = -EDQUOT; + goto cleanup; + } + /* At this point, we received the gpadl created msg */ *gpadl_handle = gpadlmsg->gpadl; -- 2.7.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-11-27 5:22 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-26 2:28 [PATCH 0/2] Drivers: hv: vmbus: Miscellaneous fixes kys 2018-11-26 2:29 ` [PATCH 1/2] Drivers: hv: vmbus: check the creation_status in vmbus_establish_gpadl() kys 2018-11-26 2:29 ` [PATCH 2/2] Drivers: hv: vmbus: offload the handling of channels to two workqueues kys 2018-11-26 5:23 ` Sasha Levin 2018-11-26 19:35 ` Greg KH 2018-11-26 21:12 ` Dexuan Cui 2018-11-27 5:22 ` KY Srinivasan [not found] ` <20181126095254.824D420672@mail.kernel.org> 2018-11-26 21:02 ` [PATCH 1/2] Drivers: hv: vmbus: check the creation_status in vmbus_establish_gpadl() Dexuan Cui
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).