[PATCH v3] xen/balloon: don't online new memory initially

[PATCH v3] xen/balloon: don't online new memory initially

[Juergen Gross]

When setting up the Xenstore watch for the memory target size the new
watch will fire at once. Don't try to reach the configured target size
by onlining new memory in this case, as the current memory size will
be smaller in almost all cases due to e.g. BIOS reserved pages.

Onlining new memory will lead to more problems e.g. undesired conflicts
with NVMe devices meant to be operated as block devices.

Instead remember the difference between target size and current size
when the watch fires for the first time and apply it to any further
size changes, too.

In order to avoid races between balloon.c and xen-balloon.c init calls
do the xen-balloon.c initialization from balloon.c.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/balloon.c     |  3 +++
 drivers/xen/xen-balloon.c | 22 ++++++++++++----------
 include/xen/balloon.h     |  8 ++++++++
 3 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index 50dcb68d8070..ab609255a0f3 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -780,6 +780,9 @@ static int __init balloon_init(void)
 	}
 #endif
 
+	/* Init the xen-balloon driver. */
+	xen_balloon_init();
+
 	return 0;
 }
 subsys_initcall(balloon_init);
diff --git a/drivers/xen/xen-balloon.c b/drivers/xen/xen-balloon.c
index e7715cb62eef..e89136ab851e 100644
--- a/drivers/xen/xen-balloon.c
+++ b/drivers/xen/xen-balloon.c
@@ -59,6 +59,8 @@ static void watch_target(struct xenbus_watch *watch,
 {
 	unsigned long long new_target;
 	int err;
+	static bool watch_fired;
+	static long target_diff;
 
 	err = xenbus_scanf(XBT_NIL, "memory", "target", "%llu", &new_target);
 	if (err != 1) {
@@ -69,7 +71,14 @@ static void watch_target(struct xenbus_watch *watch,
 	/* The given memory/target value is in KiB, so it needs converting to
 	 * pages. PAGE_SHIFT converts bytes to pages, hence PAGE_SHIFT - 10.
 	 */
-	balloon_set_new_target(new_target >> (PAGE_SHIFT - 10));
+	new_target >>= PAGE_SHIFT - 10;
+	if (watch_fired) {
+		balloon_set_new_target(new_target - target_diff);
+		return;
+	}
+
+	watch_fired = true;
+	target_diff = new_target - balloon_stats.target_pages;
 }
 static struct xenbus_watch target_watch = {
 	.node = "memory/target",
@@ -94,22 +103,15 @@ static struct notifier_block xenstore_notifier = {
 	.notifier_call = balloon_init_watcher,
 };
 
-static int __init balloon_init(void)
+void xen_balloon_init(void)
 {
-	if (!xen_domain())
-		return -ENODEV;
-
-	pr_info("Initialising balloon driver\n");
-
 	register_balloon(&balloon_dev);
 
 	register_xen_selfballooning(&balloon_dev);
 
 	register_xenstore_notifier(&xenstore_notifier);
-
-	return 0;
 }
-subsys_initcall(balloon_init);
+EXPORT_SYMBOL_GPL(xen_balloon_init);
 
 #define BALLOON_SHOW(name, format, args...)				\
 	static ssize_t show_##name(struct device *dev,			\
diff --git a/include/xen/balloon.h b/include/xen/balloon.h
index d1767dfb0d95..8906361bb50c 100644
--- a/include/xen/balloon.h
+++ b/include/xen/balloon.h
@@ -35,3 +35,11 @@ static inline int register_xen_selfballooning(struct device *dev)
 	return -ENOSYS;
 }
 #endif
+
+#ifdef CONFIG_XEN_BALLOON
+void xen_balloon_init(void);
+#else
+static inline void xen_balloon_init(void)
+{
+}
+#endif
-- 
2.12.3

Re: [PATCH v3] xen/balloon: don't online new memory initially

[Boris Ostrovsky]

On 07/10/2017 04:10 AM, Juergen Gross wrote:
> When setting up the Xenstore watch for the memory target size the new
> watch will fire at once. Don't try to reach the configured target size
> by onlining new memory in this case, as the current memory size will
> be smaller in almost all cases due to e.g. BIOS reserved pages.
>
> Onlining new memory will lead to more problems e.g. undesired conflicts
> with NVMe devices meant to be operated as block devices.
>
> Instead remember the difference between target size and current size
> when the watch fires for the first time and apply it to any further
> size changes, too.

I don't think I understand how deferring setting the target until next
watch firing will help with the original problem. Could you explain?

Thanks.
-boris


>
> In order to avoid races between balloon.c and xen-balloon.c init calls
> do the xen-balloon.c initialization from balloon.c.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  drivers/xen/balloon.c     |  3 +++
>  drivers/xen/xen-balloon.c | 22 ++++++++++++----------
>  include/xen/balloon.h     |  8 ++++++++
>  3 files changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index 50dcb68d8070..ab609255a0f3 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -780,6 +780,9 @@ static int __init balloon_init(void)
>  	}
>  #endif
>  
> +	/* Init the xen-balloon driver. */
> +	xen_balloon_init();
> +
>  	return 0;
>  }
>  subsys_initcall(balloon_init);
> diff --git a/drivers/xen/xen-balloon.c b/drivers/xen/xen-balloon.c
> index e7715cb62eef..e89136ab851e 100644
> --- a/drivers/xen/xen-balloon.c
> +++ b/drivers/xen/xen-balloon.c
> @@ -59,6 +59,8 @@ static void watch_target(struct xenbus_watch *watch,
>  {
>  	unsigned long long new_target;
>  	int err;
> +	static bool watch_fired;
> +	static long target_diff;
>  
>  	err = xenbus_scanf(XBT_NIL, "memory", "target", "%llu", &new_target);
>  	if (err != 1) {
> @@ -69,7 +71,14 @@ static void watch_target(struct xenbus_watch *watch,
>  	/* The given memory/target value is in KiB, so it needs converting to
>  	 * pages. PAGE_SHIFT converts bytes to pages, hence PAGE_SHIFT - 10.
>  	 */
> -	balloon_set_new_target(new_target >> (PAGE_SHIFT - 10));
> +	new_target >>= PAGE_SHIFT - 10;
> +	if (watch_fired) {
> +		balloon_set_new_target(new_target - target_diff);
> +		return;
> +	}
> +
> +	watch_fired = true;
> +	target_diff = new_target - balloon_stats.target_pages;
>  }
>  static struct xenbus_watch target_watch = {
>  	.node = "memory/target",
> @@ -94,22 +103,15 @@ static struct notifier_block xenstore_notifier = {
>  	.notifier_call = balloon_init_watcher,
>  };
>  
> -static int __init balloon_init(void)
> +void xen_balloon_init(void)
>  {
> -	if (!xen_domain())
> -		return -ENODEV;
> -
> -	pr_info("Initialising balloon driver\n");
> -
>  	register_balloon(&balloon_dev);
>  
>  	register_xen_selfballooning(&balloon_dev);
>  
>  	register_xenstore_notifier(&xenstore_notifier);
> -
> -	return 0;
>  }
> -subsys_initcall(balloon_init);
> +EXPORT_SYMBOL_GPL(xen_balloon_init);
>  
>  #define BALLOON_SHOW(name, format, args...)				\
>  	static ssize_t show_##name(struct device *dev,			\
> diff --git a/include/xen/balloon.h b/include/xen/balloon.h
> index d1767dfb0d95..8906361bb50c 100644
> --- a/include/xen/balloon.h
> +++ b/include/xen/balloon.h
> @@ -35,3 +35,11 @@ static inline int register_xen_selfballooning(struct device *dev)
>  	return -ENOSYS;
>  }
>  #endif
> +
> +#ifdef CONFIG_XEN_BALLOON
> +void xen_balloon_init(void);
> +#else
> +static inline void xen_balloon_init(void)
> +{
> +}
> +#endif

Re: [PATCH v3] xen/balloon: don't online new memory initially

[Boris Ostrovsky]

On 07/10/2017 04:10 AM, Juergen Gross wrote:
> When setting up the Xenstore watch for the memory target size the new
> watch will fire at once. Don't try to reach the configured target size
> by onlining new memory in this case, as the current memory size will
> be smaller in almost all cases due to e.g. BIOS reserved pages.
>
> Onlining new memory will lead to more problems e.g. undesired conflicts
> with NVMe devices meant to be operated as block devices.
>
> Instead remember the difference between target size and current size
> when the watch fires for the first time and apply it to any further
> size changes, too.

I don't think I understand how deferring setting the target until next
watch firing will help with the original problem. Could you explain?

Thanks.
-boris


>
> In order to avoid races between balloon.c and xen-balloon.c init calls
> do the xen-balloon.c initialization from balloon.c.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  drivers/xen/balloon.c     |  3 +++
>  drivers/xen/xen-balloon.c | 22 ++++++++++++----------
>  include/xen/balloon.h     |  8 ++++++++
>  3 files changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index 50dcb68d8070..ab609255a0f3 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -780,6 +780,9 @@ static int __init balloon_init(void)
>  	}
>  #endif
>  
> +	/* Init the xen-balloon driver. */
> +	xen_balloon_init();
> +
>  	return 0;
>  }
>  subsys_initcall(balloon_init);
> diff --git a/drivers/xen/xen-balloon.c b/drivers/xen/xen-balloon.c
> index e7715cb62eef..e89136ab851e 100644
> --- a/drivers/xen/xen-balloon.c
> +++ b/drivers/xen/xen-balloon.c
> @@ -59,6 +59,8 @@ static void watch_target(struct xenbus_watch *watch,
>  {
>  	unsigned long long new_target;
>  	int err;
> +	static bool watch_fired;
> +	static long target_diff;
>  
>  	err = xenbus_scanf(XBT_NIL, "memory", "target", "%llu", &new_target);
>  	if (err != 1) {
> @@ -69,7 +71,14 @@ static void watch_target(struct xenbus_watch *watch,
>  	/* The given memory/target value is in KiB, so it needs converting to
>  	 * pages. PAGE_SHIFT converts bytes to pages, hence PAGE_SHIFT - 10.
>  	 */
> -	balloon_set_new_target(new_target >> (PAGE_SHIFT - 10));
> +	new_target >>= PAGE_SHIFT - 10;
> +	if (watch_fired) {
> +		balloon_set_new_target(new_target - target_diff);
> +		return;
> +	}
> +
> +	watch_fired = true;
> +	target_diff = new_target - balloon_stats.target_pages;
>  }
>  static struct xenbus_watch target_watch = {
>  	.node = "memory/target",
> @@ -94,22 +103,15 @@ static struct notifier_block xenstore_notifier = {
>  	.notifier_call = balloon_init_watcher,
>  };
>  
> -static int __init balloon_init(void)
> +void xen_balloon_init(void)
>  {
> -	if (!xen_domain())
> -		return -ENODEV;
> -
> -	pr_info("Initialising balloon driver\n");
> -
>  	register_balloon(&balloon_dev);
>  
>  	register_xen_selfballooning(&balloon_dev);
>  
>  	register_xenstore_notifier(&xenstore_notifier);
> -
> -	return 0;
>  }
> -subsys_initcall(balloon_init);
> +EXPORT_SYMBOL_GPL(xen_balloon_init);
>  
>  #define BALLOON_SHOW(name, format, args...)				\
>  	static ssize_t show_##name(struct device *dev,			\
> diff --git a/include/xen/balloon.h b/include/xen/balloon.h
> index d1767dfb0d95..8906361bb50c 100644
> --- a/include/xen/balloon.h
> +++ b/include/xen/balloon.h
> @@ -35,3 +35,11 @@ static inline int register_xen_selfballooning(struct device *dev)
>  	return -ENOSYS;
>  }
>  #endif
> +
> +#ifdef CONFIG_XEN_BALLOON
> +void xen_balloon_init(void);
> +#else
> +static inline void xen_balloon_init(void)
> +{
> +}
> +#endif


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v3] xen/balloon: don't online new memory initially

[Juergen Gross]

On 18/07/17 18:08, Boris Ostrovsky wrote:
> On 07/10/2017 04:10 AM, Juergen Gross wrote:
>> When setting up the Xenstore watch for the memory target size the new
>> watch will fire at once. Don't try to reach the configured target size
>> by onlining new memory in this case, as the current memory size will
>> be smaller in almost all cases due to e.g. BIOS reserved pages.
>>
>> Onlining new memory will lead to more problems e.g. undesired conflicts
>> with NVMe devices meant to be operated as block devices.
>>
>> Instead remember the difference between target size and current size
>> when the watch fires for the first time and apply it to any further
>> size changes, too.
> 
> I don't think I understand how deferring setting the target until next
> watch firing will help with the original problem. Could you explain?

The difference of target and current size will be evaluated at the
time the watch fires for the first time. This difference is subtracted
from the target on later watch events.


Juergen

> 
> Thanks.
> -boris
> 
> 
>>
>> In order to avoid races between balloon.c and xen-balloon.c init calls
>> do the xen-balloon.c initialization from balloon.c.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  drivers/xen/balloon.c     |  3 +++
>>  drivers/xen/xen-balloon.c | 22 ++++++++++++----------
>>  include/xen/balloon.h     |  8 ++++++++
>>  3 files changed, 23 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
>> index 50dcb68d8070..ab609255a0f3 100644
>> --- a/drivers/xen/balloon.c
>> +++ b/drivers/xen/balloon.c
>> @@ -780,6 +780,9 @@ static int __init balloon_init(void)
>>  	}
>>  #endif
>>  
>> +	/* Init the xen-balloon driver. */
>> +	xen_balloon_init();
>> +
>>  	return 0;
>>  }
>>  subsys_initcall(balloon_init);
>> diff --git a/drivers/xen/xen-balloon.c b/drivers/xen/xen-balloon.c
>> index e7715cb62eef..e89136ab851e 100644
>> --- a/drivers/xen/xen-balloon.c
>> +++ b/drivers/xen/xen-balloon.c
>> @@ -59,6 +59,8 @@ static void watch_target(struct xenbus_watch *watch,
>>  {
>>  	unsigned long long new_target;
>>  	int err;
>> +	static bool watch_fired;
>> +	static long target_diff;
>>  
>>  	err = xenbus_scanf(XBT_NIL, "memory", "target", "%llu", &new_target);
>>  	if (err != 1) {
>> @@ -69,7 +71,14 @@ static void watch_target(struct xenbus_watch *watch,
>>  	/* The given memory/target value is in KiB, so it needs converting to
>>  	 * pages. PAGE_SHIFT converts bytes to pages, hence PAGE_SHIFT - 10.
>>  	 */
>> -	balloon_set_new_target(new_target >> (PAGE_SHIFT - 10));
>> +	new_target >>= PAGE_SHIFT - 10;
>> +	if (watch_fired) {
>> +		balloon_set_new_target(new_target - target_diff);
>> +		return;
>> +	}
>> +
>> +	watch_fired = true;
>> +	target_diff = new_target - balloon_stats.target_pages;
>>  }
>>  static struct xenbus_watch target_watch = {
>>  	.node = "memory/target",
>> @@ -94,22 +103,15 @@ static struct notifier_block xenstore_notifier = {
>>  	.notifier_call = balloon_init_watcher,
>>  };
>>  
>> -static int __init balloon_init(void)
>> +void xen_balloon_init(void)
>>  {
>> -	if (!xen_domain())
>> -		return -ENODEV;
>> -
>> -	pr_info("Initialising balloon driver\n");
>> -
>>  	register_balloon(&balloon_dev);
>>  
>>  	register_xen_selfballooning(&balloon_dev);
>>  
>>  	register_xenstore_notifier(&xenstore_notifier);
>> -
>> -	return 0;
>>  }
>> -subsys_initcall(balloon_init);
>> +EXPORT_SYMBOL_GPL(xen_balloon_init);
>>  
>>  #define BALLOON_SHOW(name, format, args...)				\
>>  	static ssize_t show_##name(struct device *dev,			\
>> diff --git a/include/xen/balloon.h b/include/xen/balloon.h
>> index d1767dfb0d95..8906361bb50c 100644
>> --- a/include/xen/balloon.h
>> +++ b/include/xen/balloon.h
>> @@ -35,3 +35,11 @@ static inline int register_xen_selfballooning(struct device *dev)
>>  	return -ENOSYS;
>>  }
>>  #endif
>> +
>> +#ifdef CONFIG_XEN_BALLOON
>> +void xen_balloon_init(void);
>> +#else
>> +static inline void xen_balloon_init(void)
>> +{
>> +}
>> +#endif
> 
> 

Re: [PATCH v3] xen/balloon: don't online new memory initially

[Juergen Gross]

On 18/07/17 18:08, Boris Ostrovsky wrote:
> On 07/10/2017 04:10 AM, Juergen Gross wrote:
>> When setting up the Xenstore watch for the memory target size the new
>> watch will fire at once. Don't try to reach the configured target size
>> by onlining new memory in this case, as the current memory size will
>> be smaller in almost all cases due to e.g. BIOS reserved pages.
>>
>> Onlining new memory will lead to more problems e.g. undesired conflicts
>> with NVMe devices meant to be operated as block devices.
>>
>> Instead remember the difference between target size and current size
>> when the watch fires for the first time and apply it to any further
>> size changes, too.
> 
> I don't think I understand how deferring setting the target until next
> watch firing will help with the original problem. Could you explain?

The difference of target and current size will be evaluated at the
time the watch fires for the first time. This difference is subtracted
from the target on later watch events.


Juergen

> 
> Thanks.
> -boris
> 
> 
>>
>> In order to avoid races between balloon.c and xen-balloon.c init calls
>> do the xen-balloon.c initialization from balloon.c.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  drivers/xen/balloon.c     |  3 +++
>>  drivers/xen/xen-balloon.c | 22 ++++++++++++----------
>>  include/xen/balloon.h     |  8 ++++++++
>>  3 files changed, 23 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
>> index 50dcb68d8070..ab609255a0f3 100644
>> --- a/drivers/xen/balloon.c
>> +++ b/drivers/xen/balloon.c
>> @@ -780,6 +780,9 @@ static int __init balloon_init(void)
>>  	}
>>  #endif
>>  
>> +	/* Init the xen-balloon driver. */
>> +	xen_balloon_init();
>> +
>>  	return 0;
>>  }
>>  subsys_initcall(balloon_init);
>> diff --git a/drivers/xen/xen-balloon.c b/drivers/xen/xen-balloon.c
>> index e7715cb62eef..e89136ab851e 100644
>> --- a/drivers/xen/xen-balloon.c
>> +++ b/drivers/xen/xen-balloon.c
>> @@ -59,6 +59,8 @@ static void watch_target(struct xenbus_watch *watch,
>>  {
>>  	unsigned long long new_target;
>>  	int err;
>> +	static bool watch_fired;
>> +	static long target_diff;
>>  
>>  	err = xenbus_scanf(XBT_NIL, "memory", "target", "%llu", &new_target);
>>  	if (err != 1) {
>> @@ -69,7 +71,14 @@ static void watch_target(struct xenbus_watch *watch,
>>  	/* The given memory/target value is in KiB, so it needs converting to
>>  	 * pages. PAGE_SHIFT converts bytes to pages, hence PAGE_SHIFT - 10.
>>  	 */
>> -	balloon_set_new_target(new_target >> (PAGE_SHIFT - 10));
>> +	new_target >>= PAGE_SHIFT - 10;
>> +	if (watch_fired) {
>> +		balloon_set_new_target(new_target - target_diff);
>> +		return;
>> +	}
>> +
>> +	watch_fired = true;
>> +	target_diff = new_target - balloon_stats.target_pages;
>>  }
>>  static struct xenbus_watch target_watch = {
>>  	.node = "memory/target",
>> @@ -94,22 +103,15 @@ static struct notifier_block xenstore_notifier = {
>>  	.notifier_call = balloon_init_watcher,
>>  };
>>  
>> -static int __init balloon_init(void)
>> +void xen_balloon_init(void)
>>  {
>> -	if (!xen_domain())
>> -		return -ENODEV;
>> -
>> -	pr_info("Initialising balloon driver\n");
>> -
>>  	register_balloon(&balloon_dev);
>>  
>>  	register_xen_selfballooning(&balloon_dev);
>>  
>>  	register_xenstore_notifier(&xenstore_notifier);
>> -
>> -	return 0;
>>  }
>> -subsys_initcall(balloon_init);
>> +EXPORT_SYMBOL_GPL(xen_balloon_init);
>>  
>>  #define BALLOON_SHOW(name, format, args...)				\
>>  	static ssize_t show_##name(struct device *dev,			\
>> diff --git a/include/xen/balloon.h b/include/xen/balloon.h
>> index d1767dfb0d95..8906361bb50c 100644
>> --- a/include/xen/balloon.h
>> +++ b/include/xen/balloon.h
>> @@ -35,3 +35,11 @@ static inline int register_xen_selfballooning(struct device *dev)
>>  	return -ENOSYS;
>>  }
>>  #endif
>> +
>> +#ifdef CONFIG_XEN_BALLOON
>> +void xen_balloon_init(void);
>> +#else
>> +static inline void xen_balloon_init(void)
>> +{
>> +}
>> +#endif
> 
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v3] xen/balloon: don't online new memory initially

[Boris Ostrovsky]

On 07/18/2017 12:12 PM, Juergen Gross wrote:
> On 18/07/17 18:08, Boris Ostrovsky wrote:
>> On 07/10/2017 04:10 AM, Juergen Gross wrote:
>>> When setting up the Xenstore watch for the memory target size the new
>>> watch will fire at once. Don't try to reach the configured target size
>>> by onlining new memory in this case, as the current memory size will
>>> be smaller in almost all cases due to e.g. BIOS reserved pages.
>>>
>>> Onlining new memory will lead to more problems e.g. undesired conflicts
>>> with NVMe devices meant to be operated as block devices.
>>>
>>> Instead remember the difference between target size and current size
>>> when the watch fires for the first time and apply it to any further
>>> size changes, too.
>> I don't think I understand how deferring setting the target until next
>> watch firing will help with the original problem. Could you explain?
> The difference of target and current size will be evaluated at the
> time the watch fires for the first time. This difference is subtracted
> from the target on later watch events.

OK, I clearly completely missed the point here.

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>



>
>
> Juergen
>
>> Thanks.
>> -boris
>>
>>
>>> In order to avoid races between balloon.c and xen-balloon.c init calls
>>> do the xen-balloon.c initialization from balloon.c.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>>  drivers/xen/balloon.c     |  3 +++
>>>  drivers/xen/xen-balloon.c | 22 ++++++++++++----------
>>>  include/xen/balloon.h     |  8 ++++++++
>>>  3 files changed, 23 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
>>> index 50dcb68d8070..ab609255a0f3 100644
>>> --- a/drivers/xen/balloon.c
>>> +++ b/drivers/xen/balloon.c
>>> @@ -780,6 +780,9 @@ static int __init balloon_init(void)
>>>  	}
>>>  #endif
>>>  
>>> +	/* Init the xen-balloon driver. */
>>> +	xen_balloon_init();
>>> +
>>>  	return 0;
>>>  }
>>>  subsys_initcall(balloon_init);
>>> diff --git a/drivers/xen/xen-balloon.c b/drivers/xen/xen-balloon.c
>>> index e7715cb62eef..e89136ab851e 100644
>>> --- a/drivers/xen/xen-balloon.c
>>> +++ b/drivers/xen/xen-balloon.c
>>> @@ -59,6 +59,8 @@ static void watch_target(struct xenbus_watch *watch,
>>>  {
>>>  	unsigned long long new_target;
>>>  	int err;
>>> +	static bool watch_fired;
>>> +	static long target_diff;
>>>  
>>>  	err = xenbus_scanf(XBT_NIL, "memory", "target", "%llu", &new_target);
>>>  	if (err != 1) {
>>> @@ -69,7 +71,14 @@ static void watch_target(struct xenbus_watch *watch,
>>>  	/* The given memory/target value is in KiB, so it needs converting to
>>>  	 * pages. PAGE_SHIFT converts bytes to pages, hence PAGE_SHIFT - 10.
>>>  	 */
>>> -	balloon_set_new_target(new_target >> (PAGE_SHIFT - 10));
>>> +	new_target >>= PAGE_SHIFT - 10;
>>> +	if (watch_fired) {
>>> +		balloon_set_new_target(new_target - target_diff);
>>> +		return;
>>> +	}
>>> +
>>> +	watch_fired = true;
>>> +	target_diff = new_target - balloon_stats.target_pages;
>>>  }
>>>  static struct xenbus_watch target_watch = {
>>>  	.node = "memory/target",
>>> @@ -94,22 +103,15 @@ static struct notifier_block xenstore_notifier = {
>>>  	.notifier_call = balloon_init_watcher,
>>>  };
>>>  
>>> -static int __init balloon_init(void)
>>> +void xen_balloon_init(void)
>>>  {
>>> -	if (!xen_domain())
>>> -		return -ENODEV;
>>> -
>>> -	pr_info("Initialising balloon driver\n");
>>> -
>>>  	register_balloon(&balloon_dev);
>>>  
>>>  	register_xen_selfballooning(&balloon_dev);
>>>  
>>>  	register_xenstore_notifier(&xenstore_notifier);
>>> -
>>> -	return 0;
>>>  }
>>> -subsys_initcall(balloon_init);
>>> +EXPORT_SYMBOL_GPL(xen_balloon_init);
>>>  
>>>  #define BALLOON_SHOW(name, format, args...)				\
>>>  	static ssize_t show_##name(struct device *dev,			\
>>> diff --git a/include/xen/balloon.h b/include/xen/balloon.h
>>> index d1767dfb0d95..8906361bb50c 100644
>>> --- a/include/xen/balloon.h
>>> +++ b/include/xen/balloon.h
>>> @@ -35,3 +35,11 @@ static inline int register_xen_selfballooning(struct device *dev)
>>>  	return -ENOSYS;
>>>  }
>>>  #endif
>>> +
>>> +#ifdef CONFIG_XEN_BALLOON
>>> +void xen_balloon_init(void);
>>> +#else
>>> +static inline void xen_balloon_init(void)
>>> +{
>>> +}
>>> +#endif
>>

Re: [PATCH v3] xen/balloon: don't online new memory initially

[Boris Ostrovsky]

On 07/18/2017 12:12 PM, Juergen Gross wrote:
> On 18/07/17 18:08, Boris Ostrovsky wrote:
>> On 07/10/2017 04:10 AM, Juergen Gross wrote:
>>> When setting up the Xenstore watch for the memory target size the new
>>> watch will fire at once. Don't try to reach the configured target size
>>> by onlining new memory in this case, as the current memory size will
>>> be smaller in almost all cases due to e.g. BIOS reserved pages.
>>>
>>> Onlining new memory will lead to more problems e.g. undesired conflicts
>>> with NVMe devices meant to be operated as block devices.
>>>
>>> Instead remember the difference between target size and current size
>>> when the watch fires for the first time and apply it to any further
>>> size changes, too.
>> I don't think I understand how deferring setting the target until next
>> watch firing will help with the original problem. Could you explain?
> The difference of target and current size will be evaluated at the
> time the watch fires for the first time. This difference is subtracted
> from the target on later watch events.

OK, I clearly completely missed the point here.

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>



>
>
> Juergen
>
>> Thanks.
>> -boris
>>
>>
>>> In order to avoid races between balloon.c and xen-balloon.c init calls
>>> do the xen-balloon.c initialization from balloon.c.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>>  drivers/xen/balloon.c     |  3 +++
>>>  drivers/xen/xen-balloon.c | 22 ++++++++++++----------
>>>  include/xen/balloon.h     |  8 ++++++++
>>>  3 files changed, 23 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
>>> index 50dcb68d8070..ab609255a0f3 100644
>>> --- a/drivers/xen/balloon.c
>>> +++ b/drivers/xen/balloon.c
>>> @@ -780,6 +780,9 @@ static int __init balloon_init(void)
>>>  	}
>>>  #endif
>>>  
>>> +	/* Init the xen-balloon driver. */
>>> +	xen_balloon_init();
>>> +
>>>  	return 0;
>>>  }
>>>  subsys_initcall(balloon_init);
>>> diff --git a/drivers/xen/xen-balloon.c b/drivers/xen/xen-balloon.c
>>> index e7715cb62eef..e89136ab851e 100644
>>> --- a/drivers/xen/xen-balloon.c
>>> +++ b/drivers/xen/xen-balloon.c
>>> @@ -59,6 +59,8 @@ static void watch_target(struct xenbus_watch *watch,
>>>  {
>>>  	unsigned long long new_target;
>>>  	int err;
>>> +	static bool watch_fired;
>>> +	static long target_diff;
>>>  
>>>  	err = xenbus_scanf(XBT_NIL, "memory", "target", "%llu", &new_target);
>>>  	if (err != 1) {
>>> @@ -69,7 +71,14 @@ static void watch_target(struct xenbus_watch *watch,
>>>  	/* The given memory/target value is in KiB, so it needs converting to
>>>  	 * pages. PAGE_SHIFT converts bytes to pages, hence PAGE_SHIFT - 10.
>>>  	 */
>>> -	balloon_set_new_target(new_target >> (PAGE_SHIFT - 10));
>>> +	new_target >>= PAGE_SHIFT - 10;
>>> +	if (watch_fired) {
>>> +		balloon_set_new_target(new_target - target_diff);
>>> +		return;
>>> +	}
>>> +
>>> +	watch_fired = true;
>>> +	target_diff = new_target - balloon_stats.target_pages;
>>>  }
>>>  static struct xenbus_watch target_watch = {
>>>  	.node = "memory/target",
>>> @@ -94,22 +103,15 @@ static struct notifier_block xenstore_notifier = {
>>>  	.notifier_call = balloon_init_watcher,
>>>  };
>>>  
>>> -static int __init balloon_init(void)
>>> +void xen_balloon_init(void)
>>>  {
>>> -	if (!xen_domain())
>>> -		return -ENODEV;
>>> -
>>> -	pr_info("Initialising balloon driver\n");
>>> -
>>>  	register_balloon(&balloon_dev);
>>>  
>>>  	register_xen_selfballooning(&balloon_dev);
>>>  
>>>  	register_xenstore_notifier(&xenstore_notifier);
>>> -
>>> -	return 0;
>>>  }
>>> -subsys_initcall(balloon_init);
>>> +EXPORT_SYMBOL_GPL(xen_balloon_init);
>>>  
>>>  #define BALLOON_SHOW(name, format, args...)				\
>>>  	static ssize_t show_##name(struct device *dev,			\
>>> diff --git a/include/xen/balloon.h b/include/xen/balloon.h
>>> index d1767dfb0d95..8906361bb50c 100644
>>> --- a/include/xen/balloon.h
>>> +++ b/include/xen/balloon.h
>>> @@ -35,3 +35,11 @@ static inline int register_xen_selfballooning(struct device *dev)
>>>  	return -ENOSYS;
>>>  }
>>>  #endif
>>> +
>>> +#ifdef CONFIG_XEN_BALLOON
>>> +void xen_balloon_init(void);
>>> +#else
>>> +static inline void xen_balloon_init(void)
>>> +{
>>> +}
>>> +#endif
>>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3] xen/balloon: don't online new memory initially

[HW42]

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

Juergen Gross:
> When setting up the Xenstore watch for the memory target size the new
> watch will fire at once. Don't try to reach the configured target size
> by onlining new memory in this case, as the current memory size will
> be smaller in almost all cases due to e.g. BIOS reserved pages.
> 
> Onlining new memory will lead to more problems e.g. undesired conflicts
> with NVMe devices meant to be operated as block devices.
> 
> Instead remember the difference between target size and current size
> when the watch fires for the first time and apply it to any further
> size changes, too.
> 
> In order to avoid races between balloon.c and xen-balloon.c init calls
> do the xen-balloon.c initialization from balloon.c.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

This patch seems to introduce a regression. If I boot a HVM or PVH
domain with memory != maxmem then the kernel inside the domain reports
that it has maxmem available even though Xen reports only what is set as
memory. Sooner or later Xen logs "out of PoD memory!" and kills the
domain. If I revert the corresponding commit (96edd61d) then everything
works as expected.

Tested this with Xen 4.9.0 and Linux 4.13.4.


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

Re: [PATCH v3] xen/balloon: don't online new memory initially

[HW42]

[-- Attachment #1.1.1: Type: text/plain, Size: 1225 bytes --]

Juergen Gross:
> When setting up the Xenstore watch for the memory target size the new
> watch will fire at once. Don't try to reach the configured target size
> by onlining new memory in this case, as the current memory size will
> be smaller in almost all cases due to e.g. BIOS reserved pages.
> 
> Onlining new memory will lead to more problems e.g. undesired conflicts
> with NVMe devices meant to be operated as block devices.
> 
> Instead remember the difference between target size and current size
> when the watch fires for the first time and apply it to any further
> size changes, too.
> 
> In order to avoid races between balloon.c and xen-balloon.c init calls
> do the xen-balloon.c initialization from balloon.c.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

This patch seems to introduce a regression. If I boot a HVM or PVH
domain with memory != maxmem then the kernel inside the domain reports
that it has maxmem available even though Xen reports only what is set as
memory. Sooner or later Xen logs "out of PoD memory!" and kills the
domain. If I revert the corresponding commit (96edd61d) then everything
works as expected.

Tested this with Xen 4.9.0 and Linux 4.13.4.


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

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3] xen/balloon: don't online new memory initially

[Boris Ostrovsky]

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

On 10/02/2017 05:37 PM, HW42 wrote:
> Juergen Gross:
>> When setting up the Xenstore watch for the memory target size the new
>> watch will fire at once. Don't try to reach the configured target size
>> by onlining new memory in this case, as the current memory size will
>> be smaller in almost all cases due to e.g. BIOS reserved pages.
>>
>> Onlining new memory will lead to more problems e.g. undesired conflicts
>> with NVMe devices meant to be operated as block devices.
>>
>> Instead remember the difference between target size and current size
>> when the watch fires for the first time and apply it to any further
>> size changes, too.
>>
>> In order to avoid races between balloon.c and xen-balloon.c init calls
>> do the xen-balloon.c initialization from balloon.c.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> This patch seems to introduce a regression. If I boot a HVM or PVH
> domain with memory != maxmem then the kernel inside the domain reports
> that it has maxmem available even though Xen reports only what is set as
> memory. Sooner or later Xen logs "out of PoD memory!" and kills the
> domain. If I revert the corresponding commit (96edd61d) then everything
> works as expected.
>
> Tested this with Xen 4.9.0 and Linux 4.13.4.
>


Yes, this indeed doesn't look like it's doing the right thing (although
I haven't seen the "out of memory" error).

I wonder whether target_diff should be computed against xenstore's
"static-max" and not "target" and the memory should be ballooned down
immediately and not on a subsequent watch firing.

Unfortunately Juergen is out for a couple of weeks and I'd like to hear
from him first since he is the one who wrote this patch.

-boris


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

Re: [PATCH v3] xen/balloon: don't online new memory initially

[Boris Ostrovsky]

[-- Attachment #1.1.1: Type: text/plain, Size: 1748 bytes --]

On 10/02/2017 05:37 PM, HW42 wrote:
> Juergen Gross:
>> When setting up the Xenstore watch for the memory target size the new
>> watch will fire at once. Don't try to reach the configured target size
>> by onlining new memory in this case, as the current memory size will
>> be smaller in almost all cases due to e.g. BIOS reserved pages.
>>
>> Onlining new memory will lead to more problems e.g. undesired conflicts
>> with NVMe devices meant to be operated as block devices.
>>
>> Instead remember the difference between target size and current size
>> when the watch fires for the first time and apply it to any further
>> size changes, too.
>>
>> In order to avoid races between balloon.c and xen-balloon.c init calls
>> do the xen-balloon.c initialization from balloon.c.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> This patch seems to introduce a regression. If I boot a HVM or PVH
> domain with memory != maxmem then the kernel inside the domain reports
> that it has maxmem available even though Xen reports only what is set as
> memory. Sooner or later Xen logs "out of PoD memory!" and kills the
> domain. If I revert the corresponding commit (96edd61d) then everything
> works as expected.
>
> Tested this with Xen 4.9.0 and Linux 4.13.4.
>


Yes, this indeed doesn't look like it's doing the right thing (although
I haven't seen the "out of memory" error).

I wonder whether target_diff should be computed against xenstore's
"static-max" and not "target" and the memory should be ballooned down
immediately and not on a subsequent watch firing.

Unfortunately Juergen is out for a couple of weeks and I'd like to hear
from him first since he is the one who wrote this patch.

-boris


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

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3] xen/balloon: don't online new memory initially

[Juergen Gross]

On 03/10/17 23:33, Boris Ostrovsky wrote:
> On 10/02/2017 05:37 PM, HW42 wrote:
>> Juergen Gross:
>>> When setting up the Xenstore watch for the memory target size the new
>>> watch will fire at once. Don't try to reach the configured target size
>>> by onlining new memory in this case, as the current memory size will
>>> be smaller in almost all cases due to e.g. BIOS reserved pages.
>>>
>>> Onlining new memory will lead to more problems e.g. undesired conflicts
>>> with NVMe devices meant to be operated as block devices.
>>>
>>> Instead remember the difference between target size and current size
>>> when the watch fires for the first time and apply it to any further
>>> size changes, too.
>>>
>>> In order to avoid races between balloon.c and xen-balloon.c init calls
>>> do the xen-balloon.c initialization from balloon.c.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> This patch seems to introduce a regression. If I boot a HVM or PVH
>> domain with memory != maxmem then the kernel inside the domain reports
>> that it has maxmem available even though Xen reports only what is set as
>> memory. Sooner or later Xen logs "out of PoD memory!" and kills the
>> domain. If I revert the corresponding commit (96edd61d) then everything
>> works as expected.
>>
>> Tested this with Xen 4.9.0 and Linux 4.13.4.
>>
> 
> 
> Yes, this indeed doesn't look like it's doing the right thing (although
> I haven't seen the "out of memory" error).

You need to use enough memory (e.g. via memhog).

> I wonder whether target_diff should be computed against xenstore's
> "static-max" and not "target" and the memory should be ballooned down
> immediately and not on a subsequent watch firing.

Right. And we need to keep target_diff = 0 for PV domains.

Patch coming soon.


Juergen

Re: [PATCH v3] xen/balloon: don't online new memory initially

[Juergen Gross]

On 03/10/17 23:33, Boris Ostrovsky wrote:
> On 10/02/2017 05:37 PM, HW42 wrote:
>> Juergen Gross:
>>> When setting up the Xenstore watch for the memory target size the new
>>> watch will fire at once. Don't try to reach the configured target size
>>> by onlining new memory in this case, as the current memory size will
>>> be smaller in almost all cases due to e.g. BIOS reserved pages.
>>>
>>> Onlining new memory will lead to more problems e.g. undesired conflicts
>>> with NVMe devices meant to be operated as block devices.
>>>
>>> Instead remember the difference between target size and current size
>>> when the watch fires for the first time and apply it to any further
>>> size changes, too.
>>>
>>> In order to avoid races between balloon.c and xen-balloon.c init calls
>>> do the xen-balloon.c initialization from balloon.c.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> This patch seems to introduce a regression. If I boot a HVM or PVH
>> domain with memory != maxmem then the kernel inside the domain reports
>> that it has maxmem available even though Xen reports only what is set as
>> memory. Sooner or later Xen logs "out of PoD memory!" and kills the
>> domain. If I revert the corresponding commit (96edd61d) then everything
>> works as expected.
>>
>> Tested this with Xen 4.9.0 and Linux 4.13.4.
>>
> 
> 
> Yes, this indeed doesn't look like it's doing the right thing (although
> I haven't seen the "out of memory" error).

You need to use enough memory (e.g. via memhog).

> I wonder whether target_diff should be computed against xenstore's
> "static-max" and not "target" and the memory should be ballooned down
> immediately and not on a subsequent watch firing.

Right. And we need to keep target_diff = 0 for PV domains.

Patch coming soon.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v3] xen/balloon: don't online new memory initially

[Juergen Gross]

When setting up the Xenstore watch for the memory target size the new
watch will fire at once. Don't try to reach the configured target size
by onlining new memory in this case, as the current memory size will
be smaller in almost all cases due to e.g. BIOS reserved pages.

Onlining new memory will lead to more problems e.g. undesired conflicts
with NVMe devices meant to be operated as block devices.

Instead remember the difference between target size and current size
when the watch fires for the first time and apply it to any further
size changes, too.

In order to avoid races between balloon.c and xen-balloon.c init calls
do the xen-balloon.c initialization from balloon.c.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/balloon.c     |  3 +++
 drivers/xen/xen-balloon.c | 22 ++++++++++++----------
 include/xen/balloon.h     |  8 ++++++++
 3 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index 50dcb68d8070..ab609255a0f3 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -780,6 +780,9 @@ static int __init balloon_init(void)
 	}
 #endif
 
+	/* Init the xen-balloon driver. */
+	xen_balloon_init();
+
 	return 0;
 }
 subsys_initcall(balloon_init);
diff --git a/drivers/xen/xen-balloon.c b/drivers/xen/xen-balloon.c
index e7715cb62eef..e89136ab851e 100644
--- a/drivers/xen/xen-balloon.c
+++ b/drivers/xen/xen-balloon.c
@@ -59,6 +59,8 @@ static void watch_target(struct xenbus_watch *watch,
 {
 	unsigned long long new_target;
 	int err;
+	static bool watch_fired;
+	static long target_diff;
 
 	err = xenbus_scanf(XBT_NIL, "memory", "target", "%llu", &new_target);
 	if (err != 1) {
@@ -69,7 +71,14 @@ static void watch_target(struct xenbus_watch *watch,
 	/* The given memory/target value is in KiB, so it needs converting to
 	 * pages. PAGE_SHIFT converts bytes to pages, hence PAGE_SHIFT - 10.
 	 */
-	balloon_set_new_target(new_target >> (PAGE_SHIFT - 10));
+	new_target >>= PAGE_SHIFT - 10;
+	if (watch_fired) {
+		balloon_set_new_target(new_target - target_diff);
+		return;
+	}
+
+	watch_fired = true;
+	target_diff = new_target - balloon_stats.target_pages;
 }
 static struct xenbus_watch target_watch = {
 	.node = "memory/target",
@@ -94,22 +103,15 @@ static struct notifier_block xenstore_notifier = {
 	.notifier_call = balloon_init_watcher,
 };
 
-static int __init balloon_init(void)
+void xen_balloon_init(void)
 {
-	if (!xen_domain())
-		return -ENODEV;
-
-	pr_info("Initialising balloon driver\n");
-
 	register_balloon(&balloon_dev);
 
 	register_xen_selfballooning(&balloon_dev);
 
 	register_xenstore_notifier(&xenstore_notifier);
-
-	return 0;
 }
-subsys_initcall(balloon_init);
+EXPORT_SYMBOL_GPL(xen_balloon_init);
 
 #define BALLOON_SHOW(name, format, args...)				\
 	static ssize_t show_##name(struct device *dev,			\
diff --git a/include/xen/balloon.h b/include/xen/balloon.h
index d1767dfb0d95..8906361bb50c 100644
--- a/include/xen/balloon.h
+++ b/include/xen/balloon.h
@@ -35,3 +35,11 @@ static inline int register_xen_selfballooning(struct device *dev)
 	return -ENOSYS;
 }
 #endif
+
+#ifdef CONFIG_XEN_BALLOON
+void xen_balloon_init(void);
+#else
+static inline void xen_balloon_init(void)
+{
+}
+#endif
-- 
2.12.3


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel