[PATCH v4 0/2] memory-hotplug: add automatic onlining policy for the newly added memory

[PATCH v4 0/2] memory-hotplug: add automatic onlining policy for the newly added memory

[Vitaly Kuznetsov]

Changes since v3:
- Add support for the policy to Xen balloon driver [Daniel Kiper, David Vrabel]
- I found an issue with PATCH v3: when memory auto onlining was requested we
  do nothing to memblocks states so in sysfs they stay 'offline' (while in
  reality they're online). Modify register_new_memory() (and its only caller,
  __add_section()) to create memblocks in the proper state.

Original description:

Currently, all newly added memory blocks remain in 'offline' state unless
someone onlines them, some linux distributions carry special udev rules
like:

SUBSYSTEM=="memory", ACTION=="add", ATTR{state}=="offline", ATTR{state}="online"

to make this happen automatically. This is not a great solution for virtual
machines where memory hotplug is being used to address high memory pressure
situations as such onlining is slow and a userspace process doing this
(udev) has a chance of being killed by the OOM killer as it will probably
require to allocate some memory.

Introduce default policy for the newly added memory blocks in
/sys/devices/system/memory/auto_online_blocks file with two possible
values: "offline" which preserves the current behavior and "online" which
causes all newly added memory blocks to go online as soon as they're added.
The default is "offline".

Vitaly Kuznetsov (2):
  memory-hotplug: add automatic onlining policy for the newly added
    memory
  xen_balloon: support memory auto onlining policy

 Documentation/memory-hotplug.txt | 19 +++++++++++++++----
 drivers/base/memory.c            | 40 ++++++++++++++++++++++++++++++++++++----
 drivers/xen/Kconfig              | 20 +++++++++++++-------
 drivers/xen/balloon.c            | 30 +++++++++++++++++++-----------
 include/linux/memory.h           |  3 ++-
 include/linux/memory_hotplug.h   |  4 +++-
 mm/memory_hotplug.c              | 18 +++++++++++++++---
 7 files changed, 103 insertions(+), 31 deletions(-)

-- 
2.5.0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v4 1/2] memory-hotplug: add automatic onlining policy for the newly added memory

[Vitaly Kuznetsov]

Currently, all newly added memory blocks remain in 'offline' state unless
someone onlines them, some linux distributions carry special udev rules
like:

SUBSYSTEM=="memory", ACTION=="add", ATTR{state}=="offline", ATTR{state}="online"

to make this happen automatically. This is not a great solution for virtual
machines where memory hotplug is being used to address high memory pressure
situations as such onlining is slow and a userspace process doing this
(udev) has a chance of being killed by the OOM killer as it will probably
require to allocate some memory.

Introduce default policy for the newly added memory blocks in
/sys/devices/system/memory/auto_online_blocks file with two possible
values: "offline" which preserves the current behavior and "online" which
causes all newly added memory blocks to go online as soon as they're added.
The default is "offline".

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 Documentation/memory-hotplug.txt | 19 +++++++++++++++----
 drivers/base/memory.c            | 40 ++++++++++++++++++++++++++++++++++++----
 drivers/xen/balloon.c            |  2 +-
 include/linux/memory.h           |  3 ++-
 include/linux/memory_hotplug.h   |  4 +++-
 mm/memory_hotplug.c              | 18 +++++++++++++++---
 6 files changed, 72 insertions(+), 14 deletions(-)

diff --git a/Documentation/memory-hotplug.txt b/Documentation/memory-hotplug.txt
index ce2cfcf..ceaf40c 100644
--- a/Documentation/memory-hotplug.txt
+++ b/Documentation/memory-hotplug.txt
@@ -254,12 +254,23 @@ If the memory block is online, you'll read "online".
 If the memory block is offline, you'll read "offline".
 
 
-5.2. How to online memory
+5.2. Memory onlining
 ------------
-Even if the memory is hot-added, it is not at ready-to-use state.
-For using newly added memory, you have to "online" the memory block.
+When the memory is hot-added, the kernel decides whether or not to "online"
+it according to the policy which can be read from "auto_online_blocks" file:
 
-For onlining, you have to write "online" to the memory block's state file as:
+% cat /sys/devices/system/memory/auto_online_blocks
+
+The default is "offline" which means the newly added memory is not in a
+ready-to-use state and you have to "online" the newly added memory blocks
+manually. Automatic onlining can be requested by writing "online" to
+"auto_online_blocks" file:
+
+% echo online > /sys/devices/system/memory/auto_online_blocks
+
+If the automatic onlining wasn't requested or some memory block was offlined
+it is possible to change the individual block's state by writing to the "state"
+file:
 
 % echo online > /sys/devices/system/memory/memoryXXX/state
 
diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 25425d3..7008edc 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -439,6 +439,37 @@ print_block_size(struct device *dev, struct device_attribute *attr,
 static DEVICE_ATTR(block_size_bytes, 0444, print_block_size, NULL);
 
 /*
+ * Memory auto online policy.
+ */
+
+static ssize_t
+show_auto_online_blocks(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	if (memhp_auto_online)
+		return sprintf(buf, "online\n");
+	else
+		return sprintf(buf, "offline\n");
+}
+
+static ssize_t
+store_auto_online_blocks(struct device *dev, struct device_attribute *attr,
+			 const char *buf, size_t count)
+{
+	if (sysfs_streq(buf, "online"))
+		memhp_auto_online = true;
+	else if (sysfs_streq(buf, "offline"))
+		memhp_auto_online = false;
+	else
+		return -EINVAL;
+
+	return count;
+}
+
+static DEVICE_ATTR(auto_online_blocks, 0644, show_auto_online_blocks,
+		   store_auto_online_blocks);
+
+/*
  * Some architectures will have custom drivers to do this, and
  * will not need to do it from userspace.  The fake hot-add code
  * as well as ppc64 will do all of their discovery in userspace
@@ -654,10 +685,10 @@ static int add_memory_block(int base_section_nr)
 
 
 /*
- * need an interface for the VM to add new memory regions,
- * but without onlining it.
+ * add new memory regions keeping their state.
  */
-int register_new_memory(int nid, struct mem_section *section)
+int register_new_memory(int nid, struct mem_section *section,
+			unsigned long state)
 {
 	int ret = 0;
 	struct memory_block *mem;
@@ -669,7 +700,7 @@ int register_new_memory(int nid, struct mem_section *section)
 		mem->section_count++;
 		put_device(&mem->dev);
 	} else {
-		ret = init_memory_block(&mem, section, MEM_OFFLINE);
+		ret = init_memory_block(&mem, section, state);
 		if (ret)
 			goto out;
 	}
@@ -737,6 +768,7 @@ static struct attribute *memory_root_attrs[] = {
 #endif
 
 	&dev_attr_block_size_bytes.attr,
+	&dev_attr_auto_online_blocks.attr,
 	NULL
 };
 
diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index 12eab50..890c3b5 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -338,7 +338,7 @@ static enum bp_state reserve_additional_memory(void)
 	}
 #endif
 
-	rc = add_memory_resource(nid, resource);
+	rc = add_memory_resource(nid, resource, false);
 	if (rc) {
 		pr_warn("Cannot add additional memory (%i)\n", rc);
 		goto err;
diff --git a/include/linux/memory.h b/include/linux/memory.h
index 8b8d8d1..1544f48 100644
--- a/include/linux/memory.h
+++ b/include/linux/memory.h
@@ -108,7 +108,8 @@ extern int register_memory_notifier(struct notifier_block *nb);
 extern void unregister_memory_notifier(struct notifier_block *nb);
 extern int register_memory_isolate_notifier(struct notifier_block *nb);
 extern void unregister_memory_isolate_notifier(struct notifier_block *nb);
-extern int register_new_memory(int, struct mem_section *);
+extern int register_new_memory(int nid, struct mem_section *section,
+			       unsigned long state);
 #ifdef CONFIG_MEMORY_HOTREMOVE
 extern int unregister_memory_section(struct mem_section *);
 #endif
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 2ea574f..4b7949a 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -99,6 +99,8 @@ extern void __online_page_free(struct page *page);
 
 extern int try_online_node(int nid);
 
+extern bool memhp_auto_online;
+
 #ifdef CONFIG_MEMORY_HOTREMOVE
 extern bool is_pageblock_removable_nolock(struct page *page);
 extern int arch_remove_memory(u64 start, u64 size);
@@ -267,7 +269,7 @@ static inline void remove_memory(int nid, u64 start, u64 size) {}
 extern int walk_memory_range(unsigned long start_pfn, unsigned long end_pfn,
 		void *arg, int (*func)(struct memory_block *, void *));
 extern int add_memory(int nid, u64 start, u64 size);
-extern int add_memory_resource(int nid, struct resource *resource);
+extern int add_memory_resource(int nid, struct resource *resource, bool online);
 extern int zone_for_memory(int nid, u64 start, u64 size, int zone_default,
 		bool for_device);
 extern int arch_add_memory(int nid, u64 start, u64 size, bool for_device);
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index a042a9d..9c3637e 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -76,6 +76,9 @@ static struct {
 #define memhp_lock_acquire()      lock_map_acquire(&mem_hotplug.dep_map)
 #define memhp_lock_release()      lock_map_release(&mem_hotplug.dep_map)
 
+bool memhp_auto_online;
+EXPORT_SYMBOL_GPL(memhp_auto_online);
+
 void get_online_mems(void)
 {
 	might_sleep();
@@ -476,6 +479,7 @@ static int __meminit __add_section(int nid, struct zone *zone,
 					unsigned long phys_start_pfn)
 {
 	int ret;
+	unsigned long state;
 
 	if (pfn_valid(phys_start_pfn))
 		return -EEXIST;
@@ -490,7 +494,10 @@ static int __meminit __add_section(int nid, struct zone *zone,
 	if (ret < 0)
 		return ret;
 
-	return register_new_memory(nid, __pfn_to_section(phys_start_pfn));
+	state = memhp_auto_online ? MEM_ONLINE : MEM_OFFLINE;
+
+	return register_new_memory(nid, __pfn_to_section(phys_start_pfn),
+				   state);
 }
 
 /*
@@ -1232,7 +1239,7 @@ int zone_for_memory(int nid, u64 start, u64 size, int zone_default,
 }
 
 /* we are OK calling __meminit stuff here - we have CONFIG_MEMORY_HOTPLUG */
-int __ref add_memory_resource(int nid, struct resource *res)
+int __ref add_memory_resource(int nid, struct resource *res, bool online)
 {
 	u64 start, size;
 	pg_data_t *pgdat = NULL;
@@ -1292,6 +1299,11 @@ int __ref add_memory_resource(int nid, struct resource *res)
 	/* create new memmap entry */
 	firmware_map_add_hotplug(start, start + size, "System RAM");
 
+	/* online pages if requested */
+	if (online)
+		online_pages(start >> PAGE_SHIFT, size >> PAGE_SHIFT,
+			     MMOP_ONLINE_KEEP);
+
 	goto out;
 
 error:
@@ -1315,7 +1327,7 @@ int __ref add_memory(int nid, u64 start, u64 size)
 	if (!res)
 		return -EEXIST;
 
-	ret = add_memory_resource(nid, res);
+	ret = add_memory_resource(nid, res, memhp_auto_online);
 	if (ret < 0)
 		release_memory_resource(res);
 	return ret;
-- 
2.5.0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 1/2] memory-hotplug: add automatic onlining policy for the newly added memory

[David Rientjes]

On Tue, 12 Jan 2016, Vitaly Kuznetsov wrote:

> diff --git a/Documentation/memory-hotplug.txt b/Documentation/memory-hotplug.txt
> index ce2cfcf..ceaf40c 100644
> --- a/Documentation/memory-hotplug.txt
> +++ b/Documentation/memory-hotplug.txt
> @@ -254,12 +254,23 @@ If the memory block is online, you'll read "online".
>  If the memory block is offline, you'll read "offline".
>  
>  
> -5.2. How to online memory
> +5.2. Memory onlining

Idk why you're changing this title since you didn't change it in the table 
of contents and it already pairs with "6.2. How to offline memory".

This makes it seem like you're covering all memory onlining operations in 
the kernel (including xen onlining) rather than just memory onlined by 
root.  It doesn't cover the fact that xen onlining can be done without 
automatic onlining, so I would leave this section's title as it is and 
only cover aspects of memory onlining that users are triggering 
themselves.

>  ------------
> -Even if the memory is hot-added, it is not at ready-to-use state.
> -For using newly added memory, you have to "online" the memory block.
> +When the memory is hot-added, the kernel decides whether or not to "online"
> +it according to the policy which can be read from "auto_online_blocks" file:
>  
> -For onlining, you have to write "online" to the memory block's state file as:
> +% cat /sys/devices/system/memory/auto_online_blocks
> +
> +The default is "offline" which means the newly added memory is not in a
> +ready-to-use state and you have to "online" the newly added memory blocks
> +manually. Automatic onlining can be requested by writing "online" to
> +"auto_online_blocks" file:
> +
> +% echo online > /sys/devices/system/memory/auto_online_blocks
> +

I would explicitly point out that this is a global policy and impacts all 
memory blocks that will subsequently be hotplugged.

> +If the automatic onlining wasn't requested or some memory block was offlined
> +it is possible to change the individual block's state by writing to the "state"
> +file:
>  
>  % echo online > /sys/devices/system/memory/memoryXXX/state
>  
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index 25425d3..7008edc 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -439,6 +439,37 @@ print_block_size(struct device *dev, struct device_attribute *attr,
>  static DEVICE_ATTR(block_size_bytes, 0444, print_block_size, NULL);
>  
>  /*
> + * Memory auto online policy.
> + */
> +
> +static ssize_t
> +show_auto_online_blocks(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	if (memhp_auto_online)
> +		return sprintf(buf, "online\n");
> +	else
> +		return sprintf(buf, "offline\n");
> +}
> +
> +static ssize_t
> +store_auto_online_blocks(struct device *dev, struct device_attribute *attr,
> +			 const char *buf, size_t count)
> +{
> +	if (sysfs_streq(buf, "online"))
> +		memhp_auto_online = true;
> +	else if (sysfs_streq(buf, "offline"))
> +		memhp_auto_online = false;
> +	else
> +		return -EINVAL;
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR(auto_online_blocks, 0644, show_auto_online_blocks,
> +		   store_auto_online_blocks);
> +
> +/*
>   * Some architectures will have custom drivers to do this, and
>   * will not need to do it from userspace.  The fake hot-add code
>   * as well as ppc64 will do all of their discovery in userspace
> @@ -654,10 +685,10 @@ static int add_memory_block(int base_section_nr)
>  
>  
>  /*
> - * need an interface for the VM to add new memory regions,
> - * but without onlining it.
> + * add new memory regions keeping their state.
>   */
> -int register_new_memory(int nid, struct mem_section *section)
> +int register_new_memory(int nid, struct mem_section *section,
> +			unsigned long state)
>  {
>  	int ret = 0;
>  	struct memory_block *mem;
> @@ -669,7 +700,7 @@ int register_new_memory(int nid, struct mem_section *section)
>  		mem->section_count++;
>  		put_device(&mem->dev);
>  	} else {
> -		ret = init_memory_block(&mem, section, MEM_OFFLINE);
> +		ret = init_memory_block(&mem, section, state);
>  		if (ret)
>  			goto out;
>  	}
> @@ -737,6 +768,7 @@ static struct attribute *memory_root_attrs[] = {
>  #endif
>  
>  	&dev_attr_block_size_bytes.attr,
> +	&dev_attr_auto_online_blocks.attr,
>  	NULL
>  };
>  
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index 12eab50..890c3b5 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -338,7 +338,7 @@ static enum bp_state reserve_additional_memory(void)
>  	}
>  #endif
>  
> -	rc = add_memory_resource(nid, resource);
> +	rc = add_memory_resource(nid, resource, false);
>  	if (rc) {
>  		pr_warn("Cannot add additional memory (%i)\n", rc);
>  		goto err;
> diff --git a/include/linux/memory.h b/include/linux/memory.h
> index 8b8d8d1..1544f48 100644
> --- a/include/linux/memory.h
> +++ b/include/linux/memory.h
> @@ -108,7 +108,8 @@ extern int register_memory_notifier(struct notifier_block *nb);
>  extern void unregister_memory_notifier(struct notifier_block *nb);
>  extern int register_memory_isolate_notifier(struct notifier_block *nb);
>  extern void unregister_memory_isolate_notifier(struct notifier_block *nb);
> -extern int register_new_memory(int, struct mem_section *);
> +extern int register_new_memory(int nid, struct mem_section *section,
> +			       unsigned long state);
>  #ifdef CONFIG_MEMORY_HOTREMOVE
>  extern int unregister_memory_section(struct mem_section *);
>  #endif
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 2ea574f..4b7949a 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -99,6 +99,8 @@ extern void __online_page_free(struct page *page);
>  
>  extern int try_online_node(int nid);
>  
> +extern bool memhp_auto_online;
> +
>  #ifdef CONFIG_MEMORY_HOTREMOVE
>  extern bool is_pageblock_removable_nolock(struct page *page);
>  extern int arch_remove_memory(u64 start, u64 size);
> @@ -267,7 +269,7 @@ static inline void remove_memory(int nid, u64 start, u64 size) {}
>  extern int walk_memory_range(unsigned long start_pfn, unsigned long end_pfn,
>  		void *arg, int (*func)(struct memory_block *, void *));
>  extern int add_memory(int nid, u64 start, u64 size);
> -extern int add_memory_resource(int nid, struct resource *resource);
> +extern int add_memory_resource(int nid, struct resource *resource, bool online);
>  extern int zone_for_memory(int nid, u64 start, u64 size, int zone_default,
>  		bool for_device);
>  extern int arch_add_memory(int nid, u64 start, u64 size, bool for_device);
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index a042a9d..9c3637e 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -76,6 +76,9 @@ static struct {
>  #define memhp_lock_acquire()      lock_map_acquire(&mem_hotplug.dep_map)
>  #define memhp_lock_release()      lock_map_release(&mem_hotplug.dep_map)
>  
> +bool memhp_auto_online;
> +EXPORT_SYMBOL_GPL(memhp_auto_online);
> +
>  void get_online_mems(void)
>  {
>  	might_sleep();
> @@ -476,6 +479,7 @@ static int __meminit __add_section(int nid, struct zone *zone,
>  					unsigned long phys_start_pfn)
>  {
>  	int ret;
> +	unsigned long state;
>  
>  	if (pfn_valid(phys_start_pfn))
>  		return -EEXIST;
> @@ -490,7 +494,10 @@ static int __meminit __add_section(int nid, struct zone *zone,
>  	if (ret < 0)
>  		return ret;
>  
> -	return register_new_memory(nid, __pfn_to_section(phys_start_pfn));
> +	state = memhp_auto_online ? MEM_ONLINE : MEM_OFFLINE;
> +
> +	return register_new_memory(nid, __pfn_to_section(phys_start_pfn),
> +				   state);
>  }
>  
>  /*
> @@ -1232,7 +1239,7 @@ int zone_for_memory(int nid, u64 start, u64 size, int zone_default,
>  }
>  
>  /* we are OK calling __meminit stuff here - we have CONFIG_MEMORY_HOTPLUG */
> -int __ref add_memory_resource(int nid, struct resource *res)
> +int __ref add_memory_resource(int nid, struct resource *res, bool online)
>  {
>  	u64 start, size;
>  	pg_data_t *pgdat = NULL;
> @@ -1292,6 +1299,11 @@ int __ref add_memory_resource(int nid, struct resource *res)
>  	/* create new memmap entry */
>  	firmware_map_add_hotplug(start, start + size, "System RAM");
>  
> +	/* online pages if requested */
> +	if (online)
> +		online_pages(start >> PAGE_SHIFT, size >> PAGE_SHIFT,
> +			     MMOP_ONLINE_KEEP);
> +
>  	goto out;
>  
>  error:

Well, shucks, what happens if online_pages() fails, such as if a memory 
hot-add notifier returns an errno for MEMORY_GOING_ONLINE?  The memory was 
added but not subsequently onlined, although auto onlining was set, so how 
does userspace know the state it is in?

> @@ -1315,7 +1327,7 @@ int __ref add_memory(int nid, u64 start, u64 size)
>  	if (!res)
>  		return -EEXIST;
>  
> -	ret = add_memory_resource(nid, res);
> +	ret = add_memory_resource(nid, res, memhp_auto_online);
>  	if (ret < 0)
>  		release_memory_resource(res);
>  	return ret;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 1/2] memory-hotplug: add automatic onlining policy for the newly added memory

[Vitaly Kuznetsov]

David Rientjes <rientjes@google.com> writes:

> On Tue, 12 Jan 2016, Vitaly Kuznetsov wrote:
>
>> diff --git a/Documentation/memory-hotplug.txt b/Documentation/memory-hotplug.txt
>> index ce2cfcf..ceaf40c 100644
>> --- a/Documentation/memory-hotplug.txt
>> +++ b/Documentation/memory-hotplug.txt
>> @@ -254,12 +254,23 @@ If the memory block is online, you'll read "online".
>>  If the memory block is offline, you'll read "offline".
>>  
>>  
>> -5.2. How to online memory
>> +5.2. Memory onlining
>
> Idk why you're changing this title since you didn't change it in the table 
> of contents and it already pairs with "6.2. How to offline memory".
>
> This makes it seem like you're covering all memory onlining operations in 
> the kernel (including xen onlining) rather than just memory onlined by 
> root.  It doesn't cover the fact that xen onlining can be done without 
> automatic onlining, so I would leave this section's title as it is and 
> only cover aspects of memory onlining that users are triggering 
> themselves.

Ok, I changed the title to reflect the fact that a special action to
online memory is not always required any more but as the global policy
stays 'offline' by default for now let's keep the original title.

[skip]

>>  
>> +	/* online pages if requested */
>> +	if (online)
>> +		online_pages(start >> PAGE_SHIFT, size >> PAGE_SHIFT,
>> +			     MMOP_ONLINE_KEEP);
>> +
>>  	goto out;
>>  
>>  error:
>
> Well, shucks, what happens if online_pages() fails, such as if a memory 
> hot-add notifier returns an errno for MEMORY_GOING_ONLINE?  The memory was 
> added but not subsequently onlined, although auto onlining was set, so how 
> does userspace know the state it is in?

Bad ... we could have checked the return value but I don't see a proper
way to handling it here: if we managed to online some blocks we can't
revert back. We'll probably have to online pages block-by-block (e.g. by
utilizing memory_block_change_state()) handling possible failures.

>
>> @@ -1315,7 +1327,7 @@ int __ref add_memory(int nid, u64 start, u64 size)
>>  	if (!res)
>>  		return -EEXIST;
>>  
>> -	ret = add_memory_resource(nid, res);
>> +	ret = add_memory_resource(nid, res, memhp_auto_online);
>>  	if (ret < 0)
>>  		release_memory_resource(res);
>>  	return ret;

-- 
  Vitaly

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 1/2] memory-hotplug: add automatic onlining policy for the newly added memory

[David Rientjes]

On Wed, 13 Jan 2016, Vitaly Kuznetsov wrote:

> >> diff --git a/Documentation/memory-hotplug.txt b/Documentation/memory-hotplug.txt
> >> index ce2cfcf..ceaf40c 100644
> >> --- a/Documentation/memory-hotplug.txt
> >> +++ b/Documentation/memory-hotplug.txt
> >> @@ -254,12 +254,23 @@ If the memory block is online, you'll read "online".
> >>  If the memory block is offline, you'll read "offline".
> >>  
> >>  
> >> -5.2. How to online memory
> >> +5.2. Memory onlining
> >
> > Idk why you're changing this title since you didn't change it in the table 
> > of contents and it already pairs with "6.2. How to offline memory".
> >
> > This makes it seem like you're covering all memory onlining operations in 
> > the kernel (including xen onlining) rather than just memory onlined by 
> > root.  It doesn't cover the fact that xen onlining can be done without 
> > automatic onlining, so I would leave this section's title as it is and 
> > only cover aspects of memory onlining that users are triggering 
> > themselves.
> 
> Ok, I changed the title to reflect the fact that a special action to
> online memory is not always required any more but as the global policy
> stays 'offline' by default for now let's keep the original title.
> 

Thanks.

> >> +	/* online pages if requested */
> >> +	if (online)
> >> +		online_pages(start >> PAGE_SHIFT, size >> PAGE_SHIFT,
> >> +			     MMOP_ONLINE_KEEP);
> >> +
> >>  	goto out;
> >>  
> >>  error:
> >
> > Well, shucks, what happens if online_pages() fails, such as if a memory 
> > hot-add notifier returns an errno for MEMORY_GOING_ONLINE?  The memory was 
> > added but not subsequently onlined, although auto onlining was set, so how 
> > does userspace know the state it is in?
> 
> Bad ... we could have checked the return value but I don't see a proper
> way to handling it here: if we managed to online some blocks we can't
> revert back. We'll probably have to online pages block-by-block (e.g. by
> utilizing memory_block_change_state()) handling possible failures.
> 

My suggestion is to just simply document that auto-onlining can add the 
memory but fail to online it and the failure is silent to userspace.  If 
userspace cares, it can check the online status of the added memory blocks 
itself.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 1/2] memory-hotplug: add automatic onlining policy for the newly added memory

[Vitaly Kuznetsov]

David Rientjes <rientjes@google.com> writes:

> On Wed, 13 Jan 2016, Vitaly Kuznetsov wrote:
>
>> >> diff --git a/Documentation/memory-hotplug.txt b/Documentation/memory-hotplug.txt
>> >> index ce2cfcf..ceaf40c 100644
>> >> --- a/Documentation/memory-hotplug.txt
>> >> +++ b/Documentation/memory-hotplug.txt
>> >> @@ -254,12 +254,23 @@ If the memory block is online, you'll read "online".
>> >>  If the memory block is offline, you'll read "offline".
>> >>  
>> >>  
>> >> -5.2. How to online memory
>> >> +5.2. Memory onlining
>> >
>> > Idk why you're changing this title since you didn't change it in the table 
>> > of contents and it already pairs with "6.2. How to offline memory".
>> >
>> > This makes it seem like you're covering all memory onlining operations in 
>> > the kernel (including xen onlining) rather than just memory onlined by 
>> > root.  It doesn't cover the fact that xen onlining can be done without 
>> > automatic onlining, so I would leave this section's title as it is and 
>> > only cover aspects of memory onlining that users are triggering 
>> > themselves.
>> 
>> Ok, I changed the title to reflect the fact that a special action to
>> online memory is not always required any more but as the global policy
>> stays 'offline' by default for now let's keep the original title.
>> 
>
> Thanks.
>
>> >> +	/* online pages if requested */
>> >> +	if (online)
>> >> +		online_pages(start >> PAGE_SHIFT, size >> PAGE_SHIFT,
>> >> +			     MMOP_ONLINE_KEEP);
>> >> +
>> >>  	goto out;
>> >>  
>> >>  error:
>> >
>> > Well, shucks, what happens if online_pages() fails, such as if a memory 
>> > hot-add notifier returns an errno for MEMORY_GOING_ONLINE?  The memory was 
>> > added but not subsequently onlined, although auto onlining was set, so how 
>> > does userspace know the state it is in?
>> 
>> Bad ... we could have checked the return value but I don't see a proper
>> way to handling it here: if we managed to online some blocks we can't
>> revert back. We'll probably have to online pages block-by-block (e.g. by
>> utilizing memory_block_change_state()) handling possible failures.
>> 
>
> My suggestion is to just simply document that auto-onlining can add the 
> memory but fail to online it and the failure is silent to userspace.  If 
> userspace cares, it can check the online status of the added memory blocks 
> itself.

The problem is not only that it's silent, but also that
/sys/devices/system/memory/*/state will lie as we create all memory
blocks in MEM_ONLINE state and from online_pages() error we can't figure
out which particular block failed. 'v5' which I sent yesterday is
supposed to fix the issue (blocks are onlined with
memory_block_change_state() which handles failures.

-- 
  Vitaly

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 1/2] memory-hotplug: add automatic onlining policy for the newly added memory

[David Rientjes]

On Thu, 14 Jan 2016, Vitaly Kuznetsov wrote:

> > My suggestion is to just simply document that auto-onlining can add the 
> > memory but fail to online it and the failure is silent to userspace.  If 
> > userspace cares, it can check the online status of the added memory blocks 
> > itself.
> 
> The problem is not only that it's silent, but also that
> /sys/devices/system/memory/*/state will lie as we create all memory
> blocks in MEM_ONLINE state and from online_pages() error we can't figure
> out which particular block failed. 'v5' which I sent yesterday is
> supposed to fix the issue (blocks are onlined with
> memory_block_change_state() which handles failures.
> 

Would you mind documenting that in the memory-hotplug.txt as an add-on 
patch to your v5, which appears ready to go?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 1/2] memory-hotplug: add automatic onlining policy for the newly added memory

[Vitaly Kuznetsov]

David Rientjes <rientjes@google.com> writes:

> On Thu, 14 Jan 2016, Vitaly Kuznetsov wrote:
>
>> > My suggestion is to just simply document that auto-onlining can add the 
>> > memory but fail to online it and the failure is silent to userspace.  If 
>> > userspace cares, it can check the online status of the added memory blocks 
>> > itself.
>> 
>> The problem is not only that it's silent, but also that
>> /sys/devices/system/memory/*/state will lie as we create all memory
>> blocks in MEM_ONLINE state and from online_pages() error we can't figure
>> out which particular block failed. 'v5' which I sent yesterday is
>> supposed to fix the issue (blocks are onlined with
>> memory_block_change_state() which handles failures.
>> 
>
> Would you mind documenting that in the memory-hotplug.txt as an add-on 
> patch to your v5, which appears ready to go?

Sure,

I'll mention possible failures diring automatic onlining. It seems v5
wasn't picked by Andrew and I also have one nitpick in PATCH 2 to
address so I'll send v6.

Thanks,

-- 
  Vitaly

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 1/2] memory-hotplug: add automatic onlining policy for the newly added memory

[Daniel Kiper]

On Tue, Jan 12, 2016 at 05:56:16PM +0100, Vitaly Kuznetsov wrote:
> Currently, all newly added memory blocks remain in 'offline' state unless
> someone onlines them, some linux distributions carry special udev rules
> like:
>
> SUBSYSTEM=="memory", ACTION=="add", ATTR{state}=="offline", ATTR{state}="online"
>
> to make this happen automatically. This is not a great solution for virtual
> machines where memory hotplug is being used to address high memory pressure
> situations as such onlining is slow and a userspace process doing this
> (udev) has a chance of being killed by the OOM killer as it will probably
> require to allocate some memory.
>
> Introduce default policy for the newly added memory blocks in
> /sys/devices/system/memory/auto_online_blocks file with two possible
> values: "offline" which preserves the current behavior and "online" which
> causes all newly added memory blocks to go online as soon as they're added.
> The default is "offline".
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  Documentation/memory-hotplug.txt | 19 +++++++++++++++----
>  drivers/base/memory.c            | 40 ++++++++++++++++++++++++++++++++++++----
>  drivers/xen/balloon.c            |  2 +-
>  include/linux/memory.h           |  3 ++-
>  include/linux/memory_hotplug.h   |  4 +++-
>  mm/memory_hotplug.c              | 18 +++++++++++++++---
>  6 files changed, 72 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/memory-hotplug.txt b/Documentation/memory-hotplug.txt
> index ce2cfcf..ceaf40c 100644
> --- a/Documentation/memory-hotplug.txt
> +++ b/Documentation/memory-hotplug.txt
> @@ -254,12 +254,23 @@ If the memory block is online, you'll read "online".
>  If the memory block is offline, you'll read "offline".
>
>
> -5.2. How to online memory
> +5.2. Memory onlining
>  ------------
> -Even if the memory is hot-added, it is not at ready-to-use state.
> -For using newly added memory, you have to "online" the memory block.
> +When the memory is hot-added, the kernel decides whether or not to "online"
> +it according to the policy which can be read from "auto_online_blocks" file:
>
> -For onlining, you have to write "online" to the memory block's state file as:
> +% cat /sys/devices/system/memory/auto_online_blocks
> +
> +The default is "offline" which means the newly added memory is not in a
> +ready-to-use state and you have to "online" the newly added memory blocks
> +manually. Automatic onlining can be requested by writing "online" to
> +"auto_online_blocks" file:
> +
> +% echo online > /sys/devices/system/memory/auto_online_blocks

It looks that you forgot to mention that setting auto_online_blocks to
online does not online currently offlined blocks automatically.

Daniel

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v4 2/2] xen_balloon: support memory auto onlining policy

[Vitaly Kuznetsov]

Add support for the newly added kernel memory auto onlining policy to Xen
ballon driver.

Suggested-by: Daniel Kiper <daniel.kiper@oracle.com>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/xen/Kconfig   | 20 +++++++++++++-------
 drivers/xen/balloon.c | 30 +++++++++++++++++++-----------
 2 files changed, 32 insertions(+), 18 deletions(-)

diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index 73708ac..098ab49 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -37,23 +37,29 @@ config XEN_BALLOON_MEMORY_HOTPLUG
 
 	  Memory could be hotplugged in following steps:
 
-	    1) dom0: xl mem-max <domU> <maxmem>
+	    1) domU: ensure that memory auto online policy is in effect by
+	       checking /sys/devices/system/memory/auto_online_blocks file
+	       (should be 'online').
+
+	    2) dom0: xl mem-max <domU> <maxmem>
 	       where <maxmem> is >= requested memory size,
 
-	    2) dom0: xl mem-set <domU> <memory>
+	    3) dom0: xl mem-set <domU> <memory>
 	       where <memory> is requested memory size; alternatively memory
 	       could be added by writing proper value to
 	       /sys/devices/system/xen_memory/xen_memory0/target or
 	       /sys/devices/system/xen_memory/xen_memory0/target_kb on dumU,
 
-	    3) domU: for i in /sys/devices/system/memory/memory*/state; do \
-	               [ "`cat "$i"`" = offline ] && echo online > "$i"; done
+	  Alternatively, if memory auto onlining was not requested at step 1
+	  the newly added memory can be manually onlined in domU by doing the
+	  following:
 
-	  Memory could be onlined automatically on domU by adding following line to udev rules:
+		for i in /sys/devices/system/memory/memory*/state; do \
+		  [ "`cat "$i"`" = offline ] && echo online > "$i"; done
 
-	  SUBSYSTEM=="memory", ACTION=="add", RUN+="/bin/sh -c '[ -f /sys$devpath/state ] && echo online > /sys$devpath/state'"
+	  or by adding the following line to udev rules:
 
-	  In that case step 3 should be omitted.
+	  SUBSYSTEM=="memory", ACTION=="add", RUN+="/bin/sh -c '[ -f /sys$devpath/state ] && echo online > /sys$devpath/state'"
 
 config XEN_BALLOON_MEMORY_HOTPLUG_LIMIT
 	int "Hotplugged memory limit (in GiB) for a PV guest"
diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index 890c3b5..68f0aa2 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -284,7 +284,7 @@ static void release_memory_resource(struct resource *resource)
 	kfree(resource);
 }
 
-static enum bp_state reserve_additional_memory(void)
+static enum bp_state reserve_additional_memory(bool online)
 {
 	long credit;
 	struct resource *resource;
@@ -338,7 +338,18 @@ static enum bp_state reserve_additional_memory(void)
 	}
 #endif
 
-	rc = add_memory_resource(nid, resource, false);
+	/*
+	 * add_memory_resource() will call online_pages() which in its turn
+	 * will call xen_online_page() callback causing deadlock if we don't
+	 * release balloon_mutex here. It is safe because there can only be
+	 * one balloon_process() running at a time and balloon_mutex is
+	 * internal to Xen driver, generic memory hotplug code doesn't mess
+	 * with it.
+	 */
+	mutex_unlock(&balloon_mutex);
+	rc = add_memory_resource(nid, resource, online);
+	mutex_lock(&balloon_mutex);
+
 	if (rc) {
 		pr_warn("Cannot add additional memory (%i)\n", rc);
 		goto err;
@@ -562,14 +573,11 @@ static void balloon_process(struct work_struct *work)
 
 		credit = current_credit();
 
-		if (credit > 0) {
-			if (balloon_is_inflated())
-				state = increase_reservation(credit);
-			else
-				state = reserve_additional_memory();
-		}
-
-		if (credit < 0)
+		if (credit > 0 && balloon_is_inflated())
+			state = increase_reservation(credit);
+		else if (credit > 0)
+			state = reserve_additional_memory(memhp_auto_online);
+		else if (credit < 0)
 			state = decrease_reservation(-credit, GFP_BALLOON);
 
 		state = update_schedule(state);
@@ -599,7 +607,7 @@ static int add_ballooned_pages(int nr_pages)
 	enum bp_state st;
 
 	if (xen_hotplug_unpopulated) {
-		st = reserve_additional_memory();
+		st = reserve_additional_memory(false);
 		if (st != BP_ECANCELED) {
 			mutex_unlock(&balloon_mutex);
 			wait_event(balloon_wq,
-- 
2.5.0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [Xen-devel] [PATCH v4 2/2] xen_balloon: support memory auto onlining policy

[David Vrabel]

On 12/01/16 16:56, Vitaly Kuznetsov wrote:
> Add support for the newly added kernel memory auto onlining policy to Xen
> ballon driver.
[...]
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -37,23 +37,29 @@ config XEN_BALLOON_MEMORY_HOTPLUG
>  
>  	  Memory could be hotplugged in following steps:
>  
> -	    1) dom0: xl mem-max <domU> <maxmem>
> +	    1) domU: ensure that memory auto online policy is in effect by
> +	       checking /sys/devices/system/memory/auto_online_blocks file
> +	       (should be 'online').

Step 1 applies to dom0 and domUs.

> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -284,7 +284,7 @@ static void release_memory_resource(struct resource *resource)
>  	kfree(resource);
>  }
>  
> -static enum bp_state reserve_additional_memory(void)
> +static enum bp_state reserve_additional_memory(bool online)
>  {
>  	long credit;
>  	struct resource *resource;
> @@ -338,7 +338,18 @@ static enum bp_state reserve_additional_memory(void)
>  	}
>  #endif
>  
> -	rc = add_memory_resource(nid, resource, false);
> +	/*
> +	 * add_memory_resource() will call online_pages() which in its turn
> +	 * will call xen_online_page() callback causing deadlock if we don't
> +	 * release balloon_mutex here. It is safe because there can only be
> +	 * one balloon_process() running at a time and balloon_mutex is
> +	 * internal to Xen driver, generic memory hotplug code doesn't mess
> +	 * with it.

There are multiple callers of reserve_additional_memory() and these are
not all serialized via the balloon process.  Replace the "It is safe..."
sentence with:

"Unlocking here is safe because the callers drop the mutex before trying
again."

> +	 */
> +	mutex_unlock(&balloon_mutex);
> +	rc = add_memory_resource(nid, resource, online);

This should always be memhp_auto_online, because...

> @@ -562,14 +573,11 @@ static void balloon_process(struct work_struct *work)
>  
>  		credit = current_credit();
>  
> -		if (credit > 0) {
> -			if (balloon_is_inflated())
> -				state = increase_reservation(credit);
> -			else
> -				state = reserve_additional_memory();
> -		}
> -
> -		if (credit < 0)
> +		if (credit > 0 && balloon_is_inflated())
> +			state = increase_reservation(credit);
> +		else if (credit > 0)
> +			state = reserve_additional_memory(memhp_auto_online);
> +		else if (credit < 0)
>  			state = decrease_reservation(-credit, GFP_BALLOON);

I'd have preferred this refactored as:

if (credit > 0) {
    if (balloon_is_inflated())
        ...
    else
        ...
} else if (credit < 0) {
    ...
}
>  
>  		state = update_schedule(state);
> @@ -599,7 +607,7 @@ static int add_ballooned_pages(int nr_pages)
>  	enum bp_state st;
>  
>  	if (xen_hotplug_unpopulated) {
> -		st = reserve_additional_memory();
> +		st = reserve_additional_memory(false);

... we want to auto-online this memory as well.

>  		if (st != BP_ECANCELED) {
>  			mutex_unlock(&balloon_mutex);
>  			wait_event(balloon_wq,
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [Xen-devel] [PATCH v4 2/2] xen_balloon: support memory auto onlining policy

[Daniel Kiper]

On Tue, Jan 12, 2016 at 05:38:32PM +0000, David Vrabel wrote:
> On 12/01/16 16:56, Vitaly Kuznetsov wrote:
> > Add support for the newly added kernel memory auto onlining policy to Xen
> > ballon driver.
> [...]
> > --- a/drivers/xen/Kconfig
> > +++ b/drivers/xen/Kconfig
> > @@ -37,23 +37,29 @@ config XEN_BALLOON_MEMORY_HOTPLUG
> >
> >  	  Memory could be hotplugged in following steps:
> >
> > -	    1) dom0: xl mem-max <domU> <maxmem>
> > +	    1) domU: ensure that memory auto online policy is in effect by
> > +	       checking /sys/devices/system/memory/auto_online_blocks file
> > +	       (should be 'online').
>
> Step 1 applies to dom0 and domUs.
>
> > --- a/drivers/xen/balloon.c
> > +++ b/drivers/xen/balloon.c
> > @@ -284,7 +284,7 @@ static void release_memory_resource(struct resource *resource)
> >  	kfree(resource);
> >  }
> >
> > -static enum bp_state reserve_additional_memory(void)
> > +static enum bp_state reserve_additional_memory(bool online)
> >  {
> >  	long credit;
> >  	struct resource *resource;
> > @@ -338,7 +338,18 @@ static enum bp_state reserve_additional_memory(void)
> >  	}
> >  #endif
> >
> > -	rc = add_memory_resource(nid, resource, false);
> > +	/*
> > +	 * add_memory_resource() will call online_pages() which in its turn
> > +	 * will call xen_online_page() callback causing deadlock if we don't
> > +	 * release balloon_mutex here. It is safe because there can only be
> > +	 * one balloon_process() running at a time and balloon_mutex is
> > +	 * internal to Xen driver, generic memory hotplug code doesn't mess
> > +	 * with it.
>
> There are multiple callers of reserve_additional_memory() and these are
> not all serialized via the balloon process.  Replace the "It is safe..."
> sentence with:
>
> "Unlocking here is safe because the callers drop the mutex before trying
> again."
>
> > +	 */
> > +	mutex_unlock(&balloon_mutex);
> > +	rc = add_memory_resource(nid, resource, online);
>
> This should always be memhp_auto_online, because...
>
> > @@ -562,14 +573,11 @@ static void balloon_process(struct work_struct *work)
> >
> >  		credit = current_credit();
> >
> > -		if (credit > 0) {
> > -			if (balloon_is_inflated())
> > -				state = increase_reservation(credit);
> > -			else
> > -				state = reserve_additional_memory();
> > -		}
> > -
> > -		if (credit < 0)
> > +		if (credit > 0 && balloon_is_inflated())
> > +			state = increase_reservation(credit);
> > +		else if (credit > 0)
> > +			state = reserve_additional_memory(memhp_auto_online);
> > +		else if (credit < 0)
> >  			state = decrease_reservation(-credit, GFP_BALLOON);
>
> I'd have preferred this refactored as:
>
> if (credit > 0) {
>     if (balloon_is_inflated())
>         ...
>     else
>         ...
> } else if (credit < 0) {
>     ...
> }
> >
> >  		state = update_schedule(state);
> > @@ -599,7 +607,7 @@ static int add_ballooned_pages(int nr_pages)
> >  	enum bp_state st;
> >
> >  	if (xen_hotplug_unpopulated) {
> > -		st = reserve_additional_memory();
> > +		st = reserve_additional_memory(false);
>
> ... we want to auto-online this memory as well.

Ugh... It looks that David is right. So, please forget everything which
I said about reserve_additional_memory() earlier. Sorry for confusion.

Daniel

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [Xen-devel] [PATCH v4 2/2] xen_balloon: support memory auto onlining policy

[Vitaly Kuznetsov]

David Vrabel <david.vrabel@citrix.com> writes:

> On 12/01/16 16:56, Vitaly Kuznetsov wrote:
>> Add support for the newly added kernel memory auto onlining policy to Xen
>> ballon driver.
> [...]
>> --- a/drivers/xen/Kconfig
>> +++ b/drivers/xen/Kconfig
>> @@ -37,23 +37,29 @@ config XEN_BALLOON_MEMORY_HOTPLUG
>>  
>>  	  Memory could be hotplugged in following steps:
>>  
>> -	    1) dom0: xl mem-max <domU> <maxmem>
>> +	    1) domU: ensure that memory auto online policy is in effect by
>> +	       checking /sys/devices/system/memory/auto_online_blocks file
>> +	       (should be 'online').
>
> Step 1 applies to dom0 and domUs.
>

domU here (even before my patch) rather means 'the domain we're trying
to add memory to', not sure how to work it shorter. What about 'target
domain'?

>> --- a/drivers/xen/balloon.c
>> +++ b/drivers/xen/balloon.c
>> @@ -284,7 +284,7 @@ static void release_memory_resource(struct resource *resource)
>>  	kfree(resource);
>>  }
>>  
>> -static enum bp_state reserve_additional_memory(void)
>> +static enum bp_state reserve_additional_memory(bool online)
>>  {
>>  	long credit;
>>  	struct resource *resource;
>> @@ -338,7 +338,18 @@ static enum bp_state reserve_additional_memory(void)
>>  	}
>>  #endif
>>  
>> -	rc = add_memory_resource(nid, resource, false);
>> +	/*
>> +	 * add_memory_resource() will call online_pages() which in its turn
>> +	 * will call xen_online_page() callback causing deadlock if we don't
>> +	 * release balloon_mutex here. It is safe because there can only be
>> +	 * one balloon_process() running at a time and balloon_mutex is
>> +	 * internal to Xen driver, generic memory hotplug code doesn't mess
>> +	 * with it.
>
> There are multiple callers of reserve_additional_memory() and these are
> not all serialized via the balloon process.  Replace the "It is safe..."
> sentence with:
>
> "Unlocking here is safe because the callers drop the mutex before trying
> again."
>
>> +	 */
>> +	mutex_unlock(&balloon_mutex);
>> +	rc = add_memory_resource(nid, resource, online);
>
> This should always be memhp_auto_online, because...
>
>> @@ -562,14 +573,11 @@ static void balloon_process(struct work_struct *work)
>>  
>>  		credit = current_credit();
>>  
>> -		if (credit > 0) {
>> -			if (balloon_is_inflated())
>> -				state = increase_reservation(credit);
>> -			else
>> -				state = reserve_additional_memory();
>> -		}
>> -
>> -		if (credit < 0)
>> +		if (credit > 0 && balloon_is_inflated())
>> +			state = increase_reservation(credit);
>> +		else if (credit > 0)
>> +			state = reserve_additional_memory(memhp_auto_online);
>> +		else if (credit < 0)
>>  			state = decrease_reservation(-credit, GFP_BALLOON);
>
> I'd have preferred this refactored as:
>
> if (credit > 0) {
>     if (balloon_is_inflated())

That's what we had before and what caused the
'reserve_additional_memory' line to become > 80 chars after adding a
parameter. But as we'll be always calling add_memory_resource() with
'memhp_auto_online' the parameter is redundant and we can keep things as
they are.

>         ...
>     else
>         ...
> } else if (credit < 0) {
>     ...
> }

>>  
>>  		state = update_schedule(state);
>> @@ -599,7 +607,7 @@ static int add_ballooned_pages(int nr_pages)
>>  	enum bp_state st;
>>  
>>  	if (xen_hotplug_unpopulated) {
>> -		st = reserve_additional_memory();
>> +		st = reserve_additional_memory(false);
>
> ... we want to auto-online this memory as well.
>
>>  		if (st != BP_ECANCELED) {
>>  			mutex_unlock(&balloon_mutex);
>>  			wait_event(balloon_wq,
>> 

-- 
  Vitaly

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>