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

[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
>