Drivers: hv: vmbus: Miscellaneous cleanup
cover Drivers: hv: vmbus: Miscellaneous cleanup
In addition to fixing an issue in the vmbus_open failure path, this patch-set improves the distribution of channels/sub-channels across the available CPUs in the guest.
Vitaly Kuznetsov (7):
Drivers: hv: vmbus: do cleanup on all vmbus_open() failure paths
Drivers: hv: vmbus: kill tasklets on module unload
Drivers: hv: vmbus: unify calls to percpu_channel_enq()
Drivers: hv: vmbus: briefly comment num_sc and next_oc
Drivers: hv: vmbus: decrease num_sc on subchannel removal
Drivers: hv: vmbus: move init_vp_index() call to
vmbus_process_offer()
Drivers: hv: vmbus: distribute subchannels among all vcpus
drivers/hv/channel.c | 13 +++---- drivers/hv/channel_mgmt.c | 88 ++++++++++++++++++++++++++------------------ drivers/hv/vmbus_drv.c | 5 ++- include/linux/hyperv.h | 12 +++++-- 4 files changed, 71 insertions(+), 47 deletions(-)
Signature · 1 line
1.7.4.1
PATCH 1/7 Drivers: hv: vmbus: do cleanup on all vmbus_open() failure paths
From: Vitaly Kuznetsov <[email protected]>
In case there was an error reported in the response to the CHANNELMSG_OPENCHANNEL call we need to do the cleanup as a vmbus_open() user won't be doing it after receiving an error. The cleanup should be done on all failure paths. We also need to avoid returning open_info->response.open_result.status as the return value as all other errors we return from vmbus_open() are -EXXX and vmbus_open() callers are not supposed to analyze host error codes.
--- drivers/hv/channel.c | 13 ++++++------- 1 files changed, 6 insertions(+), 7 deletions(-)
drivers/hv/channel.c +6 -7
@@ -178,19 +178,18 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 send_ringbuffer_size, goto error1; } - -if (open_info->response.open_result.status) - err = open_info->response.open_result.status; -spin_lock_irqsave (&vmbus_connection.channelmsg_lock, flags);list_del (&open_info->msglistentry);spin_unlock_irqrestore (&vmbus_connection.channelmsg_lock, flags); -if (err ==0 ) - newchannel->state =CHANNEL_OPENED_STATE ; +if (open_info->response.open_result.status) { + err = -EAGAIN ; + goto error_gpadl; + } + newchannel->state =CHANNEL_OPENED_STATE ;kfree (open_info); -return err; +return 0 ; error1:spin_lock_irqsave (&vmbus_connection.channelmsg_lock, flags);
Signature · 1 line
1.7.4.1
PATCH 2/7 Drivers: hv: vmbus: kill tasklets on module unload
From: Vitaly Kuznetsov <[email protected]>
Explicitly kill tasklets we create on module unload.
--- drivers/hv/vmbus_drv.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
drivers/hv/vmbus_drv.c +4 -1
@@ -1108,6 +1108,7 @@ static void __exit vmbus_exit(void)hv_synic_clockevents_cleanup ();vmbus_disconnect ();hv_remove_vmbus_irq (); +tasklet_kill (&msg_dpc);vmbus_free_channels ();if (ms_hyperv.features &HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE ) { atomic_notifier_chain_unregister(&panic_notifier_list,
@@ -1115,8 +1116,10 @@ static void __exit vmbus_exit(void) }bus_unregister (&hv_bus);hv_cleanup (); -for_each_online_cpu (cpu) +for_each_online_cpu (cpu) { +tasklet_kill (hv_context.event_dpc[cpu]);smp_call_function_single (cpu, hv_synic_cleanup,NULL ,1 ); + }acpi_bus_unregister_driver (&vmbus_acpi_driver);hv_cpu_hotplug_quirk (false); }
Signature · 1 line
1.7.4.1
PATCH 3/7 Drivers: hv: vmbus: unify calls to percpu_channel_enq()
In reply to K. Y. SrinivasanFrom: Vitaly Kuznetsov <[email protected]>
Remove some code duplication, no functional change intended.
--- drivers/hv/channel_mgmt.c | 51 +++++++++++++++----------------------------- 1 files changed, 18 insertions(+), 33 deletions(-)
drivers/hv/channel_mgmt.c +18 -33
@@ -233,7 +233,6 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel) {struct vmbus_channel *channel;bool fnew = true; -bool enq = false;unsigned long flags;/* Make sure this is a new offer */
@@ -249,25 +248,12 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel) } } -if (fnew) { +if (fnew)list_add_tail (&newchannel->listentry, &vmbus_connection.chn_list); - enq = true; - }spin_unlock_irqrestore (&vmbus_connection.channel_lock, flags); -if (enq) { -if (newchannel->target_cpu !=get_cpu ()) { -put_cpu (); -smp_call_function_single (newchannel->target_cpu, - percpu_channel_enq, - newchannel, true); - }else { -percpu_channel_enq (newchannel); -put_cpu (); - } - }if (!fnew) {/* * Check to see if this is a sub-channel.
@@ -280,26 +266,19 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)spin_lock_irqsave (&channel->lock, flags);list_add_tail (&newchannel->sc_list, &channel->sc_list);spin_unlock_irqrestore (&channel->lock, flags); - -if (newchannel->target_cpu !=get_cpu ()) { -put_cpu (); -smp_call_function_single (newchannel->target_cpu, - percpu_channel_enq, - newchannel, true); - }else { -percpu_channel_enq (newchannel); -put_cpu (); - } - - newchannel->state =CHANNEL_OPEN_STATE ; channel->num_sc++; -if (channel->sc_creation_callback !=NULL ) - channel->sc_creation_callback (newchannel); - -return ; - } + }else + goto err_free_chan; + } - goto err_free_chan; +if (newchannel->target_cpu !=get_cpu ()) { +put_cpu (); +smp_call_function_single (newchannel->target_cpu, + percpu_channel_enq, + newchannel, true); + }else { +percpu_channel_enq (newchannel); +put_cpu (); }/*
@@ -309,6 +288,12 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)*/ newchannel->state =CHANNEL_OPEN_STATE ; +if (!fnew) { +if (channel->sc_creation_callback !=NULL ) + channel->sc_creation_callback (newchannel); +return ; + } +/* * Start the process of binding this offer to the driver * We need to set the DeviceObject field before calling
Signature · 1 line
1.7.4.1
PATCH 4/7 Drivers: hv: vmbus: briefly comment num_sc and next_oc
In reply to K. Y. SrinivasanFrom: Vitaly Kuznetsov <[mailto:[email protected]]>
next_oc and num_sc fields of struct vmbus_channel deserve a description. Move them closer to sc_list as these fields are related to it.
--- include/linux/hyperv.h | 12 +++++++++--- 1 files changed, 9 insertions(+), 3 deletions(-)
include/linux/hyperv.h +9 -3
@@ -727,6 +727,15 @@ struct vmbus_channel {*/ 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. */
@@ -740,9 +749,6 @@ struct vmbus_channel {* link up channels based on their CPU affinity. */ struct list_head percpu_list; - -int num_sc; -int next_oc; };static inline void set_channel_read_state (struct vmbus_channel *c,bool state)
Signature · 1 line
1.7.4.1
PATCH 5/7 Drivers: hv: vmbus: decrease num_sc on subchannel removal
In reply to K. Y. SrinivasanFrom: Vitaly Kuznetsov <[email protected]>
It is unlikely that that host will ask us to close only one subchannel for a device but let's be consistent. Do both num_sc++ and num_sc-- with channel->lock to be on the safe side.
--- drivers/hv/channel_mgmt.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
drivers/hv/channel_mgmt.c +2 -1
@@ -205,6 +205,7 @@ void hv_process_channel_removal(struct vmbus_channel *channel, u32 relid) 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); }free_channel (channel);
@@ -265,8 +266,8 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel) 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); channel->num_sc++; +spin_unlock_irqrestore (&channel->lock, flags); }else goto err_free_chan; }
Signature · 1 line
1.7.4.1
PATCH 6/7 Drivers: hv: vmbus: move init_vp_index() call to vmbus_process_offer()
In reply to K. Y. SrinivasanFrom: Vitaly Kuznetsov <[email protected]>
We need to call init_vp_index() after we added the channel to the appropriate list (global or subchannel) to be able to use this information when assigning the channel to the particular vcpu. To do so we need to move a couple of functions around. The only real change is the init_vp_index() call. This is a small refactoring without a functional change.
--- drivers/hv/channel_mgmt.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-)
drivers/hv/channel_mgmt.c +5 -2
@@ -32,6 +32,9 @@#include "hyperv_vmbus.h" +static void init_vp_index (struct vmbus_channel *channel, +const uuid_le *type_guid); +/** * vmbus_prep_negotiate_resp() - Create default response for Hyper-V Negotiate message * @icmsghdrp: Pointer to msg header structure
@@ -272,6 +275,8 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel) goto err_free_chan; } +init_vp_index (newchannel, &newchannel->offermsg.offer.if_type); +if (newchannel->target_cpu !=get_cpu ()) {put_cpu (); smp_call_function_single(newchannel->target_cpu,
@@ -476,8 +481,6 @@ static void vmbus_onoffer(struct vmbus_channel_message_header *hdr) offer->connection_id; } -init_vp_index (newchannel, &offer->offer.if_type); -memcpy (&newchannel->offermsg, offer,sizeof (struct vmbus_channel_offer_channel )); newchannel->monitor_grp = (u8 )offer->monitorid /32 ;
Signature · 1 line
1.7.4.1
PATCH 7/7 Drivers: hv: vmbus: distribute subchannels among all vcpus
In reply to K. Y. SrinivasanFrom: Vitaly Kuznetsov <[email protected]>
Primary channels are distributed evenly across all vcpus we have. When the host asks us to create subchannels it usually makes us num_cpus-1 offers and we are supposed to distribute the work evenly among the channel itself and all its subchannels. Make sure they are all assigned to different vcpus.
--- drivers/hv/channel_mgmt.c | 29 ++++++++++++++++++++++++++++- 1 files changed, 28 insertions(+), 1 deletions(-)
drivers/hv/channel_mgmt.c +28 -1
@@ -387,6 +387,8 @@ static void init_vp_index(struct vmbus_channel *channel, const uuid_le *type_guiint i;bool perf_chn = false;u32 max_cpus =num_online_cpus (); +struct vmbus_channel *primary = channel->primary_channel, *prev; +unsigned long flags; for (i = IDE; i < MAX_PERF_CHN; i++) { if (!memcmp(type_guid->b, hp_devs[i].guid,
@@ -407,7 +409,32 @@ static void init_vp_index(struct vmbus_channel *channel, const uuid_le *type_gui channel->target_vp =0 ;return ; } - cur_cpu = (++next_vp % max_cpus); + +/* +* Primary channels are distributed evenly across all vcpus we have. +* When the host asks us to create subchannels it usually makes us +* num_cpus-1 offers and we are supposed to distribute the work evenly +* among the channel itself and all its subchannels. Make sure they are +* all assigned to different vcpus. +*/ +if (!primary) + cur_cpu = (++next_vp % max_cpus); +else { +/* +* Let's assign the first subchannel of a channel to the +* primary->target_cpu+1 and all the subsequent channels to +* the prev->target_cpu+1. +*/ +spin_lock_irqsave (&primary->lock, flags); +if (primary->num_sc ==1 ) + cur_cpu = (primary->target_cpu +1 ) % max_cpus; +else { + prev =list_prev_entry (channel, sc_list); + cur_cpu = (prev->target_cpu +1 ) % max_cpus; + } +spin_unlock_irqrestore (&primary->lock, flags); + } + channel->target_cpu = cur_cpu; channel->target_vp = hv_context.vp_index[cur_cpu]; }
Signature · 1 line
1.7.4.1