[PATCH 0/4] Drivers: hv: vmbus: Eliminate calls to BUG_ON()

[PATCH 0/4] Drivers: hv: vmbus: Eliminate calls to BUG_ON()

[K. Y. Srinivasan]

Cleanup the channel management code and eliminate calls to BUG_ON()

K. Y. Srinivasan (4):
  Drivers: hv: vmbus: Cleanup vmbus_post_msg()
  Drivers: hv: vmbus: Cleanup vmbus_teardown_gpadl()
  Drivers: hv: vmbus: Cleanup vmbus_close_internal()
  Drivers: hv: vmbus: Cleanup vmbus_establish_gpadl()

 drivers/hv/channel.c    |   48 +++++++++++++++++++++++++++++++---------------
 drivers/hv/connection.c |   13 +++++++++--
 2 files changed, 42 insertions(+), 19 deletions(-)

-- 
1.7.4.1

[PATCH 1/4] Drivers: hv: vmbus: Cleanup vmbus_post_msg()

[K. Y. Srinivasan]

Posting messages to the host can fail because of transient resource
related failures. Correctly deal with these failures and increase the
number of attempts to post the message before giving up.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Cc: <stable@vger.kernel.org>
---
 drivers/hv/connection.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index ae22e3c..16c2ccb 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -427,10 +427,17 @@ int vmbus_post_msg(void *buffer, size_t buflen)
 	 * insufficient resources. Retry the operation a couple of
 	 * times before giving up.
 	 */
-	while (retries < 3) {
-		ret =  hv_post_message(conn_id, 1, buffer, buflen);
-		if (ret != HV_STATUS_INSUFFICIENT_BUFFERS)
+	while (retries < 10) {
+		ret = hv_post_message(conn_id, 1, buffer, buflen);
+
+		switch (ret) {
+		case HV_STATUS_INSUFFICIENT_BUFFERS:
+		case -ENOMEM:
+			break;
+		default:
 			return ret;
+		}
+
 		retries++;
 		msleep(100);
 	}
-- 
1.7.4.1

[PATCH 2/4] Drivers: hv: vmbus: Cleanup vmbus_teardown_gpadl()

[K. Y. Srinivasan]

Eliminate calls to BUG_ON() by properly handling errors. In cases where
rollback is possible, we will return the appropriate error to have the
calling code decide how to rollback state. In the case where we are
transferring ownership of the guest physical pages to the host,
we will wait for the host to respond.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Cc: <stable@vger.kernel.org>
---
 drivers/hv/channel.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 531a593..0206314 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -435,7 +435,7 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32 gpadl_handle)
 	struct vmbus_channel_gpadl_teardown *msg;
 	struct vmbus_channel_msginfo *info;
 	unsigned long flags;
-	int ret, t;
+	int ret;
 
 	info = kmalloc(sizeof(*info) +
 		       sizeof(struct vmbus_channel_gpadl_teardown), GFP_KERNEL);
@@ -457,11 +457,12 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32 gpadl_handle)
 	ret = vmbus_post_msg(msg,
 			       sizeof(struct vmbus_channel_gpadl_teardown));
 
-	BUG_ON(ret != 0);
-	t = wait_for_completion_timeout(&info->waitevent, 5*HZ);
-	BUG_ON(t == 0);
+	if (ret)
+		goto post_msg_err;
+
+	wait_for_completion(&info->waitevent);
 
-	/* Received a torndown response */
+post_msg_err:
 	spin_lock_irqsave(&vmbus_connection.channelmsg_lock, flags);
 	list_del(&info->msglistentry);
 	spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
-- 
1.7.4.1

[PATCH 3/4] Drivers: hv: vmbus: Cleanup vmbus_close_internal()

[K. Y. Srinivasan]

Eliminate calls to BUG_ON() in vmbus_close_internal().
We have chosen to potentially leak memory, than crash the guest
in case of failures.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Cc: <stable@vger.kernel.org>
---
 drivers/hv/channel.c |   32 +++++++++++++++++++++++++-------
 1 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 0206314..d6c8ea2 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -479,10 +479,10 @@ static void reset_channel_cb(void *arg)
 	channel->onchannel_callback = NULL;
 }
 
-static void vmbus_close_internal(struct vmbus_channel *channel)
+static int vmbus_close_internal(struct vmbus_channel *channel)
 {
 	struct vmbus_channel_close_channel *msg;
-	int ret;
+	int ret = 0;
 
 	channel->state = CHANNEL_OPEN_STATE;
 	channel->sc_creation_callback = NULL;
@@ -502,11 +502,28 @@ static void vmbus_close_internal(struct vmbus_channel *channel)
 
 	ret = vmbus_post_msg(msg, sizeof(struct vmbus_channel_close_channel));
 
-	BUG_ON(ret != 0);
+	if (ret) {
+		pr_err("Close failed: close post msg return is %d\n", ret);
+		/*
+		 * If we failed to post the close msg,
+		 * it is perhaps better to leak memory.
+		 */
+		goto close_err;
+	}
+
 	/* Tear down the gpadl for the channel's ring buffer */
-	if (channel->ringbuffer_gpadlhandle)
-		vmbus_teardown_gpadl(channel,
-					  channel->ringbuffer_gpadlhandle);
+	if (channel->ringbuffer_gpadlhandle) {
+		ret = vmbus_teardown_gpadl(channel,
+					   channel->ringbuffer_gpadlhandle);
+		if (ret) {
+			pr_err("Close failed: teardown gpadl return %d\n", ret);
+			/*
+			 * If we failed to teardown gpadl,
+			 * it is perhaps better to leak memory.
+			 */
+			goto close_err;
+		}
+	}
 
 	/* Cleanup the ring buffers for this channel */
 	hv_ringbuffer_cleanup(&channel->outbound);
@@ -515,7 +532,8 @@ static void vmbus_close_internal(struct vmbus_channel *channel)
 	free_pages((unsigned long)channel->ringbuffer_pages,
 		get_order(channel->ringbuffer_pagecount * PAGE_SIZE));
 
-
+close_err:
+	return ret;
 }
 
 /*
-- 
1.7.4.1

Re: [PATCH 3/4] Drivers: hv: vmbus: Cleanup vmbus_close_internal()

[Dan Carpenter]

On Tue, Aug 26, 2014 at 12:05:51PM -0700, K. Y. Srinivasan wrote:
> -static void vmbus_close_internal(struct vmbus_channel *channel)
> +static int vmbus_close_internal(struct vmbus_channel *channel)
>  {
>  	struct vmbus_channel_close_channel *msg;
> -	int ret;
> +	int ret = 0;

GCC has a feature which warns about uninitialized variables.  Those
features are there to help prevent bugs.  You are turning the feature
off here by initializing it with a bogus value.  Don't do that.

>  
>  	channel->state = CHANNEL_OPEN_STATE;
>  	channel->sc_creation_callback = NULL;
> @@ -502,11 +502,28 @@ static void vmbus_close_internal(struct vmbus_channel *channel)
>  
>  	ret = vmbus_post_msg(msg, sizeof(struct vmbus_channel_close_channel));
>  
> -	BUG_ON(ret != 0);
> +	if (ret) {
> +		pr_err("Close failed: close post msg return is %d\n", ret);
> +		/*
> +		 * If we failed to post the close msg,
> +		 * it is perhaps better to leak memory.
> +		 */
> +		goto close_err;


Just return directly.  Don't introduce do-nothing gotos to lead the
reader through a series of pointless goto hops.

The goto label is poorly chosen.  Label names should be based on the
thing which they do.  "close_err" implies that something is closed but
that's not the case, the label doesn't do anything.

regards,
dan carpenter

[PATCH 4/4] Drivers: hv: vmbus: Cleanup vmbus_establish_gpadl()

[K. Y. Srinivasan]

Eliminate the call to BUG_ON() by waiting for the host to respond. We are
trying to reclaim the ownership of memory that was given to the host and so
we will have to wait until the host responds.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Cc: <stable@vger.kernel.org>
---
 drivers/hv/channel.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index d6c8ea2..c7ffd42 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -363,7 +363,6 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
 	u32 next_gpadl_handle;
 	unsigned long flags;
 	int ret = 0;
-	int t;
 
 	next_gpadl_handle = atomic_read(&vmbus_connection.next_gpadl_handle);
 	atomic_inc(&vmbus_connection.next_gpadl_handle);
@@ -410,9 +409,7 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
 
 		}
 	}
-	t = wait_for_completion_timeout(&msginfo->waitevent, 5*HZ);
-	BUG_ON(t == 0);
-
+	wait_for_completion(&msginfo->waitevent);
 
 	/* At this point, we received the gpadl created msg */
 	*gpadl_handle = gpadlmsg->gpadl;
-- 
1.7.4.1

Re: [PATCH 0/4] Drivers: hv: vmbus: Eliminate calls to BUG_ON()

[Sitsofe Wheeler]

On Tue, Aug 26, 2014 at 12:05:22PM -0700, K. Y. Srinivasan wrote:
> Cleanup the channel management code and eliminate calls to BUG_ON()
> 
> K. Y. Srinivasan (4):
>   Drivers: hv: vmbus: Cleanup vmbus_post_msg()
>   Drivers: hv: vmbus: Cleanup vmbus_teardown_gpadl()
>   Drivers: hv: vmbus: Cleanup vmbus_close_internal()
>   Drivers: hv: vmbus: Cleanup vmbus_establish_gpadl()

I've applied these on top of 3.17-rc1 and my Hyper-V guest with most
verification on bar DEBUG_PAGEALLOC boots without issue so long as I
have more than one CPU assigned (that issue is being investigated in
https://lkml.org/lkml/2014/8/26/271, [PANIC, hyperv] BUG: unable to
handle kernel paging request at ffff880077800004 (hv_ringbuffer_write)).

With DEBUG_PAGEALLOC on I hit https://lkml.org/lkml/2014/8/19/227 (BUG:
unable to handle kernel paging request at ffff8801f3febe63
(netvsc_select_queue)) but that's different to the above BUG_ONs.

-- 
Sitsofe | http://sucs.org/~sits/