[PATCH 1/1] xen/balloon: Enforce various limits on target

[PATCH 1/1] xen/balloon: Enforce various limits on target

[Daniel Kiper]

This patch enforces on target limit statically defined in Linux Kernel
source and limit defined by hypervisor or host.

Particularly this patch fixes bug which led to flood
of dom0 kernel log with messages similar to:

System RAM resource [mem 0x1b8000000-0x1bfffffff] cannot be added
xen_balloon: reserve_additional_memory: add_memory() failed: -17

It does not allow balloon driver to execute infinite
loops when target exceeds limits in other cases too.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 drivers/xen/balloon.c |   47 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 46 insertions(+), 1 deletion(-)

diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index a56776d..07da753 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -65,6 +65,7 @@
 #include <xen/balloon.h>
 #include <xen/features.h>
 #include <xen/page.h>
+#include <xen/xenbus.h>
 
 /*
  * balloon_process() state:
@@ -490,11 +491,55 @@ static void balloon_process(struct work_struct *work)
 	mutex_unlock(&balloon_mutex);
 }
 
-/* Resets the Xen limit, sets new target, and kicks off processing. */
+/* Enforce limits, set new target and kick off processing. */
 void balloon_set_new_target(unsigned long target)
 {
+	domid_t domid = DOMID_SELF;
+	int rc;
+	unsigned long long host_limit;
+
+	/* Enforce statically defined limit. */
+	target = min(target, MAX_DOMAIN_PAGES);
+
+	if (xen_initial_domain()) {
+		rc = HYPERVISOR_memory_op(XENMEM_maximum_reservation, &domid);
+
+		/* Limit is not enforced by hypervisor. */
+		if (rc == -EPERM)
+			goto no_host_limit;
+
+		if (rc <= 0) {
+			pr_info("xen_balloon: %s: Initial domain target limit "
+				"could not be established: %i\n", __func__, rc);
+			goto no_host_limit;
+		}
+
+		host_limit = rc;
+	} else {
+		rc = xenbus_scanf(XBT_NIL, "memory", "static-max",
+							"%llu", &host_limit);
+
+		if (rc != 1) {
+			pr_info("xen_balloon: %s: Guest domain target limit "
+				"could not be established: %i\n", __func__, rc);
+			goto no_host_limit;
+		}
+
+		/*
+		 * The given memory target limit value is in KiB, so it needs
+		 * converting to pages. PAGE_SHIFT converts bytes to pages,
+		 * hence PAGE_SHIFT - 10.
+		 */
+		host_limit >>= (PAGE_SHIFT - 10);
+	}
+
+	/* Enforce hypervisor/host defined limit. */
+	target = min(target, (unsigned long)host_limit);
+
+no_host_limit:
 	/* No need for lock. Not read-modify-write updates. */
 	balloon_stats.target_pages = target;
+
 	schedule_delayed_work(&balloon_worker, 0);
 }
 EXPORT_SYMBOL_GPL(balloon_set_new_target);
-- 
1.7.10.4

Re: [PATCH 1/1] xen/balloon: Enforce various limits on target

[Konrad Rzeszutek Wilk]

On Mon, Mar 04, 2013 at 10:14:51PM +0100, Daniel Kiper wrote:
> This patch enforces on target limit statically defined in Linux Kernel
> source and limit defined by hypervisor or host.
> 
> Particularly this patch fixes bug which led to flood
> of dom0 kernel log with messages similar to:
> 
> System RAM resource [mem 0x1b8000000-0x1bfffffff] cannot be added
> xen_balloon: reserve_additional_memory: add_memory() failed: -17
> 
> It does not allow balloon driver to execute infinite
> loops when target exceeds limits in other cases too.
> 
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>

One nitpick below; David, could you take a look just for a extra
set of eyes.
> ---
>  drivers/xen/balloon.c |   47 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index a56776d..07da753 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -65,6 +65,7 @@
>  #include <xen/balloon.h>
>  #include <xen/features.h>
>  #include <xen/page.h>
> +#include <xen/xenbus.h>
>  
>  /*
>   * balloon_process() state:
> @@ -490,11 +491,55 @@ static void balloon_process(struct work_struct *work)
>  	mutex_unlock(&balloon_mutex);
>  }
>  
> -/* Resets the Xen limit, sets new target, and kicks off processing. */
> +/* Enforce limits, set new target and kick off processing. */
>  void balloon_set_new_target(unsigned long target)
>  {
> +	domid_t domid = DOMID_SELF;
> +	int rc;
> +	unsigned long long host_limit;
> +
> +	/* Enforce statically defined limit. */
> +	target = min(target, MAX_DOMAIN_PAGES);
> +
> +	if (xen_initial_domain()) {
> +		rc = HYPERVISOR_memory_op(XENMEM_maximum_reservation, &domid);
> +
> +		/* Limit is not enforced by hypervisor. */
> +		if (rc == -EPERM)
> +			goto no_host_limit;
> +
> +		if (rc <= 0) {
> +			pr_info("xen_balloon: %s: Initial domain target limit "
> +				"could not be established: %i\n", __func__, rc);

Probably pr_debug. As this means the user booted without dom0_mem_max argument.
(B/c rc == UINT_MAX) I would say if you can check for that and it as part of
if (rc == -EPERM) check.


> +			goto no_host_limit;
> +		}
> +
> +		host_limit = rc;
> +	} else {
> +		rc = xenbus_scanf(XBT_NIL, "memory", "static-max",
> +							"%llu", &host_limit);
> +
> +		if (rc != 1) {
> +			pr_info("xen_balloon: %s: Guest domain target limit "
> +				"could not be established: %i\n", __func__, rc);
> +			goto no_host_limit;
> +		}
> +
> +		/*
> +		 * The given memory target limit value is in KiB, so it needs
> +		 * converting to pages. PAGE_SHIFT converts bytes to pages,
> +		 * hence PAGE_SHIFT - 10.
> +		 */
> +		host_limit >>= (PAGE_SHIFT - 10);
> +	}
> +
> +	/* Enforce hypervisor/host defined limit. */
> +	target = min(target, (unsigned long)host_limit);
> +
> +no_host_limit:
>  	/* No need for lock. Not read-modify-write updates. */
>  	balloon_stats.target_pages = target;
> +
>  	schedule_delayed_work(&balloon_worker, 0);
>  }
>  EXPORT_SYMBOL_GPL(balloon_set_new_target);
> -- 
> 1.7.10.4
> 

Re: [PATCH 1/1] xen/balloon: Enforce various limits on target

[David Vrabel]

On 04/03/13 21:14, Daniel Kiper wrote:
> This patch enforces on target limit statically defined in Linux Kernel
> source and limit defined by hypervisor or host.
> 
> Particularly this patch fixes bug which led to flood
> of dom0 kernel log with messages similar to:
> 
> System RAM resource [mem 0x1b8000000-0x1bfffffff] cannot be added
> xen_balloon: reserve_additional_memory: add_memory() failed: -17

I think this helps in some cases, but because
reserve_additional_memory() places the hotplugged memory after max_pfn
without checking if there is anything already there, there are still
ways it can repeatedly fail.

e.g.,

1. If dom0 has had its maximum reservation limited initially (with the
dom0_mem option) /and/ the max reservation and target is subsequently
raised then the balloon driver will attempt to hotplug memory that
overlaps with E820_UNUSABLE regions in the e820 map and the hotplug will
fail.

2. If a domU has passed-through PCI devices max_pfn is before the PCI
memory window then the balloon driver will attempt to hotplug memory
over the PCI device BARs.

I think reserve_additional_memory() should check the current resource
map and the e820 map to find a large enough unused region.  This can be
done as an additional patch at a later date.

> It does not allow balloon driver to execute infinite
> loops when target exceeds limits in other cases too.

This sentence confuses me.

> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
>  drivers/xen/balloon.c |   47 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index a56776d..07da753 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -65,6 +65,7 @@
>  #include <xen/balloon.h>
>  #include <xen/features.h>
>  #include <xen/page.h>
> +#include <xen/xenbus.h>
>  
>  /*
>   * balloon_process() state:
> @@ -490,11 +491,55 @@ static void balloon_process(struct work_struct *work)
>  	mutex_unlock(&balloon_mutex);
>  }
>  
> -/* Resets the Xen limit, sets new target, and kicks off processing. */
> +/* Enforce limits, set new target and kick off processing. */
>  void balloon_set_new_target(unsigned long target)
>  {
> +	domid_t domid = DOMID_SELF;
> +	int rc;
> +	unsigned long long host_limit;
> +
> +	/* Enforce statically defined limit. */
> +	target = min(target, MAX_DOMAIN_PAGES);
> +
> +	if (xen_initial_domain()) {
> +		rc = HYPERVISOR_memory_op(XENMEM_maximum_reservation, &domid);
> +
> +		/* Limit is not enforced by hypervisor. */
> +		if (rc == -EPERM)
> +			goto no_host_limit;
> +
> +		if (rc <= 0) {
> +			pr_info("xen_balloon: %s: Initial domain target limit "
> +				"could not be established: %i\n", __func__, rc);
> +			goto no_host_limit;
> +		}
> +
> +		host_limit = rc;

I think you should use this method for both dom0 and domUs.  No need to
check static-max from xenstore.

> +	} else {
> +		rc = xenbus_scanf(XBT_NIL, "memory", "static-max",
> +							"%llu", &host_limit);
> +
> +		if (rc != 1) {
> +			pr_info("xen_balloon: %s: Guest domain target limit "
> +				"could not be established: %i\n", __func__, rc);
> +			goto no_host_limit;
> +		}
> +
> +		/*
> +		 * The given memory target limit value is in KiB, so it needs
> +		 * converting to pages. PAGE_SHIFT converts bytes to pages,
> +		 * hence PAGE_SHIFT - 10.
> +		 */
> +		host_limit >>= (PAGE_SHIFT - 10);
> +	}
> +
> +	/* Enforce hypervisor/host defined limit. */
> +	target = min(target, (unsigned long)host_limit);

With the change above, you can change host_limit to unsigned long and
avoid the cast.

David

Re: [PATCH 1/1] xen/balloon: Enforce various limits on target

[Daniel Kiper]

On Wed, Mar 06, 2013 at 11:05:03AM +0000, David Vrabel wrote:
> On 04/03/13 21:14, Daniel Kiper wrote:
> > This patch enforces on target limit statically defined in Linux Kernel
> > source and limit defined by hypervisor or host.
> >
> > Particularly this patch fixes bug which led to flood
> > of dom0 kernel log with messages similar to:
> >
> > System RAM resource [mem 0x1b8000000-0x1bfffffff] cannot be added
> > xen_balloon: reserve_additional_memory: add_memory() failed: -17
>
> I think this helps in some cases, but because
> reserve_additional_memory() places the hotplugged memory after max_pfn
> without checking if there is anything already there, there are still
> ways it can repeatedly fail.
>
> e.g.,
>
> 1. If dom0 has had its maximum reservation limited initially (with the
> dom0_mem option) /and/ the max reservation and target is subsequently
> raised then the balloon driver will attempt to hotplug memory that
> overlaps with E820_UNUSABLE regions in the e820 map and the hotplug will
> fail.
>
> 2. If a domU has passed-through PCI devices max_pfn is before the PCI
> memory window then the balloon driver will attempt to hotplug memory
> over the PCI device BARs.

You are right. During work on this patch I discovered that but decided
to enforce limits because it is more generic. Please look below why.
However, I stated that it should be fixed too. I added it to my todo list.
It requires a bit more work because I think new algorithm should cover
many different cases. Probably add_memory() (it requires changes in
mm/memory_hotplug.c) should be modified to look for range having
sufficient size and not conflicting with others.

> I think reserve_additional_memory() should check the current resource
> map and the e820 map to find a large enough unused region.  This can be
> done as an additional patch at a later date.
>
> > It does not allow balloon driver to execute infinite
> > loops when target exceeds limits in other cases too.
>
> This sentence confuses me.

For example:
  - boot guest domain with memory = 2048 and maxmem = 2048,
  - xl mem-set 1024,
  - xl mem-max 1536,
  - echo 2097152 > /sys/devices/system/xen_memory/xen_memory0/target_kb,
  - ...and balloon driver without this patch attempts to increase
    reservation until limit is not set to at least 2048...

That is why this patch is more generic.

> > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> > ---
> >  drivers/xen/balloon.c |   47 ++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 46 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> > index a56776d..07da753 100644
> > --- a/drivers/xen/balloon.c
> > +++ b/drivers/xen/balloon.c
> > @@ -65,6 +65,7 @@
> >  #include <xen/balloon.h>
> >  #include <xen/features.h>
> >  #include <xen/page.h>
> > +#include <xen/xenbus.h>
> >
> >  /*
> >   * balloon_process() state:
> > @@ -490,11 +491,55 @@ static void balloon_process(struct work_struct *work)
> >  	mutex_unlock(&balloon_mutex);
> >  }
> >
> > -/* Resets the Xen limit, sets new target, and kicks off processing. */
> > +/* Enforce limits, set new target and kick off processing. */
> >  void balloon_set_new_target(unsigned long target)
> >  {
> > +	domid_t domid = DOMID_SELF;
> > +	int rc;
> > +	unsigned long long host_limit;
> > +
> > +	/* Enforce statically defined limit. */
> > +	target = min(target, MAX_DOMAIN_PAGES);
> > +
> > +	if (xen_initial_domain()) {
> > +		rc = HYPERVISOR_memory_op(XENMEM_maximum_reservation, &domid);
> > +
> > +		/* Limit is not enforced by hypervisor. */
> > +		if (rc == -EPERM)
> > +			goto no_host_limit;
> > +
> > +		if (rc <= 0) {
> > +			pr_info("xen_balloon: %s: Initial domain target limit "
> > +				"could not be established: %i\n", __func__, rc);
> > +			goto no_host_limit;
> > +		}
> > +
> > +		host_limit = rc;
>
> I think you should use this method for both dom0 and domUs.  No need to
> check static-max from xenstore.

Sadly XENMEM_maximum_reservation for domU returns value which is set by xl mem-set
not by xl mem-max :-(((... That is why I get this value from xenstore.

> > +	} else {
> > +		rc = xenbus_scanf(XBT_NIL, "memory", "static-max",
> > +							"%llu", &host_limit);
> > +
> > +		if (rc != 1) {
> > +			pr_info("xen_balloon: %s: Guest domain target limit "
> > +				"could not be established: %i\n", __func__, rc);
> > +			goto no_host_limit;
> > +		}
> > +
> > +		/*
> > +		 * The given memory target limit value is in KiB, so it needs
> > +		 * converting to pages. PAGE_SHIFT converts bytes to pages,
> > +		 * hence PAGE_SHIFT - 10.
> > +		 */
> > +		host_limit >>= (PAGE_SHIFT - 10);
> > +	}
> > +
> > +	/* Enforce hypervisor/host defined limit. */
> > +	target = min(target, (unsigned long)host_limit);
>
> With the change above, you can change host_limit to unsigned long and
> avoid the cast.

It is not possible. Please look above why...

Daniel

Re: [PATCH 1/1] xen/balloon: Enforce various limits on target

[David Vrabel]

On 06/03/13 16:47, Daniel Kiper wrote:
> On Wed, Mar 06, 2013 at 11:05:03AM +0000, David Vrabel wrote:
>> On 04/03/13 21:14, Daniel Kiper wrote:
>>> This patch enforces on target limit statically defined in Linux Kernel
>>> source and limit defined by hypervisor or host.
>>>
>>> Particularly this patch fixes bug which led to flood
>>> of dom0 kernel log with messages similar to:
>>>
>>> System RAM resource [mem 0x1b8000000-0x1bfffffff] cannot be added
>>> xen_balloon: reserve_additional_memory: add_memory() failed: -17
>>
>> I think this helps in some cases, but because
>> reserve_additional_memory() places the hotplugged memory after max_pfn
>> without checking if there is anything already there, there are still
>> ways it can repeatedly fail.
>>
>> e.g.,
>>
>> 1. If dom0 has had its maximum reservation limited initially (with the
>> dom0_mem option) /and/ the max reservation and target is subsequently
>> raised then the balloon driver will attempt to hotplug memory that
>> overlaps with E820_UNUSABLE regions in the e820 map and the hotplug will
>> fail.
>>
>> 2. If a domU has passed-through PCI devices max_pfn is before the PCI
>> memory window then the balloon driver will attempt to hotplug memory
>> over the PCI device BARs.
> 
> You are right. During work on this patch I discovered that but decided
> to enforce limits because it is more generic. Please look below why.
> However, I stated that it should be fixed too. I added it to my todo list.
> It requires a bit more work because I think new algorithm should cover
> many different cases. Probably add_memory() (it requires changes in
> mm/memory_hotplug.c) should be modified to look for range having
> sufficient size and not conflicting with others.

Ok, so we're agreed, this patch doesn't fix everything and that's fine.

>> I think reserve_additional_memory() should check the current resource
>> map and the e820 map to find a large enough unused region.  This can be
>> done as an additional patch at a later date.
>>
>>> It does not allow balloon driver to execute infinite
>>> loops when target exceeds limits in other cases too.
>>
>> This sentence confuses me.

I'm just confused by the English.  Perhaps it should say:

"The balloon driver will limit target to the maximum reservation as any
attempt to populate pages above the maximum reservation will always fail."

?

> That is why this patch is more generic.
> 
>>> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
>>> ---
>>>  drivers/xen/balloon.c |   47 ++++++++++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 46 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
>>> index a56776d..07da753 100644
>>> --- a/drivers/xen/balloon.c
>>> +++ b/drivers/xen/balloon.c
>>> @@ -65,6 +65,7 @@
>>>  #include <xen/balloon.h>
>>>  #include <xen/features.h>
>>>  #include <xen/page.h>
>>> +#include <xen/xenbus.h>
>>>
>>>  /*
>>>   * balloon_process() state:
>>> @@ -490,11 +491,55 @@ static void balloon_process(struct work_struct *work)
>>>  	mutex_unlock(&balloon_mutex);
>>>  }
>>>
>>> -/* Resets the Xen limit, sets new target, and kicks off processing. */
>>> +/* Enforce limits, set new target and kick off processing. */
>>>  void balloon_set_new_target(unsigned long target)
>>>  {
>>> +	domid_t domid = DOMID_SELF;
>>> +	int rc;
>>> +	unsigned long long host_limit;
>>> +
>>> +	/* Enforce statically defined limit. */
>>> +	target = min(target, MAX_DOMAIN_PAGES);
>>> +
>>> +	if (xen_initial_domain()) {
>>> +		rc = HYPERVISOR_memory_op(XENMEM_maximum_reservation, &domid);
>>> +
>>> +		/* Limit is not enforced by hypervisor. */
>>> +		if (rc == -EPERM)
>>> +			goto no_host_limit;
>>> +
>>> +		if (rc <= 0) {
>>> +			pr_info("xen_balloon: %s: Initial domain target limit "
>>> +				"could not be established: %i\n", __func__, rc);
>>> +			goto no_host_limit;
>>> +		}
>>> +
>>> +		host_limit = rc;
>>
>> I think you should use this method for both dom0 and domUs.  No need to
>> check static-max from xenstore.
> 
> Sadly XENMEM_maximum_reservation for domU returns value which is set by xl mem-set
> not by xl mem-max :-(((... That is why I get this value from xenstore.

It gets d->max_pages which the limit for d->tot_pages. d->max_pages is
set by xl mem-max (and xl mem-set as it uses the enforce option to
libxl_set_memory_target()).

If you set the target above d->max_pages you won't be able to populate them.

So, using the maximum_reservation call seems like the right thing to me.

David

Re: [PATCH 1/1] xen/balloon: Enforce various limits on target

[Daniel Kiper]

On Wed, Mar 06, 2013 at 05:52:28PM +0000, David Vrabel wrote:
> On 06/03/13 16:47, Daniel Kiper wrote:

[...]

> >> I think reserve_additional_memory() should check the current resource
> >> map and the e820 map to find a large enough unused region.  This can be
> >> done as an additional patch at a later date.
> >>
> >>> It does not allow balloon driver to execute infinite
> >>> loops when target exceeds limits in other cases too.
> >>
> >> This sentence confuses me.
>
> I'm just confused by the English.  Perhaps it should say:
>
> "The balloon driver will limit target to the maximum reservation as any
> attempt to populate pages above the maximum reservation will always fail."
>
> ?

That is OK.

[...]

> >>> +	if (xen_initial_domain()) {
> >>> +		rc = HYPERVISOR_memory_op(XENMEM_maximum_reservation, &domid);
> >>> +
> >>> +		/* Limit is not enforced by hypervisor. */
> >>> +		if (rc == -EPERM)
> >>> +			goto no_host_limit;
> >>> +
> >>> +		if (rc <= 0) {
> >>> +			pr_info("xen_balloon: %s: Initial domain target limit "
> >>> +				"could not be established: %i\n", __func__, rc);
> >>> +			goto no_host_limit;
> >>> +		}
> >>> +
> >>> +		host_limit = rc;
> >>
> >> I think you should use this method for both dom0 and domUs.  No need to
> >> check static-max from xenstore.
> >
> > Sadly XENMEM_maximum_reservation for domU returns value which is set by xl mem-set
> > not by xl mem-max :-(((... That is why I get this value from xenstore.
>
> It gets d->max_pages which the limit for d->tot_pages. d->max_pages is
> set by xl mem-max (and xl mem-set as it uses the enforce option to

No, it was tested by me and d->max_pages is set by xl mem-set (it is
current target). d->tot_pages has count of pages for a given moment.
It is confusing for me but this is the reality.

> libxl_set_memory_target()).
>
> If you set the target above d->max_pages you won't be able to populate them.
>
> So, using the maximum_reservation call seems like the right thing to me.

Please look above. If you use this value you would not
be able to increase reservation.

Daniel

Re: [PATCH 1/1] xen/balloon: Enforce various limits on target

[David Vrabel]

On 07/03/13 11:28, Daniel Kiper wrote:
> On Wed, Mar 06, 2013 at 05:52:28PM +0000, David Vrabel wrote:
>
>> If you set the target above d->max_pages you won't be able to populate them.
>>
>> So, using the maximum_reservation call seems like the right thing to me.
> 
> Please look above. If you use this value you would not
> be able to increase reservation.

I don't think I'm understanding the use case you're talking about.

Do you mean we should allow a target <= d->tot_pages even if this is
above d->max_pages?  I agree with this.

Something like this:

void balloon_set_new_target(unsigned long target)
{
	domid_t domid = DOMID_SELF;
	int rc;
	unsigned long host_limit;

	target = min(target, MAX_DOMAIN_PAGES);

	/* Prevent target from attempting the expand the reservation
	   above the max enforced by the hypervisor. */
	rc = HYPERVISOR_memory_op(XENMEM_maximum_reservation, &domid);
	if (rc > 0) {
		host_limit = rc;
		target = min(target,
			max(host_limit, balloon_stats.current_pages));
	}

	balloon_stats.target_pages = target;
	schedule_delayed_work(&balloon_worker, 0);
}

David

Re: [PATCH 1/1] xen/balloon: Enforce various limits on target

[Daniel Kiper]

On Thu, Mar 07, 2013 at 12:07:09PM +0000, David Vrabel wrote:
> On 07/03/13 11:28, Daniel Kiper wrote:
> > On Wed, Mar 06, 2013 at 05:52:28PM +0000, David Vrabel wrote:
> >
> >> If you set the target above d->max_pages you won't be able to populate them.
> >>
> >> So, using the maximum_reservation call seems like the right thing to me.
> >
> > Please look above. If you use this value you would not
> > be able to increase reservation.
>
> I don't think I'm understanding the use case you're talking about.
>
> Do you mean we should allow a target <= d->tot_pages even if this is
> above d->max_pages?  I agree with this.
>
> Something like this:
>
> void balloon_set_new_target(unsigned long target)
> {
> 	domid_t domid = DOMID_SELF;
> 	int rc;
> 	unsigned long host_limit;
>
> 	target = min(target, MAX_DOMAIN_PAGES);
>
> 	/* Prevent target from attempting the expand the reservation
> 	   above the max enforced by the hypervisor. */
> 	rc = HYPERVISOR_memory_op(XENMEM_maximum_reservation, &domid);
> 	if (rc > 0) {
> 		host_limit = rc;
> 		target = min(target,
> 			max(host_limit, balloon_stats.current_pages));
> 	}
>
> 	balloon_stats.target_pages = target;
> 	schedule_delayed_work(&balloon_worker, 0);
> }

I dug deeper into Xen source and found some strange things for me.

If you call xl mem-set it does two things:
  - sets target in xenstore,
  - sets d->max_pages (sic!).

Last thing is very confusing because at least command comment
does mention nothing about this behavior. Comment for this
command states: "Set the current memory usage for a domain".
It does say nothing about setting memory usage limit. IMO it is
the role of xl mem-max (another strange thing is that xl mem-max
sets d->max_pages but does not touch static-max; I think it should
be changed too; now it is not possible to increase limit above
maxmem defined at startup which makes memory hotplug practically
unusable). That is why xl mem-set behavior should be changed
(IIRC xm sets only target in xenstore) or comment should be aligned
to what xl code doas in real. Then we could back to discussion about
new balloon_set_new_target().

Daniel