* [PATCH] [REPOST for the char-misc-linus branch] Drivers: hv: vmbus: Offload the handling of channels to two workqueues
@ 2018-12-03 0:54 Dexuan Cui
[not found] ` <20181203141604.7FED32082F@mail.kernel.org>
0 siblings, 1 reply; 5+ messages in thread
From: Dexuan Cui @ 2018-12-03 0:54 UTC (permalink / raw)
To: gregkh, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
linux-kernel, devel
Cc: apw, vkuznets, olaf, jasowang, Michael Kelley
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>
---
[Dec. 2, 2018] Dexuan: I rebased this patch to today's char-misc's
char-misc-linus branch, whose top commit is:
d8f190ee836a ("Merge branch 'akpm' (patches from Andrew)")
Previously KY posted the patch with his Signed-off-by (which is kept in this
repost), but there was a conflict issue.
We have done a full testing with the patch.
We hope the important patch can be in the final v4.20. Thanks!
drivers/hv/channel_mgmt.c | 189 ++++++++++++++++++++++++++++++----------------
drivers/hv/connection.c | 24 +++++-
drivers/hv/hyperv_vmbus.h | 7 ++
include/linux/hyperv.h | 7 ++
4 files changed, 161 insertions(+), 66 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 6277597d..edd34c1 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -435,61 +435,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);
@@ -507,27 +462,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,
@@ -556,13 +510,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();
@@ -570,14 +539,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
@@ -613,6 +672,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.
@@ -695,6 +756,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 f4d08c8..4fe117b7 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 72eaba3..87d3d7d 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -335,7 +335,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 b3e2436..14131b6 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -905,6 +905,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] 5+ messages in thread
* 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; 5+ messages in thread
From: Dexuan Cui @ 2018-12-03 20:11 UTC (permalink / raw)
To: Sasha Levin, gregkh
Cc: apw, stable, Stephen Hemminger, KY Srinivasan, Haiyang Zhang, stable
[-- 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] 5+ messages in thread
* Re: [PATCH] [REPOST for the char-misc-linus branch] Drivers: hv: vmbus: Offload the handling of channels to two workqueues
2018-12-03 20:11 ` Dexuan Cui
@ 2018-12-04 8:57 ` Sasha Levin
2018-12-04 9:05 ` Dexuan Cui
2018-12-11 14:09 ` gregkh
1 sibling, 1 reply; 5+ messages in thread
From: Sasha Levin @ 2018-12-04 8:57 UTC (permalink / raw)
To: Dexuan Cui
Cc: gregkh, apw, stable, Stephen Hemminger, KY Srinivasan, Haiyang Zhang
On Mon, Dec 03, 2018 at 08:11:18PM +0000, Dexuan Cui wrote:
>> 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.
Why is c2e5df616e1a needed on 4.14? We usually prefer to take a
dependency or two to avoid custom backports, but a sysfs ABI change
which causes only trivial conflicts for the actual fix is a bit too
much.
--
Thanks,
Sasha
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] [REPOST for the char-misc-linus branch] Drivers: hv: vmbus: Offload the handling of channels to two workqueues
2018-12-04 8:57 ` Sasha Levin
@ 2018-12-04 9:05 ` Dexuan Cui
0 siblings, 0 replies; 5+ messages in thread
From: Dexuan Cui @ 2018-12-04 9:05 UTC (permalink / raw)
To: Sasha Levin
Cc: gregkh, apw, stable, Stephen Hemminger, KY Srinivasan, Haiyang Zhang
> From: Sasha Levin <sashal@kernel.org>
> Sent: Tuesday, December 4, 2018 12:58 AM
> To: Dexuan Cui <decui@microsoft.com>
> Cc: gregkh@linuxfoundation.org; apw@canonical.com; stable@vger.kernel.org;
> Stephen Hemminger <sthemmin@microsoft.com>; KY Srinivasan
> <kys@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>
> Subject: Re: [PATCH] [REPOST for the char-misc-linus branch] Drivers: hv: vmbus:
> Offload the handling of channels to two workqueues
>
> On Mon, Dec 03, 2018 at 08:11:18PM +0000, Dexuan Cui wrote:
> >> 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.
>
> Why is c2e5df616e1a needed on 4.14? We usually prefer to take a
> dependency or two to avoid custom backports, but a sysfs ABI change
> which causes only trivial conflicts for the actual fix is a bit too
> much.
>
> --
> Thanks,
> Sasha
Thanks for the explanation. Then I'm fine with the second solution. I attached
For-v4.14.85-0001-Drivers-hv-vmbus-Offload-the-handling-of-channels-to.patch
in a prior mail for v4.14.85.
Thanks,
-- Dexuan
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] [REPOST for the char-misc-linus branch] Drivers: hv: vmbus: Offload the handling of channels to two workqueues
2018-12-03 20:11 ` Dexuan Cui
2018-12-04 8:57 ` Sasha Levin
@ 2018-12-11 14:09 ` gregkh
1 sibling, 0 replies; 5+ messages in thread
From: gregkh @ 2018-12-11 14:09 UTC (permalink / raw)
To: Dexuan Cui
Cc: Sasha Levin, apw, stable, Stephen Hemminger, KY Srinivasan,
Haiyang Zhang
On Mon, Dec 03, 2018 at 08:11:18PM +0000, Dexuan Cui wrote:
> > 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've taken the attached patch instead.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-12-11 14:09 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-03 0:54 [PATCH] [REPOST for the char-misc-linus branch] Drivers: hv: vmbus: Offload the handling of channels to two workqueues Dexuan Cui
[not found] ` <20181203141604.7FED32082F@mail.kernel.org>
2018-12-03 20:11 ` Dexuan Cui
2018-12-04 8:57 ` Sasha Levin
2018-12-04 9:05 ` Dexuan Cui
2018-12-11 14:09 ` gregkh
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.