Drivers: hv: vmbus: Resolve more races involving init_vp_index()

init_vp_index() uses the (per-node) hv_numa_map[] masks to record the CPUs allocated for channel interrupts at a given time, and distribute the performance-critical channels across the available CPUs: in part., the mask of "candidate" target CPUs in a given NUMA node, for a newly offered channel, is determined by XOR-ing the node's CPU mask and the node's hv_numa_map. This operation/mechanism assumes that no offline CPUs is set in the hv_numa_map mask, an assumption that does not hold since such mask is currently not updated when a channel is removed or assigned to a different CPU. To address the issues described above, this adds hooks in the channel removal path (hv_process_channel_removal()) and in target_cpu_store() in order to clear, resp. to update, the hv_numa_map[] masks as needed. This also adds a (missed) update of the masks in init_vp_index() (cf., e.g., the memory-allocation failure path in this function). Like in the case of init_vp_index(), such hooks require to determine if the given channel is performance critical. init_vp_index() does this by parsing the channel's offer, it can not rely on the device data structure (device_obj) to retrieve such information because the device data structure has not been allocated/linked with the channel by the time that init_vp_index() executes. A similar situation may hold in hv_is_alloced_cpu() (defined below); the adopted approach is to "cache" the device type of the channel, as computed by parsing the channel's offer, in the channel structure itself.

Andrea Parri (Microsoft) committed 6 years ago

Commit afaa33d1 parent a949e864 files · +84 −12Browse files

Unified · Split

4 files

drivers/hv/channel_mgmt.c +16 −6
drivers/hv/hyperv_vmbus.h +48 −0
drivers/hv/vmbus_drv.c +13 −6
include/linux/hyperv.h +7 −0

drivers/hv/channel_mgmt.c+16−6

 #include "hyperv_vmbus.h"
-static void init_vp_index(struct vmbus_channel *channel, u16 dev_type);
+static void init_vp_index(struct vmbus_channel *channel);
-static const struct vmbus_device vmbus_devs[] = {
+const struct vmbus_device vmbus_devs[] = {
 	/* IDE */
 	{ .dev_type = HV_IDE,
 	  HV_IDE_GUID,
 		spin_unlock_irqrestore(&primary_channel->lock, flags);
 	}
+	/*
+	 * If this is a "perf" channel, updates the hv_numa_map[] masks so that
+	 * init_vp_index() can (re-)use the CPU.
+	 */
+	if (hv_is_perf_channel(channel))
+		hv_clear_alloced_cpu(channel->target_cpu);
 	/*
 	 * Upon suspend, an in-use hv_sock channel is marked as "rescinded" and
 	 * the relid is invalidated; after hibernation, when the user-space app
 	if (!newchannel->device_obj)
 		goto err_deq_chan;
-	newchannel->device_obj->device_id = hv_get_dev_type(newchannel);
+	newchannel->device_obj->device_id = newchannel->device_id;
 	/*
 	 * Add the new device to the bus. This will kick off device-driver
 	 * binding which eventually invokes the device driver's AddDevice()
 	 */
 	mutex_lock(&vmbus_connection.channel_mutex);
-	init_vp_index(newchannel, hv_get_dev_type(newchannel));
+	init_vp_index(newchannel);
 	/* Remember the channels that should be cleaned up upon suspend. */
 	if (is_hvsock_channel(newchannel) || is_sub_channel(newchannel))
  * evenly among all the available NUMA nodes.  Once the node is assigned,
  * we will assign the CPU based on a simple round robin scheme.
  */
-static void init_vp_index(struct vmbus_channel *channel, u16 dev_type)
+static void init_vp_index(struct vmbus_channel *channel)
 {
-	bool perf_chn = vmbus_devs[dev_type].perf_device;
+	bool perf_chn = hv_is_perf_channel(channel);
 	cpumask_var_t available_mask;
 	struct cpumask *alloced_mask;
 	u32 target_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;
 	}
 	       sizeof(struct vmbus_channel_offer_channel));
 	channel->monitor_grp = (u8)offer->monitorid / 32;
 	channel->monitor_bit = (u8)offer->monitorid % 32;
+	channel->device_id = hv_get_dev_type(channel);
 }
 /*

drivers/hv/hyperv_vmbus.h+48−0

@@ -395,6 +395,54 @@ enum delay { · 394 unmodified lines
395	MESSAGE_DELAY = 1,	395	MESSAGE_DELAY = 1,
396	};	396	};
397		397
		398	extern const struct vmbus_device vmbus_devs[];
		399
		400	static inline bool hv_is_perf_channel(struct vmbus_channel *channel)
		401	{
		402	return vmbus_devs[channel->device_id].perf_device;
		403	}
		404
		405	static inline bool hv_is_alloced_cpu(unsigned int cpu)
		406	{
		407	struct vmbus_channel channel, sc;
		408
		409	lockdep_assert_held(&vmbus_connection.channel_mutex);
		410	/*
		411	* List additions/deletions as well as updates of the target CPUs are
		412	* protected by channel_mutex.
		413	*/
		414	list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) {
		415	if (!hv_is_perf_channel(channel))
		416	continue;
		417	if (channel->target_cpu == cpu)
		418	return true;
		419	list_for_each_entry(sc, &channel->sc_list, sc_list) {
		420	if (sc->target_cpu == cpu)
		421	return true;
		422	}
		423	}
		424	return false;
		425	}
		426
		427	static inline void hv_set_alloced_cpu(unsigned int cpu)
		428	{
		429	cpumask_set_cpu(cpu, &hv_context.hv_numa_map[cpu_to_node(cpu)]);
		430	}
		431
		432	static inline void hv_clear_alloced_cpu(unsigned int cpu)
		433	{
		434	if (hv_is_alloced_cpu(cpu))
		435	return;
		436	cpumask_clear_cpu(cpu, &hv_context.hv_numa_map[cpu_to_node(cpu)]);
		437	}
		438
		439	static inline void hv_update_alloced_cpus(unsigned int old_cpu,
		440	unsigned int new_cpu)
		441	{
		442	hv_set_alloced_cpu(new_cpu);
		443	hv_clear_alloced_cpu(old_cpu);
		444	}
		445
398	#ifdef CONFIG_HYPERV_TESTING	446	#ifdef CONFIG_HYPERV_TESTING
399		447
400	int hv_debug_add_dev_dir(struct hv_device *dev);	448	int hv_debug_add_dev_dir(struct hv_device *dev);

drivers/hv/vmbus_drv.c+13−6

 static ssize_t target_cpu_store(struct vmbus_channel *channel,
 				const char *buf, size_t count)
 {
+	u32 target_cpu, origin_cpu;
 	ssize_t ret = count;
-	u32 target_cpu;
 	if (vmbus_proto_version < VERSION_WIN10_V4_1)
 		return -EIO;
 		goto cpu_store_unlock;
 	}
-	if (channel->target_cpu == target_cpu)
+	origin_cpu = channel->target_cpu;
+	if (target_cpu == origin_cpu)
 		goto cpu_store_unlock;
 	if (vmbus_send_modifychannel(channel->offermsg.child_relid,
 	 * in on a CPU that is different from the channel target_cpu value.
 	 */
-	if (channel->change_target_cpu_callback)
-		(*channel->change_target_cpu_callback)(channel,
-				channel->target_cpu, target_cpu);
 	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(). */
+	if (hv_is_perf_channel(channel))
+		hv_update_alloced_cpus(origin_cpu, target_cpu);
+	/* Currently set only for storvsc channels. */
+	if (channel->change_target_cpu_callback) {
+		(*channel->change_target_cpu_callback)(channel,
+				origin_cpu, target_cpu);
+	}
 cpu_store_unlock:
 	mutex_unlock(&vmbus_connection.channel_mutex);
 	cpus_read_unlock();

include/linux/hyperv.h+7−0

@@ -901,6 +901,13 @@ struct vmbus_channel { · 900 unmodified lines
901		901
902	bool probe_done;	902	bool probe_done;
903		903
		904	/*
		905	* Cache the device ID here for easy access; this is useful, in
		906	* particular, in situations where the channel's device_obj has
		907	* not been allocated/initialized yet.
		908	*/
		909	u16 device_id;
		910
904	/*	911	/*
905	* We must offload the handling of the primary/sub channels	912	* We must offload the handling of the primary/sub channels
906	* from the single-threaded vmbus_connection.work_queue to	913	* from the single-threaded vmbus_connection.work_queue to