[PATCH] memory-hotplug: add sysfs zone_to_online attribute

[PATCH] memory-hotplug: add sysfs zone_to_online attribute

[Zhang Zhen]

Currently memory-hotplug has two limits:
1. If the memory block is in ZONE_NORMAL, you can change it to
ZONE_MOVABLE, but this memory block must be adjacent to ZONE_MOVABLE.
2. If the memory block is in ZONE_MOVABLE, you can change it to
ZONE_NORMAL, but this memory block must be adjacent to ZONE_NORMAL.

With this patch, we can easy to know a memory block can be added to
which zone, and we don't need to know the above two limits.

Updated the related Documentation.

Signed-off-by: Zhang Zhen <zhenzhang.zhang@huawei.com>
---
 Documentation/ABI/testing/sysfs-devices-memory | 10 +++++
 Documentation/memory-hotplug.txt               |  4 +-
 drivers/base/memory.c                          | 62 +++++++++++++++++++++++++-
 include/linux/memory_hotplug.h                 |  1 +
 mm/memory_hotplug.c                            |  4 +-
 5 files changed, 77 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-devices-memory b/Documentation/ABI/testing/sysfs-devices-memory
index 7405de2..0e58924 100644
--- a/Documentation/ABI/testing/sysfs-devices-memory
+++ b/Documentation/ABI/testing/sysfs-devices-memory
@@ -61,6 +61,16 @@ Users:		hotplug memory remove tools
 		http://www.ibm.com/developerworks/wikis/display/LinuxP/powerpc-utils


+What:           /sys/devices/system/memory/memoryX/zone_to_online
+Date:           July 2014
+Contact:	Zhang Zhen <zhenzhang.zhang@huawei.com>
+Description:
+		The file /sys/devices/system/memory/memoryX/zone_to_online
+		is read-only and is designed to show which zone this memory block can
+		be onlined to.
+Users:		hotplug memory remove tools
+		http://www.ibm.com/developerworks/wikis/display/LinuxP/powerpc-utils			
+
 What:		/sys/devices/system/memoryX/nodeY
 Date:		October 2009
 Contact:	Linux Memory Management list <linux-mm@kvack.org>
diff --git a/Documentation/memory-hotplug.txt b/Documentation/memory-hotplug.txt
index 45134dc..09e3d37 100644
--- a/Documentation/memory-hotplug.txt
+++ b/Documentation/memory-hotplug.txt
@@ -155,6 +155,7 @@ Under each memory block, you can see 4 files:
 /sys/devices/system/memory/memoryXXX/phys_device
 /sys/devices/system/memory/memoryXXX/state
 /sys/devices/system/memory/memoryXXX/removable
+/sys/devices/system/memory/memoryXXX/zone_to_online

 'phys_index'      : read-only and contains memory block id, same as XXX.
 'state'           : read-write
@@ -170,6 +171,8 @@ Under each memory block, you can see 4 files:
                     block is removable and a value of 0 indicates that
                     it is not removable. A memory block is removable only if
                     every section in the block is removable.
+'zone_to_online'  : read-only: designed to show which zone this memory block
+		    can be onlined to.

 NOTE:
   These directories/files appear after physical memory hotplug phase.
@@ -408,7 +411,6 @@ node if necessary.
   - allowing memory hot-add to ZONE_MOVABLE. maybe we need some switch like
     sysctl or new control file.
   - showing memory block and physical device relationship.
-  - showing memory block is under ZONE_MOVABLE or not
   - test and make it better memory offlining.
   - support HugeTLB page migration and offlining.
   - memmap removing at memory offline.
diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index a2e13e2..044353c 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -373,11 +373,70 @@ static ssize_t show_phys_device(struct device *dev,
 	return sprintf(buf, "%d\n", mem->phys_device);
 }

+static ssize_t show_zone_to_online(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct memory_block *mem = to_memory_block(dev);
+	unsigned long start_pfn, end_pfn;
+	unsigned long nr_pages = PAGES_PER_SECTION * sections_per_block;
+	struct page *first_page;
+	struct zone *zone, *zone_prev, *zone_next;
+
+	first_page = pfn_to_page(mem->start_section_nr << PFN_SECTION_SHIFT);
+	start_pfn = page_to_pfn(first_page);
+	end_pfn = start_pfn + nr_pages;
+
+	/*The block contains more than one zone can not be offlined.*/
+	if (!test_pages_in_a_zone(start_pfn, end_pfn))
+		return sprintf(buf, "NULL\n");
+
+	zone = page_zone(first_page);
+
+	/*The mem block is the last block of memory.*/
+	if (!pfn_valid(end_pfn + 1)) {
+#ifdef CONFIG_HIGHMEM
+		if (zone_idx(zone) == ZONE_HIGHMEM)
+			return sprintf(buf, "%s %s\n", zone->name, (zone + 1)->name);
+#else
+		if (zone_idx(zone) == ZONE_NORMAL)
+			return sprintf(buf, "%s %s\n", zone->name, (zone + 1)->name);
+#endif
+		if (zone_idx(zone) == ZONE_MOVABLE) {
+			if (pfn_valid(start_pfn - nr_pages)) {
+				zone_prev = page_zone(first_page - nr_pages);
+				if (zone_idx(zone_prev) != ZONE_MOVABLE)
+					return sprintf(buf, "%s %s\n", zone->name, (zone - 1)->name);
+			} else
+				return sprintf(buf, "%s %s\n", zone->name, (zone - 1)->name);
+		}
+		return sprintf(buf, "%s\n", zone->name);
+		
+	}
+
+	zone_next = page_zone(first_page + nr_pages + 1);
+#ifdef CONFIG_HIGHMEM
+	if (zone_idx(zone) == ZONE_HIGHMEM && zone_idx(zone_next) == ZONE_MOVABLE)
+		return sprintf(buf, "%s %s\n", zone->name, zone_next->name);
+#else
+	if (zone_idx(zone) == ZONE_NORMAL && zone_idx(zone_next) == ZONE_MOVABLE)
+		return sprintf(buf, "%s %s\n", zone->name, zone_next->name);
+#endif
+	if (zone_idx(zone) == ZONE_MOVABLE) {
+		if (pfn_valid(start_pfn - nr_pages)) {
+			zone_prev = page_zone(first_page - nr_pages);
+			if (zone_idx(zone_prev) != ZONE_MOVABLE)
+				return sprintf(buf, "%s %s\n", zone->name, (zone - 1)->name);
+		} else
+			return sprintf(buf, "%s %s\n", zone->name, (zone - 1)->name);
+	}
+	return sprintf(buf, "%s\n", zone->name);
+}
+
 static DEVICE_ATTR(phys_index, 0444, show_mem_start_phys_index, NULL);
 static DEVICE_ATTR(state, 0644, show_mem_state, store_mem_state);
 static DEVICE_ATTR(phys_device, 0444, show_phys_device, NULL);
 static DEVICE_ATTR(removable, 0444, show_mem_removable, NULL);
-
+static DEVICE_ATTR(zone_to_online, 0444, show_zone_to_online, NULL);
 /*
  * Block size attribute stuff
  */
@@ -523,6 +582,7 @@ static struct attribute *memory_memblk_attrs[] = {
 	&dev_attr_state.attr,
 	&dev_attr_phys_device.attr,
 	&dev_attr_removable.attr,
+	&dev_attr_zone_to_online.attr,
 	NULL
 };

diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index d9524c4..8f1a419 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -84,6 +84,7 @@ extern int zone_grow_waitqueues(struct zone *zone, unsigned long nr_pages);
 extern int add_one_highpage(struct page *page, int pfn, int bad_ppro);
 /* VM interface that may be used by firmware interface */
 extern int online_pages(unsigned long, unsigned long, int);
+extern int test_pages_in_a_zone(unsigned long, unsigned long);
 extern void __offline_isolated_pages(unsigned long, unsigned long);

 typedef void (*online_page_callback_t)(struct page *page);
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 2ff8c23..785b7e6 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1307,7 +1307,7 @@ int is_mem_section_removable(unsigned long start_pfn, unsigned long nr_pages)
 /*
  * Confirm all pages in a range [start, end) is belongs to the same zone.
  */
-static int test_pages_in_a_zone(unsigned long start_pfn, unsigned long end_pfn)
+int test_pages_in_a_zone(unsigned long start_pfn, unsigned long end_pfn)
 {
 	unsigned long pfn;
 	struct zone *zone = NULL;
@@ -1638,7 +1638,7 @@ static void node_states_clear_node(int node, struct memory_notify *arg)
 		node_clear_state(node, N_MEMORY);
 }

-static int __ref __offline_pages(unsigned long start_pfn,
+int __ref __offline_pages(unsigned long start_pfn,
 		  unsigned long end_pfn, unsigned long timeout)
 {
 	unsigned long pfn, nr_pages, expire;
-- 
1.8.1.2


.




--
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] memory-hotplug: add sysfs zone_to_online attribute

[Zhang Zhen]

On 2014/8/11 15:18, Zhang Zhen wrote:
> Currently memory-hotplug has two limits:
> 1. If the memory block is in ZONE_NORMAL, you can change it to
> ZONE_MOVABLE, but this memory block must be adjacent to ZONE_MOVABLE.
> 2. If the memory block is in ZONE_MOVABLE, you can change it to
> ZONE_NORMAL, but this memory block must be adjacent to ZONE_NORMAL.
> 
> With this patch, we can easy to know a memory block can be added to
> which zone, and we don't need to know the above two limits.
> 
> Updated the related Documentation.
> 
> Signed-off-by: Zhang Zhen <zhenzhang.zhang@huawei.com>
> ---
>  Documentation/ABI/testing/sysfs-devices-memory | 10 +++++
>  Documentation/memory-hotplug.txt               |  4 +-
>  drivers/base/memory.c                          | 62 +++++++++++++++++++++++++-
>  include/linux/memory_hotplug.h                 |  1 +
>  mm/memory_hotplug.c                            |  4 +-
>  5 files changed, 77 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-devices-memory b/Documentation/ABI/testing/sysfs-devices-memory
> index 7405de2..0e58924 100644
> --- a/Documentation/ABI/testing/sysfs-devices-memory
> +++ b/Documentation/ABI/testing/sysfs-devices-memory
> @@ -61,6 +61,16 @@ Users:		hotplug memory remove tools
>  		http://www.ibm.com/developerworks/wikis/display/LinuxP/powerpc-utils
> 
> 
> +What:           /sys/devices/system/memory/memoryX/zone_to_online
> +Date:           July 2014
> +Contact:	Zhang Zhen <zhenzhang.zhang@huawei.com>
> +Description:
> +		The file /sys/devices/system/memory/memoryX/zone_to_online
> +		is read-only and is designed to show which zone this memory block can
> +		be onlined to.
> +Users:		hotplug memory remove tools
> +		http://www.ibm.com/developerworks/wikis/display/LinuxP/powerpc-utils			
> +
>  What:		/sys/devices/system/memoryX/nodeY
>  Date:		October 2009
>  Contact:	Linux Memory Management list <linux-mm@kvack.org>
> diff --git a/Documentation/memory-hotplug.txt b/Documentation/memory-hotplug.txt
> index 45134dc..09e3d37 100644
> --- a/Documentation/memory-hotplug.txt
> +++ b/Documentation/memory-hotplug.txt
> @@ -155,6 +155,7 @@ Under each memory block, you can see 4 files:
>  /sys/devices/system/memory/memoryXXX/phys_device
>  /sys/devices/system/memory/memoryXXX/state
>  /sys/devices/system/memory/memoryXXX/removable
> +/sys/devices/system/memory/memoryXXX/zone_to_online
> 
>  'phys_index'      : read-only and contains memory block id, same as XXX.
>  'state'           : read-write
> @@ -170,6 +171,8 @@ Under each memory block, you can see 4 files:
>                      block is removable and a value of 0 indicates that
>                      it is not removable. A memory block is removable only if
>                      every section in the block is removable.
> +'zone_to_online'  : read-only: designed to show which zone this memory block
> +		    can be onlined to.
> 
>  NOTE:
>    These directories/files appear after physical memory hotplug phase.
> @@ -408,7 +411,6 @@ node if necessary.
>    - allowing memory hot-add to ZONE_MOVABLE. maybe we need some switch like
>      sysctl or new control file.
>    - showing memory block and physical device relationship.
> -  - showing memory block is under ZONE_MOVABLE or not
>    - test and make it better memory offlining.
>    - support HugeTLB page migration and offlining.
>    - memmap removing at memory offline.
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index a2e13e2..044353c 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -373,11 +373,70 @@ static ssize_t show_phys_device(struct device *dev,
>  	return sprintf(buf, "%d\n", mem->phys_device);
>  }
> 
> +static ssize_t show_zone_to_online(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct memory_block *mem = to_memory_block(dev);
> +	unsigned long start_pfn, end_pfn;
> +	unsigned long nr_pages = PAGES_PER_SECTION * sections_per_block;
> +	struct page *first_page;
> +	struct zone *zone, *zone_prev, *zone_next;
> +
> +	first_page = pfn_to_page(mem->start_section_nr << PFN_SECTION_SHIFT);
> +	start_pfn = page_to_pfn(first_page);
> +	end_pfn = start_pfn + nr_pages;
> +
> +	/*The block contains more than one zone can not be offlined.*/
> +	if (!test_pages_in_a_zone(start_pfn, end_pfn))
> +		return sprintf(buf, "NULL\n");
> +
> +	zone = page_zone(first_page);
> +
> +	/*The mem block is the last block of memory.*/
> +	if (!pfn_valid(end_pfn + 1)) {
> +#ifdef CONFIG_HIGHMEM
> +		if (zone_idx(zone) == ZONE_HIGHMEM)
> +			return sprintf(buf, "%s %s\n", zone->name, (zone + 1)->name);
> +#else
> +		if (zone_idx(zone) == ZONE_NORMAL)
> +			return sprintf(buf, "%s %s\n", zone->name, (zone + 1)->name);
> +#endif
> +		if (zone_idx(zone) == ZONE_MOVABLE) {
> +			if (pfn_valid(start_pfn - nr_pages)) {
> +				zone_prev = page_zone(first_page - nr_pages);
> +				if (zone_idx(zone_prev) != ZONE_MOVABLE)
> +					return sprintf(buf, "%s %s\n", zone->name, (zone - 1)->name);
> +			} else
> +				return sprintf(buf, "%s %s\n", zone->name, (zone - 1)->name);
> +		}
> +		return sprintf(buf, "%s\n", zone->name);
> +		
> +	}
> +
> +	zone_next = page_zone(first_page + nr_pages + 1);
> +#ifdef CONFIG_HIGHMEM
> +	if (zone_idx(zone) == ZONE_HIGHMEM && zone_idx(zone_next) == ZONE_MOVABLE)
> +		return sprintf(buf, "%s %s\n", zone->name, zone_next->name);
> +#else
> +	if (zone_idx(zone) == ZONE_NORMAL && zone_idx(zone_next) == ZONE_MOVABLE)
> +		return sprintf(buf, "%s %s\n", zone->name, zone_next->name);
> +#endif
> +	if (zone_idx(zone) == ZONE_MOVABLE) {
> +		if (pfn_valid(start_pfn - nr_pages)) {
> +			zone_prev = page_zone(first_page - nr_pages);
> +			if (zone_idx(zone_prev) != ZONE_MOVABLE)
> +				return sprintf(buf, "%s %s\n", zone->name, (zone - 1)->name);
> +		} else
> +			return sprintf(buf, "%s %s\n", zone->name, (zone - 1)->name);
> +	}
> +	return sprintf(buf, "%s\n", zone->name);
> +}
> +
>  static DEVICE_ATTR(phys_index, 0444, show_mem_start_phys_index, NULL);
>  static DEVICE_ATTR(state, 0644, show_mem_state, store_mem_state);
>  static DEVICE_ATTR(phys_device, 0444, show_phys_device, NULL);
>  static DEVICE_ATTR(removable, 0444, show_mem_removable, NULL);
> -
> +static DEVICE_ATTR(zone_to_online, 0444, show_zone_to_online, NULL);
>  /*
>   * Block size attribute stuff
>   */
> @@ -523,6 +582,7 @@ static struct attribute *memory_memblk_attrs[] = {
>  	&dev_attr_state.attr,
>  	&dev_attr_phys_device.attr,
>  	&dev_attr_removable.attr,
> +	&dev_attr_zone_to_online.attr,
>  	NULL
>  };
> 
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index d9524c4..8f1a419 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -84,6 +84,7 @@ extern int zone_grow_waitqueues(struct zone *zone, unsigned long nr_pages);
>  extern int add_one_highpage(struct page *page, int pfn, int bad_ppro);
>  /* VM interface that may be used by firmware interface */
>  extern int online_pages(unsigned long, unsigned long, int);
> +extern int test_pages_in_a_zone(unsigned long, unsigned long);
>  extern void __offline_isolated_pages(unsigned long, unsigned long);
> 
>  typedef void (*online_page_callback_t)(struct page *page);
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 2ff8c23..785b7e6 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1307,7 +1307,7 @@ int is_mem_section_removable(unsigned long start_pfn, unsigned long nr_pages)
>  /*
>   * Confirm all pages in a range [start, end) is belongs to the same zone.
>   */
> -static int test_pages_in_a_zone(unsigned long start_pfn, unsigned long end_pfn)
> +int test_pages_in_a_zone(unsigned long start_pfn, unsigned long end_pfn)
>  {
>  	unsigned long pfn;
>  	struct zone *zone = NULL;
> @@ -1638,7 +1638,7 @@ static void node_states_clear_node(int node, struct memory_notify *arg)
>  		node_clear_state(node, N_MEMORY);
>  }
> 
> -static int __ref __offline_pages(unsigned long start_pfn,
> +int __ref __offline_pages(unsigned long start_pfn,
Sorry, here is needless, i will remove it in next version.
>  		  unsigned long end_pfn, unsigned long timeout)
>  {
>  	unsigned long pfn, nr_pages, expire;
> 


--
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] memory-hotplug: add sysfs zone_to_online attribute

[Dave Hansen]

On 08/11/2014 12:18 AM, Zhang Zhen wrote:
> +What:           /sys/devices/system/memory/memoryX/zone_to_online
> +Date:           July 2014
> +Contact:	Zhang Zhen <zhenzhang.zhang@huawei.com>
> +Description:
> +		The file /sys/devices/system/memory/memoryX/zone_to_online
> +		is read-only and is designed to show which zone this memory block can
> +		be onlined to.
> +Users:		hotplug memory remove tools
> +		http://www.ibm.com/developerworks/wikis/display/LinuxP/powerpc-utils			

I went and looked there.  It redirects immediately to:

	https://www.ibm.com/developerworks/community/wikis/home?lang=en

Where a search for "hotplug memory remove tools" finds nothing.

What hardware and hypervisor are you looking at this for?

>  What:		/sys/devices/system/memoryX/nodeY
>  Date:		October 2009
>  Contact:	Linux Memory Management list <linux-mm@kvack.org>
> diff --git a/Documentation/memory-hotplug.txt b/Documentation/memory-hotplug.txt
> index 45134dc..09e3d37 100644
> --- a/Documentation/memory-hotplug.txt
> +++ b/Documentation/memory-hotplug.txt
> @@ -155,6 +155,7 @@ Under each memory block, you can see 4 files:
>  /sys/devices/system/memory/memoryXXX/phys_device
>  /sys/devices/system/memory/memoryXXX/state
>  /sys/devices/system/memory/memoryXXX/removable
> +/sys/devices/system/memory/memoryXXX/zone_to_online
> 
>  'phys_index'      : read-only and contains memory block id, same as XXX.
>  'state'           : read-write
> @@ -170,6 +171,8 @@ Under each memory block, you can see 4 files:
>                      block is removable and a value of 0 indicates that
>                      it is not removable. A memory block is removable only if
>                      every section in the block is removable.
> +'zone_to_online'  : read-only: designed to show which zone this memory block
> +		    can be onlined to.

Zone to online makes it sound like this is the zone that is about to be
onlined, not the section.  "zone_online_to" would be a bit more
appropriate, or maybe "online_zones_possible".

Isn't it possible to have more than a single zone show up?

>  NOTE:
>    These directories/files appear after physical memory hotplug phase.
> @@ -408,7 +411,6 @@ node if necessary.
>    - allowing memory hot-add to ZONE_MOVABLE. maybe we need some switch like
>      sysctl or new control file.
>    - showing memory block and physical device relationship.
> -  - showing memory block is under ZONE_MOVABLE or not
>    - test and make it better memory offlining.
>    - support HugeTLB page migration and offlining.
>    - memmap removing at memory offline.
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index a2e13e2..044353c 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -373,11 +373,70 @@ static ssize_t show_phys_device(struct device *dev,
>  	return sprintf(buf, "%d\n", mem->phys_device);
>  }
> 
> +static ssize_t show_zone_to_online(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct memory_block *mem = to_memory_block(dev);
> +	unsigned long start_pfn, end_pfn;
> +	unsigned long nr_pages = PAGES_PER_SECTION * sections_per_block;
> +	struct page *first_page;
> +	struct zone *zone, *zone_prev, *zone_next;
> +
> +	first_page = pfn_to_page(mem->start_section_nr << PFN_SECTION_SHIFT);

Is there something wrong with section_nr_to_pfn()?

> +	start_pfn = page_to_pfn(first_page);

Nit: Why do pfn_to_page() then page_to_pfn() immediately?

> +	end_pfn = start_pfn + nr_pages;
> +
> +	/*The block contains more than one zone can not be offlined.*/

Do you mean onlined?

> +	if (!test_pages_in_a_zone(start_pfn, end_pfn))
> +		return sprintf(buf, "NULL\n");

"(none)" is a bit more normal nomenclature in sysfs.

> +	zone = page_zone(first_page);
> +
> +	/*The mem block is the last block of memory.*/
> +	if (!pfn_valid(end_pfn + 1)) {

This check only looks for a *hole* in the page after this block ends.
It does not check all of the other pages *above* end_pfn+1, which would
be needed to guarantee that this is "the last block of memory".

> +#ifdef CONFIG_HIGHMEM
> +		if (zone_idx(zone) == ZONE_HIGHMEM)
> +			return sprintf(buf, "%s %s\n", zone->name, (zone + 1)->name);
> +#else
> +		if (zone_idx(zone) == ZONE_NORMAL)
> +			return sprintf(buf, "%s %s\n", zone->name, (zone + 1)->name);
> +#endif
> +		if (zone_idx(zone) == ZONE_MOVABLE) {
> +			if (pfn_valid(start_pfn - nr_pages)) {
> +				zone_prev = page_zone(first_page - nr_pages);
> +				if (zone_idx(zone_prev) != ZONE_MOVABLE)
> +					return sprintf(buf, "%s %s\n", zone->name, (zone - 1)->name);
> +			} else
> +				return sprintf(buf, "%s %s\n", zone->name, (zone - 1)->name);
> +		}
> +		return sprintf(buf, "%s\n", zone->name);
> +		
> +	}
> +
> +	zone_next = page_zone(first_page + nr_pages + 1);

Is the +1 necessary?  It doesn't matter in practice since we don't have
single-page zones, but it is a bit confusing.  Seems to me like this
will get page[1] instead of page[0].

Furthermore, this might cross a section boundary where 'struct page'
arithmetic is not guaranteed not work.  That makes it _definitely_ broken.

> +#ifdef CONFIG_HIGHMEM
> +	if (zone_idx(zone) == ZONE_HIGHMEM && zone_idx(zone_next) == ZONE_MOVABLE)
> +		return sprintf(buf, "%s %s\n", zone->name, zone_next->name);
> +#else
> +	if (zone_idx(zone) == ZONE_NORMAL && zone_idx(zone_next) == ZONE_MOVABLE)
> +		return sprintf(buf, "%s %s\n", zone->name, zone_next->name);
> +#endif
> +	if (zone_idx(zone) == ZONE_MOVABLE) {
> +		if (pfn_valid(start_pfn - nr_pages)) {
> +			zone_prev = page_zone(first_page - nr_pages);
> +			if (zone_idx(zone_prev) != ZONE_MOVABLE)
> +				return sprintf(buf, "%s %s\n", zone->name, (zone - 1)->name);
> +		} else
> +			return sprintf(buf, "%s %s\n", zone->name, (zone - 1)->name);
> +	}
> +	return sprintf(buf, "%s\n", zone->name);
> +}

Can you please work to clean this up a bit.  I think it's quite a mess.
 I see two #ifdefs in a .c file, and a fairly large block of (virtually)
uncommented and copy-n-pasted code.

The two copies of this:

>> +	if (zone_idx(zone) == ZONE_MOVABLE) {
>> +		if (pfn_valid(start_pfn - nr_pages)) {
>> +			zone_prev = page_zone(first_page - nr_pages);
>> +			if (zone_idx(zone_prev) != ZONE_MOVABLE)
>> +				return sprintf(buf, "%s %s\n", zone->name, (zone - 1)->name);
>> +		} else
>> +			return sprintf(buf, "%s %s\n", zone->name, (zone - 1)->name);
>> +	}
>> +	return sprintf(buf, "%s\n", zone->name);

are identical except for the indentation.

This is also supposed to be enumerating rules that are enforced by some
other bit of code.  There are, as far as I can see, no calls in to that
other code, or comments about what code it depends on.  To me, this
looks like an invitation for the two copies to diverge.


--
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] memory-hotplug: add sysfs zone_to_online attribute

[Zhang Zhen]

On 2014/8/11 23:19, Dave Hansen wrote:
> On 08/11/2014 12:18 AM, Zhang Zhen wrote:
>> +What:           /sys/devices/system/memory/memoryX/zone_to_online
>> +Date:           July 2014
>> +Contact:	Zhang Zhen <zhenzhang.zhang@huawei.com>
>> +Description:
>> +		The file /sys/devices/system/memory/memoryX/zone_to_online
>> +		is read-only and is designed to show which zone this memory block can
>> +		be onlined to.
>> +Users:		hotplug memory remove tools
>> +		http://www.ibm.com/developerworks/wikis/display/LinuxP/powerpc-utils			
> 
> I went and looked there.  It redirects immediately to:
> 
> 	https://www.ibm.com/developerworks/community/wikis/home?lang=en
> 
> Where a search for "hotplug memory remove tools" finds nothing.
> 
> What hardware and hypervisor are you looking at this for?
> 
I tested this patch on a X86_64 machine.

Sorry, i copied this url from the description of /sys/devices/system/memory/memoryX/state.
Anyone try to online a memory block want to know which zone the block can be onlined to.
I will delete the Users.

>>  What:		/sys/devices/system/memoryX/nodeY
>>  Date:		October 2009
>>  Contact:	Linux Memory Management list <linux-mm@kvack.org>
>> diff --git a/Documentation/memory-hotplug.txt b/Documentation/memory-hotplug.txt
>> index 45134dc..09e3d37 100644
>> --- a/Documentation/memory-hotplug.txt
>> +++ b/Documentation/memory-hotplug.txt
>> @@ -155,6 +155,7 @@ Under each memory block, you can see 4 files:
>>  /sys/devices/system/memory/memoryXXX/phys_device
>>  /sys/devices/system/memory/memoryXXX/state
>>  /sys/devices/system/memory/memoryXXX/removable
>> +/sys/devices/system/memory/memoryXXX/zone_to_online
>>
>>  'phys_index'      : read-only and contains memory block id, same as XXX.
>>  'state'           : read-write
>> @@ -170,6 +171,8 @@ Under each memory block, you can see 4 files:
>>                      block is removable and a value of 0 indicates that
>>                      it is not removable. A memory block is removable only if
>>                      every section in the block is removable.
>> +'zone_to_online'  : read-only: designed to show which zone this memory block
>> +		    can be onlined to.
> 
> Zone to online makes it sound like this is the zone that is about to be
> onlined, not the section.  "zone_online_to" would be a bit more
> appropriate, or maybe "online_zones_possible".
> 
> Isn't it possible to have more than a single zone show up?
> 
Thanks for your suggestion, i think "zones_online_to" is appropriate.
Yeah, it is possible.
The sample as follows:
bash-4.2# echo 0x20000000 > probe
[ 1241.991476] init_memory_mapping: [mem 0x20000000-0x27ffffff]
bash-4.2# echo online > memory4/state
bash-4.2# echo 0x28000000 > probe
[ 1262.539036] init_memory_mapping: [mem 0x28000000-0x2fffffff]
bash-4.2# echo online_movable > memory5/state
bash-4.2# echo 0x30000000 > probe
[ 1278.999325] init_memory_mapping: [mem 0x30000000-0x37ffffff]
bash-4.2# cat memory*/zone_to_online
NULL
DMA32
DMA32
DMA32
Normal Movable
Movable Normal
Movable

>>  NOTE:
>>    These directories/files appear after physical memory hotplug phase.
>> @@ -408,7 +411,6 @@ node if necessary.
>>    - allowing memory hot-add to ZONE_MOVABLE. maybe we need some switch like
>>      sysctl or new control file.
>>    - showing memory block and physical device relationship.
>> -  - showing memory block is under ZONE_MOVABLE or not
>>    - test and make it better memory offlining.
>>    - support HugeTLB page migration and offlining.
>>    - memmap removing at memory offline.
>> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>> index a2e13e2..044353c 100644
>> --- a/drivers/base/memory.c
>> +++ b/drivers/base/memory.c
>> @@ -373,11 +373,70 @@ static ssize_t show_phys_device(struct device *dev,
>>  	return sprintf(buf, "%d\n", mem->phys_device);
>>  }
>>
>> +static ssize_t show_zone_to_online(struct device *dev,
>> +				struct device_attribute *attr, char *buf)
>> +{
>> +	struct memory_block *mem = to_memory_block(dev);
>> +	unsigned long start_pfn, end_pfn;
>> +	unsigned long nr_pages = PAGES_PER_SECTION * sections_per_block;
>> +	struct page *first_page;
>> +	struct zone *zone, *zone_prev, *zone_next;
>> +
>> +	first_page = pfn_to_page(mem->start_section_nr << PFN_SECTION_SHIFT);
> 
> Is there something wrong with section_nr_to_pfn()?

OK, i will use section_nr_to_pfn() in next version.
> 
>> +	start_pfn = page_to_pfn(first_page);
> 
> Nit: Why do pfn_to_page() then page_to_pfn() immediately?
> 
>> +	end_pfn = start_pfn + nr_pages;
>> +
>> +	/*The block contains more than one zone can not be offlined.*/
> 
> Do you mean onlined?

Yes.
As in the example above, the memory0 contains DMA and DMA32 zones, so it can not be offlined.
Another case, for a x86_64 machine booted with "mem=400M", the last block memory3 cantains DMA32
and NORMAL zones, it can not be offlined too.
> 
>> +	if (!test_pages_in_a_zone(start_pfn, end_pfn))
>> +		return sprintf(buf, "NULL\n");
> 
> "(none)" is a bit more normal nomenclature in sysfs.
> 
Ok, thanks!

>> +	zone = page_zone(first_page);
>> +
>> +	/*The mem block is the last block of memory.*/
>> +	if (!pfn_valid(end_pfn + 1)) {
> 
> This check only looks for a *hole* in the page after this block ends.
> It does not check all of the other pages *above* end_pfn+1, which would
> be needed to guarantee that this is "the last block of memory".
> 
The purpose here is to guarantee that this is "the last block of memory".
Maybe this is not the best way.
I would like to follow your better suggestion.

>> +#ifdef CONFIG_HIGHMEM
>> +		if (zone_idx(zone) == ZONE_HIGHMEM)
>> +			return sprintf(buf, "%s %s\n", zone->name, (zone + 1)->name);
>> +#else
>> +		if (zone_idx(zone) == ZONE_NORMAL)
>> +			return sprintf(buf, "%s %s\n", zone->name, (zone + 1)->name);
>> +#endif
>> +		if (zone_idx(zone) == ZONE_MOVABLE) {
>> +			if (pfn_valid(start_pfn - nr_pages)) {
>> +				zone_prev = page_zone(first_page - nr_pages);
>> +				if (zone_idx(zone_prev) != ZONE_MOVABLE)
>> +					return sprintf(buf, "%s %s\n", zone->name, (zone - 1)->name);
>> +			} else
>> +				return sprintf(buf, "%s %s\n", zone->name, (zone - 1)->name);
>> +		}
>> +		return sprintf(buf, "%s\n", zone->name);
>> +		
>> +	}
>> +
>> +	zone_next = page_zone(first_page + nr_pages + 1);
> 
> Is the +1 necessary?  It doesn't matter in practice since we don't have
> single-page zones, but it is a bit confusing.  Seems to me like this
> will get page[1] instead of page[0].
> 
You are right, +1 here is not necessary.

> Furthermore, this might cross a section boundary where 'struct page'
> arithmetic is not guaranteed not work.  That makes it _definitely_ broken.
> 
>> +#ifdef CONFIG_HIGHMEM
>> +	if (zone_idx(zone) == ZONE_HIGHMEM && zone_idx(zone_next) == ZONE_MOVABLE)
>> +		return sprintf(buf, "%s %s\n", zone->name, zone_next->name);
>> +#else
>> +	if (zone_idx(zone) == ZONE_NORMAL && zone_idx(zone_next) == ZONE_MOVABLE)
>> +		return sprintf(buf, "%s %s\n", zone->name, zone_next->name);
>> +#endif
>> +	if (zone_idx(zone) == ZONE_MOVABLE) {
>> +		if (pfn_valid(start_pfn - nr_pages)) {
>> +			zone_prev = page_zone(first_page - nr_pages);
>> +			if (zone_idx(zone_prev) != ZONE_MOVABLE)
>> +				return sprintf(buf, "%s %s\n", zone->name, (zone - 1)->name);
>> +		} else
>> +			return sprintf(buf, "%s %s\n", zone->name, (zone - 1)->name);
>> +	}
>> +	return sprintf(buf, "%s\n", zone->name);
>> +}
> 
> Can you please work to clean this up a bit.  I think it's quite a mess.
>  I see two #ifdefs in a .c file, and a fairly large block of (virtually)
> uncommented and copy-n-pasted code.

Sorry, it is really ugly, i will implement it in a separate function and call it here.

> 
> The two copies of this:
> 
>>> +	if (zone_idx(zone) == ZONE_MOVABLE) {
>>> +		if (pfn_valid(start_pfn - nr_pages)) {
>>> +			zone_prev = page_zone(first_page - nr_pages);
>>> +			if (zone_idx(zone_prev) != ZONE_MOVABLE)
>>> +				return sprintf(buf, "%s %s\n", zone->name, (zone - 1)->name);
>>> +		} else
>>> +			return sprintf(buf, "%s %s\n", zone->name, (zone - 1)->name);
>>> +	}
>>> +	return sprintf(buf, "%s\n", zone->name);
> 
> are identical except for the indentation.
> 
> This is also supposed to be enumerating rules that are enforced by some
> other bit of code.  There are, as far as I can see, no calls in to that
> other code, or comments about what code it depends on.  To me, this
> looks like an invitation for the two copies to diverge.
> 
> 
> --
> 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>
> 
> .
> 


--
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>