* [PATCH 1/8] Drivers: hv: vmbus: Remove the target_vp field from the vmbus_channel struct
2020-06-17 16:46 [PATCH 0/8] Drivers: hv: vmbus: Miscellaneous cleanups Andrea Parri (Microsoft)
@ 2020-06-17 16:46 ` Andrea Parri (Microsoft)
2020-06-18 15:24 ` Michael Kelley
2020-06-17 16:46 ` [PATCH 2/8] Drivers: hv: vmbus: Remove the numa_node " Andrea Parri (Microsoft)
` (7 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Andrea Parri (Microsoft) @ 2020-06-17 16:46 UTC (permalink / raw)
To: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Michael Kelley
Cc: linux-hyperv, linux-kernel, Andrea Parri (Microsoft)
The field is read only in __vmbus_open() and it is already stored twice
(after a call to hv_cpu_number_to_vp_number()) in target_cpu_store() and
init_vp_index(); there is no need to "cache" its value in the channel
data structure.
Suggested-by: Michael Kelley <mikelley@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
drivers/hv/channel.c | 3 ++-
drivers/hv/channel_mgmt.c | 3 ---
drivers/hv/vmbus_drv.c | 2 --
include/linux/hyperv.h | 15 +++++++--------
4 files changed, 9 insertions(+), 14 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 90070b337c10d..8848d1548b3f2 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -18,6 +18,7 @@
#include <linux/uio.h>
#include <linux/interrupt.h>
#include <asm/page.h>
+#include <asm/mshyperv.h>
#include "hyperv_vmbus.h"
@@ -176,7 +177,7 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
open_msg->child_relid = newchannel->offermsg.child_relid;
open_msg->ringbuffer_gpadlhandle = newchannel->ringbuffer_gpadlhandle;
open_msg->downstream_ringbuffer_pageoffset = newchannel->ringbuffer_send_offset;
- open_msg->target_vp = newchannel->target_vp;
+ open_msg->target_vp = hv_cpu_number_to_vp_number(newchannel->target_cpu);
if (userdatalen)
memcpy(open_msg->userdata, userdata, userdatalen);
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 417a95e5094dd..278e392218079 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -704,8 +704,6 @@ static void init_vp_index(struct vmbus_channel *channel)
*/
channel->numa_node = cpu_to_node(VMBUS_CONNECT_CPU);
channel->target_cpu = VMBUS_CONNECT_CPU;
- channel->target_vp =
- hv_cpu_number_to_vp_number(VMBUS_CONNECT_CPU);
if (perf_chn)
hv_set_alloced_cpu(VMBUS_CONNECT_CPU);
return;
@@ -739,7 +737,6 @@ static void init_vp_index(struct vmbus_channel *channel)
cpumask_set_cpu(target_cpu, alloced_mask);
channel->target_cpu = target_cpu;
- channel->target_vp = hv_cpu_number_to_vp_number(target_cpu);
free_cpumask_var(available_mask);
}
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 47747755d2e1d..7e244727f5686 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -23,7 +23,6 @@
#include <linux/cpu.h>
#include <linux/sched/task_stack.h>
-#include <asm/mshyperv.h>
#include <linux/delay.h>
#include <linux/notifier.h>
#include <linux/ptrace.h>
@@ -1765,7 +1764,6 @@ static ssize_t target_cpu_store(struct vmbus_channel *channel,
*/
channel->target_cpu = target_cpu;
- channel->target_vp = hv_cpu_number_to_vp_number(target_cpu);
channel->numa_node = cpu_to_node(target_cpu);
/* See init_vp_index(). */
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 40df3103e890b..738efdb194b09 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -803,15 +803,14 @@ struct vmbus_channel {
u64 sig_event;
/*
- * Starting with win8, this field will be used to specify
- * the target virtual processor on which to deliver the interrupt for
- * the host to guest communication.
- * Prior to win8, incoming channel interrupts would only
- * be delivered on cpu 0. Setting this value to 0 would
- * preserve the earlier behavior.
+ * Starting with win8, this field will be used to specify the
+ * target CPU on which to deliver the interrupt for the host
+ * to guest communication.
+ *
+ * Prior to win8, incoming channel interrupts would only be
+ * delivered on CPU 0. Setting this value to 0 would preserve
+ * the earlier behavior.
*/
- u32 target_vp;
- /* The corresponding CPUID in the guest */
u32 target_cpu;
int numa_node;
/*
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* RE: [PATCH 1/8] Drivers: hv: vmbus: Remove the target_vp field from the vmbus_channel struct
2020-06-17 16:46 ` [PATCH 1/8] Drivers: hv: vmbus: Remove the target_vp field from the vmbus_channel struct Andrea Parri (Microsoft)
@ 2020-06-18 15:24 ` Michael Kelley
0 siblings, 0 replies; 23+ messages in thread
From: Michael Kelley @ 2020-06-18 15:24 UTC (permalink / raw)
To: Andrea Parri (Microsoft),
KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu
Cc: linux-hyperv, linux-kernel
From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, June 17, 2020 9:47 AM
>
> The field is read only in __vmbus_open() and it is already stored twice
> (after a call to hv_cpu_number_to_vp_number()) in target_cpu_store() and
> init_vp_index(); there is no need to "cache" its value in the channel
> data structure.
>
> Suggested-by: Michael Kelley <mikelley@microsoft.com>
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
> drivers/hv/channel.c | 3 ++-
> drivers/hv/channel_mgmt.c | 3 ---
> drivers/hv/vmbus_drv.c | 2 --
> include/linux/hyperv.h | 15 +++++++--------
> 4 files changed, 9 insertions(+), 14 deletions(-)
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/8] Drivers: hv: vmbus: Remove the numa_node field from the vmbus_channel struct
2020-06-17 16:46 [PATCH 0/8] Drivers: hv: vmbus: Miscellaneous cleanups Andrea Parri (Microsoft)
2020-06-17 16:46 ` [PATCH 1/8] Drivers: hv: vmbus: Remove the target_vp field from the vmbus_channel struct Andrea Parri (Microsoft)
@ 2020-06-17 16:46 ` Andrea Parri (Microsoft)
2020-06-18 15:26 ` Michael Kelley
2020-06-17 16:46 ` [PATCH 3/8] Drivers: hv: vmbus: Replace cpumask_test_cpu(, cpu_online_mask) with cpu_online() Andrea Parri (Microsoft)
` (6 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Andrea Parri (Microsoft) @ 2020-06-17 16:46 UTC (permalink / raw)
To: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Michael Kelley
Cc: linux-hyperv, linux-kernel, Andrea Parri (Microsoft)
The field is read only in numa_node_show() and it is already stored twice
(after a call to cpu_to_node()) in target_cpu_store() and init_vp_index();
there is no need to "cache" its value in the channel data structure.
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
drivers/hv/channel_mgmt.c | 2 --
drivers/hv/vmbus_drv.c | 3 +--
include/linux/hyperv.h | 1 -
3 files changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 278e392218079..36dd8b6c544a4 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -702,7 +702,6 @@ static void init_vp_index(struct vmbus_channel *channel)
* In case alloc_cpumask_var() fails, bind it to
* VMBUS_CONNECT_CPU.
*/
- channel->numa_node = cpu_to_node(VMBUS_CONNECT_CPU);
channel->target_cpu = VMBUS_CONNECT_CPU;
if (perf_chn)
hv_set_alloced_cpu(VMBUS_CONNECT_CPU);
@@ -719,7 +718,6 @@ static void init_vp_index(struct vmbus_channel *channel)
continue;
break;
}
- channel->numa_node = numa_node;
alloced_mask = &hv_context.hv_numa_map[numa_node];
if (cpumask_weight(alloced_mask) ==
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 7e244727f5686..c3205f40d1415 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -226,7 +226,7 @@ static ssize_t numa_node_show(struct device *dev,
if (!hv_dev->channel)
return -ENODEV;
- return sprintf(buf, "%d\n", hv_dev->channel->numa_node);
+ return sprintf(buf, "%d\n", cpu_to_node(hv_dev->channel->target_cpu));
}
static DEVICE_ATTR_RO(numa_node);
#endif
@@ -1764,7 +1764,6 @@ static ssize_t target_cpu_store(struct vmbus_channel *channel,
*/
channel->target_cpu = target_cpu;
- channel->numa_node = cpu_to_node(target_cpu);
/* See init_vp_index(). */
if (hv_is_perf_channel(channel))
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 738efdb194b09..690394b79d727 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -812,7 +812,6 @@ struct vmbus_channel {
* the earlier behavior.
*/
u32 target_cpu;
- int numa_node;
/*
* Support for sub-channels. For high performance devices,
* it will be useful to have multiple sub-channels to support
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* RE: [PATCH 2/8] Drivers: hv: vmbus: Remove the numa_node field from the vmbus_channel struct
2020-06-17 16:46 ` [PATCH 2/8] Drivers: hv: vmbus: Remove the numa_node " Andrea Parri (Microsoft)
@ 2020-06-18 15:26 ` Michael Kelley
0 siblings, 0 replies; 23+ messages in thread
From: Michael Kelley @ 2020-06-18 15:26 UTC (permalink / raw)
To: Andrea Parri (Microsoft),
KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu
Cc: linux-hyperv, linux-kernel
From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, June 17, 2020 9:47 AM
>
> The field is read only in numa_node_show() and it is already stored twice
> (after a call to cpu_to_node()) in target_cpu_store() and init_vp_index();
> there is no need to "cache" its value in the channel data structure.
>
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
> drivers/hv/channel_mgmt.c | 2 --
> drivers/hv/vmbus_drv.c | 3 +--
> include/linux/hyperv.h | 1 -
> 3 files changed, 1 insertion(+), 5 deletions(-)
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 3/8] Drivers: hv: vmbus: Replace cpumask_test_cpu(, cpu_online_mask) with cpu_online()
2020-06-17 16:46 [PATCH 0/8] Drivers: hv: vmbus: Miscellaneous cleanups Andrea Parri (Microsoft)
2020-06-17 16:46 ` [PATCH 1/8] Drivers: hv: vmbus: Remove the target_vp field from the vmbus_channel struct Andrea Parri (Microsoft)
2020-06-17 16:46 ` [PATCH 2/8] Drivers: hv: vmbus: Remove the numa_node " Andrea Parri (Microsoft)
@ 2020-06-17 16:46 ` Andrea Parri (Microsoft)
2020-06-18 15:27 ` Michael Kelley
2020-06-17 16:46 ` [PATCH 4/8] Drivers: hv: vmbus: Remove unnecessary channel->lock critical sections (sc_list readers) Andrea Parri (Microsoft)
` (5 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Andrea Parri (Microsoft) @ 2020-06-17 16:46 UTC (permalink / raw)
To: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Michael Kelley
Cc: linux-hyperv, linux-kernel, Andrea Parri (Microsoft)
A slight improvement in readability, and this does also remove one
memory access when NR_CPUS == 1! ;-)
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
drivers/hv/vmbus_drv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index c3205f40d1415..452c14c656e2a 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1702,7 +1702,7 @@ static ssize_t target_cpu_store(struct vmbus_channel *channel,
/* No CPUs should come up or down during this. */
cpus_read_lock();
- if (!cpumask_test_cpu(target_cpu, cpu_online_mask)) {
+ if (!cpu_online(target_cpu)) {
cpus_read_unlock();
return -EINVAL;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* RE: [PATCH 3/8] Drivers: hv: vmbus: Replace cpumask_test_cpu(, cpu_online_mask) with cpu_online()
2020-06-17 16:46 ` [PATCH 3/8] Drivers: hv: vmbus: Replace cpumask_test_cpu(, cpu_online_mask) with cpu_online() Andrea Parri (Microsoft)
@ 2020-06-18 15:27 ` Michael Kelley
0 siblings, 0 replies; 23+ messages in thread
From: Michael Kelley @ 2020-06-18 15:27 UTC (permalink / raw)
To: Andrea Parri (Microsoft),
KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu
Cc: linux-hyperv, linux-kernel
From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, June 17, 2020 9:47 AM
>
> A slight improvement in readability, and this does also remove one
> memory access when NR_CPUS == 1! ;-)
>
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
> drivers/hv/vmbus_drv.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index c3205f40d1415..452c14c656e2a 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1702,7 +1702,7 @@ static ssize_t target_cpu_store(struct vmbus_channel *channel,
> /* No CPUs should come up or down during this. */
> cpus_read_lock();
>
> - if (!cpumask_test_cpu(target_cpu, cpu_online_mask)) {
> + if (!cpu_online(target_cpu)) {
> cpus_read_unlock();
> return -EINVAL;
> }
> --
> 2.25.1
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 4/8] Drivers: hv: vmbus: Remove unnecessary channel->lock critical sections (sc_list readers)
2020-06-17 16:46 [PATCH 0/8] Drivers: hv: vmbus: Miscellaneous cleanups Andrea Parri (Microsoft)
` (2 preceding siblings ...)
2020-06-17 16:46 ` [PATCH 3/8] Drivers: hv: vmbus: Replace cpumask_test_cpu(, cpu_online_mask) with cpu_online() Andrea Parri (Microsoft)
@ 2020-06-17 16:46 ` Andrea Parri (Microsoft)
2020-06-18 15:29 ` Michael Kelley
2020-06-17 16:46 ` [PATCH 5/8] Drivers: hv: vmbus: Use channel_mutex in channel_vp_mapping_show() Andrea Parri (Microsoft)
` (4 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Andrea Parri (Microsoft) @ 2020-06-17 16:46 UTC (permalink / raw)
To: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Michael Kelley
Cc: linux-hyperv, linux-kernel, Andrea Parri (Microsoft)
Additions/deletions to/from sc_list (as well as modifications of
target_cpu(s)) are protected by channel_mutex, which hv_synic_cleanup()
and vmbus_bus_suspend() own for the duration of the channel->lock
critical section in question.
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
drivers/hv/hv.c | 3 ---
drivers/hv/vmbus_drv.c | 3 ---
2 files changed, 6 deletions(-)
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index 188b42b07f07b..0c637111e7c70 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -245,7 +245,6 @@ int hv_synic_cleanup(unsigned int cpu)
{
struct vmbus_channel *channel, *sc;
bool channel_found = false;
- unsigned long flags;
/*
* Hyper-V does not provide a way to change the connect CPU once
@@ -267,14 +266,12 @@ int hv_synic_cleanup(unsigned int cpu)
channel_found = true;
break;
}
- spin_lock_irqsave(&channel->lock, flags);
list_for_each_entry(sc, &channel->sc_list, sc_list) {
if (sc->target_cpu == cpu) {
channel_found = true;
break;
}
}
- spin_unlock_irqrestore(&channel->lock, flags);
if (channel_found)
break;
}
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 452c14c656e2a..b5ae45eb8aef7 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -2330,7 +2330,6 @@ static int vmbus_acpi_add(struct acpi_device *device)
static int vmbus_bus_suspend(struct device *dev)
{
struct vmbus_channel *channel, *sc;
- unsigned long flags;
while (atomic_read(&vmbus_connection.offer_in_progress) != 0) {
/*
@@ -2388,12 +2387,10 @@ static int vmbus_bus_suspend(struct device *dev)
continue;
}
- spin_lock_irqsave(&channel->lock, flags);
list_for_each_entry(sc, &channel->sc_list, sc_list) {
pr_err("Sub-channel not deleted!\n");
WARN_ON_ONCE(1);
}
- spin_unlock_irqrestore(&channel->lock, flags);
atomic_inc(&vmbus_connection.nr_chan_fixup_on_resume);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* RE: [PATCH 4/8] Drivers: hv: vmbus: Remove unnecessary channel->lock critical sections (sc_list readers)
2020-06-17 16:46 ` [PATCH 4/8] Drivers: hv: vmbus: Remove unnecessary channel->lock critical sections (sc_list readers) Andrea Parri (Microsoft)
@ 2020-06-18 15:29 ` Michael Kelley
0 siblings, 0 replies; 23+ messages in thread
From: Michael Kelley @ 2020-06-18 15:29 UTC (permalink / raw)
To: Andrea Parri (Microsoft),
KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu
Cc: linux-hyperv, linux-kernel
From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, June 17, 2020 9:47 AM
>
> Additions/deletions to/from sc_list (as well as modifications of
> target_cpu(s)) are protected by channel_mutex, which hv_synic_cleanup()
> and vmbus_bus_suspend() own for the duration of the channel->lock
> critical section in question.
>
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
> drivers/hv/hv.c | 3 ---
> drivers/hv/vmbus_drv.c | 3 ---
> 2 files changed, 6 deletions(-)
>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 5/8] Drivers: hv: vmbus: Use channel_mutex in channel_vp_mapping_show()
2020-06-17 16:46 [PATCH 0/8] Drivers: hv: vmbus: Miscellaneous cleanups Andrea Parri (Microsoft)
` (3 preceding siblings ...)
2020-06-17 16:46 ` [PATCH 4/8] Drivers: hv: vmbus: Remove unnecessary channel->lock critical sections (sc_list readers) Andrea Parri (Microsoft)
@ 2020-06-17 16:46 ` Andrea Parri (Microsoft)
2020-06-18 18:31 ` Michael Kelley
2020-06-17 16:46 ` [PATCH 6/8] Drivers: hv: vmbus: Remove unnecessary channel->lock critical sections (sc_list updaters) Andrea Parri (Microsoft)
` (3 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Andrea Parri (Microsoft) @ 2020-06-17 16:46 UTC (permalink / raw)
To: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Michael Kelley
Cc: linux-hyperv, linux-kernel, Andrea Parri (Microsoft)
The primitive currently uses channel->lock to protect the loop over
sc_list w.r.t. list additions/deletions but it doesn't protect the
target_cpu(s) loads w.r.t. a concurrent target_cpu_store(): while the
data races on target_cpu are hardly of any concern here, replace the
channel->lock critical section with a channel_mutex critical section
and extend the latter to include the loads of target_cpu; this same
pattern is also used in hv_synic_cleanup().
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
drivers/hv/vmbus_drv.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index b5ae45eb8aef7..9e39692dc13ee 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -507,18 +507,17 @@ static ssize_t channel_vp_mapping_show(struct device *dev,
{
struct hv_device *hv_dev = device_to_hv_device(dev);
struct vmbus_channel *channel = hv_dev->channel, *cur_sc;
- unsigned long flags;
int buf_size = PAGE_SIZE, n_written, tot_written;
struct list_head *cur;
if (!channel)
return -ENODEV;
+ mutex_lock(&vmbus_connection.channel_mutex);
+
tot_written = snprintf(buf, buf_size, "%u:%u\n",
channel->offermsg.child_relid, channel->target_cpu);
- spin_lock_irqsave(&channel->lock, flags);
-
list_for_each(cur, &channel->sc_list) {
if (tot_written >= buf_size - 1)
break;
@@ -532,7 +531,7 @@ static ssize_t channel_vp_mapping_show(struct device *dev,
tot_written += n_written;
}
- spin_unlock_irqrestore(&channel->lock, flags);
+ mutex_unlock(&vmbus_connection.channel_mutex);
return tot_written;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* RE: [PATCH 5/8] Drivers: hv: vmbus: Use channel_mutex in channel_vp_mapping_show()
2020-06-17 16:46 ` [PATCH 5/8] Drivers: hv: vmbus: Use channel_mutex in channel_vp_mapping_show() Andrea Parri (Microsoft)
@ 2020-06-18 18:31 ` Michael Kelley
0 siblings, 0 replies; 23+ messages in thread
From: Michael Kelley @ 2020-06-18 18:31 UTC (permalink / raw)
To: Andrea Parri (Microsoft),
KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu
Cc: linux-hyperv, linux-kernel
From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, June 17, 2020 9:47 AM
>
> The primitive currently uses channel->lock to protect the loop over
> sc_list w.r.t. list additions/deletions but it doesn't protect the
> target_cpu(s) loads w.r.t. a concurrent target_cpu_store(): while the
> data races on target_cpu are hardly of any concern here, replace the
> channel->lock critical section with a channel_mutex critical section
> and extend the latter to include the loads of target_cpu; this same
> pattern is also used in hv_synic_cleanup().
>
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
> drivers/hv/vmbus_drv.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
Reviewed-By: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 6/8] Drivers: hv: vmbus: Remove unnecessary channel->lock critical sections (sc_list updaters)
2020-06-17 16:46 [PATCH 0/8] Drivers: hv: vmbus: Miscellaneous cleanups Andrea Parri (Microsoft)
` (4 preceding siblings ...)
2020-06-17 16:46 ` [PATCH 5/8] Drivers: hv: vmbus: Use channel_mutex in channel_vp_mapping_show() Andrea Parri (Microsoft)
@ 2020-06-17 16:46 ` Andrea Parri (Microsoft)
2020-06-18 18:32 ` Michael Kelley
2020-06-17 16:46 ` [PATCH 7/8] scsi: storvsc: Introduce the per-storvsc_device spinlock Andrea Parri (Microsoft)
` (2 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Andrea Parri (Microsoft) @ 2020-06-17 16:46 UTC (permalink / raw)
To: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Michael Kelley
Cc: linux-hyperv, linux-kernel, Andrea Parri (Microsoft)
None of the readers/updaters of sc_list rely on channel->lock for
synchronization.
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
drivers/hv/channel_mgmt.c | 25 ++++++-------------------
1 file changed, 6 insertions(+), 19 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 36dd8b6c544a4..92f8bb2077a94 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -400,8 +400,6 @@ static void vmbus_release_relid(u32 relid)
void hv_process_channel_removal(struct vmbus_channel *channel)
{
- unsigned long flags;
-
lockdep_assert_held(&vmbus_connection.channel_mutex);
BUG_ON(!channel->rescind);
@@ -422,14 +420,10 @@ void hv_process_channel_removal(struct vmbus_channel *channel)
if (channel->offermsg.child_relid != INVALID_RELID)
vmbus_channel_unmap_relid(channel);
- if (channel->primary_channel == NULL) {
+ if (channel->primary_channel == NULL)
list_del(&channel->listentry);
- } else {
- struct vmbus_channel *primary_channel = channel->primary_channel;
- spin_lock_irqsave(&primary_channel->lock, flags);
+ else
list_del(&channel->sc_list);
- spin_unlock_irqrestore(&primary_channel->lock, flags);
- }
/*
* If this is a "perf" channel, updates the hv_numa_map[] masks so that
@@ -470,7 +464,6 @@ static void vmbus_add_channel_work(struct work_struct *work)
struct vmbus_channel *newchannel =
container_of(work, struct vmbus_channel, add_channel_work);
struct vmbus_channel *primary_channel = newchannel->primary_channel;
- unsigned long flags;
int ret;
/*
@@ -531,13 +524,10 @@ static void vmbus_add_channel_work(struct work_struct *work)
*/
newchannel->probe_done = true;
- if (primary_channel == NULL) {
+ if (primary_channel == NULL)
list_del(&newchannel->listentry);
- } else {
- spin_lock_irqsave(&primary_channel->lock, flags);
+ else
list_del(&newchannel->sc_list);
- spin_unlock_irqrestore(&primary_channel->lock, flags);
- }
/* vmbus_process_offer() has mapped the channel. */
vmbus_channel_unmap_relid(newchannel);
@@ -557,7 +547,6 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
{
struct vmbus_channel *channel;
struct workqueue_struct *wq;
- unsigned long flags;
bool fnew = true;
/*
@@ -609,10 +598,10 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
}
}
- if (fnew)
+ if (fnew) {
list_add_tail(&newchannel->listentry,
&vmbus_connection.chn_list);
- else {
+ } else {
/*
* Check to see if this is a valid sub-channel.
*/
@@ -630,9 +619,7 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
* 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);
}
vmbus_channel_map_relid(newchannel);
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* RE: [PATCH 6/8] Drivers: hv: vmbus: Remove unnecessary channel->lock critical sections (sc_list updaters)
2020-06-17 16:46 ` [PATCH 6/8] Drivers: hv: vmbus: Remove unnecessary channel->lock critical sections (sc_list updaters) Andrea Parri (Microsoft)
@ 2020-06-18 18:32 ` Michael Kelley
0 siblings, 0 replies; 23+ messages in thread
From: Michael Kelley @ 2020-06-18 18:32 UTC (permalink / raw)
To: Andrea Parri (Microsoft),
KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu
Cc: linux-hyperv, linux-kernel
From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, June 17, 2020 9:47 AM
>
> None of the readers/updaters of sc_list rely on channel->lock for
> synchronization.
>
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
> drivers/hv/channel_mgmt.c | 25 ++++++-------------------
> 1 file changed, 6 insertions(+), 19 deletions(-)
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 7/8] scsi: storvsc: Introduce the per-storvsc_device spinlock
2020-06-17 16:46 [PATCH 0/8] Drivers: hv: vmbus: Miscellaneous cleanups Andrea Parri (Microsoft)
` (5 preceding siblings ...)
2020-06-17 16:46 ` [PATCH 6/8] Drivers: hv: vmbus: Remove unnecessary channel->lock critical sections (sc_list updaters) Andrea Parri (Microsoft)
@ 2020-06-17 16:46 ` Andrea Parri (Microsoft)
2020-06-18 18:34 ` Michael Kelley
2020-06-19 16:01 ` Wei Liu
2020-06-17 16:46 ` [PATCH 8/8] Drivers: hv: vmbus: Remove the lock field from the vmbus_channel struct Andrea Parri (Microsoft)
2020-06-19 15:39 ` [PATCH 0/8] Drivers: hv: vmbus: Miscellaneous cleanups Wei Liu
8 siblings, 2 replies; 23+ messages in thread
From: Andrea Parri (Microsoft) @ 2020-06-17 16:46 UTC (permalink / raw)
To: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Michael Kelley
Cc: linux-hyperv, linux-kernel, Andrea Parri (Microsoft)
storvsc uses the spinlock of the hv_device's primary channel to
serialize modifications of stor_chns[] performed by storvsc_do_io()
and storvsc_change_target_cpu(), when it could/should use a (per-)
storvsc_device spinlock: this change untangles the synchronization
mechanism for the (storvsc-specific) stor_chns[] array from the
"generic" VMBus code and data structures, clarifying the scope of
this synchronization mechanism.
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
drivers/scsi/storvsc_drv.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 072ed87286578..8ff21e69a8be8 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -462,6 +462,11 @@ struct storvsc_device {
* Mask of CPUs bound to subchannels.
*/
struct cpumask alloced_cpus;
+ /*
+ * Serializes modifications of stor_chns[] from storvsc_do_io()
+ * and storvsc_change_target_cpu().
+ */
+ spinlock_t lock;
/* Used for vsc/vsp channel reset process */
struct storvsc_cmd_request init_request;
struct storvsc_cmd_request reset_request;
@@ -639,7 +644,7 @@ static void storvsc_change_target_cpu(struct vmbus_channel *channel, u32 old,
return;
/* See storvsc_do_io() -> get_og_chn(). */
- spin_lock_irqsave(&device->channel->lock, flags);
+ spin_lock_irqsave(&stor_device->lock, flags);
/*
* Determines if the storvsc device has other channels assigned to
@@ -676,7 +681,7 @@ static void storvsc_change_target_cpu(struct vmbus_channel *channel, u32 old,
WRITE_ONCE(stor_device->stor_chns[new], channel);
cpumask_set_cpu(new, &stor_device->alloced_cpus);
- spin_unlock_irqrestore(&device->channel->lock, flags);
+ spin_unlock_irqrestore(&stor_device->lock, flags);
}
static void handle_sc_creation(struct vmbus_channel *new_sc)
@@ -1433,14 +1438,14 @@ static int storvsc_do_io(struct hv_device *device,
}
}
} else {
- spin_lock_irqsave(&device->channel->lock, flags);
+ spin_lock_irqsave(&stor_device->lock, flags);
outgoing_channel = stor_device->stor_chns[q_num];
if (outgoing_channel != NULL) {
- spin_unlock_irqrestore(&device->channel->lock, flags);
+ spin_unlock_irqrestore(&stor_device->lock, flags);
goto found_channel;
}
outgoing_channel = get_og_chn(stor_device, q_num);
- spin_unlock_irqrestore(&device->channel->lock, flags);
+ spin_unlock_irqrestore(&stor_device->lock, flags);
}
found_channel:
@@ -1881,6 +1886,7 @@ static int storvsc_probe(struct hv_device *device,
init_waitqueue_head(&stor_device->waiting_to_drain);
stor_device->device = device;
stor_device->host = host;
+ spin_lock_init(&stor_device->lock);
hv_set_drvdata(device, stor_device);
stor_device->port_number = host->host_no;
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* RE: [PATCH 7/8] scsi: storvsc: Introduce the per-storvsc_device spinlock
2020-06-17 16:46 ` [PATCH 7/8] scsi: storvsc: Introduce the per-storvsc_device spinlock Andrea Parri (Microsoft)
@ 2020-06-18 18:34 ` Michael Kelley
2020-06-19 16:01 ` Wei Liu
1 sibling, 0 replies; 23+ messages in thread
From: Michael Kelley @ 2020-06-18 18:34 UTC (permalink / raw)
To: Andrea Parri (Microsoft),
KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu
Cc: linux-hyperv, linux-kernel
From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, June 17, 2020 9:47 AM
>
> storvsc uses the spinlock of the hv_device's primary channel to
> serialize modifications of stor_chns[] performed by storvsc_do_io()
> and storvsc_change_target_cpu(), when it could/should use a (per-)
> storvsc_device spinlock: this change untangles the synchronization
> mechanism for the (storvsc-specific) stor_chns[] array from the
> "generic" VMBus code and data structures, clarifying the scope of
> this synchronization mechanism.
>
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
> drivers/scsi/storvsc_drv.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
FWIW, this patch will need to get routed to the SCSI maintainers and go through
the SCSI tree.
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 7/8] scsi: storvsc: Introduce the per-storvsc_device spinlock
2020-06-17 16:46 ` [PATCH 7/8] scsi: storvsc: Introduce the per-storvsc_device spinlock Andrea Parri (Microsoft)
2020-06-18 18:34 ` Michael Kelley
@ 2020-06-19 16:01 ` Wei Liu
2020-06-19 16:18 ` Andrea Parri
1 sibling, 1 reply; 23+ messages in thread
From: Wei Liu @ 2020-06-19 16:01 UTC (permalink / raw)
To: Andrea Parri (Microsoft), James E.J. Bottomley, Martin K. Petersen
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Michael Kelley, linux-hyperv, linux-kernel, linux-scsi
Cc SCSI maintainers
This patch should go via the hyperv tree because a later patch is
dependent on it. It requires and ack from SCSI maintainers though.
Thanks,
Wei.
On Wed, Jun 17, 2020 at 06:46:41PM +0200, Andrea Parri (Microsoft) wrote:
> storvsc uses the spinlock of the hv_device's primary channel to
> serialize modifications of stor_chns[] performed by storvsc_do_io()
> and storvsc_change_target_cpu(), when it could/should use a (per-)
> storvsc_device spinlock: this change untangles the synchronization
> mechanism for the (storvsc-specific) stor_chns[] array from the
> "generic" VMBus code and data structures, clarifying the scope of
> this synchronization mechanism.
>
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
> drivers/scsi/storvsc_drv.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 072ed87286578..8ff21e69a8be8 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -462,6 +462,11 @@ struct storvsc_device {
> * Mask of CPUs bound to subchannels.
> */
> struct cpumask alloced_cpus;
> + /*
> + * Serializes modifications of stor_chns[] from storvsc_do_io()
> + * and storvsc_change_target_cpu().
> + */
> + spinlock_t lock;
> /* Used for vsc/vsp channel reset process */
> struct storvsc_cmd_request init_request;
> struct storvsc_cmd_request reset_request;
> @@ -639,7 +644,7 @@ static void storvsc_change_target_cpu(struct vmbus_channel *channel, u32 old,
> return;
>
> /* See storvsc_do_io() -> get_og_chn(). */
> - spin_lock_irqsave(&device->channel->lock, flags);
> + spin_lock_irqsave(&stor_device->lock, flags);
>
> /*
> * Determines if the storvsc device has other channels assigned to
> @@ -676,7 +681,7 @@ static void storvsc_change_target_cpu(struct vmbus_channel *channel, u32 old,
> WRITE_ONCE(stor_device->stor_chns[new], channel);
> cpumask_set_cpu(new, &stor_device->alloced_cpus);
>
> - spin_unlock_irqrestore(&device->channel->lock, flags);
> + spin_unlock_irqrestore(&stor_device->lock, flags);
> }
>
> static void handle_sc_creation(struct vmbus_channel *new_sc)
> @@ -1433,14 +1438,14 @@ static int storvsc_do_io(struct hv_device *device,
> }
> }
> } else {
> - spin_lock_irqsave(&device->channel->lock, flags);
> + spin_lock_irqsave(&stor_device->lock, flags);
> outgoing_channel = stor_device->stor_chns[q_num];
> if (outgoing_channel != NULL) {
> - spin_unlock_irqrestore(&device->channel->lock, flags);
> + spin_unlock_irqrestore(&stor_device->lock, flags);
> goto found_channel;
> }
> outgoing_channel = get_og_chn(stor_device, q_num);
> - spin_unlock_irqrestore(&device->channel->lock, flags);
> + spin_unlock_irqrestore(&stor_device->lock, flags);
> }
>
> found_channel:
> @@ -1881,6 +1886,7 @@ static int storvsc_probe(struct hv_device *device,
> init_waitqueue_head(&stor_device->waiting_to_drain);
> stor_device->device = device;
> stor_device->host = host;
> + spin_lock_init(&stor_device->lock);
> hv_set_drvdata(device, stor_device);
>
> stor_device->port_number = host->host_no;
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 7/8] scsi: storvsc: Introduce the per-storvsc_device spinlock
2020-06-19 16:01 ` Wei Liu
@ 2020-06-19 16:18 ` Andrea Parri
2020-06-20 2:58 ` Martin K. Petersen
0 siblings, 1 reply; 23+ messages in thread
From: Andrea Parri @ 2020-06-19 16:18 UTC (permalink / raw)
To: Wei Liu
Cc: James E.J. Bottomley, Martin K. Petersen, K . Y . Srinivasan,
Haiyang Zhang, Stephen Hemminger, Michael Kelley, linux-hyperv,
linux-kernel, linux-scsi
On Fri, Jun 19, 2020 at 04:01:36PM +0000, Wei Liu wrote:
> Cc SCSI maintainers
>
> This patch should go via the hyperv tree because a later patch is
> dependent on it. It requires and ack from SCSI maintainers though.
Right. Sorry for the Cc: omission... ;-/
SCSI maintainers, please let me know if you prefer me to send you a new
series with this patch (7/8) and the later/dependent hyperv patch (8/8).
(1-6/8 of this series are hyperv-specific only and have been applied to
the hyperv tree, so this would only 7-8/8 of this series out.)
Thanks,
Andrea
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 7/8] scsi: storvsc: Introduce the per-storvsc_device spinlock
2020-06-19 16:18 ` Andrea Parri
@ 2020-06-20 2:58 ` Martin K. Petersen
2020-06-20 9:15 ` Wei Liu
0 siblings, 1 reply; 23+ messages in thread
From: Martin K. Petersen @ 2020-06-20 2:58 UTC (permalink / raw)
To: Andrea Parri
Cc: Wei Liu, James E.J. Bottomley, Martin K. Petersen,
K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger,
Michael Kelley, linux-hyperv, linux-kernel, linux-scsi
Andrea,
>> This patch should go via the hyperv tree because a later patch is
>> dependent on it. It requires and ack from SCSI maintainers though.
Looks OK to me.
Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 7/8] scsi: storvsc: Introduce the per-storvsc_device spinlock
2020-06-20 2:58 ` Martin K. Petersen
@ 2020-06-20 9:15 ` Wei Liu
0 siblings, 0 replies; 23+ messages in thread
From: Wei Liu @ 2020-06-20 9:15 UTC (permalink / raw)
To: Martin K. Petersen
Cc: Andrea Parri, Wei Liu, James E.J. Bottomley, K . Y . Srinivasan,
Haiyang Zhang, Stephen Hemminger, Michael Kelley, linux-hyperv,
linux-kernel, linux-scsi
On Fri, Jun 19, 2020 at 10:58:40PM -0400, Martin K. Petersen wrote:
>
> Andrea,
>
> >> This patch should go via the hyperv tree because a later patch is
> >> dependent on it. It requires and ack from SCSI maintainers though.
>
> Looks OK to me.
>
> Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
Thanks Martin.
>
> --
> Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 8/8] Drivers: hv: vmbus: Remove the lock field from the vmbus_channel struct
2020-06-17 16:46 [PATCH 0/8] Drivers: hv: vmbus: Miscellaneous cleanups Andrea Parri (Microsoft)
` (6 preceding siblings ...)
2020-06-17 16:46 ` [PATCH 7/8] scsi: storvsc: Introduce the per-storvsc_device spinlock Andrea Parri (Microsoft)
@ 2020-06-17 16:46 ` Andrea Parri (Microsoft)
2020-06-18 18:35 ` Michael Kelley
2020-06-19 15:39 ` [PATCH 0/8] Drivers: hv: vmbus: Miscellaneous cleanups Wei Liu
8 siblings, 1 reply; 23+ messages in thread
From: Andrea Parri (Microsoft) @ 2020-06-17 16:46 UTC (permalink / raw)
To: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Michael Kelley
Cc: linux-hyperv, linux-kernel, Andrea Parri (Microsoft)
The spinlock is (now) *not used to protect test-and-set accesses
to attributes of the structure or sc_list operations.
There is, AFAICT, a distinct lack of {WRITE,READ}_ONCE()s in the
handling of channel->state, but the changes below do not seem to
make things "worse". ;-)
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
drivers/hv/channel.c | 6 +-----
drivers/hv/channel_mgmt.c | 1 -
include/linux/hyperv.h | 6 ------
3 files changed, 1 insertion(+), 12 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 8848d1548b3f2..3ebda7707e46a 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -129,12 +129,8 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
send_pages = newchannel->ringbuffer_send_offset;
recv_pages = newchannel->ringbuffer_pagecount - send_pages;
- spin_lock_irqsave(&newchannel->lock, flags);
- if (newchannel->state != CHANNEL_OPEN_STATE) {
- spin_unlock_irqrestore(&newchannel->lock, flags);
+ if (newchannel->state != CHANNEL_OPEN_STATE)
return -EINVAL;
- }
- spin_unlock_irqrestore(&newchannel->lock, flags);
newchannel->state = CHANNEL_OPENING_STATE;
newchannel->onchannel_callback = onchannelcallback;
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 92f8bb2077a94..591106cf58fc0 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -317,7 +317,6 @@ static struct vmbus_channel *alloc_channel(void)
return NULL;
spin_lock_init(&channel->sched_lock);
- spin_lock_init(&channel->lock);
init_completion(&channel->rescind_event);
INIT_LIST_HEAD(&channel->sc_list);
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 690394b79d727..38100e80360ac 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -840,12 +840,6 @@ struct vmbus_channel {
*/
void (*chn_rescind_callback)(struct vmbus_channel *channel);
- /*
- * The spinlock to protect the structure. It is being used to protect
- * test-and-set access to various attributes of the structure as well
- * as all sc_list operations.
- */
- spinlock_t lock;
/*
* All Sub-channels of a primary channel are linked here.
*/
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* RE: [PATCH 8/8] Drivers: hv: vmbus: Remove the lock field from the vmbus_channel struct
2020-06-17 16:46 ` [PATCH 8/8] Drivers: hv: vmbus: Remove the lock field from the vmbus_channel struct Andrea Parri (Microsoft)
@ 2020-06-18 18:35 ` Michael Kelley
0 siblings, 0 replies; 23+ messages in thread
From: Michael Kelley @ 2020-06-18 18:35 UTC (permalink / raw)
To: Andrea Parri (Microsoft),
KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu
Cc: linux-hyperv, linux-kernel
From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, June 17, 2020 9:47 AM
>
> The spinlock is (now) *not used to protect test-and-set accesses
> to attributes of the structure or sc_list operations.
>
> There is, AFAICT, a distinct lack of {WRITE,READ}_ONCE()s in the
> handling of channel->state, but the changes below do not seem to
> make things "worse". ;-)
>
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
> drivers/hv/channel.c | 6 +-----
> drivers/hv/channel_mgmt.c | 1 -
> include/linux/hyperv.h | 6 ------
> 3 files changed, 1 insertion(+), 12 deletions(-)
>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/8] Drivers: hv: vmbus: Miscellaneous cleanups
2020-06-17 16:46 [PATCH 0/8] Drivers: hv: vmbus: Miscellaneous cleanups Andrea Parri (Microsoft)
` (7 preceding siblings ...)
2020-06-17 16:46 ` [PATCH 8/8] Drivers: hv: vmbus: Remove the lock field from the vmbus_channel struct Andrea Parri (Microsoft)
@ 2020-06-19 15:39 ` Wei Liu
2020-06-19 15:56 ` Wei Liu
8 siblings, 1 reply; 23+ messages in thread
From: Wei Liu @ 2020-06-19 15:39 UTC (permalink / raw)
To: Andrea Parri (Microsoft)
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Michael Kelley, linux-hyperv, linux-kernel
On Wed, Jun 17, 2020 at 06:46:34PM +0200, Andrea Parri (Microsoft) wrote:
> Hi all,
>
> I went back to my "cleanup list" recently and I wrote these patches:
> here you can find, among other things,
>
> 1) the removal of the fields 'target_vp' and 'numa_node' from the
> channel data structure, as suggested by Michael back in May;
>
> 2) various cleanups for channel->lock, which is actually *removed
> by the end of this series! ;-)
>
> I'm sure there is room for further "cleanups", ;-) but let me check
> if these (relatively small) changes make sense first...
>
> Thanks,
> Andrea
>
> Andrea Parri (Microsoft) (8):
> Drivers: hv: vmbus: Remove the target_vp field from the vmbus_channel
> struct
> Drivers: hv: vmbus: Remove the numa_node field from the vmbus_channel
> struct
> Drivers: hv: vmbus: Replace cpumask_test_cpu(, cpu_online_mask) with
> cpu_online()
> Drivers: hv: vmbus: Remove unnecessary channel->lock critical sections
> (sc_list readers)
> Drivers: hv: vmbus: Use channel_mutex in channel_vp_mapping_show()
> Drivers: hv: vmbus: Remove unnecessary channel->lock critical sections
> (sc_list updaters)
> scsi: storvsc: Introduce the per-storvsc_device spinlock
> Drivers: hv: vmbus: Remove the lock field from the vmbus_channel
> struct
I've queued these up to hyperv-next except for the scsi patch.
Thanks,
Wei.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/8] Drivers: hv: vmbus: Miscellaneous cleanups
2020-06-19 15:39 ` [PATCH 0/8] Drivers: hv: vmbus: Miscellaneous cleanups Wei Liu
@ 2020-06-19 15:56 ` Wei Liu
0 siblings, 0 replies; 23+ messages in thread
From: Wei Liu @ 2020-06-19 15:56 UTC (permalink / raw)
To: Andrea Parri (Microsoft)
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Michael Kelley, linux-hyperv, linux-kernel
On Fri, Jun 19, 2020 at 03:39:54PM +0000, Wei Liu wrote:
> On Wed, Jun 17, 2020 at 06:46:34PM +0200, Andrea Parri (Microsoft) wrote:
> > Hi all,
> >
> > I went back to my "cleanup list" recently and I wrote these patches:
> > here you can find, among other things,
> >
> > 1) the removal of the fields 'target_vp' and 'numa_node' from the
> > channel data structure, as suggested by Michael back in May;
> >
> > 2) various cleanups for channel->lock, which is actually *removed
> > by the end of this series! ;-)
> >
> > I'm sure there is room for further "cleanups", ;-) but let me check
> > if these (relatively small) changes make sense first...
> >
> > Thanks,
> > Andrea
> >
> > Andrea Parri (Microsoft) (8):
> > Drivers: hv: vmbus: Remove the target_vp field from the vmbus_channel
> > struct
> > Drivers: hv: vmbus: Remove the numa_node field from the vmbus_channel
> > struct
> > Drivers: hv: vmbus: Replace cpumask_test_cpu(, cpu_online_mask) with
> > cpu_online()
> > Drivers: hv: vmbus: Remove unnecessary channel->lock critical sections
> > (sc_list readers)
> > Drivers: hv: vmbus: Use channel_mutex in channel_vp_mapping_show()
> > Drivers: hv: vmbus: Remove unnecessary channel->lock critical sections
> > (sc_list updaters)
> > scsi: storvsc: Introduce the per-storvsc_device spinlock
> > Drivers: hv: vmbus: Remove the lock field from the vmbus_channel
> > struct
>
> I've queued these up to hyperv-next except for the scsi patch.
>
Andrea just pointed out that the last patch can't build without the scsi
patch, so I've only applied only the first 6 patches for now.
Wei.
> Thanks,
> Wei.
^ permalink raw reply [flat|nested] 23+ messages in thread