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
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) <[email protected]>
---
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
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) <[email protected]>
---
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
From: Andrea Parri (Microsoft) <[email protected]> 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) <[email protected]>
> ---
> 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 <[email protected]>
From: Andrea Parri (Microsoft) <[email protected]> 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) <[email protected]>
> ---
> 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 <[email protected]>
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.
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.