[PATCH] xen: fix booting ballooned down hvm guest

[PATCH] xen: fix booting ballooned down hvm guest

[Juergen Gross]

Commit 96edd61dcf44362d3ef0bed1a5361e0ac7886a63 ("xen/balloon: don't
online new memory initially") introduced a regression when booting a
HVM domain with memory less than mem-max: instead of ballooning down
immediately the system would try to use the memory up to mem-max
resulting in Xen crashing the domain.

For HVM domains the current size will be reflected in Xenstore node
memory/static-max instead of memory/target.

Additionally we have to trigger the ballooning process at once.

Cc: <stable@vger.kernel.org> # 4.13
Fixes: 96edd61dcf44362d3ef0bed1a5361e0ac7886a63 ("xen/balloon: don't
       online new memory initially")

Suggested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/xen-balloon.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/xen/xen-balloon.c b/drivers/xen/xen-balloon.c
index e89136ab851e..3745748d9644 100644
--- a/drivers/xen/xen-balloon.c
+++ b/drivers/xen/xen-balloon.c
@@ -57,7 +57,7 @@ static int register_balloon(struct device *dev);
 static void watch_target(struct xenbus_watch *watch,
 			 const char *path, const char *token)
 {
-	unsigned long long new_target;
+	unsigned long long new_target, static_max;
 	int err;
 	static bool watch_fired;
 	static long target_diff;
@@ -72,13 +72,19 @@ static void watch_target(struct xenbus_watch *watch,
 	 * pages. PAGE_SHIFT converts bytes to pages, hence PAGE_SHIFT - 10.
 	 */
 	new_target >>= PAGE_SHIFT - 10;
-	if (watch_fired) {
-		balloon_set_new_target(new_target - target_diff);
-		return;
+
+	if (!watch_fired) {
+		watch_fired = true;
+		err = xenbus_scanf(XBT_NIL, "memory", "static-max", "%llu",
+				   &static_max);
+		if (err != 1)
+			static_max = new_target;
+		static_max >>= PAGE_SHIFT - 10;
+		target_diff = xen_pv_domain() ? 0
+				: static_max - balloon_stats.target_pages;
 	}
 
-	watch_fired = true;
-	target_diff = new_target - balloon_stats.target_pages;
+	balloon_set_new_target(new_target - target_diff);
 }
 static struct xenbus_watch target_watch = {
 	.node = "memory/target",
-- 
2.12.3

Re: [PATCH] xen: fix booting ballooned down hvm guest

[Boris Ostrovsky]

On 10/24/2017 04:10 AM, Juergen Gross wrote:
> Commit 96edd61dcf44362d3ef0bed1a5361e0ac7886a63 ("xen/balloon: don't
> online new memory initially") introduced a regression when booting a
> HVM domain with memory less than mem-max: instead of ballooning down
> immediately the system would try to use the memory up to mem-max
> resulting in Xen crashing the domain.
>
> For HVM domains the current size will be reflected in Xenstore node
> memory/static-max instead of memory/target.
>
> Additionally we have to trigger the ballooning process at once.
>
> Cc: <stable@vger.kernel.org> # 4.13
> Fixes: 96edd61dcf44362d3ef0bed1a5361e0ac7886a63 ("xen/balloon: don't
>        online new memory initially")
>
> Suggested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reported-by: HW42 <hw42@ipsumj.de>

> ---
>  drivers/xen/xen-balloon.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/xen/xen-balloon.c b/drivers/xen/xen-balloon.c
> index e89136ab851e..3745748d9644 100644
> --- a/drivers/xen/xen-balloon.c
> +++ b/drivers/xen/xen-balloon.c
> @@ -57,7 +57,7 @@ static int register_balloon(struct device *dev);
>  static void watch_target(struct xenbus_watch *watch,
>  			 const char *path, const char *token)
>  {
> -	unsigned long long new_target;
> +	unsigned long long new_target, static_max;
>  	int err;
>  	static bool watch_fired;
>  	static long target_diff;
> @@ -72,13 +72,19 @@ static void watch_target(struct xenbus_watch *watch,
>  	 * pages. PAGE_SHIFT converts bytes to pages, hence PAGE_SHIFT - 10.
>  	 */
>  	new_target >>= PAGE_SHIFT - 10;
> -	if (watch_fired) {
> -		balloon_set_new_target(new_target - target_diff);
> -		return;
> +
> +	if (!watch_fired) {
> +		watch_fired = true;
> +		err = xenbus_scanf(XBT_NIL, "memory", "static-max", "%llu",
> +				   &static_max);
> +		if (err != 1)
> +			static_max = new_target;
> +		static_max >>= PAGE_SHIFT - 10;

if you set static_max to new_target you've already done the shift,
haven't you?


> +		target_diff = xen_pv_domain() ? 0

Why do we special-case PV?

-boris

> +				: static_max - balloon_stats.target_pages;
>  	}
>  
> -	watch_fired = true;
> -	target_diff = new_target - balloon_stats.target_pages;
> +	balloon_set_new_target(new_target - target_diff);
>  }
>  static struct xenbus_watch target_watch = {
>  	.node = "memory/target",

Re: [PATCH] xen: fix booting ballooned down hvm guest

[Juergen Gross]

On 24/10/17 16:33, Boris Ostrovsky wrote:
> On 10/24/2017 04:10 AM, Juergen Gross wrote:
>> Commit 96edd61dcf44362d3ef0bed1a5361e0ac7886a63 ("xen/balloon: don't
>> online new memory initially") introduced a regression when booting a
>> HVM domain with memory less than mem-max: instead of ballooning down
>> immediately the system would try to use the memory up to mem-max
>> resulting in Xen crashing the domain.
>>
>> For HVM domains the current size will be reflected in Xenstore node
>> memory/static-max instead of memory/target.
>>
>> Additionally we have to trigger the ballooning process at once.
>>
>> Cc: <stable@vger.kernel.org> # 4.13
>> Fixes: 96edd61dcf44362d3ef0bed1a5361e0ac7886a63 ("xen/balloon: don't
>>        online new memory initially")
>>
>> Suggested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Reported-by: HW42 <hw42@ipsumj.de>

Hmm, is an anonymous Reported-by: tag okay?

> 
>> ---
>>  drivers/xen/xen-balloon.c | 18 ++++++++++++------
>>  1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/xen/xen-balloon.c b/drivers/xen/xen-balloon.c
>> index e89136ab851e..3745748d9644 100644
>> --- a/drivers/xen/xen-balloon.c
>> +++ b/drivers/xen/xen-balloon.c
>> @@ -57,7 +57,7 @@ static int register_balloon(struct device *dev);
>>  static void watch_target(struct xenbus_watch *watch,
>>  			 const char *path, const char *token)
>>  {
>> -	unsigned long long new_target;
>> +	unsigned long long new_target, static_max;
>>  	int err;
>>  	static bool watch_fired;
>>  	static long target_diff;
>> @@ -72,13 +72,19 @@ static void watch_target(struct xenbus_watch *watch,
>>  	 * pages. PAGE_SHIFT converts bytes to pages, hence PAGE_SHIFT - 10.
>>  	 */
>>  	new_target >>= PAGE_SHIFT - 10;
>> -	if (watch_fired) {
>> -		balloon_set_new_target(new_target - target_diff);
>> -		return;
>> +
>> +	if (!watch_fired) {
>> +		watch_fired = true;
>> +		err = xenbus_scanf(XBT_NIL, "memory", "static-max", "%llu",
>> +				   &static_max);
>> +		if (err != 1)
>> +			static_max = new_target;
>> +		static_max >>= PAGE_SHIFT - 10;
> 
> if you set static_max to new_target you've already done the shift,
> haven't you?

Aah, right. I moved reading static-max down into the if without
adjustment after having it right at the start of the function
initially. Thanks for catching this.

>> +		target_diff = xen_pv_domain() ? 0
> 
> Why do we special-case PV?

Because the initial value of balloon_stats.target_pages is special-cased
for PV, too.


Juergen

Re: [PATCH] xen: fix booting ballooned down hvm guest

[Boris Ostrovsky]

On 10/24/2017 10:41 AM, Juergen Gross wrote:
> On 24/10/17 16:33, Boris Ostrovsky wrote:
>> On 10/24/2017 04:10 AM, Juergen Gross wrote:
>>> Commit 96edd61dcf44362d3ef0bed1a5361e0ac7886a63 ("xen/balloon: don't
>>> online new memory initially") introduced a regression when booting a
>>> HVM domain with memory less than mem-max: instead of ballooning down
>>> immediately the system would try to use the memory up to mem-max
>>> resulting in Xen crashing the domain.
>>>
>>> For HVM domains the current size will be reflected in Xenstore node
>>> memory/static-max instead of memory/target.
>>>
>>> Additionally we have to trigger the ballooning process at once.
>>>
>>> Cc: <stable@vger.kernel.org> # 4.13
>>> Fixes: 96edd61dcf44362d3ef0bed1a5361e0ac7886a63 ("xen/balloon: don't
>>>        online new memory initially")
>>>
>>> Suggested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> Reported-by: HW42 <hw42@ipsumj.de>
> Hmm, is an anonymous Reported-by: tag okay?

Oh, I don't know what the rules are for this kind of address. I'd
probably still add it but it's up to you.

-boris

Re: [PATCH] xen: fix booting ballooned down hvm guest

[Juergen Gross]

On 24/10/17 16:56, Boris Ostrovsky wrote:
> On 10/24/2017 10:41 AM, Juergen Gross wrote:
>> On 24/10/17 16:33, Boris Ostrovsky wrote:
>>> On 10/24/2017 04:10 AM, Juergen Gross wrote:
>>>> Commit 96edd61dcf44362d3ef0bed1a5361e0ac7886a63 ("xen/balloon: don't
>>>> online new memory initially") introduced a regression when booting a
>>>> HVM domain with memory less than mem-max: instead of ballooning down
>>>> immediately the system would try to use the memory up to mem-max
>>>> resulting in Xen crashing the domain.
>>>>
>>>> For HVM domains the current size will be reflected in Xenstore node
>>>> memory/static-max instead of memory/target.
>>>>
>>>> Additionally we have to trigger the ballooning process at once.
>>>>
>>>> Cc: <stable@vger.kernel.org> # 4.13
>>>> Fixes: 96edd61dcf44362d3ef0bed1a5361e0ac7886a63 ("xen/balloon: don't
>>>>        online new memory initially")
>>>>
>>>> Suggested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> Reported-by: HW42 <hw42@ipsumj.de>
>> Hmm, is an anonymous Reported-by: tag okay?
> 
> Oh, I don't know what the rules are for this kind of address. I'd
> probably still add it but it's up to you.

The docs say each tag should be:

  tag: Full Name <email address>  optional-other-stuff

I don't think HW42 can be regarded to be a full name.


Juergen

Re: [PATCH] xen: fix booting ballooned down hvm guest

[HW42]

[-- Attachment #1.1: Type: text/plain, Size: 1613 bytes --]

Juergen Gross:
> On 24/10/17 16:56, Boris Ostrovsky wrote:
>> On 10/24/2017 10:41 AM, Juergen Gross wrote:
>>> On 24/10/17 16:33, Boris Ostrovsky wrote:
>>>> On 10/24/2017 04:10 AM, Juergen Gross wrote:
>>>>> Commit 96edd61dcf44362d3ef0bed1a5361e0ac7886a63 ("xen/balloon: don't
>>>>> online new memory initially") introduced a regression when booting a
>>>>> HVM domain with memory less than mem-max: instead of ballooning down
>>>>> immediately the system would try to use the memory up to mem-max
>>>>> resulting in Xen crashing the domain.
>>>>>
>>>>> For HVM domains the current size will be reflected in Xenstore node
>>>>> memory/static-max instead of memory/target.
>>>>>
>>>>> Additionally we have to trigger the ballooning process at once.
>>>>>
>>>>> Cc: <stable@vger.kernel.org> # 4.13
>>>>> Fixes: 96edd61dcf44362d3ef0bed1a5361e0ac7886a63 ("xen/balloon: don't
>>>>>        online new memory initially")
>>>>>
>>>>> Suggested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>> Reported-by: HW42 <hw42@ipsumj.de>
>>> Hmm, is an anonymous Reported-by: tag okay?
>>
>> Oh, I don't know what the rules are for this kind of address. I'd
>> probably still add it but it's up to you.
> 
> The docs say each tag should be:
> 
>   tag: Full Name <email address>  optional-other-stuff
> 
> I don't think HW42 can be regarded to be a full name.

I think you really should allow pseudonymous contributions. But in my
case my nickname is anyway linked to my legal name so fell free to use:
Simon Gaiser <hw42@ipsumj.de>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH] xen: fix booting ballooned down hvm guest

[Juergen Gross]

On 24/10/17 16:59, HW42 wrote:
> Juergen Gross:
>> On 24/10/17 16:56, Boris Ostrovsky wrote:
>>> On 10/24/2017 10:41 AM, Juergen Gross wrote:
>>>> On 24/10/17 16:33, Boris Ostrovsky wrote:
>>>>> On 10/24/2017 04:10 AM, Juergen Gross wrote:
>>>>>> Commit 96edd61dcf44362d3ef0bed1a5361e0ac7886a63 ("xen/balloon: don't
>>>>>> online new memory initially") introduced a regression when booting a
>>>>>> HVM domain with memory less than mem-max: instead of ballooning down
>>>>>> immediately the system would try to use the memory up to mem-max
>>>>>> resulting in Xen crashing the domain.
>>>>>>
>>>>>> For HVM domains the current size will be reflected in Xenstore node
>>>>>> memory/static-max instead of memory/target.
>>>>>>
>>>>>> Additionally we have to trigger the ballooning process at once.
>>>>>>
>>>>>> Cc: <stable@vger.kernel.org> # 4.13
>>>>>> Fixes: 96edd61dcf44362d3ef0bed1a5361e0ac7886a63 ("xen/balloon: don't
>>>>>>        online new memory initially")
>>>>>>
>>>>>> Suggested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>> Reported-by: HW42 <hw42@ipsumj.de>
>>>> Hmm, is an anonymous Reported-by: tag okay?
>>>
>>> Oh, I don't know what the rules are for this kind of address. I'd
>>> probably still add it but it's up to you.
>>
>> The docs say each tag should be:
>>
>>   tag: Full Name <email address>  optional-other-stuff
>>
>> I don't think HW42 can be regarded to be a full name.
> 
> I think you really should allow pseudonymous contributions. But in my
> case my nickname is anyway linked to my legal name so fell free to use:
> Simon Gaiser <hw42@ipsumj.de>
> 

Thanks, will add it in V2.


Juergen

Re: [PATCH] xen: fix booting ballooned down hvm guest

[Willy Tarreau]

On Tue, Oct 24, 2017 at 02:59:00PM +0000, HW42 wrote:
> I think you really should allow pseudonymous contributions. But in my
> case my nickname is anyway linked to my legal name so fell free to use:
> Simon Gaiser <hw42@ipsumj.de>

I personally always have difficulties with pseudos in emails, I think
it's just a matter of respect for all other contributors.  It's easier
to write to a contributor using "Simon what's your opinion" than
"XW;75_@!XVn what's your opinion". Just like your boss probably
doesn't call you "HW42", it's understandable that the people having
to deal with your work might prefer to call you with your real name
as well.

Willy