[PATCH 0/2] Drivers: hv: vmbus: make bus ids in sysfs persistent

[PATCH 0/2] Drivers: hv: vmbus: make bus ids in sysfs persistent

[Vitaly Kuznetsov]

Bus ids for VMBus devices in /sys/bus/vmbus/devices/ are not guaranteed
to be persistent across reboot or kernel restart and this causes problems
for some tools. E.g. kexec tools use these ids to identify NIC on kdump.
Fix the issue by using relid from channel offer as the unique id instead
of an auto incremented counter.

Vitaly Kuznetsov (2):
  Drivers: hv: make VMBus bus ids persistent
  Drivers: hv: get rid of id in struct vmbus_channel

 drivers/hv/channel_mgmt.c | 2 --
 drivers/hv/vmbus_drv.c    | 2 +-
 include/linux/hyperv.h    | 3 ---
 3 files changed, 1 insertion(+), 6 deletions(-)

-- 
2.7.4

[PATCH 1/2] Drivers: hv: make VMBus bus ids persistent

[Vitaly Kuznetsov]

Some tools use bus ids to identify devices and they count on the fact
that these ids are persistent across reboot. This may be not true for
VMBus as we use auto incremented counter from alloc_channel() as such
id. Switch to using child_relid from channel offer, this id is supposed
to be persistent.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.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 e82f7e1..ef42909 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -962,7 +962,7 @@ int vmbus_device_register(struct hv_device *child_device_obj)
 	int ret = 0;
 
 	dev_set_name(&child_device_obj->device, "vmbus_%d",
-		     child_device_obj->channel->id);
+		     child_device_obj->channel->offermsg.child_relid);
 
 	child_device_obj->device.bus = &hv_bus;
 	child_device_obj->device.parent = &hv_acpi_dev->dev;
-- 
2.7.4

[PATCH 2/2] Drivers: hv: get rid of id in struct vmbus_channel

[Vitaly Kuznetsov]

The auto incremented counter is not being used anymore, get rid of it.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/hv/channel_mgmt.c | 2 --
 include/linux/hyperv.h    | 3 ---
 2 files changed, 5 deletions(-)

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index b6c1211..4b4a41d 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -251,14 +251,12 @@ EXPORT_SYMBOL_GPL(vmbus_prep_negotiate_resp);
  */
 static struct vmbus_channel *alloc_channel(void)
 {
-	static atomic_t chan_num = ATOMIC_INIT(0);
 	struct vmbus_channel *channel;
 
 	channel = kzalloc(sizeof(*channel), GFP_ATOMIC);
 	if (!channel)
 		return NULL;
 
-	channel->id = atomic_inc_return(&chan_num);
 	channel->acquire_ring_lock = true;
 	spin_lock_init(&channel->inbound_lock);
 	spin_lock_init(&channel->lock);
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index b10954a..d9371a7 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -701,9 +701,6 @@ struct vmbus_device {
 };
 
 struct vmbus_channel {
-	/* Unique channel id */
-	int id;
-
 	struct list_head listentry;
 
 	struct hv_device *device_obj;
-- 
2.7.4

RE: [PATCH 0/2] Drivers: hv: vmbus: make bus ids in sysfs persistent

[KY Srinivasan]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Tuesday, August 9, 2016 1:46 AM
> To: devel@linuxdriverproject.org
> Cc: linux-kernel@vger.kernel.org; Haiyang Zhang <haiyangz@microsoft.com>;
> KY Srinivasan <kys@microsoft.com>
> Subject: [PATCH 0/2] Drivers: hv: vmbus: make bus ids in sysfs persistent
> 
> Bus ids for VMBus devices in /sys/bus/vmbus/devices/ are not guaranteed
> to be persistent across reboot or kernel restart and this causes problems
> for some tools. E.g. kexec tools use these ids to identify NIC on kdump.
> Fix the issue by using relid from channel offer as the unique id instead
> of an auto incremented counter.

Relids are not persistent. It is only valid between a channel offer message and a relid released message (or an unload or initiate contact message, which invalidates all channels). This is an opaque number that the root generates and uses to track channels. There is no guarantee that the same type of channel (networking, storage, etc) will get the same relid on each reboot.

Regards,

K. Y
> 
> Vitaly Kuznetsov (2):
>   Drivers: hv: make VMBus bus ids persistent
>   Drivers: hv: get rid of id in struct vmbus_channel
> 
>  drivers/hv/channel_mgmt.c | 2 --
>  drivers/hv/vmbus_drv.c    | 2 +-
>  include/linux/hyperv.h    | 3 ---
>  3 files changed, 1 insertion(+), 6 deletions(-)
> 
> --
> 2.7.4

Re: [PATCH 0/2] Drivers: hv: vmbus: make bus ids in sysfs persistent

[Vitaly Kuznetsov]

KY Srinivasan <kys@microsoft.com> writes:

>> -----Original Message-----
>> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
>> Sent: Tuesday, August 9, 2016 1:46 AM
>> To: devel@linuxdriverproject.org
>> Cc: linux-kernel@vger.kernel.org; Haiyang Zhang <haiyangz@microsoft.com>;
>> KY Srinivasan <kys@microsoft.com>
>> Subject: [PATCH 0/2] Drivers: hv: vmbus: make bus ids in sysfs persistent
>> 
>> Bus ids for VMBus devices in /sys/bus/vmbus/devices/ are not guaranteed
>> to be persistent across reboot or kernel restart and this causes problems
>> for some tools. E.g. kexec tools use these ids to identify NIC on kdump.
>> Fix the issue by using relid from channel offer as the unique id instead
>> of an auto incremented counter.
>
> Relids are not persistent. It is only valid between a channel offer
> message and a relid released message (or an unload or initiate contact
> message, which invalidates all channels). This is an opaque number
> that the root generates and uses to track channels. There is no
> guarantee that the same type of channel (networking, storage, etc)
> will get the same relid on each reboot.
>

Thanks for the info,

can we use device_id (offermsg.offer.if_instance.b) instead?

-- 
  Vitaly

RE: [PATCH 0/2] Drivers: hv: vmbus: make bus ids in sysfs persistent

[KY Srinivasan]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Thursday, August 11, 2016 2:17 AM
> To: KY Srinivasan <kys@microsoft.com>
> Cc: devel@linuxdriverproject.org; linux-kernel@vger.kernel.org; Haiyang Zhang
> <haiyangz@microsoft.com>
> Subject: Re: [PATCH 0/2] Drivers: hv: vmbus: make bus ids in sysfs persistent
> 
> KY Srinivasan <kys@microsoft.com> writes:
> 
> >> -----Original Message-----
> >> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> >> Sent: Tuesday, August 9, 2016 1:46 AM
> >> To: devel@linuxdriverproject.org
> >> Cc: linux-kernel@vger.kernel.org; Haiyang Zhang
> >> <haiyangz@microsoft.com>; KY Srinivasan <kys@microsoft.com>
> >> Subject: [PATCH 0/2] Drivers: hv: vmbus: make bus ids in sysfs
> >> persistent
> >>
> >> Bus ids for VMBus devices in /sys/bus/vmbus/devices/ are not
> >> guaranteed to be persistent across reboot or kernel restart and this
> >> causes problems for some tools. E.g. kexec tools use these ids to identify
> NIC on kdump.
> >> Fix the issue by using relid from channel offer as the unique id
> >> instead of an auto incremented counter.
> >
> > Relids are not persistent. It is only valid between a channel offer
> > message and a relid released message (or an unload or initiate contact
> > message, which invalidates all channels). This is an opaque number
> > that the root generates and uses to track channels. There is no
> > guarantee that the same type of channel (networking, storage, etc)
> > will get the same relid on each reboot.
> >
> 
> Thanks for the info,
> 
> can we use device_id (offermsg.offer.if_instance.b) instead?

I think you could; I am going to verify and get back to you on this.
Sometime back I removed all the non-determinism in the vmbus device
ID generation. Now, the current scheme of generating the device IDs does
result in persistent IDs across boot (as long as the host presents the devices
to the guest in the same order across boots). Do you have this fix?

Regards,

K. Y
> 
> --
>   Vitaly

Re: [PATCH 0/2] Drivers: hv: vmbus: make bus ids in sysfs persistent

[Vitaly Kuznetsov]

KY Srinivasan <kys@microsoft.com> writes:

>> -----Original Message-----
>> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
>> Sent: Thursday, August 11, 2016 2:17 AM
>> To: KY Srinivasan <kys@microsoft.com>
>> Cc: devel@linuxdriverproject.org; linux-kernel@vger.kernel.org; Haiyang Zhang
>> <haiyangz@microsoft.com>
>> Subject: Re: [PATCH 0/2] Drivers: hv: vmbus: make bus ids in sysfs persistent
>> 
>> KY Srinivasan <kys@microsoft.com> writes:
>> 
>> >> -----Original Message-----
>> >> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
>> >> Sent: Tuesday, August 9, 2016 1:46 AM
>> >> To: devel@linuxdriverproject.org
>> >> Cc: linux-kernel@vger.kernel.org; Haiyang Zhang
>> >> <haiyangz@microsoft.com>; KY Srinivasan <kys@microsoft.com>
>> >> Subject: [PATCH 0/2] Drivers: hv: vmbus: make bus ids in sysfs
>> >> persistent
>> >>
>> >> Bus ids for VMBus devices in /sys/bus/vmbus/devices/ are not
>> >> guaranteed to be persistent across reboot or kernel restart and this
>> >> causes problems for some tools. E.g. kexec tools use these ids to identify
>> NIC on kdump.
>> >> Fix the issue by using relid from channel offer as the unique id
>> >> instead of an auto incremented counter.
>> >
>> > Relids are not persistent. It is only valid between a channel offer
>> > message and a relid released message (or an unload or initiate contact
>> > message, which invalidates all channels). This is an opaque number
>> > that the root generates and uses to track channels. There is no
>> > guarantee that the same type of channel (networking, storage, etc)
>> > will get the same relid on each reboot.
>> >
>> 
>> Thanks for the info,
>> 
>> can we use device_id (offermsg.offer.if_instance.b) instead?
>
> I think you could; I am going to verify and get back to you on this.

Thanks!

> Sometime back I removed all the non-determinism in the vmbus device
> ID generation. Now, the current scheme of generating the device IDs does
> result in persistent IDs across boot (as long as the host presents the devices
> to the guest in the same order across boots). Do you have this fix?

Yes, I think I do. The issue I'm trying to address happens rearly on
kdump when we get devices present to us in a different order.

-- 
  Vitaly

RE: [PATCH 0/2] Drivers: hv: vmbus: make bus ids in sysfs persistent

[KY Srinivasan]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Monday, August 15, 2016 9:11 AM
> To: KY Srinivasan <kys@microsoft.com>
> Cc: devel@linuxdriverproject.org; linux-kernel@vger.kernel.org; Haiyang Zhang
> <haiyangz@microsoft.com>
> Subject: Re: [PATCH 0/2] Drivers: hv: vmbus: make bus ids in sysfs persistent
> 
> KY Srinivasan <kys@microsoft.com> writes:
> 
> >> -----Original Message-----
> >> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> >> Sent: Thursday, August 11, 2016 2:17 AM
> >> To: KY Srinivasan <kys@microsoft.com>
> >> Cc: devel@linuxdriverproject.org; linux-kernel@vger.kernel.org; Haiyang
> Zhang
> >> <haiyangz@microsoft.com>
> >> Subject: Re: [PATCH 0/2] Drivers: hv: vmbus: make bus ids in sysfs persistent
> >>
> >> KY Srinivasan <kys@microsoft.com> writes:
> >>
> >> >> -----Original Message-----
> >> >> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> >> >> Sent: Tuesday, August 9, 2016 1:46 AM
> >> >> To: devel@linuxdriverproject.org
> >> >> Cc: linux-kernel@vger.kernel.org; Haiyang Zhang
> >> >> <haiyangz@microsoft.com>; KY Srinivasan <kys@microsoft.com>
> >> >> Subject: [PATCH 0/2] Drivers: hv: vmbus: make bus ids in sysfs
> >> >> persistent
> >> >>
> >> >> Bus ids for VMBus devices in /sys/bus/vmbus/devices/ are not
> >> >> guaranteed to be persistent across reboot or kernel restart and this
> >> >> causes problems for some tools. E.g. kexec tools use these ids to identify
> >> NIC on kdump.
> >> >> Fix the issue by using relid from channel offer as the unique id
> >> >> instead of an auto incremented counter.
> >> >
> >> > Relids are not persistent. It is only valid between a channel offer
> >> > message and a relid released message (or an unload or initiate contact
> >> > message, which invalidates all channels). This is an opaque number
> >> > that the root generates and uses to track channels. There is no
> >> > guarantee that the same type of channel (networking, storage, etc)
> >> > will get the same relid on each reboot.
> >> >
> >>
> >> Thanks for the info,
> >>
> >> can we use device_id (offermsg.offer.if_instance.b) instead?
> >
> > I think you could; I am going to verify and get back to you on this.
> 
> Thanks!

The instance GUID is guaranteed to be persistent between boots and also across live migration.

K. Y

[PATCH 0/2] Drivers: hv: vmbus: Use persistent IDs for vmbus device names

[kys]

From: K. Y. Srinivasan <kys@microsoft.com>

Use IDs that persist across reboots as vmbus device IDs.

Vitaly Kuznetsov (2):
  Drivers: hv: make VMBus bus ids persistent
  Drivers: hv: get rid of id in struct vmbus_channel

 drivers/hv/channel_mgmt.c |    2 --
 drivers/hv/vmbus_drv.c    |    4 ++--
 include/linux/hyperv.h    |    3 ---
 3 files changed, 2 insertions(+), 7 deletions(-)

-- 
1.7.4.1

[PATCH 1/1] Drivers: hv: hv_util: Avoid dynamic allocation in time synch

[kys]

From: Vivek yadav <vyadav@microsoft.com>

Under stress, we have seen allocation failure in time synch code. Avoid
this dynamic allocation.

Signed-off-by: Vivek Yadav <vyadav@microsoft.com>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/hv/hv_util.c |   39 ++++++++++++++++++++++++++++-----------
 1 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
index 6286bdc..4aa3cb6 100644
--- a/drivers/hv/hv_util.c
+++ b/drivers/hv/hv_util.c
@@ -64,9 +64,14 @@ static struct hv_util_service util_shutdown = {
 	.util_cb = shutdown_onchannelcallback,
 };
 
+static int hv_timesync_init(struct hv_util_service *srv);
+static void hv_timesync_deinit(void);
+
 static void timesync_onchannelcallback(void *context);
 static struct hv_util_service util_timesynch = {
 	.util_cb = timesync_onchannelcallback,
+	.util_init = hv_timesync_init,
+	.util_deinit = hv_timesync_deinit,
 };
 
 static void heartbeat_onchannelcallback(void *context);
@@ -201,7 +206,6 @@ static void hv_set_host_time(struct work_struct *work)
 	host_ts = ns_to_timespec(host_tns);
 
 	do_settimeofday(&host_ts);
-	kfree(wrk);
 }
 
 /*
@@ -217,22 +221,24 @@ static void hv_set_host_time(struct work_struct *work)
  * typically used as a hint to the guest. The guest is under no obligation
  * to discipline the clock.
  */
+static struct adj_time_work  wrk;
 static inline void adj_guesttime(u64 hosttime, u64 reftime, u8 flags)
 {
-	struct adj_time_work    *wrk;
 
-	wrk = kmalloc(sizeof(struct adj_time_work), GFP_ATOMIC);
-	if (wrk == NULL)
+	/*
+	 * This check is safe since we are executing in the
+	 * interrupt context and time synch messages arre always
+	 * delivered on the same CPU.
+	 */
+	if (work_pending(&wrk.work))
 		return;
 
-	wrk->host_time = hosttime;
-	wrk->ref_time = reftime;
-	wrk->flags = flags;
+	wrk.host_time = hosttime;
+	wrk.ref_time = reftime;
+	wrk.flags = flags;
 	if ((flags & (ICTIMESYNCFLAG_SYNC | ICTIMESYNCFLAG_SAMPLE)) != 0) {
-		INIT_WORK(&wrk->work, hv_set_host_time);
-		schedule_work(&wrk->work);
-	} else
-		kfree(wrk);
+		schedule_work(&wrk.work);
+	}
 }
 
 /*
@@ -457,6 +463,17 @@ static  struct hv_driver util_drv = {
 	.remove =  util_remove,
 };
 
+static int hv_timesync_init(struct hv_util_service *srv)
+{
+	INIT_WORK(&wrk.work, hv_set_host_time);
+	return 0;
+}
+
+static void hv_timesync_deinit(void)
+{
+	cancel_work_sync(&wrk.work);
+}
+
 static int __init init_hyperv_utils(void)
 {
 	pr_info("Registering HyperV Utility Driver\n");
-- 
1.7.4.1

[PATCH 2/2] Drivers: hv: get rid of id in struct vmbus_channel

[kys]

From: Vitaly Kuznetsov <vkuznets@redhat.com>

The auto incremented counter is not being used anymore, get rid of it.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/hv/channel_mgmt.c |    2 --
 include/linux/hyperv.h    |    3 ---
 2 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 759ba4d..96a85cd 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -274,14 +274,12 @@ EXPORT_SYMBOL_GPL(vmbus_prep_negotiate_resp);
  */
 static struct vmbus_channel *alloc_channel(void)
 {
-	static atomic_t chan_num = ATOMIC_INIT(0);
 	struct vmbus_channel *channel;
 
 	channel = kzalloc(sizeof(*channel), GFP_ATOMIC);
 	if (!channel)
 		return NULL;
 
-	channel->id = atomic_inc_return(&chan_num);
 	channel->acquire_ring_lock = true;
 	spin_lock_init(&channel->inbound_lock);
 	spin_lock_init(&channel->lock);
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 7d7cbff..cd184bd 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -706,9 +706,6 @@ struct vmbus_device {
 };
 
 struct vmbus_channel {
-	/* Unique channel id */
-	int id;
-
 	struct list_head listentry;
 
 	struct hv_device *device_obj;
-- 
1.7.4.1