[PATCH 3/3] drivers/xen/balloon.c: Xen memory balloon driver with memory hotplug support

[PATCH 3/3] drivers/xen/balloon.c: Xen memory balloon driver with memory hotplug support

[Daniel Kiper]

Features and fixes:
  - new version of memory hotplug patch which supports
    among others memory allocation policies during errors
    (try until success or stop at first error),
  - this version of patch was tested with tmem
    (selfballooning and frontswap) and works
    very well with it,
  - some other minor fixes.

Signed-off-by: Daniel Kiper <dkiper@net-space.pl>
---
 drivers/xen/Kconfig   |   10 ++
 drivers/xen/balloon.c |  222 ++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 221 insertions(+), 11 deletions(-)

diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index 60d71e9..ada8ef5 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -9,6 +9,16 @@ config XEN_BALLOON
 	  the system to expand the domain's memory allocation, or alternatively
 	  return unneeded memory to the system.
 
+config XEN_BALLOON_MEMORY_HOTPLUG
+	bool "Xen memory balloon driver with memory hotplug support"
+	default n
+	depends on XEN_BALLOON && MEMORY_HOTPLUG
+	help
+	  Xen memory balloon driver with memory hotplug support allows expanding
+	  memory available for the system above limit declared at system startup.
+	  It is very useful on critical systems which require long run without
+	  rebooting.
+
 config XEN_SCRUB_PAGES
 	bool "Scrub pages before returning them to system"
 	depends on XEN_BALLOON
diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index 06dbdad..69d9367 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -6,6 +6,7 @@
  * Copyright (c) 2003, B Dragovic
  * Copyright (c) 2003-2004, M Williamson, K Fraser
  * Copyright (c) 2005 Dan M. Smith, IBM Corporation
+ * Copyright (c) 2010 Daniel Kiper
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License version 2
@@ -44,6 +45,7 @@
 #include <linux/list.h>
 #include <linux/sysdev.h>
 #include <linux/gfp.h>
+#include <linux/memory.h>
 
 #include <asm/page.h>
 #include <asm/pgalloc.h>
@@ -65,6 +67,9 @@
 
 #define BALLOON_CLASS_NAME "xen_memory"
 
+#define MH_POLICY_TRY_UNTIL_SUCCESS	0
+#define MH_POLICY_STOP_AT_FIRST_ERROR	1
+
 struct balloon_stats {
 	/* We aim for 'current allocation' == 'target allocation'. */
 	unsigned long current_pages;
@@ -74,6 +79,10 @@ struct balloon_stats {
 	unsigned long balloon_high;
 	unsigned long schedule_delay;
 	unsigned long max_schedule_delay;
+#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG
+	unsigned long boot_max_pfn;
+	unsigned long mh_policy;
+#endif
 };
 
 static DEFINE_MUTEX(balloon_mutex);
@@ -193,17 +202,194 @@ static void update_schedule_delay(int cmd)
 	balloon_stats.schedule_delay = new_schedule_delay;
 }
 
-static unsigned long current_target(void)
+#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG
+static inline int allocate_memory_resource(struct resource **r, unsigned long nr_pages)
+{
+	int rc;
+	resource_size_t r_min, r_size;
+
+	/*
+	 * Look for first unused memory region starting at page
+	 * boundary. Skip last memory section created at boot time
+	 * becuase it may contains unused memory pages with PG_reserved
+	 * bit not set (online_pages require PG_reserved bit set).
+	 */
+
+	*r = kzalloc(sizeof(struct resource), GFP_KERNEL);
+
+	if (!*r)
+		return -ENOMEM;
+
+	(*r)->name = "System RAM";
+	(*r)->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
+	r_min = PFN_PHYS(section_nr_to_pfn(pfn_to_section_nr(balloon_stats.boot_max_pfn) + 1));
+	r_size = nr_pages << PAGE_SHIFT;
+
+	rc = allocate_resource(&iomem_resource, *r, r_size, r_min,
+					ULONG_MAX, PAGE_SIZE, NULL, NULL);
+
+	if (rc < 0) {
+		kfree(*r);
+		*r = NULL;
+	}
+
+	return rc;
+}
+
+static inline void adjust_memory_resource(struct resource **r, unsigned long nr_pages)
+{
+	if ((*r)->end + 1 - (nr_pages << PAGE_SHIFT) == (*r)->start) {
+		BUG_ON(release_resource(*r) < 0);
+		kfree(*r);
+		*r = NULL;
+		return;
+	}
+
+	BUG_ON(adjust_resource(*r, (*r)->start, (*r)->end + 1 - (*r)->start -
+				(nr_pages << PAGE_SHIFT)) < 0);
+}
+
+static inline int allocate_additional_memory(struct resource *r, unsigned long nr_pages)
+{
+	int rc;
+	struct xen_memory_reservation reservation = {
+		.address_bits = 0,
+		.extent_order = 0,
+		.domid        = DOMID_SELF
+	};
+	unsigned long flags, i, pfn, pfn_start;
+
+	if (!nr_pages)
+		return 0;
+
+	pfn_start = PFN_UP(r->end) - nr_pages;
+
+	if (nr_pages > ARRAY_SIZE(frame_list))
+		nr_pages = ARRAY_SIZE(frame_list);
+
+	for (i = 0, pfn = pfn_start; i < nr_pages; ++i, ++pfn)
+		frame_list[i] = pfn;
+
+	set_xen_guest_handle(reservation.extent_start, frame_list);
+	reservation.nr_extents = nr_pages;
+
+	spin_lock_irqsave(&xen_reservation_lock, flags);
+
+	rc = HYPERVISOR_memory_op(XENMEM_populate_physmap, &reservation);
+
+	if (rc <= 0)
+		return (rc < 0) ? rc : -ENOMEM;
+
+	for (i = 0, pfn = pfn_start; i < rc; ++i, ++pfn) {
+		BUG_ON(!xen_feature(XENFEAT_auto_translated_physmap) &&
+		       phys_to_machine_mapping_valid(pfn));
+		set_phys_to_machine(pfn, frame_list[i]);
+	}
+
+	spin_unlock_irqrestore(&xen_reservation_lock, flags);
+
+	return rc;
+}
+
+static inline void hotplug_allocated_memory(struct resource **r)
 {
-	unsigned long target = balloon_stats.target_pages;
+	int nid, rc;
+	resource_size_t r_size;
+	struct memory_block *mem;
+	unsigned long pfn;
+
+	r_size = (*r)->end + 1 - (*r)->start;
+	nid = memory_add_physaddr_to_nid((*r)->start);
+
+	rc = add_registered_memory(nid, (*r)->start, r_size);
+
+	if (rc) {
+		pr_err("%s: add_registered_memory: Memory hotplug failed: %i\n",
+			__func__, rc);
+		balloon_stats.target_pages = balloon_stats.current_pages;
+		*r = NULL;
+		return;
+	}
+
+	if (xen_pv_domain())
+		for (pfn = PFN_DOWN((*r)->start); pfn < PFN_UP((*r)->end); ++pfn)
+			if (!PageHighMem(pfn_to_page(pfn)))
+				BUG_ON(HYPERVISOR_update_va_mapping(
+					(unsigned long)__va(pfn << PAGE_SHIFT),
+					mfn_pte(pfn_to_mfn(pfn), PAGE_KERNEL), 0));
+
+	rc = online_pages(PFN_DOWN((*r)->start), r_size >> PAGE_SHIFT);
+
+	if (rc) {
+		pr_err("%s: online_pages: Failed: %i\n", __func__, rc);
+		balloon_stats.target_pages = balloon_stats.current_pages;
+		*r = NULL;
+		return;
+	}
+
+	for (pfn = PFN_DOWN((*r)->start); pfn < PFN_UP((*r)->end); pfn += PAGES_PER_SECTION) {
+		mem = find_memory_block(__pfn_to_section(pfn));
+		BUG_ON(!mem);
+		BUG_ON(!present_section_nr(mem->phys_index));
+		mutex_lock(&mem->state_mutex);
+		mem->state = MEM_ONLINE;
+		mutex_unlock(&mem->state_mutex);
+	}
+
+	balloon_stats.current_pages += r_size >> PAGE_SHIFT;
+
+	*r = NULL;
+}
+
+static inline int request_additional_memory(long credit)
+{
+	int rc;
+	static struct resource *r;
+	static unsigned long pages_left;
+
+	if ((credit <= 0 || balloon_stats.balloon_low ||
+				balloon_stats.balloon_high) && !r)
+		return 0;
 
-	target = min(target,
-		     balloon_stats.current_pages +
-		     balloon_stats.balloon_low +
-		     balloon_stats.balloon_high);
+	if (!r) {
+		rc = allocate_memory_resource(&r, credit);
 
-	return target;
+		if (rc)
+			return rc;
+
+		pages_left = credit;
+	}
+
+	rc = allocate_additional_memory(r, pages_left);
+
+	if (rc < 0) {
+		if (balloon_stats.mh_policy == MH_POLICY_TRY_UNTIL_SUCCESS)
+			return rc;
+
+		adjust_memory_resource(&r, pages_left);
+
+		if (!r)
+			return rc;
+	} else {
+		pages_left -= rc;
+
+		if (pages_left)
+			return 1;
+	}
+
+	hotplug_allocated_memory(&r);
+
+	return 0;
 }
+#else
+static inline int request_additional_memory(long credit)
+{
+	if (balloon_stats.balloon_low && balloon_stats.balloon_high &&
+			balloon_stats.target_pages > balloon_stats.current_pages)
+		balloon_stats.target_pages = balloon_stats.current_pages;
+	return 0;
+}
+#endif /* CONFIG_XEN_BALLOON_MEMORY_HOTPLUG */
 
 static int increase_reservation(unsigned long nr_pages)
 {
@@ -348,13 +534,13 @@ static int decrease_reservation(unsigned long nr_pages)
  */
 static void balloon_process(struct work_struct *work)
 {
-	int rc, state = 0;
+	int rc, state;
 	long credit;
 
 	mutex_lock(&balloon_mutex);
 
 	do {
-		credit = current_target() - balloon_stats.current_pages;
+		credit = balloon_stats.target_pages - balloon_stats.current_pages;
 
 		/*
 		 * state > 0: hungry,
@@ -362,7 +548,9 @@ static void balloon_process(struct work_struct *work)
 		 * state < 0: error, go to sleep.
 		 */
 
-		if (credit > 0) {
+		state = request_additional_memory(credit);
+
+		if (credit > 0 && !state) {
 			rc = increase_reservation(credit);
 			state = (rc < 0) ? rc : state;
 		}
@@ -454,6 +642,11 @@ static int __init balloon_init(void)
 	balloon_stats.schedule_delay = 1;
 	balloon_stats.max_schedule_delay = 32;
 
+#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG
+	balloon_stats.boot_max_pfn = max_pfn;
+	balloon_stats.mh_policy = MH_POLICY_STOP_AT_FIRST_ERROR;
+#endif
+
 	register_balloon(&balloon_sysdev);
 
 	/* Initialise the balloon with excess memory space. */
@@ -497,6 +690,10 @@ BALLOON_SHOW(high_kb, "%lu\n", PAGES2KB(balloon_stats.balloon_high));
 static SYSDEV_ULONG_ATTR(schedule_delay, 0644, balloon_stats.schedule_delay);
 static SYSDEV_ULONG_ATTR(max_schedule_delay, 0644, balloon_stats.max_schedule_delay);
 
+#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG
+static SYSDEV_ULONG_ATTR(memory_hotplug_policy, 0644, balloon_stats.mh_policy);
+#endif
+
 static ssize_t show_target_kb(struct sys_device *dev, struct sysdev_attribute *attr,
 			      char *buf)
 {
@@ -559,7 +756,10 @@ static struct sysdev_attribute *balloon_attrs[] = {
 	&attr_target_kb,
 	&attr_target,
 	&attr_schedule_delay.attr,
-	&attr_max_schedule_delay.attr
+	&attr_max_schedule_delay.attr,
+#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG
+	&attr_memory_hotplug_policy.attr
+#endif
 };
 
 static struct attribute *balloon_info_attrs[] = {
-- 
1.4.4.4

Re: [Xen-devel] [PATCH 3/3] drivers/xen/balloon.c: Xen memory balloon driver with memory hotplug support

[Konrad Rzeszutek Wilk]

On Mon, Dec 20, 2010 at 02:48:03PM +0100, Daniel Kiper wrote:
> Features and fixes:
>   - new version of memory hotplug patch which supports
>     among others memory allocation policies during errors
>     (try until success or stop at first error),
>   - this version of patch was tested with tmem
>     (selfballooning and frontswap) and works
>     very well with it,
>   - some other minor fixes.
> 
> Signed-off-by: Daniel Kiper <dkiper@net-space.pl>
> ---
>  drivers/xen/Kconfig   |   10 ++
>  drivers/xen/balloon.c |  222 ++++++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 221 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> index 60d71e9..ada8ef5 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -9,6 +9,16 @@ config XEN_BALLOON
>  	  the system to expand the domain's memory allocation, or alternatively
>  	  return unneeded memory to the system.
>  
> +config XEN_BALLOON_MEMORY_HOTPLUG
> +	bool "Xen memory balloon driver with memory hotplug support"
> +	default n
> +	depends on XEN_BALLOON && MEMORY_HOTPLUG
> +	help
> +	  Xen memory balloon driver with memory hotplug support allows expanding
> +	  memory available for the system above limit declared at system startup.
> +	  It is very useful on critical systems which require long run without
> +	  rebooting.
> +
>  config XEN_SCRUB_PAGES
>  	bool "Scrub pages before returning them to system"
>  	depends on XEN_BALLOON
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index 06dbdad..69d9367 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -6,6 +6,7 @@
>   * Copyright (c) 2003, B Dragovic
>   * Copyright (c) 2003-2004, M Williamson, K Fraser
>   * Copyright (c) 2005 Dan M. Smith, IBM Corporation
> + * Copyright (c) 2010 Daniel Kiper
>   *
>   * This program is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU General Public License version 2
> @@ -44,6 +45,7 @@
>  #include <linux/list.h>
>  #include <linux/sysdev.h>
>  #include <linux/gfp.h>
> +#include <linux/memory.h>
>  
>  #include <asm/page.h>
>  #include <asm/pgalloc.h>
> @@ -65,6 +67,9 @@
>  
>  #define BALLOON_CLASS_NAME "xen_memory"
>  
> +#define MH_POLICY_TRY_UNTIL_SUCCESS	0
> +#define MH_POLICY_STOP_AT_FIRST_ERROR	1
> +
>  struct balloon_stats {
>  	/* We aim for 'current allocation' == 'target allocation'. */
>  	unsigned long current_pages;
> @@ -74,6 +79,10 @@ struct balloon_stats {
>  	unsigned long balloon_high;
>  	unsigned long schedule_delay;
>  	unsigned long max_schedule_delay;
> +#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG
> +	unsigned long boot_max_pfn;
> +	unsigned long mh_policy;
> +#endif
>  };
>  
>  static DEFINE_MUTEX(balloon_mutex);
> @@ -193,17 +202,194 @@ static void update_schedule_delay(int cmd)
>  	balloon_stats.schedule_delay = new_schedule_delay;
>  }
>  
> -static unsigned long current_target(void)
> +#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG
> +static inline int allocate_memory_resource(struct resource **r, unsigned long nr_pages)
> +{
> +	int rc;
> +	resource_size_t r_min, r_size;
> +
> +	/*
> +	 * Look for first unused memory region starting at page
> +	 * boundary. Skip last memory section created at boot time
> +	 * becuase it may contains unused memory pages with PG_reserved
> +	 * bit not set (online_pages require PG_reserved bit set).
> +	 */
> +
> +	*r = kzalloc(sizeof(struct resource), GFP_KERNEL);
> +
> +	if (!*r)
> +		return -ENOMEM;
> +
> +	(*r)->name = "System RAM";
> +	(*r)->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
> +	r_min = PFN_PHYS(section_nr_to_pfn(pfn_to_section_nr(balloon_stats.boot_max_pfn) + 1));
> +	r_size = nr_pages << PAGE_SHIFT;
> +
> +	rc = allocate_resource(&iomem_resource, *r, r_size, r_min,
> +					ULONG_MAX, PAGE_SIZE, NULL, NULL);
> +
> +	if (rc < 0) {
> +		kfree(*r);
> +		*r = NULL;
> +	}
> +
> +	return rc;
> +}
> +
> +static inline void adjust_memory_resource(struct resource **r, unsigned long nr_pages)
> +{
> +	if ((*r)->end + 1 - (nr_pages << PAGE_SHIFT) == (*r)->start) {
> +		BUG_ON(release_resource(*r) < 0);
> +		kfree(*r);
> +		*r = NULL;
> +		return;
> +	}
> +
> +	BUG_ON(adjust_resource(*r, (*r)->start, (*r)->end + 1 - (*r)->start -
> +				(nr_pages << PAGE_SHIFT)) < 0);

Why not return a value here instead of BUG-ing out?

> +}
> +
> +static inline int allocate_additional_memory(struct resource *r, unsigned long nr_pages)
> +{
> +	int rc;
> +	struct xen_memory_reservation reservation = {
> +		.address_bits = 0,
> +		.extent_order = 0,
> +		.domid        = DOMID_SELF
> +	};
> +	unsigned long flags, i, pfn, pfn_start;
> +
> +	if (!nr_pages)
> +		return 0;
> +
> +	pfn_start = PFN_UP(r->end) - nr_pages;
> +
> +	if (nr_pages > ARRAY_SIZE(frame_list))
> +		nr_pages = ARRAY_SIZE(frame_list);
> +
> +	for (i = 0, pfn = pfn_start; i < nr_pages; ++i, ++pfn)
> +		frame_list[i] = pfn;
> +
> +	set_xen_guest_handle(reservation.extent_start, frame_list);
> +	reservation.nr_extents = nr_pages;
> +
> +	spin_lock_irqsave(&xen_reservation_lock, flags);
> +
> +	rc = HYPERVISOR_memory_op(XENMEM_populate_physmap, &reservation);
> +
> +	if (rc <= 0)
> +		return (rc < 0) ? rc : -ENOMEM;
> +
> +	for (i = 0, pfn = pfn_start; i < rc; ++i, ++pfn) {
> +		BUG_ON(!xen_feature(XENFEAT_auto_translated_physmap) &&
> +		       phys_to_machine_mapping_valid(pfn));
> +		set_phys_to_machine(pfn, frame_list[i]);
> +	}
> +
> +	spin_unlock_irqrestore(&xen_reservation_lock, flags);
> +
> +	return rc;
> +}
> +
> +static inline void hotplug_allocated_memory(struct resource **r)
>  {
> -	unsigned long target = balloon_stats.target_pages;
> +	int nid, rc;
> +	resource_size_t r_size;
> +	struct memory_block *mem;
> +	unsigned long pfn;
> +
> +	r_size = (*r)->end + 1 - (*r)->start;
> +	nid = memory_add_physaddr_to_nid((*r)->start);
> +
> +	rc = add_registered_memory(nid, (*r)->start, r_size);
> +
> +	if (rc) {
> +		pr_err("%s: add_registered_memory: Memory hotplug failed: %i\n",
> +			__func__, rc);
> +		balloon_stats.target_pages = balloon_stats.current_pages;
> +		*r = NULL;
> +		return;
> +	}
> +
> +	if (xen_pv_domain())
> +		for (pfn = PFN_DOWN((*r)->start); pfn < PFN_UP((*r)->end); ++pfn)
> +			if (!PageHighMem(pfn_to_page(pfn)))
> +				BUG_ON(HYPERVISOR_update_va_mapping(
> +					(unsigned long)__va(pfn << PAGE_SHIFT),
> +					mfn_pte(pfn_to_mfn(pfn), PAGE_KERNEL), 0));

Could we just stop here instead of bugging out? I mean we adding memory so
if it does not work, the failure path seems to not add the memory?

> +
> +	rc = online_pages(PFN_DOWN((*r)->start), r_size >> PAGE_SHIFT);
> +
> +	if (rc) {
> +		pr_err("%s: online_pages: Failed: %i\n", __func__, rc);
> +		balloon_stats.target_pages = balloon_stats.current_pages;
> +		*r = NULL;
> +		return;
> +	}
> +
> +	for (pfn = PFN_DOWN((*r)->start); pfn < PFN_UP((*r)->end); pfn += PAGES_PER_SECTION) {
> +		mem = find_memory_block(__pfn_to_section(pfn));
> +		BUG_ON(!mem);
> +		BUG_ON(!present_section_nr(mem->phys_index));
> +		mutex_lock(&mem->state_mutex);
> +		mem->state = MEM_ONLINE;
> +		mutex_unlock(&mem->state_mutex);
> +	}
> +
> +	balloon_stats.current_pages += r_size >> PAGE_SHIFT;
> +
> +	*r = NULL;
> +}
> +
> +static inline int request_additional_memory(long credit)
> +{
> +	int rc;
> +	static struct resource *r;

static?
> +	static unsigned long pages_left;
> +
> +	if ((credit <= 0 || balloon_stats.balloon_low ||
> +				balloon_stats.balloon_high) && !r)
> +		return 0;
>  
> -	target = min(target,
> -		     balloon_stats.current_pages +
> -		     balloon_stats.balloon_low +
> -		     balloon_stats.balloon_high);
> +	if (!r) {
> +		rc = allocate_memory_resource(&r, credit);
>  
> -	return target;
> +		if (rc)
> +			return rc;
> +
> +		pages_left = credit;
> +	}
> +
> +	rc = allocate_additional_memory(r, pages_left);
> +
> +	if (rc < 0) {
> +		if (balloon_stats.mh_policy == MH_POLICY_TRY_UNTIL_SUCCESS)
> +			return rc;

I think you are going to hit a memory leak. If we fail, you do not
kfree(*r), should you be doing that?

> +
> +		adjust_memory_resource(&r, pages_left);

Would it make sense to check here as well?
> +
> +		if (!r)
> +			return rc;
> +	} else {
> +		pages_left -= rc;
> +
> +		if (pages_left)
> +			return 1;

So wouldn't that mean we could still online the 'rc' amount of pages?
> +	}
> +
> +	hotplug_allocated_memory(&r);
> +
> +	return 0;
>  }
> +#else
> +static inline int request_additional_memory(long credit)
> +{
> +	if (balloon_stats.balloon_low && balloon_stats.balloon_high &&
> +			balloon_stats.target_pages > balloon_stats.current_pages)
> +		balloon_stats.target_pages = balloon_stats.current_pages;
> +	return 0;
> +}
> +#endif /* CONFIG_XEN_BALLOON_MEMORY_HOTPLUG */
>  
>  static int increase_reservation(unsigned long nr_pages)
>  {
> @@ -348,13 +534,13 @@ static int decrease_reservation(unsigned long nr_pages)
>   */
>  static void balloon_process(struct work_struct *work)
>  {
> -	int rc, state = 0;
> +	int rc, state;
>  	long credit;
>  
>  	mutex_lock(&balloon_mutex);
>  
>  	do {
> -		credit = current_target() - balloon_stats.current_pages;
> +		credit = balloon_stats.target_pages - balloon_stats.current_pages;
>  
>  		/*
>  		 * state > 0: hungry,
> @@ -362,7 +548,9 @@ static void balloon_process(struct work_struct *work)
>  		 * state < 0: error, go to sleep.
>  		 */
>  
> -		if (credit > 0) {
> +		state = request_additional_memory(credit);
> +
> +		if (credit > 0 && !state) {
>  			rc = increase_reservation(credit);
>  			state = (rc < 0) ? rc : state;
>  		}
> @@ -454,6 +642,11 @@ static int __init balloon_init(void)
>  	balloon_stats.schedule_delay = 1;
>  	balloon_stats.max_schedule_delay = 32;
>  
> +#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG
> +	balloon_stats.boot_max_pfn = max_pfn;
> +	balloon_stats.mh_policy = MH_POLICY_STOP_AT_FIRST_ERROR;
> +#endif
> +
>  	register_balloon(&balloon_sysdev);
>  
>  	/* Initialise the balloon with excess memory space. */
> @@ -497,6 +690,10 @@ BALLOON_SHOW(high_kb, "%lu\n", PAGES2KB(balloon_stats.balloon_high));
>  static SYSDEV_ULONG_ATTR(schedule_delay, 0644, balloon_stats.schedule_delay);
>  static SYSDEV_ULONG_ATTR(max_schedule_delay, 0644, balloon_stats.max_schedule_delay);
>  
> +#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG
> +static SYSDEV_ULONG_ATTR(memory_hotplug_policy, 0644, balloon_stats.mh_policy);
> +#endif
> +
>  static ssize_t show_target_kb(struct sys_device *dev, struct sysdev_attribute *attr,
>  			      char *buf)
>  {
> @@ -559,7 +756,10 @@ static struct sysdev_attribute *balloon_attrs[] = {
>  	&attr_target_kb,
>  	&attr_target,
>  	&attr_schedule_delay.attr,
> -	&attr_max_schedule_delay.attr
> +	&attr_max_schedule_delay.attr,
> +#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG
> +	&attr_memory_hotplug_policy.attr
> +#endif
>  };
>  
>  static struct attribute *balloon_info_attrs[] = {
> -- 
> 1.4.4.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [Xen-devel] [PATCH 3/3] drivers/xen/balloon.c: Xen memory balloon driver with memory hotplug support

[Daniel Kiper]

Hi,

On Mon, Dec 27, 2010 at 10:25:49AM -0500, Konrad Rzeszutek Wilk wrote:
> On Mon, Dec 20, 2010 at 02:48:03PM +0100, Daniel Kiper wrote:
> > +static inline void adjust_memory_resource(struct resource **r, unsigned long nr_pages)
> > +{
> > +	if ((*r)->end + 1 - (nr_pages << PAGE_SHIFT) == (*r)->start) {
> > +		BUG_ON(release_resource(*r) < 0);
> > +		kfree(*r);
> > +		*r = NULL;
> > +		return;
> > +	}
> > +
> > +	BUG_ON(adjust_resource(*r, (*r)->start, (*r)->end + 1 - (*r)->start -
> > +				(nr_pages << PAGE_SHIFT)) < 0);
>
> Why not return a value here instead of BUG-ing out?

If release_resource()/adjust_resource() fail it means
there is no possibility to align resource descpription with
real memory address space. I think it is fatal error. If
we decide to leave system in that state it could lead
to data corruption or crash.

> > +	if (xen_pv_domain())
> > +		for (pfn = PFN_DOWN((*r)->start); pfn < PFN_UP((*r)->end); ++pfn)
> > +			if (!PageHighMem(pfn_to_page(pfn)))
> > +				BUG_ON(HYPERVISOR_update_va_mapping(
> > +					(unsigned long)__va(pfn << PAGE_SHIFT),
> > +					mfn_pte(pfn_to_mfn(pfn), PAGE_KERNEL), 0));
>
> Could we just stop here instead of bugging out? I mean we adding memory so
> if it does not work, the failure path seems to not add the memory?

Very good question. I based that fragment of code on original
increase_reservation()/decrease_reservation(). I attempted to find
any good explanation for that however, without success. That is why
I decided to not touch that fragment of code. If I had any
good argument to change that I will do that.

> > +static inline int request_additional_memory(long credit)
> > +{
> > +	int rc;
> > +	static struct resource *r;
>
> static?

Yes, request_additional_memory() allocate memory in chunks
and r is used between subsequent calls until all memory
is allocated.

> > +	static unsigned long pages_left;
> > +
> > +	if ((credit <= 0 || balloon_stats.balloon_low ||
> > +				balloon_stats.balloon_high) && !r)
> > +		return 0;
> >
> > -	target = min(target,
> > -		     balloon_stats.current_pages +
> > -		     balloon_stats.balloon_low +
> > -		     balloon_stats.balloon_high);
> > +	if (!r) {
> > +		rc = allocate_memory_resource(&r, credit);
> >
> > -	return target;
> > +		if (rc)
> > +			return rc;
> > +
> > +		pages_left = credit;
> > +	}
> > +
> > +	rc = allocate_additional_memory(r, pages_left);
> > +
> > +	if (rc < 0) {
> > +		if (balloon_stats.mh_policy == MH_POLICY_TRY_UNTIL_SUCCESS)
> > +			return rc;
>
> I think you are going to hit a memory leak. If we fail, you do not
> kfree(*r), should you be doing that?

If balloon_stats.mh_policy == MH_POLICY_TRY_UNTIL_SUCCESS then
balloon process attempt to allocate requested memory until success.
*r is freed only when any error appears and no memory is allocated.

> > +
> > +		adjust_memory_resource(&r, pages_left);
>
> Would it make sense to check here as well?

Please look above.

> > +
> > +		if (!r)
> > +			return rc;
> > +	} else {
> > +		pages_left -= rc;
> > +
> > +		if (pages_left)
> > +			return 1;
>
> So wouldn't that mean we could still online the 'rc' amount of pages?

Memory is onlined only when all of it is allocated.

> > +	}
> > +
> > +	hotplug_allocated_memory(&r);
> > +
> > +	return 0;
> >  }

Daniel