[PATCH V3,net-next, 0/3] Add vmbus dev_num and allow netvsc probe options

[PATCH V3,net-next, 0/3] Add vmbus dev_num and allow netvsc probe options

[Haiyang Zhang]

Add dev_num for vmbus device based on channel offer sequence.
User programs can use this number for nic naming.
Async probing option is allowed for netvsc. Sync probing is still
the default.

Haiyang Zhang (3):
  Drivers: hv: vmbus: Add a dev_num variable based on channel offer sequence
  Drivers: hv: vmbus: Add dev_num to sysfs
  hv_netvsc: Set probe_type to PROBE_DEFAULT_STRATEGY

 Documentation/ABI/stable/sysfs-bus-vmbus |  8 ++++++
 drivers/hv/channel_mgmt.c                | 46 ++++++++++++++++++++++++++++++--
 drivers/hv/vmbus_drv.c                   | 13 +++++++++
 drivers/net/hyperv/netvsc_drv.c          |  2 +-
 include/linux/hyperv.h                   |  6 +++++
 5 files changed, 72 insertions(+), 3 deletions(-)

-- 
1.8.3.1

[PATCH V3,net-next, 1/3] Drivers: hv: vmbus: Add a dev_num variable based on channel offer sequence

[Haiyang Zhang]

This number is set to the first available number, starting from zero,
when a VMBus device’s primary channel is offered.
It will be used for stable naming when Async probing is used.

Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
Changes
V3:
 Rename a local var, add code comments.

V2:
 Use nest loops in hv_set_devnum, instead of goto.
---
 drivers/hv/channel_mgmt.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
 include/linux/hyperv.h    |  6 ++++++
 2 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 8eb1675..68adfa1 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -315,6 +315,8 @@ static struct vmbus_channel *alloc_channel(void)
 	if (!channel)
 		return NULL;
 
+	channel->dev_num = HV_DEV_NUM_INVALID;
+
 	spin_lock_init(&channel->lock);
 	init_completion(&channel->rescind_event);
 
@@ -541,6 +543,44 @@ static void vmbus_add_channel_work(struct work_struct *work)
 }
 
 /*
+ * Get the first available device number of its type, then
+ * record it in the channel structure.
+ */
+static void hv_set_devnum(struct vmbus_channel *newchannel)
+{
+	struct vmbus_channel *channel;
+	int i = -1;
+	bool in_use;
+
+	BUG_ON(!mutex_is_locked(&vmbus_connection.channel_mutex));
+
+	/*
+	 * Iterate through each possible device number starting at zero.
+	 * If the device number is already in use for a device of this type,
+	 * try the next device number until finding one that is not in use.
+	 * This approach selects the smallest device number that is not in
+	 * use, and so reuses any numbers that are freed by devices that
+	 * have been removed.
+	 */
+	do {
+		i++;
+		in_use = false;
+
+		list_for_each_entry(channel, &vmbus_connection.chn_list,
+				    listentry) {
+			if (i == channel->dev_num &&
+			    guid_equal(&channel->offermsg.offer.if_type,
+				       &newchannel->offermsg.offer.if_type)) {
+				in_use = true;
+				break;
+			}
+		}
+	} while (in_use);
+
+	newchannel->dev_num = i;
+}
+
+/*
  * vmbus_process_offer - Process the offer by creating a channel/device
  * associated with this offer
  */
@@ -573,10 +613,12 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
 		}
 	}
 
-	if (fnew)
+	if (fnew) {
+		hv_set_devnum(newchannel);
+
 		list_add_tail(&newchannel->listentry,
 			      &vmbus_connection.chn_list);
-	else {
+	} else {
 		/*
 		 * Check to see if this is a valid sub-channel.
 		 */
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 26f3aee..4f110c5 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -718,6 +718,8 @@ struct vmbus_device {
 	bool perf_device;
 };
 
+#define HV_DEV_NUM_INVALID (-1)
+
 struct vmbus_channel {
 	struct list_head listentry;
 
@@ -849,6 +851,10 @@ struct vmbus_channel {
 	 */
 	struct vmbus_channel *primary_channel;
 	/*
+	 * Used for device naming based on channel offer sequence.
+	 */
+	int dev_num;
+	/*
 	 * Support per-channel state for use by vmbus drivers.
 	 */
 	void *per_channel_state;
-- 
1.8.3.1

[PATCH V3,net-next, 2/3] Drivers: hv: vmbus: Add dev_num to sysfs

[Haiyang Zhang]

It's a number based on the VMBus device offer sequence.
Useful for device naming when using Async probing.

Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>

---
Changes
V3:
 Use "VMBus" instead of vmbus in text.
---
 Documentation/ABI/stable/sysfs-bus-vmbus |  8 ++++++++
 drivers/hv/vmbus_drv.c                   | 13 +++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/Documentation/ABI/stable/sysfs-bus-vmbus b/Documentation/ABI/stable/sysfs-bus-vmbus
index 8e8d167..4c4dc86 100644
--- a/Documentation/ABI/stable/sysfs-bus-vmbus
+++ b/Documentation/ABI/stable/sysfs-bus-vmbus
@@ -49,6 +49,14 @@ Contact:	Stephen Hemminger <sthemmin@microsoft.com>
 Description:	This NUMA node to which the VMBUS device is
 		attached, or -1 if the node is unknown.
 
+What:           /sys/bus/vmbus/devices/<UUID>/dev_num
+Date:		Dec 2019
+KernelVersion:	5.5
+Contact:	Haiyang Zhang <haiyangz@microsoft.com>
+Description:	A number based on the VMBus device offer sequence.
+		Useful for device naming when using Async probing.
+Users:		Debugging tools and userspace drivers
+
 What:		/sys/bus/vmbus/devices/<UUID>/channels/<N>
 Date:		September. 2017
 KernelVersion:	4.14
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 4ef5a66..fe7aefa 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -214,6 +214,18 @@ static ssize_t numa_node_show(struct device *dev,
 static DEVICE_ATTR_RO(numa_node);
 #endif
 
+static ssize_t dev_num_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	struct hv_device *hv_dev = device_to_hv_device(dev);
+
+	if (!hv_dev->channel)
+		return -ENODEV;
+
+	return sprintf(buf, "%d\n", hv_dev->channel->dev_num);
+}
+static DEVICE_ATTR_RO(dev_num);
+
 static ssize_t server_monitor_pending_show(struct device *dev,
 					   struct device_attribute *dev_attr,
 					   char *buf)
@@ -598,6 +610,7 @@ static ssize_t driver_override_show(struct device *dev,
 #ifdef CONFIG_NUMA
 	&dev_attr_numa_node.attr,
 #endif
+	&dev_attr_dev_num.attr,
 	&dev_attr_server_monitor_pending.attr,
 	&dev_attr_client_monitor_pending.attr,
 	&dev_attr_server_monitor_latency.attr,
-- 
1.8.3.1

[PATCH V3,net-next, 3/3] hv_netvsc: Set probe_type to PROBE_DEFAULT_STRATEGY

[Haiyang Zhang]

The dev_num field in VMBus channel structure is assigned to the first
available number when the channel is offered.

Udev or other user programs can use this value from sysfs to set proper
device names.

Set probe_type to PROBE_DEFAULT_STRATEGY, so the user has an option to
enable Async probing when needed.

Sync probing mode is still the default.

Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/net/hyperv/netvsc_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index f3f9eb8..4a2f294 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -2496,7 +2496,7 @@ static int netvsc_resume(struct hv_device *dev)
 	.suspend = netvsc_suspend,
 	.resume = netvsc_resume,
 	.driver = {
-		.probe_type = PROBE_FORCE_SYNCHRONOUS,
+		.probe_type = PROBE_DEFAULT_STRATEGY,
 	},
 };
 
-- 
1.8.3.1

Re: [PATCH V3,net-next, 0/3] Add vmbus dev_num and allow netvsc probe options

[David Miller]

From: Haiyang Zhang <haiyangz@microsoft.com>
Date: Tue, 31 Dec 2019 14:13:31 -0800

> Add dev_num for vmbus device based on channel offer sequence.
> User programs can use this number for nic naming.
> Async probing option is allowed for netvsc. Sync probing is still
> the default.

I don't like this at all, sorry.

If 4 devices get channels we will have IDs 1, 2, 3, 4.

Then if channel 3 is taken down, the next one will get 3 which
is not in order any more.

It is not even clear what semantics these numbers have in any
particular sequence or probe situation.

You have to use something more persistent across boots to number
and strictly identify these virtual devices.

I'm not applying this.

RE: [PATCH V3,net-next, 0/3] Add vmbus dev_num and allow netvsc probe options

[Haiyang Zhang]

> -----Original Message-----
> From: David Miller <davem@davemloft.net>
> Sent: Thursday, January 2, 2020 7:30 PM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: sashal@kernel.org; linux-hyperv@vger.kernel.org; netdev@vger.kernel.org;
> KY Srinivasan <kys@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; olaf@aepfle.de; vkuznets
> <vkuznets@redhat.com>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH V3,net-next, 0/3] Add vmbus dev_num and allow netvsc
> probe options
> 
> From: Haiyang Zhang <haiyangz@microsoft.com>
> Date: Tue, 31 Dec 2019 14:13:31 -0800
> 
> > Add dev_num for vmbus device based on channel offer sequence.
> > User programs can use this number for nic naming.
> > Async probing option is allowed for netvsc. Sync probing is still the
> > default.
> 
> I don't like this at all, sorry.
> 
> If 4 devices get channels we will have IDs 1, 2, 3, 4.
> 
> Then if channel 3 is taken down, the next one will get 3 which is not in order
> any more.
> 
> It is not even clear what semantics these numbers have in any particular
> sequence or probe situation.
> 
> You have to use something more persistent across boots to number and strictly
> identify these virtual devices.
> 
> I'm not applying this.

The idea of this patch set is to make the naming of async probing same as that
in sync probing.

So, the semantics of this dev_num is actually same as the default "eth%d" naming --
	"Find the smallest number >=0, which is not in use."

In cases:
1) There is no hot add/remove devices in current boot, this scheme does provide 
persistent dev_num across reboot, because Hyper-V hosts offer the primary channels
in order. So the results based on this number with async probing will be the same 
as existing Sync probing.

2) If there is hot add/remove devices, this scheme generates the same results (when
user program use dev_num)  as the default naming mode -- by using the smallest
available number N in the format ethN.

In case of hot add/remove of virtual NICs, the removed NIC are gone, and the newly 
added NIC is a completely new virtual device with new device instance UUID. So if we 
don't reuse the previous numbers, the device name ethN will grow unbounded. For 
example, hot add/remove a virtual NIC 100 times, you will have a name like eth100. This 
not what the default naming scheme does, and we are not doing it for dev_num here 
either.

So the semantics is: "Find smallest number >=0, and not in use".

But if any customer wants to have a 1:1 mapping between the UUID and device name, they
can still implement that in user mode... And that's why this patch set doesn't change
the kernel naming from driver -- it just provides a new variable, "dev_num", so user 
mode program has the option to use it in async mode with knowledge of the channel
offer sequence.

Thanks,
- Haiyang