* RE: FAILED: patch "[PATCH] vmbus: fix subchannel removal" failed to apply to 4.20-stable tree
2019-01-28 12:34 FAILED: patch "[PATCH] vmbus: fix subchannel removal" failed to apply to 4.20-stable tree gregkh
@ 2019-01-28 18:43 ` Dexuan Cui
2019-01-28 22:10 ` Sasha Levin
0 siblings, 1 reply; 3+ messages in thread
From: Dexuan Cui @ 2019-01-28 18:43 UTC (permalink / raw)
To: gregkh@linuxfoundation.org, sashal@kernel.org, Stephen Hemminger
Cc: stable@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 8390 bytes --]
> From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>
> Sent: Monday, January 28, 2019 4:35 AM
> To: Dexuan Cui <decui@microsoft.com>; sashal@kernel.org; Stephen
> Hemminger <sthemmin@microsoft.com>
> Cc: stable@vger.kernel.org
> Subject: FAILED: patch "[PATCH] vmbus: fix subchannel removal" failed to apply
> to 4.20-stable tree
>
>
> The patch below does not apply to the 4.20-stable tree.
> If someone wants it applied there, or to any other stable or longterm
> tree, then please email the backport, including the original git commit
> id to <stable@vger.kernel.org>.
>
> thanks,
>
> greg k-h
>
> ------------------ original commit in Linus's tree ------------------
>
> From b5679cebf780c6f1c2451a73bf1842a4409840e7 Mon Sep 17 00:00:00
> 2001
> From: Dexuan Cui <decui@microsoft.com>
> Date: Wed, 9 Jan 2019 20:56:06 +0000
> Subject: [PATCH] vmbus: fix subchannel removal
>
> The changes to split ring allocation from open/close, broke
> the cleanup of subchannels. This resulted in problems using
> uio on network devices because the subchannel was left behind
> when the network device was unbound.
>
> The cause was in the disconnect logic which used list splice
> to move the subchannel list into a local variable. This won't
> work because the subchannel list is needed later during the
> process of the rescind messages (relid2channel).
>
> The fix is to just leave the subchannel list in place
> which is what the original code did. The list is cleaned
> up later when the host rescind is processed.
>
> Without the fix, we have a lot of "hang" issues in netvsc when we
> try to change the NIC's MTU, set the number of channels, etc.
>
> Fixes: ae6935ed7d42 ("vmbus: split ring buffer allocation from open")
> Cc: stable@vger.kernel.org
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
>
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index ce0ba2062723..bea4c9850247 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -701,19 +701,12 @@ static int vmbus_close_internal(struct
> vmbus_channel *channel)
> int vmbus_disconnect_ring(struct vmbus_channel *channel)
> {
> struct vmbus_channel *cur_channel, *tmp;
> - unsigned long flags;
> - LIST_HEAD(list);
> int ret;
>
> if (channel->primary_channel != NULL)
> return -EINVAL;
>
> - /* Snapshot the list of subchannels */
> - spin_lock_irqsave(&channel->lock, flags);
> - list_splice_init(&channel->sc_list, &list);
> - spin_unlock_irqrestore(&channel->lock, flags);
> -
> - list_for_each_entry_safe(cur_channel, tmp, &list, sc_list) {
> + list_for_each_entry_safe(cur_channel, tmp, &channel->sc_list, sc_list) {
> if (cur_channel->rescind)
> wait_for_completion(&cur_channel->rescind_event);
>
Hi Sasha, Greg,
We need to backport this patch to linux-4.20.y. To cleanly cherry-pick
this patch (i.e. b5679cebf780 ("vmbus: fix subchannel removal")), we
need to backport another patch first:
4d3c5c69191f ("Drivers: hv: vmbus: Remove the useless API vmbus_get_outgoing_channel()")
Unluckily we can't cleanly git-cherry-pick 4d3c5c69191f, as a trivial rebase
is required. I attached the rebased version, and let me also paste the patch
below for your convenience.
Thanks,
-- Dexuan
From 5975e6868fd7294d66664fffad48b61f576a9e11 Mon Sep 17 00:00:00 2001
From: Dexuan Cui <decui@microsoft.com>
Date: Mon, 26 Nov 2018 02:17:56 +0000
Subject: [PATCH] Drivers: hv: vmbus: Remove the useless API
vmbus_get_outgoing_channel()
Commit d86adf482b84 ("scsi: storvsc: Enable multi-queue support") removed
the usage of the API in Jan 2017, and the API is not used since then.
netvsc and storvsc have their own algorithms to determine the outgoing
channel, so this API is useless.
And the API is potentially unsafe, because it reads primary->num_sc without
any lock held. This can be risky considering the RESCIND-OFFER message.
Let's remove the API.
Cc: Long Li <longli@microsoft.com>
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>
Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
drivers/hv/channel.c | 1 -
drivers/hv/channel_mgmt.c | 44 ---------------------------------------
include/linux/hyperv.h | 17 ---------------
3 files changed, 62 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index fe00b12e4417..ce0ba2062723 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -711,7 +711,6 @@ int vmbus_disconnect_ring(struct vmbus_channel *channel)
/* Snapshot the list of subchannels */
spin_lock_irqsave(&channel->lock, flags);
list_splice_init(&channel->sc_list, &list);
- channel->num_sc = 0;
spin_unlock_irqrestore(&channel->lock, flags);
list_for_each_entry_safe(cur_channel, tmp, &list, sc_list) {
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index edd34c167a9b..d01689079e9b 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -405,7 +405,6 @@ void hv_process_channel_removal(struct vmbus_channel *channel)
primary_channel = channel->primary_channel;
spin_lock_irqsave(&primary_channel->lock, flags);
list_del(&channel->sc_list);
- primary_channel->num_sc--;
spin_unlock_irqrestore(&primary_channel->lock, flags);
}
@@ -1302,49 +1301,6 @@ int vmbus_request_offers(void)
return ret;
}
-/*
- * Retrieve the (sub) channel on which to send an outgoing request.
- * When a primary channel has multiple sub-channels, we try to
- * distribute the load equally amongst all available channels.
- */
-struct vmbus_channel *vmbus_get_outgoing_channel(struct vmbus_channel *primary)
-{
- struct list_head *cur, *tmp;
- int cur_cpu;
- struct vmbus_channel *cur_channel;
- struct vmbus_channel *outgoing_channel = primary;
- int next_channel;
- int i = 1;
-
- if (list_empty(&primary->sc_list))
- return outgoing_channel;
-
- next_channel = primary->next_oc++;
-
- if (next_channel > (primary->num_sc)) {
- primary->next_oc = 0;
- return outgoing_channel;
- }
-
- cur_cpu = hv_cpu_number_to_vp_number(smp_processor_id());
- list_for_each_safe(cur, tmp, &primary->sc_list) {
- cur_channel = list_entry(cur, struct vmbus_channel, sc_list);
- if (cur_channel->state != CHANNEL_OPENED_STATE)
- continue;
-
- if (cur_channel->target_vp == cur_cpu)
- return cur_channel;
-
- if (i == next_channel)
- return cur_channel;
-
- i++;
- }
-
- return outgoing_channel;
-}
-EXPORT_SYMBOL_GPL(vmbus_get_outgoing_channel);
-
static void invoke_sc_cb(struct vmbus_channel *primary_channel)
{
struct list_head *cur, *tmp;
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 14131b6fae68..f0885cc01db6 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -830,15 +830,6 @@ struct vmbus_channel {
* All Sub-channels of a primary channel are linked here.
*/
struct list_head sc_list;
- /*
- * Current number of sub-channels.
- */
- int num_sc;
- /*
- * Number of a sub-channel (position within sc_list) which is supposed
- * to be used as the next outgoing channel.
- */
- int next_oc;
/*
* The primary channel this sub-channel belongs to.
* This will be NULL for the primary channel.
@@ -972,14 +963,6 @@ void vmbus_set_sc_create_callback(struct vmbus_channel *primary_channel,
void vmbus_set_chn_rescind_callback(struct vmbus_channel *channel,
void (*chn_rescind_cb)(struct vmbus_channel *));
-/*
- * Retrieve the (sub) channel on which to send an outgoing request.
- * When a primary channel has multiple sub-channels, we choose a
- * channel whose VCPU binding is closest to the VCPU on which
- * this call is being made.
- */
-struct vmbus_channel *vmbus_get_outgoing_channel(struct vmbus_channel *primary);
-
/*
* Check if sub-channels have already been offerred. This API will be useful
* when the driver is unloaded after establishing sub-channels. In this case,
--
2.17.1
[-- Attachment #2: for-4.20.y-0001-Drivers-hv-vmbus-Remove-the-useless-API-vmbus_get_ou.patch --]
[-- Type: application/octet-stream, Size: 4824 bytes --]
From 5975e6868fd7294d66664fffad48b61f576a9e11 Mon Sep 17 00:00:00 2001
From: Dexuan Cui <decui@microsoft.com>
Date: Mon, 26 Nov 2018 02:17:56 +0000
Subject: [PATCH] Drivers: hv: vmbus: Remove the useless API
vmbus_get_outgoing_channel()
Commit d86adf482b84 ("scsi: storvsc: Enable multi-queue support") removed
the usage of the API in Jan 2017, and the API is not used since then.
netvsc and storvsc have their own algorithms to determine the outgoing
channel, so this API is useless.
And the API is potentially unsafe, because it reads primary->num_sc without
any lock held. This can be risky considering the RESCIND-OFFER message.
Let's remove the API.
Cc: Long Li <longli@microsoft.com>
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>
Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
drivers/hv/channel.c | 1 -
drivers/hv/channel_mgmt.c | 44 ---------------------------------------
include/linux/hyperv.h | 17 ---------------
3 files changed, 62 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index fe00b12e4417..ce0ba2062723 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -711,7 +711,6 @@ int vmbus_disconnect_ring(struct vmbus_channel *channel)
/* Snapshot the list of subchannels */
spin_lock_irqsave(&channel->lock, flags);
list_splice_init(&channel->sc_list, &list);
- channel->num_sc = 0;
spin_unlock_irqrestore(&channel->lock, flags);
list_for_each_entry_safe(cur_channel, tmp, &list, sc_list) {
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index edd34c167a9b..d01689079e9b 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -405,7 +405,6 @@ void hv_process_channel_removal(struct vmbus_channel *channel)
primary_channel = channel->primary_channel;
spin_lock_irqsave(&primary_channel->lock, flags);
list_del(&channel->sc_list);
- primary_channel->num_sc--;
spin_unlock_irqrestore(&primary_channel->lock, flags);
}
@@ -1302,49 +1301,6 @@ int vmbus_request_offers(void)
return ret;
}
-/*
- * Retrieve the (sub) channel on which to send an outgoing request.
- * When a primary channel has multiple sub-channels, we try to
- * distribute the load equally amongst all available channels.
- */
-struct vmbus_channel *vmbus_get_outgoing_channel(struct vmbus_channel *primary)
-{
- struct list_head *cur, *tmp;
- int cur_cpu;
- struct vmbus_channel *cur_channel;
- struct vmbus_channel *outgoing_channel = primary;
- int next_channel;
- int i = 1;
-
- if (list_empty(&primary->sc_list))
- return outgoing_channel;
-
- next_channel = primary->next_oc++;
-
- if (next_channel > (primary->num_sc)) {
- primary->next_oc = 0;
- return outgoing_channel;
- }
-
- cur_cpu = hv_cpu_number_to_vp_number(smp_processor_id());
- list_for_each_safe(cur, tmp, &primary->sc_list) {
- cur_channel = list_entry(cur, struct vmbus_channel, sc_list);
- if (cur_channel->state != CHANNEL_OPENED_STATE)
- continue;
-
- if (cur_channel->target_vp == cur_cpu)
- return cur_channel;
-
- if (i == next_channel)
- return cur_channel;
-
- i++;
- }
-
- return outgoing_channel;
-}
-EXPORT_SYMBOL_GPL(vmbus_get_outgoing_channel);
-
static void invoke_sc_cb(struct vmbus_channel *primary_channel)
{
struct list_head *cur, *tmp;
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 14131b6fae68..f0885cc01db6 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -830,15 +830,6 @@ struct vmbus_channel {
* All Sub-channels of a primary channel are linked here.
*/
struct list_head sc_list;
- /*
- * Current number of sub-channels.
- */
- int num_sc;
- /*
- * Number of a sub-channel (position within sc_list) which is supposed
- * to be used as the next outgoing channel.
- */
- int next_oc;
/*
* The primary channel this sub-channel belongs to.
* This will be NULL for the primary channel.
@@ -972,14 +963,6 @@ void vmbus_set_sc_create_callback(struct vmbus_channel *primary_channel,
void vmbus_set_chn_rescind_callback(struct vmbus_channel *channel,
void (*chn_rescind_cb)(struct vmbus_channel *));
-/*
- * Retrieve the (sub) channel on which to send an outgoing request.
- * When a primary channel has multiple sub-channels, we choose a
- * channel whose VCPU binding is closest to the VCPU on which
- * this call is being made.
- */
-struct vmbus_channel *vmbus_get_outgoing_channel(struct vmbus_channel *primary);
-
/*
* Check if sub-channels have already been offerred. This API will be useful
* when the driver is unloaded after establishing sub-channels. In this case,
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread