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

[David Vrabel <david.vrabel@citrix.com>]

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