[PATCH 0/2] Drivers: hv: balloon: Fix some issues in the balloon driver

[PATCH 0/2] Drivers: hv: balloon: Fix some issues in the balloon driver

[K. Y. Srinivasan]

This patch set adresses a couple of issues discovered in the balloon driver:

1. Prevent stale pressure report being posted.
2. If memory is not brought online within the allowed time,
   do not roll back hot add state if hot add succeeded.

K. Y. Srinivasan (2):
  Drivers: hv: balloon: Fix a bug in the hot-add code
  Drivers: hv: balloon: Do not post pressure status if interrupted

 drivers/hv/hv_balloon.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

-- 
1.7.4.1

[PATCH 1/2] Drivers: hv: balloon: Fix a bug in the hot-add code

[K. Y. Srinivasan]

As we hot-add 128 MB chunks of memory, we wait to ensure that the memory
is onlined before attempting to hot-add the next chunk. If the udev rule for
memory hot-add is not executed within the allowed time, we would rollback the
state and abort further hot-add. Since the hot-add has succeeded and the only
failure is that the memory is not onlined within the allowed time, we should not
be rolling back the state. Fix this bug.

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

diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
index 4c605c7..bfc7be2 100644
--- a/drivers/hv/hv_balloon.c
+++ b/drivers/hv/hv_balloon.c
@@ -562,7 +562,7 @@ static void hv_mem_hot_add(unsigned long start, unsigned long size,
 				struct hv_hotadd_state *has)
 {
 	int ret = 0;
-	int i, nid, t;
+	int i, nid;
 	unsigned long start_pfn;
 	unsigned long processed_pfn;
 	unsigned long total_pfn = pfn_count;
@@ -608,13 +608,7 @@ static void hv_mem_hot_add(unsigned long start, unsigned long size,
 		/*
 		 * Wait for the memory block to be onlined.
 		 */
-		t = wait_for_completion_timeout(&dm_device.ol_waitevent, 5*HZ);
-		if (t == 0) {
-			pr_info("hot_add memory timedout\n");
-			has->ha_end_pfn -= HA_CHUNK;
-			has->covered_end_pfn -=  processed_pfn;
-			break;
-		}
+		wait_for_completion_timeout(&dm_device.ol_waitevent, 5*HZ);
 
 	}
 
-- 
1.7.4.1

[PATCH 2/2] Drivers: hv: balloon: Do not post pressure status if interrupted

[K. Y. Srinivasan]

When we are posting pressure status, we may get interrupted and handle
the un-balloon operation. In this case just don't post the status as we
know the pressure status is stale.

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

diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
index bfc7be2..4abaccf 100644
--- a/drivers/hv/hv_balloon.c
+++ b/drivers/hv/hv_balloon.c
@@ -972,6 +972,14 @@ static void post_status(struct hv_dynmem_device *dm)
 				dm->num_pages_ballooned +
 				compute_balloon_floor();
 
+	/*
+	 * If our transaction ID is no longer current, just don't
+	 * send the status. This can happen if we were interrupted
+	 * after we picked our transaction ID.
+	 */
+	if (status.hdr.trans_id != atomic_read(&trans_id))
+		return;
+
 	vmbus_sendpacket(dm->dev->channel, &status,
 				sizeof(struct dm_status),
 				(unsigned long)NULL,
-- 
1.7.4.1

Re: [PATCH 1/2] Drivers: hv: balloon: Fix a bug in the hot-add code

[Ben Hutchings]

On Fri, Jul 12, 2013 at 06:56:14AM -0700, K. Y. Srinivasan wrote:
> As we hot-add 128 MB chunks of memory, we wait to ensure that the memory
> is onlined before attempting to hot-add the next chunk. If the udev rule for
> memory hot-add is not executed within the allowed time, we would rollback the
> state and abort further hot-add. Since the hot-add has succeeded and the only
> failure is that the memory is not onlined within the allowed time, we should not
> be rolling back the state. Fix this bug.
[...]
>  		/*
>  		 * Wait for the memory block to be onlined.
>  		 */
> -		t = wait_for_completion_timeout(&dm_device.ol_waitevent, 5*HZ);
> -		if (t == 0) {
> -			pr_info("hot_add memory timedout\n");
> -			has->ha_end_pfn -= HA_CHUNK;
> -			has->covered_end_pfn -=  processed_pfn;
> -			break;
> -		}
> +		wait_for_completion_timeout(&dm_device.ol_waitevent, 5*HZ);
>  
>  	}
>  

Well now it might look like a bug that you don't test the result
of wait_for_completion_timeout().  Maybe update the comment to
explain why it's OK to continue anyway?

Ben.

-- 
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
                                                              - Albert Camus

RE: [PATCH 1/2] Drivers: hv: balloon: Fix a bug in the hot-add code

[KY Srinivasan]

> -----Original Message-----
> From: Ben Hutchings [mailto:ben@decadent.org.uk]
> Sent: Friday, July 12, 2013 4:17 PM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> jasowang@redhat.com; Stable
> Subject: Re: [PATCH 1/2] Drivers: hv: balloon: Fix a bug in the hot-add code
> 
> On Fri, Jul 12, 2013 at 06:56:14AM -0700, K. Y. Srinivasan wrote:
> > As we hot-add 128 MB chunks of memory, we wait to ensure that the memory
> > is onlined before attempting to hot-add the next chunk. If the udev rule for
> > memory hot-add is not executed within the allowed time, we would rollback
> the
> > state and abort further hot-add. Since the hot-add has succeeded and the only
> > failure is that the memory is not onlined within the allowed time, we should not
> > be rolling back the state. Fix this bug.
> [...]
> >  		/*
> >  		 * Wait for the memory block to be onlined.
> >  		 */
> > -		t = wait_for_completion_timeout(&dm_device.ol_waitevent,
> 5*HZ);
> > -		if (t == 0) {
> > -			pr_info("hot_add memory timedout\n");
> > -			has->ha_end_pfn -= HA_CHUNK;
> > -			has->covered_end_pfn -=  processed_pfn;
> > -			break;
> > -		}
> > +		wait_for_completion_timeout(&dm_device.ol_waitevent,
> 5*HZ);
> >
> >  	}
> >
> 
> Well now it might look like a bug that you don't test the result
> of wait_for_completion_timeout().  Maybe update the comment to
> explain why it's OK to continue anyway?

I put in the comment in the patch explaining why it is ok to continue. To reiterate,
it is ok to continue because hot add has succeeded. More importantly, what I was
doing earlier - rolling back the state when in fact hot add had succeeded was incorrect.

Regards,

K. Y
> 
> Ben.
> 
> --
> Ben Hutchings
> We get into the habit of living before acquiring the habit of thinking.
>                                                               - Albert Camus
> 

Re: [PATCH 1/2] Drivers: hv: balloon: Fix a bug in the hot-add code

[Ben Hutchings]

On Fri, Jul 12, 2013 at 09:07:19PM +0000, KY Srinivasan wrote:
[...]
> > Well now it might look like a bug that you don't test the result
> > of wait_for_completion_timeout().  Maybe update the comment to
> > explain why it's OK to continue anyway?
> 
> I put in the comment in the patch explaining why it is ok to continue.
[...]

But that is not nearly as easy to see as the comment that is
already *in the code* which your patch isn't updating.

Ben.

-- 
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
                                                              - Albert Camus

RE: [PATCH 1/2] Drivers: hv: balloon: Fix a bug in the hot-add code

[KY Srinivasan]

> -----Original Message-----
> From: Ben Hutchings [mailto:ben@decadent.org.uk]
> Sent: Friday, July 12, 2013 5:13 PM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> jasowang@redhat.com; Stable
> Subject: Re: [PATCH 1/2] Drivers: hv: balloon: Fix a bug in the hot-add code
> 
> On Fri, Jul 12, 2013 at 09:07:19PM +0000, KY Srinivasan wrote:
> [...]
> > > Well now it might look like a bug that you don't test the result
> > > of wait_for_completion_timeout().  Maybe update the comment to
> > > explain why it's OK to continue anyway?
> >
> > I put in the comment in the patch explaining why it is ok to continue.
> [...]
> 
> But that is not nearly as easy to see as the comment that is
> already *in the code* which your patch isn't updating.

Agreed; I will resend the patch with comments added.

Thanks,

K. Y
> 
> Ben.
> 
> --
> Ben Hutchings
> We get into the habit of living before acquiring the habit of thinking.
>                                                               - Albert Camus
>