[PATCH 1/2] hv_balloon: Use a static page for the balloon_up send buffer

[PATCH 1/2] hv_balloon: Use a static page for the balloon_up send buffer

[Dexuan Cui]

It's unnecessary to dynamically allocate the buffer.

Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 drivers/hv/hv_balloon.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
index dd475f3bcc8a..13381ea3e3e7 100644
--- a/drivers/hv/hv_balloon.c
+++ b/drivers/hv/hv_balloon.c
@@ -504,7 +504,7 @@ enum hv_dm_state {
 
 
 static __u8 recv_buffer[PAGE_SIZE];
-static __u8 *send_buffer;
+static __u8 balloon_up_send_buffer[PAGE_SIZE];
 #define PAGES_IN_2M	512
 #define HA_CHUNK (32 * 1024)
 
@@ -1302,8 +1302,8 @@ static void balloon_up(struct work_struct *dummy)
 	}
 
 	while (!done) {
-		bl_resp = (struct dm_balloon_response *)send_buffer;
-		memset(send_buffer, 0, PAGE_SIZE);
+		memset(balloon_up_send_buffer, 0, PAGE_SIZE);
+		bl_resp = (struct dm_balloon_response *)balloon_up_send_buffer;
 		bl_resp->hdr.type = DM_BALLOON_RESPONSE;
 		bl_resp->hdr.size = sizeof(struct dm_balloon_response);
 		bl_resp->more_pages = 1;
@@ -1588,19 +1588,11 @@ static int balloon_probe(struct hv_device *dev,
 	do_hot_add = false;
 #endif
 
-	/*
-	 * First allocate a send buffer.
-	 */
-
-	send_buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
-	if (!send_buffer)
-		return -ENOMEM;
-
 	ret = vmbus_open(dev->channel, dm_ring_size, dm_ring_size, NULL, 0,
 			balloon_onchannelcallback, dev);
 
 	if (ret)
-		goto probe_error0;
+		return ret;
 
 	dm_device.dev = dev;
 	dm_device.state = DM_INITIALIZING;
@@ -1726,8 +1718,6 @@ static int balloon_probe(struct hv_device *dev,
 
 probe_error1:
 	vmbus_close(dev->channel);
-probe_error0:
-	kfree(send_buffer);
 	return ret;
 }
 
@@ -1746,7 +1736,6 @@ static int balloon_remove(struct hv_device *dev)
 
 	vmbus_close(dev->channel);
 	kthread_stop(dm->thread);
-	kfree(send_buffer);
 #ifdef CONFIG_MEMORY_HOTPLUG
 	restore_online_page_callback(&hv_online_page);
 	unregister_memory_notifier(&hv_memory_nb);
-- 
2.19.1

[PATCH 2/2] hv_balloon: Reorganize the probe function

[Dexuan Cui]

Move the code that negotiates with the host to a new function
balloon_connect_vsp() and improve the error handling.

This makes the code more readable and paves the way for the
support of hibernation in future.

Makes no real logic change here.

Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 drivers/hv/hv_balloon.c | 124 +++++++++++++++++++++-------------------
 1 file changed, 66 insertions(+), 58 deletions(-)

diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
index 13381ea3e3e7..111ea3599659 100644
--- a/drivers/hv/hv_balloon.c
+++ b/drivers/hv/hv_balloon.c
@@ -1574,50 +1574,18 @@ static void balloon_onchannelcallback(void *context)
 
 }
 
-static int balloon_probe(struct hv_device *dev,
-			const struct hv_vmbus_device_id *dev_id)
+static int balloon_connect_vsp(struct hv_device *dev)
 {
-	int ret;
-	unsigned long t;
 	struct dm_version_request version_req;
 	struct dm_capabilities cap_msg;
-
-#ifdef CONFIG_MEMORY_HOTPLUG
-	do_hot_add = hot_add;
-#else
-	do_hot_add = false;
-#endif
+	unsigned long t;
+	int ret;
 
 	ret = vmbus_open(dev->channel, dm_ring_size, dm_ring_size, NULL, 0,
-			balloon_onchannelcallback, dev);
-
+			 balloon_onchannelcallback, dev);
 	if (ret)
 		return ret;
 
-	dm_device.dev = dev;
-	dm_device.state = DM_INITIALIZING;
-	dm_device.next_version = DYNMEM_PROTOCOL_VERSION_WIN8;
-	init_completion(&dm_device.host_event);
-	init_completion(&dm_device.config_event);
-	INIT_LIST_HEAD(&dm_device.ha_region_list);
-	spin_lock_init(&dm_device.ha_lock);
-	INIT_WORK(&dm_device.balloon_wrk.wrk, balloon_up);
-	INIT_WORK(&dm_device.ha_wrk.wrk, hot_add_req);
-	dm_device.host_specified_ha_region = false;
-
-	dm_device.thread =
-		 kthread_run(dm_thread_func, &dm_device, "hv_balloon");
-	if (IS_ERR(dm_device.thread)) {
-		ret = PTR_ERR(dm_device.thread);
-		goto probe_error1;
-	}
-
-#ifdef CONFIG_MEMORY_HOTPLUG
-	set_online_page_callback(&hv_online_page);
-	register_memory_notifier(&hv_memory_nb);
-#endif
-
-	hv_set_drvdata(dev, &dm_device);
 	/*
 	 * Initiate the hand shake with the host and negotiate
 	 * a version that the host can support. We start with the
@@ -1633,16 +1601,15 @@ static int balloon_probe(struct hv_device *dev,
 	dm_device.version = version_req.version.version;
 
 	ret = vmbus_sendpacket(dev->channel, &version_req,
-				sizeof(struct dm_version_request),
-				(unsigned long)NULL,
-				VM_PKT_DATA_INBAND, 0);
+			       sizeof(struct dm_version_request),
+			       (unsigned long)NULL, VM_PKT_DATA_INBAND, 0);
 	if (ret)
-		goto probe_error2;
+		goto out;
 
 	t = wait_for_completion_timeout(&dm_device.host_event, 5*HZ);
 	if (t == 0) {
 		ret = -ETIMEDOUT;
-		goto probe_error2;
+		goto out;
 	}
 
 	/*
@@ -1650,8 +1617,8 @@ static int balloon_probe(struct hv_device *dev,
 	 * fail the probe function.
 	 */
 	if (dm_device.state == DM_INIT_ERROR) {
-		ret = -ETIMEDOUT;
-		goto probe_error2;
+		ret = -EPROTO;
+		goto out;
 	}
 
 	pr_info("Using Dynamic Memory protocol version %u.%u\n",
@@ -1684,16 +1651,15 @@ static int balloon_probe(struct hv_device *dev,
 	cap_msg.max_page_number = -1;
 
 	ret = vmbus_sendpacket(dev->channel, &cap_msg,
-				sizeof(struct dm_capabilities),
-				(unsigned long)NULL,
-				VM_PKT_DATA_INBAND, 0);
+			       sizeof(struct dm_capabilities),
+			       (unsigned long)NULL, VM_PKT_DATA_INBAND, 0);
 	if (ret)
-		goto probe_error2;
+		goto out;
 
 	t = wait_for_completion_timeout(&dm_device.host_event, 5*HZ);
 	if (t == 0) {
 		ret = -ETIMEDOUT;
-		goto probe_error2;
+		goto out;
 	}
 
 	/*
@@ -1701,23 +1667,65 @@ static int balloon_probe(struct hv_device *dev,
 	 * fail the probe function.
 	 */
 	if (dm_device.state == DM_INIT_ERROR) {
-		ret = -ETIMEDOUT;
-		goto probe_error2;
+		ret = -EPROTO;
+		goto out;
 	}
 
+	return 0;
+out:
+	vmbus_close(dev->channel);
+	return ret;
+}
+
+static int balloon_probe(struct hv_device *dev,
+			 const struct hv_vmbus_device_id *dev_id)
+{
+	int ret;
+
+#ifdef CONFIG_MEMORY_HOTPLUG
+	do_hot_add = hot_add;
+#else
+	do_hot_add = false;
+#endif
+	dm_device.dev = dev;
+	dm_device.state = DM_INITIALIZING;
+	dm_device.next_version = DYNMEM_PROTOCOL_VERSION_WIN8;
+	init_completion(&dm_device.host_event);
+	init_completion(&dm_device.config_event);
+	INIT_LIST_HEAD(&dm_device.ha_region_list);
+	spin_lock_init(&dm_device.ha_lock);
+	INIT_WORK(&dm_device.balloon_wrk.wrk, balloon_up);
+	INIT_WORK(&dm_device.ha_wrk.wrk, hot_add_req);
+	dm_device.host_specified_ha_region = false;
+
+#ifdef CONFIG_MEMORY_HOTPLUG
+	set_online_page_callback(&hv_online_page);
+	register_memory_notifier(&hv_memory_nb);
+#endif
+
+	hv_set_drvdata(dev, &dm_device);
+
+	ret = balloon_connect_vsp(dev);
+	if (ret != 0)
+		return ret;
+
 	dm_device.state = DM_INITIALIZED;
-	last_post_time = jiffies;
+
+	dm_device.thread =
+		 kthread_run(dm_thread_func, &dm_device, "hv_balloon");
+	if (IS_ERR(dm_device.thread)) {
+		ret = PTR_ERR(dm_device.thread);
+		goto probe_error;
+	}
 
 	return 0;
 
-probe_error2:
+probe_error:
+	vmbus_close(dev->channel);
 #ifdef CONFIG_MEMORY_HOTPLUG
+	unregister_memory_notifier(&hv_memory_nb);
 	restore_online_page_callback(&hv_online_page);
 #endif
-	kthread_stop(dm_device.thread);
-
-probe_error1:
-	vmbus_close(dev->channel);
 	return ret;
 }
 
@@ -1734,11 +1742,11 @@ static int balloon_remove(struct hv_device *dev)
 	cancel_work_sync(&dm->balloon_wrk.wrk);
 	cancel_work_sync(&dm->ha_wrk.wrk);
 
-	vmbus_close(dev->channel);
 	kthread_stop(dm->thread);
+	vmbus_close(dev->channel);
 #ifdef CONFIG_MEMORY_HOTPLUG
-	restore_online_page_callback(&hv_online_page);
 	unregister_memory_notifier(&hv_memory_nb);
+	restore_online_page_callback(&hv_online_page);
 #endif
 	spin_lock_irqsave(&dm_device.ha_lock, flags);
 	list_for_each_entry_safe(has, tmp, &dm->ha_region_list, list) {
-- 
2.19.1

RE: [PATCH 1/2] hv_balloon: Use a static page for the balloon_up send buffer

[Michael Kelley]

From: Dexuan Cui <decui@microsoft.com>  Sent: Friday, June 14, 2019 11:42 AM
> 
> It's unnecessary to dynamically allocate the buffer.
> 
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  drivers/hv/hv_balloon.c | 19 ++++---------------
>  1 file changed, 4 insertions(+), 15 deletions(-)
> 

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

RE: [PATCH 2/2] hv_balloon: Reorganize the probe function

[Michael Kelley]

From: Dexuan Cui <decui@microsoft.com>  Sent: Friday, June 14, 2019 11:43 AM
> 
> Move the code that negotiates with the host to a new function
> balloon_connect_vsp() and improve the error handling.
> 
> This makes the code more readable and paves the way for the
> support of hibernation in future.
> 
> Makes no real logic change here.
> 
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  drivers/hv/hv_balloon.c | 124 +++++++++++++++++++++-------------------
>  1 file changed, 66 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
> index 13381ea3e3e7..111ea3599659 100644
> --- a/drivers/hv/hv_balloon.c
> +++ b/drivers/hv/hv_balloon.c
> @@ -1574,50 +1574,18 @@ static void balloon_onchannelcallback(void *context)
> 
>  }
> 
> -static int balloon_probe(struct hv_device *dev,
> -			const struct hv_vmbus_device_id *dev_id)
> +static int balloon_connect_vsp(struct hv_device *dev)
>  {
> -	int ret;
> -	unsigned long t;
>  	struct dm_version_request version_req;
>  	struct dm_capabilities cap_msg;
> -
> -#ifdef CONFIG_MEMORY_HOTPLUG
> -	do_hot_add = hot_add;
> -#else
> -	do_hot_add = false;
> -#endif
> +	unsigned long t;
> +	int ret;
> 
>  	ret = vmbus_open(dev->channel, dm_ring_size, dm_ring_size, NULL, 0,
> -			balloon_onchannelcallback, dev);
> -
> +			 balloon_onchannelcallback, dev);
>  	if (ret)
>  		return ret;
> 
> -	dm_device.dev = dev;
> -	dm_device.state = DM_INITIALIZING;
> -	dm_device.next_version = DYNMEM_PROTOCOL_VERSION_WIN8;
> -	init_completion(&dm_device.host_event);
> -	init_completion(&dm_device.config_event);
> -	INIT_LIST_HEAD(&dm_device.ha_region_list);
> -	spin_lock_init(&dm_device.ha_lock);
> -	INIT_WORK(&dm_device.balloon_wrk.wrk, balloon_up);
> -	INIT_WORK(&dm_device.ha_wrk.wrk, hot_add_req);
> -	dm_device.host_specified_ha_region = false;
> -
> -	dm_device.thread =
> -		 kthread_run(dm_thread_func, &dm_device, "hv_balloon");
> -	if (IS_ERR(dm_device.thread)) {
> -		ret = PTR_ERR(dm_device.thread);
> -		goto probe_error1;
> -	}
> -
> -#ifdef CONFIG_MEMORY_HOTPLUG
> -	set_online_page_callback(&hv_online_page);
> -	register_memory_notifier(&hv_memory_nb);
> -#endif
> -
> -	hv_set_drvdata(dev, &dm_device);
>  	/*
>  	 * Initiate the hand shake with the host and negotiate
>  	 * a version that the host can support. We start with the
> @@ -1633,16 +1601,15 @@ static int balloon_probe(struct hv_device *dev,
>  	dm_device.version = version_req.version.version;
> 
>  	ret = vmbus_sendpacket(dev->channel, &version_req,
> -				sizeof(struct dm_version_request),
> -				(unsigned long)NULL,
> -				VM_PKT_DATA_INBAND, 0);
> +			       sizeof(struct dm_version_request),
> +			       (unsigned long)NULL, VM_PKT_DATA_INBAND, 0);
>  	if (ret)
> -		goto probe_error2;
> +		goto out;
> 
>  	t = wait_for_completion_timeout(&dm_device.host_event, 5*HZ);
>  	if (t == 0) {
>  		ret = -ETIMEDOUT;
> -		goto probe_error2;
> +		goto out;
>  	}
> 
>  	/*
> @@ -1650,8 +1617,8 @@ static int balloon_probe(struct hv_device *dev,
>  	 * fail the probe function.
>  	 */
>  	if (dm_device.state == DM_INIT_ERROR) {
> -		ret = -ETIMEDOUT;
> -		goto probe_error2;
> +		ret = -EPROTO;
> +		goto out;
>  	}
> 
>  	pr_info("Using Dynamic Memory protocol version %u.%u\n",
> @@ -1684,16 +1651,15 @@ static int balloon_probe(struct hv_device *dev,
>  	cap_msg.max_page_number = -1;
> 
>  	ret = vmbus_sendpacket(dev->channel, &cap_msg,
> -				sizeof(struct dm_capabilities),
> -				(unsigned long)NULL,
> -				VM_PKT_DATA_INBAND, 0);
> +			       sizeof(struct dm_capabilities),
> +			       (unsigned long)NULL, VM_PKT_DATA_INBAND, 0);
>  	if (ret)
> -		goto probe_error2;
> +		goto out;
> 
>  	t = wait_for_completion_timeout(&dm_device.host_event, 5*HZ);
>  	if (t == 0) {
>  		ret = -ETIMEDOUT;
> -		goto probe_error2;
> +		goto out;
>  	}
> 
>  	/*
> @@ -1701,23 +1667,65 @@ static int balloon_probe(struct hv_device *dev,
>  	 * fail the probe function.
>  	 */
>  	if (dm_device.state == DM_INIT_ERROR) {
> -		ret = -ETIMEDOUT;
> -		goto probe_error2;
> +		ret = -EPROTO;
> +		goto out;
>  	}
> 
> +	return 0;
> +out:
> +	vmbus_close(dev->channel);
> +	return ret;
> +}
> +
> +static int balloon_probe(struct hv_device *dev,
> +			 const struct hv_vmbus_device_id *dev_id)
> +{
> +	int ret;
> +
> +#ifdef CONFIG_MEMORY_HOTPLUG
> +	do_hot_add = hot_add;
> +#else
> +	do_hot_add = false;
> +#endif
> +	dm_device.dev = dev;
> +	dm_device.state = DM_INITIALIZING;
> +	dm_device.next_version = DYNMEM_PROTOCOL_VERSION_WIN8;
> +	init_completion(&dm_device.host_event);
> +	init_completion(&dm_device.config_event);
> +	INIT_LIST_HEAD(&dm_device.ha_region_list);
> +	spin_lock_init(&dm_device.ha_lock);
> +	INIT_WORK(&dm_device.balloon_wrk.wrk, balloon_up);
> +	INIT_WORK(&dm_device.ha_wrk.wrk, hot_add_req);
> +	dm_device.host_specified_ha_region = false;
> +
> +#ifdef CONFIG_MEMORY_HOTPLUG
> +	set_online_page_callback(&hv_online_page);
> +	register_memory_notifier(&hv_memory_nb);
> +#endif
> +
> +	hv_set_drvdata(dev, &dm_device);
> +
> +	ret = balloon_connect_vsp(dev);
> +	if (ret != 0)
> +		return ret;
> +
>  	dm_device.state = DM_INITIALIZED;
> -	last_post_time = jiffies;

I was curious about the above deletion.  But I guess the line
is not needed as the time_after() check in post_status() should
handle an initial value of 0 for last_post_time just fine.

> +
> +	dm_device.thread =
> +		 kthread_run(dm_thread_func, &dm_device, "hv_balloon");
> +	if (IS_ERR(dm_device.thread)) {
> +		ret = PTR_ERR(dm_device.thread);
> +		goto probe_error;
> +	}

Just an observation:  this thread creation now happens at the end of the
probing process.  But that's good, because in the old code, the thread
was started and could run before the protocol version had been
negotiated.  So I'll assume your change here is intentional.

> 
>  	return 0;
> 
> -probe_error2:
> +probe_error:
> +	vmbus_close(dev->channel);
>  #ifdef CONFIG_MEMORY_HOTPLUG
> +	unregister_memory_notifier(&hv_memory_nb);

Hmmm. Evidently the above cleanup was missing in the
old code.

>  	restore_online_page_callback(&hv_online_page);
>  #endif
> -	kthread_stop(dm_device.thread);
> -
> -probe_error1:
> -	vmbus_close(dev->channel);
>  	return ret;
>  }
> 
> @@ -1734,11 +1742,11 @@ static int balloon_remove(struct hv_device *dev)
>  	cancel_work_sync(&dm->balloon_wrk.wrk);
>  	cancel_work_sync(&dm->ha_wrk.wrk);
> 
> -	vmbus_close(dev->channel);
>  	kthread_stop(dm->thread);
> +	vmbus_close(dev->channel);

Presumably this is an intentional ordering change as well.
The kthread should be stopped before closing the channel.

>  #ifdef CONFIG_MEMORY_HOTPLUG
> -	restore_online_page_callback(&hv_online_page);
>  	unregister_memory_notifier(&hv_memory_nb);
> +	restore_online_page_callback(&hv_online_page);

And you've changed the ordering of these steps so they are
the inverse of when they are set up.  Also a good cleanup ....

>  #endif
>  	spin_lock_irqsave(&dm_device.ha_lock, flags);
>  	list_for_each_entry_safe(has, tmp, &dm->ha_region_list, list) {
> --
> 2.19.1

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

RE: [PATCH 2/2] hv_balloon: Reorganize the probe function

[Dexuan Cui]

> From: Michael Kelley <mikelley@microsoft.com>
> Sent: Friday, June 14, 2019 2:56 PM
> > ...
> > +	ret = balloon_connect_vsp(dev);
> > +	if (ret != 0)
> > +		return ret;
> > +
> >  	dm_device.state = DM_INITIALIZED;
> > -	last_post_time = jiffies;
> 
> I was curious about the above deletion.  But I guess the line
> is not needed as the time_after() check in post_status() should
> handle an initial value of 0 for last_post_time just fine.

In a 32-bit kernel, sizeof(unsigned long) is 4, and the global 32-bit
varilable "jiffies" can overflow in 49.7 days if HZ is defined as 1000;
so in theory there is a tiny chance time_after() can not work as
expected here (i.e. we're loading hv_balloon driver when the
"jiffies" is just about to overflow, which is highly unlikely in practice);
even if that happens, we do not care, since the consequence is
just that the memory pressure reporting is delayed by 1 second. :-)

> > +
> > +	dm_device.thread =
> > +		 kthread_run(dm_thread_func, &dm_device, "hv_balloon");
> > +	if (IS_ERR(dm_device.thread)) {
> > +		ret = PTR_ERR(dm_device.thread);
> > +		goto probe_error;
> > +	}
> 
> Just an observation:  this thread creation now happens at the end of the
> probing process.  But that's good, because in the old code, the thread
> was started and could run before the protocol version had been
> negotiated.  So I'll assume your change here is intentional.

Yes, this is intentional.
 
> >
> >  	return 0;
> >
> > -probe_error2:
> > +probe_error:
> > +	vmbus_close(dev->channel);
> >  #ifdef CONFIG_MEMORY_HOTPLUG
> > +	unregister_memory_notifier(&hv_memory_nb);
> 
> Hmmm. Evidently the above cleanup was missing in the
> old code.

Yes.
 
> >  	restore_online_page_callback(&hv_online_page);
> >  #endif
> > -	kthread_stop(dm_device.thread);
> > -
> > -probe_error1:
> > -	vmbus_close(dev->channel);
> >  	return ret;
> >  }
> >
> > @@ -1734,11 +1742,11 @@ static int balloon_remove(struct hv_device
> *dev)
> >  	cancel_work_sync(&dm->balloon_wrk.wrk);
> >  	cancel_work_sync(&dm->ha_wrk.wrk);
> >
> > -	vmbus_close(dev->channel);
> >  	kthread_stop(dm->thread);
> > +	vmbus_close(dev->channel);
> 
> Presumably this is an intentional ordering change as well.
> The kthread should be stopped before closing the channel.

Yes. The old code is buggy: after the vmbus_close(), there is
a small window in which the old code can still try to send
messages to the host via a freed ringbuffer, causing panic.
 
> >  #ifdef CONFIG_MEMORY_HOTPLUG
> > -	restore_online_page_callback(&hv_online_page);
> >  	unregister_memory_notifier(&hv_memory_nb);
> > +	restore_online_page_callback(&hv_online_page);
> 
> And you've changed the ordering of these steps so they are
> the inverse of when they are set up.  Also a good cleanup ....

Yes. The change is not really necessary, but let's just do it
in a better manner.
 
> 
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>

Thaks for the detailed comments!

Thanks,
-- Dexuan

Re: [PATCH 1/2] hv_balloon: Use a static page for the balloon_up send buffer

[Sasha Levin]

On Fri, Jun 14, 2019 at 06:42:17PM +0000, Dexuan Cui wrote:
>It's unnecessary to dynamically allocate the buffer.
>
>Signed-off-by: Dexuan Cui <decui@microsoft.com>

I've queued these up for hyperv-next, thanks!

--
Thanks,
Sasha