* RE: [PATCH] [REPOST for the char-misc-linus branch] Drivers: hv: vmbus: Offload the handling of channels to two workqueues
[not found] ` <20181203141604.7FED32082F@mail.kernel.org>
@ 2018-12-03 20:11 ` Dexuan Cui
2018-12-04 8:57 ` Sasha Levin
2018-12-11 14:09 ` gregkh
0 siblings, 2 replies; 4+ messages in thread
From: Dexuan Cui @ 2018-12-03 20:11 UTC (permalink / raw)
To: Sasha Levin, gregkh@linuxfoundation.org
Cc: apw@canonical.com, stable@vger.kernel.org, Stephen Hemminger,
KY Srinivasan, Haiyang Zhang, stable@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 1698 bytes --]
> From: Sasha Levin <sashal@kernel.org>
> Sent: Monday, December 3, 2018 6:16 AM
>
> Hi,
>
> [This is an automated email]
>
> This commit has been processed because it contains a "Fixes:" tag,
> fixing commit: 8195b1396ec8 hv_netvsc: fix deadlock on hotplug.
>
> The bot has tested the following trees: v4.19.6, v4.14.85,
>
> v4.19.6: Build OK!
> v4.14.85: Failed to apply! Possible dependencies:
> 50229128727f ("Drivers: hv: vmbus: Fix the offer_in_progress in
> vmbus_process_offer()")
> c2e5df616e1a ("vmbus: add per-channel sysfs info")
>
> How should we proceed with this patch?
> Sasha
BTW, the patch went into char-misc.git's char-misc-linus branch 13 hours ago:
37c2578c0c40 ("Drivers: hv: vmbus: Offload the handling of channels to two workqueues")
linux-4.14.y (v4.14.85) lacks c2e5df616e1a ("vmbus: add per-channel sysfs info")
and the other related patches so this patch can't apply cleanly.
I suggest we cherry-pick all the related patches to linux-4.14.y in this order:
(Note: 37c2578c0c40 is in char-misc.git's char-misc-linus branch and is not in Linus's tree yet)
c2e5df616e1a ("vmbus: add per-channel sysfs info")
869b5567e12f ("vmbus: unregister device_obj->channels_kset")
6712cc9c2211 ("vmbus: don't return values for uninitalized channels")
50229128727f ("Drivers: hv: vmbus: Fix the offer_in_progress in vmbus_process_offer()")
37c2578c0c40 ("Drivers: hv: vmbus: Offload the handling of channels to two workqueues")
Alternatively, we can skip c2e5df616e1a ("vmbus: add per-channel sysfs info"), and then
please use the attached rebased patch for v4.14.85.
I prefer the first solution.
Thanks,
-- Dexuan
[-- Attachment #2: For-v4.14.85-0001-Drivers-hv-vmbus-Offload-the-handling-of-channels-to.patch --]
[-- Type: application/octet-stream, Size: 12804 bytes --]
From aefca80d9974bcf7c0e741e9df2fea12f81559ad Mon Sep 17 00:00:00 2001
From: Dexuan Cui <decui@microsoft.com>
Date: Mon, 3 Dec 2018 00:54:35 +0000
Subject: [PATCH] Drivers: hv: vmbus: Offload the handling of channels to two
workqueues
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-rc4.
So actually the patch should be applied to all the existing kernels,
not only the kernels that have 8195b1396ec8.
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>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/hv/channel_mgmt.c | 185 +++++++++++++++++++++++++++++++---------------
drivers/hv/connection.c | 24 +++++-
drivers/hv/hyperv_vmbus.h | 7 ++
include/linux/hyperv.h | 7 ++
4 files changed, 160 insertions(+), 63 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 752c52f7..43eaf54 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -444,61 +444,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);
- channel->num_sc++;
- spin_unlock_irqrestore(&channel->lock, flags);
- } else {
- goto err_free_chan;
- }
- }
-
dev_type = hv_get_dev_type(newchannel);
init_vp_index(newchannel, dev_type);
@@ -516,21 +471,22 @@ 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) {
- if (channel->sc_creation_callback != NULL)
- channel->sc_creation_callback(newchannel);
+ if (primary_channel != NULL) {
+ /* newchannel is a sub-channel. */
+
+ if (primary_channel->sc_creation_callback != NULL)
+ primary_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,
@@ -559,13 +515,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();
@@ -573,14 +544,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
@@ -618,6 +679,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.
@@ -700,6 +763,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 5449fc5..4b1b707 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -161,6 +161,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);
@@ -251,10 +265,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 49569f8..a166de6 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -327,7 +327,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 0c51f75..d1324d3 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -869,6 +869,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.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread