[PATCH 0/8] Drivers: hv: vmbus: Miscellaneous cleanups

[PATCH 0/8] Drivers: hv: vmbus: Miscellaneous cleanups

[Andrea Parri (Microsoft)]

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

 drivers/hv/channel.c       |  9 +++------
 drivers/hv/channel_mgmt.c  | 31 ++++++-------------------------
 drivers/hv/hv.c            |  3 ---
 drivers/hv/vmbus_drv.c     | 17 +++++------------
 drivers/scsi/storvsc_drv.c | 16 +++++++++++-----
 include/linux/hyperv.h     | 22 +++++++---------------
 6 files changed, 32 insertions(+), 66 deletions(-)

-- 
2.25.1

[PATCH 1/8] Drivers: hv: vmbus: Remove the target_vp field from the vmbus_channel struct

[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

RE: [PATCH 1/8] Drivers: hv: vmbus: Remove the target_vp field from the vmbus_channel struct

[Michael Kelley]

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>

[PATCH 2/8] Drivers: hv: vmbus: Remove the numa_node field from the vmbus_channel struct

[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

RE: [PATCH 2/8] Drivers: hv: vmbus: Remove the numa_node field from the vmbus_channel struct

[Michael Kelley]

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>

[PATCH 3/8] Drivers: hv: vmbus: Replace cpumask_test_cpu(, cpu_online_mask) with cpu_online()

[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

RE: [PATCH 3/8] Drivers: hv: vmbus: Replace cpumask_test_cpu(, cpu_online_mask) with cpu_online()

[Michael Kelley]

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>

[PATCH 4/8] Drivers: hv: vmbus: Remove unnecessary channel->lock critical sections (sc_list readers)

[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

RE: [PATCH 4/8] Drivers: hv: vmbus: Remove unnecessary channel->lock critical sections (sc_list readers)

[Michael Kelley]

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>

[PATCH 5/8] Drivers: hv: vmbus: Use channel_mutex in channel_vp_mapping_show()

[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

RE: [PATCH 5/8] Drivers: hv: vmbus: Use channel_mutex in channel_vp_mapping_show()

[Michael Kelley]

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>

[PATCH 6/8] Drivers: hv: vmbus: Remove unnecessary channel->lock critical sections (sc_list updaters)

[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

RE: [PATCH 6/8] Drivers: hv: vmbus: Remove unnecessary channel->lock critical sections (sc_list updaters)

[Michael Kelley]

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>

[PATCH 7/8] scsi: storvsc: Introduce the per-storvsc_device spinlock

[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

RE: [PATCH 7/8] scsi: storvsc: Introduce the per-storvsc_device spinlock

[Michael Kelley]

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>

Re: [PATCH 7/8] scsi: storvsc: Introduce the per-storvsc_device spinlock

[Wei Liu]

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
> 

Re: [PATCH 7/8] scsi: storvsc: Introduce the per-storvsc_device spinlock

[Andrea Parri]

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

Re: [PATCH 7/8] scsi: storvsc: Introduce the per-storvsc_device spinlock

[Martin K. Petersen]

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

Re: [PATCH 7/8] scsi: storvsc: Introduce the per-storvsc_device spinlock

[Wei Liu]

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

[PATCH 8/8] Drivers: hv: vmbus: Remove the lock field from the vmbus_channel struct

[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

RE: [PATCH 8/8] Drivers: hv: vmbus: Remove the lock field from the vmbus_channel struct

[Michael Kelley]

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>

Re: [PATCH 0/8] Drivers: hv: vmbus: Miscellaneous cleanups

[Wei Liu]

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.

Re: [PATCH 0/8] Drivers: hv: vmbus: Miscellaneous cleanups

[Wei Liu]

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.