[PATCH v2 0/2] Fix for memory online/offline.

[PATCH v2 0/2] Fix for memory online/offline.

[Tang Chen]

These two patches does some fixes in memory online/offline process.

Tang Chen (2):
  mem-hotplug: Avoid illegal state prefixed with legal state when
    changing state of memory_block.
  mem-hotplug: Introduce MMOP_OFFLINE to replace the hard coding -1.

 drivers/base/memory.c          | 24 ++++++++++++------------
 include/linux/memory_hotplug.h |  9 +++++----
 mm/memory_hotplug.c            |  9 ++++++---
 3 files changed, 23 insertions(+), 19 deletions(-)

-- 
1.8.3.1

[PATCH v2 0/2] Fix for memory online/offline.

[Tang Chen]

These two patches does some fixes in memory online/offline process.

Tang Chen (2):
  mem-hotplug: Avoid illegal state prefixed with legal state when
    changing state of memory_block.
  mem-hotplug: Introduce MMOP_OFFLINE to replace the hard coding -1.

 drivers/base/memory.c          | 24 ++++++++++++------------
 include/linux/memory_hotplug.h |  9 +++++----
 mm/memory_hotplug.c            |  9 ++++++---
 3 files changed, 23 insertions(+), 19 deletions(-)

-- 
1.8.3.1

--
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 v2 1/2] mem-hotplug: Avoid illegal state prefixed with legal state when changing state of memory_block.

[Tang Chen]

We use the following command to online a memory_block:

echo online|online_kernel|online_movable > /sys/devices/system/memory/memoryXXX/state

But, if we do the following:

echo online_fhsjkghfkd > /sys/devices/system/memory/memoryXXX/state

the block will also be onlined.

This is because the following code in store_mem_state() does not compare the whole string,
but only the prefix of the string.

store_mem_state()
{
	......
 328         if (!strncmp(buf, "online_kernel", min_t(int, count, 13)))

Here, only compare the first 13 letters of the string. If we give "online_kernelXXXXXX",
it will be recognized as online_kernel, which is incorrect.

 329                 online_type = ONLINE_KERNEL;
 330         else if (!strncmp(buf, "online_movable", min_t(int, count, 14)))

We have the same problem here,

 331                 online_type = ONLINE_MOVABLE;
 332         else if (!strncmp(buf, "online", min_t(int, count, 6)))

here,

(Here is more problematic. If we give online_movalbe, which is a typo of online_movable,
 it will be recognized as online without noticing the author.)

 333                 online_type = ONLINE_KEEP;
 334         else if (!strncmp(buf, "offline", min_t(int, count, 7)))

and here.

 335                 online_type = -1;
 336         else {
 337                 ret = -EINVAL;
 338                 goto err;
 339         }
	......
}

This patch fix this problem by using sysfs_streq() to compare the whole string.

Reported-by: Hu Tao <hutao@cn.fujitsu.com>
Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
---

change log v1 -> v2:
	Following Andrew's suggestion, use sysfs_streq() to compare the whole string
	so that we can simplify the code.

---
---
 drivers/base/memory.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index bece691..fa664b9 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -325,13 +325,13 @@ store_mem_state(struct device *dev,
 	if (ret)
 		return ret;
 
-	if (!strncmp(buf, "online_kernel", min_t(int, count, 13)))
+	if (sysfs_streq(buf, "online_kernel"))
 		online_type = ONLINE_KERNEL;
-	else if (!strncmp(buf, "online_movable", min_t(int, count, 14)))
+	else if (sysfs_streq(buf, "online_movable"))
 		online_type = ONLINE_MOVABLE;
-	else if (!strncmp(buf, "online", min_t(int, count, 6)))
+	else if (sysfs_streq(buf, "online"))
 		online_type = ONLINE_KEEP;
-	else if (!strncmp(buf, "offline", min_t(int, count, 7)))
+	else if (sysfs_streq(buf, "offline"))
 		online_type = -1;
 	else {
 		ret = -EINVAL;
-- 
1.8.3.1

[PATCH v2 1/2] mem-hotplug: Avoid illegal state prefixed with legal state when changing state of memory_block.

[Tang Chen]

We use the following command to online a memory_block:

echo online|online_kernel|online_movable > /sys/devices/system/memory/memoryXXX/state

But, if we do the following:

echo online_fhsjkghfkd > /sys/devices/system/memory/memoryXXX/state

the block will also be onlined.

This is because the following code in store_mem_state() does not compare the whole string,
but only the prefix of the string.

store_mem_state()
{
	......
 328         if (!strncmp(buf, "online_kernel", min_t(int, count, 13)))

Here, only compare the first 13 letters of the string. If we give "online_kernelXXXXXX",
it will be recognized as online_kernel, which is incorrect.

 329                 online_type = ONLINE_KERNEL;
 330         else if (!strncmp(buf, "online_movable", min_t(int, count, 14)))

We have the same problem here,

 331                 online_type = ONLINE_MOVABLE;
 332         else if (!strncmp(buf, "online", min_t(int, count, 6)))

here,

(Here is more problematic. If we give online_movalbe, which is a typo of online_movable,
 it will be recognized as online without noticing the author.)

 333                 online_type = ONLINE_KEEP;
 334         else if (!strncmp(buf, "offline", min_t(int, count, 7)))

and here.

 335                 online_type = -1;
 336         else {
 337                 ret = -EINVAL;
 338                 goto err;
 339         }
	......
}

This patch fix this problem by using sysfs_streq() to compare the whole string.

Reported-by: Hu Tao <hutao@cn.fujitsu.com>
Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
---

change log v1 -> v2:
	Following Andrew's suggestion, use sysfs_streq() to compare the whole string
	so that we can simplify the code.

---
---
 drivers/base/memory.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index bece691..fa664b9 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -325,13 +325,13 @@ store_mem_state(struct device *dev,
 	if (ret)
 		return ret;
 
-	if (!strncmp(buf, "online_kernel", min_t(int, count, 13)))
+	if (sysfs_streq(buf, "online_kernel"))
 		online_type = ONLINE_KERNEL;
-	else if (!strncmp(buf, "online_movable", min_t(int, count, 14)))
+	else if (sysfs_streq(buf, "online_movable"))
 		online_type = ONLINE_MOVABLE;
-	else if (!strncmp(buf, "online", min_t(int, count, 6)))
+	else if (sysfs_streq(buf, "online"))
 		online_type = ONLINE_KEEP;
-	else if (!strncmp(buf, "offline", min_t(int, count, 7)))
+	else if (sysfs_streq(buf, "offline"))
 		online_type = -1;
 	else {
 		ret = -EINVAL;
-- 
1.8.3.1

--
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 v2 2/2] mem-hotplug: Introduce MMOP_OFFLINE to replace the hard coding -1.

[Tang Chen]

In store_mem_state(), we have:
......
 334         else if (!strncmp(buf, "offline", min_t(int, count, 7)))
 335                 online_type = -1;
......
 355         case -1:
 356                 ret = device_offline(&mem->dev);
 357                 break;
......

Here, "offline" is hard coded as -1.

This patch does the following renaming:
 ONLINE_KEEP     ->  MMOP_ONLINE_KEEP
 ONLINE_KERNEL   ->  MMOP_ONLINE_KERNEL
 ONLINE_MOVABLE  ->  MMOP_ONLINE_MOVABLE

and introduce MMOP_OFFLINE = -1 to avoid hard coding.

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
---
 drivers/base/memory.c          | 16 ++++++++--------
 include/linux/memory_hotplug.h |  9 +++++----
 mm/memory_hotplug.c            |  9 ++++++---
 3 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index fa664b9..0f3fa8c 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -294,7 +294,7 @@ static int memory_subsys_online(struct device *dev)
 	 * attribute and need to set the online_type.
 	 */
 	if (mem->online_type < 0)
-		mem->online_type = ONLINE_KEEP;
+		mem->online_type = MMOP_ONLINE_KEEP;
 
 	ret = memory_block_change_state(mem, MEM_ONLINE, MEM_OFFLINE);
 
@@ -326,22 +326,22 @@ store_mem_state(struct device *dev,
 		return ret;
 
 	if (sysfs_streq(buf, "online_kernel"))
-		online_type = ONLINE_KERNEL;
+		online_type = MMOP_ONLINE_KERNEL;
 	else if (sysfs_streq(buf, "online_movable"))
-		online_type = ONLINE_MOVABLE;
+		online_type = MMOP_ONLINE_MOVABLE;
 	else if (sysfs_streq(buf, "online"))
-		online_type = ONLINE_KEEP;
+		online_type = MMOP_ONLINE_KEEP;
 	else if (sysfs_streq(buf, "offline"))
-		online_type = -1;
+		online_type = MMOP_OFFLINE;
 	else {
 		ret = -EINVAL;
 		goto err;
 	}
 
 	switch (online_type) {
-	case ONLINE_KERNEL:
-	case ONLINE_MOVABLE:
-	case ONLINE_KEEP:
+	case MMOP_ONLINE_KERNEL:
+	case MMOP_ONLINE_MOVABLE:
+	case MMOP_ONLINE_KEEP:
 		/*
 		 * mem->online_type is not protected so there can be a
 		 * race here.  However, when racing online, the first
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 4ca3d95..b4240cf 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -26,11 +26,12 @@ enum {
 	MEMORY_HOTPLUG_MAX_BOOTMEM_TYPE = NODE_INFO,
 };
 
-/* Types for control the zone type of onlined memory */
+/* Types for control the zone type of onlined and offlined memory */
 enum {
-	ONLINE_KEEP,
-	ONLINE_KERNEL,
-	ONLINE_MOVABLE,
+	MMOP_OFFLINE = -1,
+	MMOP_ONLINE_KEEP,
+	MMOP_ONLINE_KERNEL,
+	MMOP_ONLINE_MOVABLE,
 };
 
 /*
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index a650db2..6075f04 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -907,19 +907,22 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
 	 */
 	zone = page_zone(pfn_to_page(pfn));
 
-	if ((zone_idx(zone) > ZONE_NORMAL || online_type == ONLINE_MOVABLE) &&
+	if ((zone_idx(zone) > ZONE_NORMAL ||
+	     online_type == MMOP_ONLINE_MOVABLE) &&
 	    !can_online_high_movable(zone)) {
 		unlock_memory_hotplug();
 		return -EINVAL;
 	}
 
-	if (online_type == ONLINE_KERNEL && zone_idx(zone) == ZONE_MOVABLE) {
+	if (online_type == MMOP_ONLINE_KERNEL &&
+	    zone_idx(zone) == ZONE_MOVABLE) {
 		if (move_pfn_range_left(zone - 1, zone, pfn, pfn + nr_pages)) {
 			unlock_memory_hotplug();
 			return -EINVAL;
 		}
 	}
-	if (online_type == ONLINE_MOVABLE && zone_idx(zone) == ZONE_MOVABLE - 1) {
+	if (online_type == MMOP_ONLINE_MOVABLE &&
+	    zone_idx(zone) == ZONE_MOVABLE - 1) {
 		if (move_pfn_range_right(zone, zone + 1, pfn, pfn + nr_pages)) {
 			unlock_memory_hotplug();
 			return -EINVAL;
-- 
1.8.3.1

[PATCH v2 2/2] mem-hotplug: Introduce MMOP_OFFLINE to replace the hard coding -1.

[Tang Chen]

In store_mem_state(), we have:
......
 334         else if (!strncmp(buf, "offline", min_t(int, count, 7)))
 335                 online_type = -1;
......
 355         case -1:
 356                 ret = device_offline(&mem->dev);
 357                 break;
......

Here, "offline" is hard coded as -1.

This patch does the following renaming:
 ONLINE_KEEP     ->  MMOP_ONLINE_KEEP
 ONLINE_KERNEL   ->  MMOP_ONLINE_KERNEL
 ONLINE_MOVABLE  ->  MMOP_ONLINE_MOVABLE

and introduce MMOP_OFFLINE = -1 to avoid hard coding.

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
---
 drivers/base/memory.c          | 16 ++++++++--------
 include/linux/memory_hotplug.h |  9 +++++----
 mm/memory_hotplug.c            |  9 ++++++---
 3 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index fa664b9..0f3fa8c 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -294,7 +294,7 @@ static int memory_subsys_online(struct device *dev)
 	 * attribute and need to set the online_type.
 	 */
 	if (mem->online_type < 0)
-		mem->online_type = ONLINE_KEEP;
+		mem->online_type = MMOP_ONLINE_KEEP;
 
 	ret = memory_block_change_state(mem, MEM_ONLINE, MEM_OFFLINE);
 
@@ -326,22 +326,22 @@ store_mem_state(struct device *dev,
 		return ret;
 
 	if (sysfs_streq(buf, "online_kernel"))
-		online_type = ONLINE_KERNEL;
+		online_type = MMOP_ONLINE_KERNEL;
 	else if (sysfs_streq(buf, "online_movable"))
-		online_type = ONLINE_MOVABLE;
+		online_type = MMOP_ONLINE_MOVABLE;
 	else if (sysfs_streq(buf, "online"))
-		online_type = ONLINE_KEEP;
+		online_type = MMOP_ONLINE_KEEP;
 	else if (sysfs_streq(buf, "offline"))
-		online_type = -1;
+		online_type = MMOP_OFFLINE;
 	else {
 		ret = -EINVAL;
 		goto err;
 	}
 
 	switch (online_type) {
-	case ONLINE_KERNEL:
-	case ONLINE_MOVABLE:
-	case ONLINE_KEEP:
+	case MMOP_ONLINE_KERNEL:
+	case MMOP_ONLINE_MOVABLE:
+	case MMOP_ONLINE_KEEP:
 		/*
 		 * mem->online_type is not protected so there can be a
 		 * race here.  However, when racing online, the first
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 4ca3d95..b4240cf 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -26,11 +26,12 @@ enum {
 	MEMORY_HOTPLUG_MAX_BOOTMEM_TYPE = NODE_INFO,
 };
 
-/* Types for control the zone type of onlined memory */
+/* Types for control the zone type of onlined and offlined memory */
 enum {
-	ONLINE_KEEP,
-	ONLINE_KERNEL,
-	ONLINE_MOVABLE,
+	MMOP_OFFLINE = -1,
+	MMOP_ONLINE_KEEP,
+	MMOP_ONLINE_KERNEL,
+	MMOP_ONLINE_MOVABLE,
 };
 
 /*
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index a650db2..6075f04 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -907,19 +907,22 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
 	 */
 	zone = page_zone(pfn_to_page(pfn));
 
-	if ((zone_idx(zone) > ZONE_NORMAL || online_type == ONLINE_MOVABLE) &&
+	if ((zone_idx(zone) > ZONE_NORMAL ||
+	     online_type == MMOP_ONLINE_MOVABLE) &&
 	    !can_online_high_movable(zone)) {
 		unlock_memory_hotplug();
 		return -EINVAL;
 	}
 
-	if (online_type == ONLINE_KERNEL && zone_idx(zone) == ZONE_MOVABLE) {
+	if (online_type == MMOP_ONLINE_KERNEL &&
+	    zone_idx(zone) == ZONE_MOVABLE) {
 		if (move_pfn_range_left(zone - 1, zone, pfn, pfn + nr_pages)) {
 			unlock_memory_hotplug();
 			return -EINVAL;
 		}
 	}
-	if (online_type == ONLINE_MOVABLE && zone_idx(zone) == ZONE_MOVABLE - 1) {
+	if (online_type == MMOP_ONLINE_MOVABLE &&
+	    zone_idx(zone) == ZONE_MOVABLE - 1) {
 		if (move_pfn_range_right(zone, zone + 1, pfn, pfn + nr_pages)) {
 			unlock_memory_hotplug();
 			return -EINVAL;
-- 
1.8.3.1

--
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 v2 2/2] mem-hotplug: Introduce MMOP_OFFLINE to replace the hard coding -1.

[Hu Tao]

On Fri, Jun 06, 2014 at 11:58:54AM +0800, Tang Chen wrote:
> In store_mem_state(), we have:
> ......
>  334         else if (!strncmp(buf, "offline", min_t(int, count, 7)))
>  335                 online_type = -1;
> ......
>  355         case -1:
>  356                 ret = device_offline(&mem->dev);
>  357                 break;
> ......
> 
> Here, "offline" is hard coded as -1.
> 
> This patch does the following renaming:
>  ONLINE_KEEP     ->  MMOP_ONLINE_KEEP
>  ONLINE_KERNEL   ->  MMOP_ONLINE_KERNEL
>  ONLINE_MOVABLE  ->  MMOP_ONLINE_MOVABLE
> 
> and introduce MMOP_OFFLINE = -1 to avoid hard coding.
> 
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> ---
>  drivers/base/memory.c          | 16 ++++++++--------
>  include/linux/memory_hotplug.h |  9 +++++----
>  mm/memory_hotplug.c            |  9 ++++++---
>  3 files changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index fa664b9..0f3fa8c 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -294,7 +294,7 @@ static int memory_subsys_online(struct device *dev)
>  	 * attribute and need to set the online_type.
>  	 */
>  	if (mem->online_type < 0)
> -		mem->online_type = ONLINE_KEEP;
> +		mem->online_type = MMOP_ONLINE_KEEP;
>  
>  	ret = memory_block_change_state(mem, MEM_ONLINE, MEM_OFFLINE);
>  
> @@ -326,22 +326,22 @@ store_mem_state(struct device *dev,
>  		return ret;
>  
>  	if (sysfs_streq(buf, "online_kernel"))
> -		online_type = ONLINE_KERNEL;
> +		online_type = MMOP_ONLINE_KERNEL;
>  	else if (sysfs_streq(buf, "online_movable"))
> -		online_type = ONLINE_MOVABLE;
> +		online_type = MMOP_ONLINE_MOVABLE;
>  	else if (sysfs_streq(buf, "online"))
> -		online_type = ONLINE_KEEP;
> +		online_type = MMOP_ONLINE_KEEP;
>  	else if (sysfs_streq(buf, "offline"))
> -		online_type = -1;
> +		online_type = MMOP_OFFLINE;
>  	else {
>  		ret = -EINVAL;
>  		goto err;
>  	}
>  
>  	switch (online_type) {
> -	case ONLINE_KERNEL:
> -	case ONLINE_MOVABLE:
> -	case ONLINE_KEEP:
> +	case MMOP_ONLINE_KERNEL:
> +	case MMOP_ONLINE_MOVABLE:
> +	case MMOP_ONLINE_KEEP:

There is a `case -1' several lines below should have been converted.

>  		/*
>  		 * mem->online_type is not protected so there can be a
>  		 * race here.  However, when racing online, the first
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 4ca3d95..b4240cf 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -26,11 +26,12 @@ enum {
>  	MEMORY_HOTPLUG_MAX_BOOTMEM_TYPE = NODE_INFO,
>  };
>  
> -/* Types for control the zone type of onlined memory */
> +/* Types for control the zone type of onlined and offlined memory */
>  enum {
> -	ONLINE_KEEP,
> -	ONLINE_KERNEL,
> -	ONLINE_MOVABLE,
> +	MMOP_OFFLINE = -1,
> +	MMOP_ONLINE_KEEP,
> +	MMOP_ONLINE_KERNEL,
> +	MMOP_ONLINE_MOVABLE,
>  };
>  
>  /*
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index a650db2..6075f04 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -907,19 +907,22 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
>  	 */
>  	zone = page_zone(pfn_to_page(pfn));
>  
> -	if ((zone_idx(zone) > ZONE_NORMAL || online_type == ONLINE_MOVABLE) &&
> +	if ((zone_idx(zone) > ZONE_NORMAL ||
> +	     online_type == MMOP_ONLINE_MOVABLE) &&
>  	    !can_online_high_movable(zone)) {
>  		unlock_memory_hotplug();
>  		return -EINVAL;
>  	}
>  
> -	if (online_type == ONLINE_KERNEL && zone_idx(zone) == ZONE_MOVABLE) {
> +	if (online_type == MMOP_ONLINE_KERNEL &&
> +	    zone_idx(zone) == ZONE_MOVABLE) {
>  		if (move_pfn_range_left(zone - 1, zone, pfn, pfn + nr_pages)) {
>  			unlock_memory_hotplug();
>  			return -EINVAL;
>  		}
>  	}
> -	if (online_type == ONLINE_MOVABLE && zone_idx(zone) == ZONE_MOVABLE - 1) {
> +	if (online_type == MMOP_ONLINE_MOVABLE &&
> +	    zone_idx(zone) == ZONE_MOVABLE - 1) {
>  		if (move_pfn_range_right(zone, zone + 1, pfn, pfn + nr_pages)) {
>  			unlock_memory_hotplug();
>  			return -EINVAL;
> -- 
> 1.8.3.1

Re: [PATCH v2 2/2] mem-hotplug: Introduce MMOP_OFFLINE to replace the hard coding -1.

[Hu Tao]

On Fri, Jun 06, 2014 at 11:58:54AM +0800, Tang Chen wrote:
> In store_mem_state(), we have:
> ......
>  334         else if (!strncmp(buf, "offline", min_t(int, count, 7)))
>  335                 online_type = -1;
> ......
>  355         case -1:
>  356                 ret = device_offline(&mem->dev);
>  357                 break;
> ......
> 
> Here, "offline" is hard coded as -1.
> 
> This patch does the following renaming:
>  ONLINE_KEEP     ->  MMOP_ONLINE_KEEP
>  ONLINE_KERNEL   ->  MMOP_ONLINE_KERNEL
>  ONLINE_MOVABLE  ->  MMOP_ONLINE_MOVABLE
> 
> and introduce MMOP_OFFLINE = -1 to avoid hard coding.
> 
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> ---
>  drivers/base/memory.c          | 16 ++++++++--------
>  include/linux/memory_hotplug.h |  9 +++++----
>  mm/memory_hotplug.c            |  9 ++++++---
>  3 files changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index fa664b9..0f3fa8c 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -294,7 +294,7 @@ static int memory_subsys_online(struct device *dev)
>  	 * attribute and need to set the online_type.
>  	 */
>  	if (mem->online_type < 0)
> -		mem->online_type = ONLINE_KEEP;
> +		mem->online_type = MMOP_ONLINE_KEEP;
>  
>  	ret = memory_block_change_state(mem, MEM_ONLINE, MEM_OFFLINE);
>  
> @@ -326,22 +326,22 @@ store_mem_state(struct device *dev,
>  		return ret;
>  
>  	if (sysfs_streq(buf, "online_kernel"))
> -		online_type = ONLINE_KERNEL;
> +		online_type = MMOP_ONLINE_KERNEL;
>  	else if (sysfs_streq(buf, "online_movable"))
> -		online_type = ONLINE_MOVABLE;
> +		online_type = MMOP_ONLINE_MOVABLE;
>  	else if (sysfs_streq(buf, "online"))
> -		online_type = ONLINE_KEEP;
> +		online_type = MMOP_ONLINE_KEEP;
>  	else if (sysfs_streq(buf, "offline"))
> -		online_type = -1;
> +		online_type = MMOP_OFFLINE;
>  	else {
>  		ret = -EINVAL;
>  		goto err;
>  	}
>  
>  	switch (online_type) {
> -	case ONLINE_KERNEL:
> -	case ONLINE_MOVABLE:
> -	case ONLINE_KEEP:
> +	case MMOP_ONLINE_KERNEL:
> +	case MMOP_ONLINE_MOVABLE:
> +	case MMOP_ONLINE_KEEP:

There is a `case -1' several lines below should have been converted.

>  		/*
>  		 * mem->online_type is not protected so there can be a
>  		 * race here.  However, when racing online, the first
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 4ca3d95..b4240cf 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -26,11 +26,12 @@ enum {
>  	MEMORY_HOTPLUG_MAX_BOOTMEM_TYPE = NODE_INFO,
>  };
>  
> -/* Types for control the zone type of onlined memory */
> +/* Types for control the zone type of onlined and offlined memory */
>  enum {
> -	ONLINE_KEEP,
> -	ONLINE_KERNEL,
> -	ONLINE_MOVABLE,
> +	MMOP_OFFLINE = -1,
> +	MMOP_ONLINE_KEEP,
> +	MMOP_ONLINE_KERNEL,
> +	MMOP_ONLINE_MOVABLE,
>  };
>  
>  /*
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index a650db2..6075f04 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -907,19 +907,22 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
>  	 */
>  	zone = page_zone(pfn_to_page(pfn));
>  
> -	if ((zone_idx(zone) > ZONE_NORMAL || online_type == ONLINE_MOVABLE) &&
> +	if ((zone_idx(zone) > ZONE_NORMAL ||
> +	     online_type == MMOP_ONLINE_MOVABLE) &&
>  	    !can_online_high_movable(zone)) {
>  		unlock_memory_hotplug();
>  		return -EINVAL;
>  	}
>  
> -	if (online_type == ONLINE_KERNEL && zone_idx(zone) == ZONE_MOVABLE) {
> +	if (online_type == MMOP_ONLINE_KERNEL &&
> +	    zone_idx(zone) == ZONE_MOVABLE) {
>  		if (move_pfn_range_left(zone - 1, zone, pfn, pfn + nr_pages)) {
>  			unlock_memory_hotplug();
>  			return -EINVAL;
>  		}
>  	}
> -	if (online_type == ONLINE_MOVABLE && zone_idx(zone) == ZONE_MOVABLE - 1) {
> +	if (online_type == MMOP_ONLINE_MOVABLE &&
> +	    zone_idx(zone) == ZONE_MOVABLE - 1) {
>  		if (move_pfn_range_right(zone, zone + 1, pfn, pfn + nr_pages)) {
>  			unlock_memory_hotplug();
>  			return -EINVAL;
> -- 
> 1.8.3.1

--
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 v2 RESEND 2/2] mem-hotplug: Introduce MMOP_OFFLINE to replace the hard coding -1.

[Tang Chen]

In store_mem_state(), we have:
......
 334         else if (!strncmp(buf, "offline", min_t(int, count, 7)))
 335                 online_type = -1;
......
 355         case -1:
 356                 ret = device_offline(&mem->dev);
 357                 break;
......

Here, "offline" is hard coded as -1.

This patch does the following renaming:
 ONLINE_KEEP     ->  MMOP_ONLINE_KEEP
 ONLINE_KERNEL   ->  MMOP_ONLINE_KERNEL
 ONLINE_MOVABLE  ->  MMOP_ONLINE_MOVABLE

and introduce MMOP_OFFLINE = -1 to avoid hard coding.

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
---
 drivers/base/memory.c          | 18 +++++++++---------
 include/linux/memory_hotplug.h |  9 +++++----
 mm/memory_hotplug.c            |  9 ++++++---
 3 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index fa664b9..904442c 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -294,7 +294,7 @@ static int memory_subsys_online(struct device *dev)
 	 * attribute and need to set the online_type.
 	 */
 	if (mem->online_type < 0)
-		mem->online_type = ONLINE_KEEP;
+		mem->online_type = MMOP_ONLINE_KEEP;
 
 	ret = memory_block_change_state(mem, MEM_ONLINE, MEM_OFFLINE);
 
@@ -326,22 +326,22 @@ store_mem_state(struct device *dev,
 		return ret;
 
 	if (sysfs_streq(buf, "online_kernel"))
-		online_type = ONLINE_KERNEL;
+		online_type = MMOP_ONLINE_KERNEL;
 	else if (sysfs_streq(buf, "online_movable"))
-		online_type = ONLINE_MOVABLE;
+		online_type = MMOP_ONLINE_MOVABLE;
 	else if (sysfs_streq(buf, "online"))
-		online_type = ONLINE_KEEP;
+		online_type = MMOP_ONLINE_KEEP;
 	else if (sysfs_streq(buf, "offline"))
-		online_type = -1;
+		online_type = MMOP_OFFLINE;
 	else {
 		ret = -EINVAL;
 		goto err;
 	}
 
 	switch (online_type) {
-	case ONLINE_KERNEL:
-	case ONLINE_MOVABLE:
-	case ONLINE_KEEP:
+	case MMOP_ONLINE_KERNEL:
+	case MMOP_ONLINE_MOVABLE:
+	case MMOP_ONLINE_KEEP:
 		/*
 		 * mem->online_type is not protected so there can be a
 		 * race here.  However, when racing online, the first
@@ -352,7 +352,7 @@ store_mem_state(struct device *dev,
 		mem->online_type = online_type;
 		ret = device_online(&mem->dev);
 		break;
-	case -1:
+	case MMOP_OFFLINE:
 		ret = device_offline(&mem->dev);
 		break;
 	default:
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 4ca3d95..b4240cf 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -26,11 +26,12 @@ enum {
 	MEMORY_HOTPLUG_MAX_BOOTMEM_TYPE = NODE_INFO,
 };
 
-/* Types for control the zone type of onlined memory */
+/* Types for control the zone type of onlined and offlined memory */
 enum {
-	ONLINE_KEEP,
-	ONLINE_KERNEL,
-	ONLINE_MOVABLE,
+	MMOP_OFFLINE = -1,
+	MMOP_ONLINE_KEEP,
+	MMOP_ONLINE_KERNEL,
+	MMOP_ONLINE_MOVABLE,
 };
 
 /*
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index a650db2..6075f04 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -907,19 +907,22 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
 	 */
 	zone = page_zone(pfn_to_page(pfn));
 
-	if ((zone_idx(zone) > ZONE_NORMAL || online_type == ONLINE_MOVABLE) &&
+	if ((zone_idx(zone) > ZONE_NORMAL ||
+	     online_type == MMOP_ONLINE_MOVABLE) &&
 	    !can_online_high_movable(zone)) {
 		unlock_memory_hotplug();
 		return -EINVAL;
 	}
 
-	if (online_type == ONLINE_KERNEL && zone_idx(zone) == ZONE_MOVABLE) {
+	if (online_type == MMOP_ONLINE_KERNEL &&
+	    zone_idx(zone) == ZONE_MOVABLE) {
 		if (move_pfn_range_left(zone - 1, zone, pfn, pfn + nr_pages)) {
 			unlock_memory_hotplug();
 			return -EINVAL;
 		}
 	}
-	if (online_type == ONLINE_MOVABLE && zone_idx(zone) == ZONE_MOVABLE - 1) {
+	if (online_type == MMOP_ONLINE_MOVABLE &&
+	    zone_idx(zone) == ZONE_MOVABLE - 1) {
 		if (move_pfn_range_right(zone, zone + 1, pfn, pfn + nr_pages)) {
 			unlock_memory_hotplug();
 			return -EINVAL;
-- 
1.8.3.1

[PATCH v2 RESEND 2/2] mem-hotplug: Introduce MMOP_OFFLINE to replace the hard coding -1.

[Tang Chen]

In store_mem_state(), we have:
......
 334         else if (!strncmp(buf, "offline", min_t(int, count, 7)))
 335                 online_type = -1;
......
 355         case -1:
 356                 ret = device_offline(&mem->dev);
 357                 break;
......

Here, "offline" is hard coded as -1.

This patch does the following renaming:
 ONLINE_KEEP     ->  MMOP_ONLINE_KEEP
 ONLINE_KERNEL   ->  MMOP_ONLINE_KERNEL
 ONLINE_MOVABLE  ->  MMOP_ONLINE_MOVABLE

and introduce MMOP_OFFLINE = -1 to avoid hard coding.

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
---
 drivers/base/memory.c          | 18 +++++++++---------
 include/linux/memory_hotplug.h |  9 +++++----
 mm/memory_hotplug.c            |  9 ++++++---
 3 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index fa664b9..904442c 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -294,7 +294,7 @@ static int memory_subsys_online(struct device *dev)
 	 * attribute and need to set the online_type.
 	 */
 	if (mem->online_type < 0)
-		mem->online_type = ONLINE_KEEP;
+		mem->online_type = MMOP_ONLINE_KEEP;
 
 	ret = memory_block_change_state(mem, MEM_ONLINE, MEM_OFFLINE);
 
@@ -326,22 +326,22 @@ store_mem_state(struct device *dev,
 		return ret;
 
 	if (sysfs_streq(buf, "online_kernel"))
-		online_type = ONLINE_KERNEL;
+		online_type = MMOP_ONLINE_KERNEL;
 	else if (sysfs_streq(buf, "online_movable"))
-		online_type = ONLINE_MOVABLE;
+		online_type = MMOP_ONLINE_MOVABLE;
 	else if (sysfs_streq(buf, "online"))
-		online_type = ONLINE_KEEP;
+		online_type = MMOP_ONLINE_KEEP;
 	else if (sysfs_streq(buf, "offline"))
-		online_type = -1;
+		online_type = MMOP_OFFLINE;
 	else {
 		ret = -EINVAL;
 		goto err;
 	}
 
 	switch (online_type) {
-	case ONLINE_KERNEL:
-	case ONLINE_MOVABLE:
-	case ONLINE_KEEP:
+	case MMOP_ONLINE_KERNEL:
+	case MMOP_ONLINE_MOVABLE:
+	case MMOP_ONLINE_KEEP:
 		/*
 		 * mem->online_type is not protected so there can be a
 		 * race here.  However, when racing online, the first
@@ -352,7 +352,7 @@ store_mem_state(struct device *dev,
 		mem->online_type = online_type;
 		ret = device_online(&mem->dev);
 		break;
-	case -1:
+	case MMOP_OFFLINE:
 		ret = device_offline(&mem->dev);
 		break;
 	default:
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 4ca3d95..b4240cf 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -26,11 +26,12 @@ enum {
 	MEMORY_HOTPLUG_MAX_BOOTMEM_TYPE = NODE_INFO,
 };
 
-/* Types for control the zone type of onlined memory */
+/* Types for control the zone type of onlined and offlined memory */
 enum {
-	ONLINE_KEEP,
-	ONLINE_KERNEL,
-	ONLINE_MOVABLE,
+	MMOP_OFFLINE = -1,
+	MMOP_ONLINE_KEEP,
+	MMOP_ONLINE_KERNEL,
+	MMOP_ONLINE_MOVABLE,
 };
 
 /*
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index a650db2..6075f04 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -907,19 +907,22 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
 	 */
 	zone = page_zone(pfn_to_page(pfn));
 
-	if ((zone_idx(zone) > ZONE_NORMAL || online_type == ONLINE_MOVABLE) &&
+	if ((zone_idx(zone) > ZONE_NORMAL ||
+	     online_type == MMOP_ONLINE_MOVABLE) &&
 	    !can_online_high_movable(zone)) {
 		unlock_memory_hotplug();
 		return -EINVAL;
 	}
 
-	if (online_type == ONLINE_KERNEL && zone_idx(zone) == ZONE_MOVABLE) {
+	if (online_type == MMOP_ONLINE_KERNEL &&
+	    zone_idx(zone) == ZONE_MOVABLE) {
 		if (move_pfn_range_left(zone - 1, zone, pfn, pfn + nr_pages)) {
 			unlock_memory_hotplug();
 			return -EINVAL;
 		}
 	}
-	if (online_type == ONLINE_MOVABLE && zone_idx(zone) == ZONE_MOVABLE - 1) {
+	if (online_type == MMOP_ONLINE_MOVABLE &&
+	    zone_idx(zone) == ZONE_MOVABLE - 1) {
 		if (move_pfn_range_right(zone, zone + 1, pfn, pfn + nr_pages)) {
 			unlock_memory_hotplug();
 			return -EINVAL;
-- 
1.8.3.1

--
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 v2 RESEND 2/2] mem-hotplug: Introduce MMOP_OFFLINE to replace the hard coding -1.

[Tang Chen]

Hi all,

Would you please help to review these two patches ?

Thanks.

On 06/06/2014 01:33 PM, Tang Chen wrote:
> In store_mem_state(), we have:
> ......
>   334         else if (!strncmp(buf, "offline", min_t(int, count, 7)))
>   335                 online_type = -1;
> ......
>   355         case -1:
>   356                 ret = device_offline(&mem->dev);
>   357                 break;
> ......
>
> Here, "offline" is hard coded as -1.
>
> This patch does the following renaming:
>   ONLINE_KEEP     ->   MMOP_ONLINE_KEEP
>   ONLINE_KERNEL   ->   MMOP_ONLINE_KERNEL
>   ONLINE_MOVABLE  ->   MMOP_ONLINE_MOVABLE
>
> and introduce MMOP_OFFLINE = -1 to avoid hard coding.
>
> Signed-off-by: Tang Chen<tangchen@cn.fujitsu.com>
> ---
>   drivers/base/memory.c          | 18 +++++++++---------
>   include/linux/memory_hotplug.h |  9 +++++----
>   mm/memory_hotplug.c            |  9 ++++++---
>   3 files changed, 20 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index fa664b9..904442c 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -294,7 +294,7 @@ static int memory_subsys_online(struct device *dev)
>   	 * attribute and need to set the online_type.
>   	 */
>   	if (mem->online_type<  0)
> -		mem->online_type = ONLINE_KEEP;
> +		mem->online_type = MMOP_ONLINE_KEEP;
>
>   	ret = memory_block_change_state(mem, MEM_ONLINE, MEM_OFFLINE);
>
> @@ -326,22 +326,22 @@ store_mem_state(struct device *dev,
>   		return ret;
>
>   	if (sysfs_streq(buf, "online_kernel"))
> -		online_type = ONLINE_KERNEL;
> +		online_type = MMOP_ONLINE_KERNEL;
>   	else if (sysfs_streq(buf, "online_movable"))
> -		online_type = ONLINE_MOVABLE;
> +		online_type = MMOP_ONLINE_MOVABLE;
>   	else if (sysfs_streq(buf, "online"))
> -		online_type = ONLINE_KEEP;
> +		online_type = MMOP_ONLINE_KEEP;
>   	else if (sysfs_streq(buf, "offline"))
> -		online_type = -1;
> +		online_type = MMOP_OFFLINE;
>   	else {
>   		ret = -EINVAL;
>   		goto err;
>   	}
>
>   	switch (online_type) {
> -	case ONLINE_KERNEL:
> -	case ONLINE_MOVABLE:
> -	case ONLINE_KEEP:
> +	case MMOP_ONLINE_KERNEL:
> +	case MMOP_ONLINE_MOVABLE:
> +	case MMOP_ONLINE_KEEP:
>   		/*
>   		 * mem->online_type is not protected so there can be a
>   		 * race here.  However, when racing online, the first
> @@ -352,7 +352,7 @@ store_mem_state(struct device *dev,
>   		mem->online_type = online_type;
>   		ret = device_online(&mem->dev);
>   		break;
> -	case -1:
> +	case MMOP_OFFLINE:
>   		ret = device_offline(&mem->dev);
>   		break;
>   	default:
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 4ca3d95..b4240cf 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -26,11 +26,12 @@ enum {
>   	MEMORY_HOTPLUG_MAX_BOOTMEM_TYPE = NODE_INFO,
>   };
>
> -/* Types for control the zone type of onlined memory */
> +/* Types for control the zone type of onlined and offlined memory */
>   enum {
> -	ONLINE_KEEP,
> -	ONLINE_KERNEL,
> -	ONLINE_MOVABLE,
> +	MMOP_OFFLINE = -1,
> +	MMOP_ONLINE_KEEP,
> +	MMOP_ONLINE_KERNEL,
> +	MMOP_ONLINE_MOVABLE,
>   };
>
>   /*
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index a650db2..6075f04 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -907,19 +907,22 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
>   	 */
>   	zone = page_zone(pfn_to_page(pfn));
>
> -	if ((zone_idx(zone)>  ZONE_NORMAL || online_type == ONLINE_MOVABLE)&&
> +	if ((zone_idx(zone)>  ZONE_NORMAL ||
> +	     online_type == MMOP_ONLINE_MOVABLE)&&
>   	!can_online_high_movable(zone)) {
>   		unlock_memory_hotplug();
>   		return -EINVAL;
>   	}
>
> -	if (online_type == ONLINE_KERNEL&&  zone_idx(zone) == ZONE_MOVABLE) {
> +	if (online_type == MMOP_ONLINE_KERNEL&&
> +	    zone_idx(zone) == ZONE_MOVABLE) {
>   		if (move_pfn_range_left(zone - 1, zone, pfn, pfn + nr_pages)) {
>   			unlock_memory_hotplug();
>   			return -EINVAL;
>   		}
>   	}
> -	if (online_type == ONLINE_MOVABLE&&  zone_idx(zone) == ZONE_MOVABLE - 1) {
> +	if (online_type == MMOP_ONLINE_MOVABLE&&
> +	    zone_idx(zone) == ZONE_MOVABLE - 1) {
>   		if (move_pfn_range_right(zone, zone + 1, pfn, pfn + nr_pages)) {
>   			unlock_memory_hotplug();
>   			return -EINVAL;

Re: [PATCH v2 RESEND 2/2] mem-hotplug: Introduce MMOP_OFFLINE to replace the hard coding -1.

[Tang Chen]

Hi all,

Would you please help to review these two patches ?

Thanks.

On 06/06/2014 01:33 PM, Tang Chen wrote:
> In store_mem_state(), we have:
> ......
>   334         else if (!strncmp(buf, "offline", min_t(int, count, 7)))
>   335                 online_type = -1;
> ......
>   355         case -1:
>   356                 ret = device_offline(&mem->dev);
>   357                 break;
> ......
>
> Here, "offline" is hard coded as -1.
>
> This patch does the following renaming:
>   ONLINE_KEEP     ->   MMOP_ONLINE_KEEP
>   ONLINE_KERNEL   ->   MMOP_ONLINE_KERNEL
>   ONLINE_MOVABLE  ->   MMOP_ONLINE_MOVABLE
>
> and introduce MMOP_OFFLINE = -1 to avoid hard coding.
>
> Signed-off-by: Tang Chen<tangchen@cn.fujitsu.com>
> ---
>   drivers/base/memory.c          | 18 +++++++++---------
>   include/linux/memory_hotplug.h |  9 +++++----
>   mm/memory_hotplug.c            |  9 ++++++---
>   3 files changed, 20 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index fa664b9..904442c 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -294,7 +294,7 @@ static int memory_subsys_online(struct device *dev)
>   	 * attribute and need to set the online_type.
>   	 */
>   	if (mem->online_type<  0)
> -		mem->online_type = ONLINE_KEEP;
> +		mem->online_type = MMOP_ONLINE_KEEP;
>
>   	ret = memory_block_change_state(mem, MEM_ONLINE, MEM_OFFLINE);
>
> @@ -326,22 +326,22 @@ store_mem_state(struct device *dev,
>   		return ret;
>
>   	if (sysfs_streq(buf, "online_kernel"))
> -		online_type = ONLINE_KERNEL;
> +		online_type = MMOP_ONLINE_KERNEL;
>   	else if (sysfs_streq(buf, "online_movable"))
> -		online_type = ONLINE_MOVABLE;
> +		online_type = MMOP_ONLINE_MOVABLE;
>   	else if (sysfs_streq(buf, "online"))
> -		online_type = ONLINE_KEEP;
> +		online_type = MMOP_ONLINE_KEEP;
>   	else if (sysfs_streq(buf, "offline"))
> -		online_type = -1;
> +		online_type = MMOP_OFFLINE;
>   	else {
>   		ret = -EINVAL;
>   		goto err;
>   	}
>
>   	switch (online_type) {
> -	case ONLINE_KERNEL:
> -	case ONLINE_MOVABLE:
> -	case ONLINE_KEEP:
> +	case MMOP_ONLINE_KERNEL:
> +	case MMOP_ONLINE_MOVABLE:
> +	case MMOP_ONLINE_KEEP:
>   		/*
>   		 * mem->online_type is not protected so there can be a
>   		 * race here.  However, when racing online, the first
> @@ -352,7 +352,7 @@ store_mem_state(struct device *dev,
>   		mem->online_type = online_type;
>   		ret = device_online(&mem->dev);
>   		break;
> -	case -1:
> +	case MMOP_OFFLINE:
>   		ret = device_offline(&mem->dev);
>   		break;
>   	default:
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 4ca3d95..b4240cf 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -26,11 +26,12 @@ enum {
>   	MEMORY_HOTPLUG_MAX_BOOTMEM_TYPE = NODE_INFO,
>   };
>
> -/* Types for control the zone type of onlined memory */
> +/* Types for control the zone type of onlined and offlined memory */
>   enum {
> -	ONLINE_KEEP,
> -	ONLINE_KERNEL,
> -	ONLINE_MOVABLE,
> +	MMOP_OFFLINE = -1,
> +	MMOP_ONLINE_KEEP,
> +	MMOP_ONLINE_KERNEL,
> +	MMOP_ONLINE_MOVABLE,
>   };
>
>   /*
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index a650db2..6075f04 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -907,19 +907,22 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
>   	 */
>   	zone = page_zone(pfn_to_page(pfn));
>
> -	if ((zone_idx(zone)>  ZONE_NORMAL || online_type == ONLINE_MOVABLE)&&
> +	if ((zone_idx(zone)>  ZONE_NORMAL ||
> +	     online_type == MMOP_ONLINE_MOVABLE)&&
>   	!can_online_high_movable(zone)) {
>   		unlock_memory_hotplug();
>   		return -EINVAL;
>   	}
>
> -	if (online_type == ONLINE_KERNEL&&  zone_idx(zone) == ZONE_MOVABLE) {
> +	if (online_type == MMOP_ONLINE_KERNEL&&
> +	    zone_idx(zone) == ZONE_MOVABLE) {
>   		if (move_pfn_range_left(zone - 1, zone, pfn, pfn + nr_pages)) {
>   			unlock_memory_hotplug();
>   			return -EINVAL;
>   		}
>   	}
> -	if (online_type == ONLINE_MOVABLE&&  zone_idx(zone) == ZONE_MOVABLE - 1) {
> +	if (online_type == MMOP_ONLINE_MOVABLE&&
> +	    zone_idx(zone) == ZONE_MOVABLE - 1) {
>   		if (move_pfn_range_right(zone, zone + 1, pfn, pfn + nr_pages)) {
>   			unlock_memory_hotplug();
>   			return -EINVAL;

--
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 v2 1/2] mem-hotplug: Avoid illegal state prefixed with legal state when changing state of memory_block.

[Toshi Kani]

On Fri, 2014-06-06 at 11:58 +0800, Tang Chen wrote:
> We use the following command to online a memory_block:
> 
> echo online|online_kernel|online_movable > /sys/devices/system/memory/memoryXXX/state
> 
> But, if we do the following:
> 
> echo online_fhsjkghfkd > /sys/devices/system/memory/memoryXXX/state
> 
> the block will also be onlined.
> 
> This is because the following code in store_mem_state() does not compare the whole string,
> but only the prefix of the string.
> 
> store_mem_state()
> {
> 	......
>  328         if (!strncmp(buf, "online_kernel", min_t(int, count, 13)))
> 
> Here, only compare the first 13 letters of the string. If we give "online_kernelXXXXXX",
> it will be recognized as online_kernel, which is incorrect.
> 
>  329                 online_type = ONLINE_KERNEL;
>  330         else if (!strncmp(buf, "online_movable", min_t(int, count, 14)))
> 
> We have the same problem here,
> 
>  331                 online_type = ONLINE_MOVABLE;
>  332         else if (!strncmp(buf, "online", min_t(int, count, 6)))
> 
> here,
> 
> (Here is more problematic. If we give online_movalbe, which is a typo of online_movable,
>  it will be recognized as online without noticing the author.)
> 
>  333                 online_type = ONLINE_KEEP;
>  334         else if (!strncmp(buf, "offline", min_t(int, count, 7)))
> 
> and here.
> 
>  335                 online_type = -1;
>  336         else {
>  337                 ret = -EINVAL;
>  338                 goto err;
>  339         }
> 	......
> }
> 
> This patch fix this problem by using sysfs_streq() to compare the whole string.
> 
> Reported-by: Hu Tao <hutao@cn.fujitsu.com>
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>

Acked-by: Toshi Kani <toshi.kani@hp.com>

Thanks,
-Toshi

Re: [PATCH v2 1/2] mem-hotplug: Avoid illegal state prefixed with legal state when changing state of memory_block.

[Toshi Kani]

On Fri, 2014-06-06 at 11:58 +0800, Tang Chen wrote:
> We use the following command to online a memory_block:
> 
> echo online|online_kernel|online_movable > /sys/devices/system/memory/memoryXXX/state
> 
> But, if we do the following:
> 
> echo online_fhsjkghfkd > /sys/devices/system/memory/memoryXXX/state
> 
> the block will also be onlined.
> 
> This is because the following code in store_mem_state() does not compare the whole string,
> but only the prefix of the string.
> 
> store_mem_state()
> {
> 	......
>  328         if (!strncmp(buf, "online_kernel", min_t(int, count, 13)))
> 
> Here, only compare the first 13 letters of the string. If we give "online_kernelXXXXXX",
> it will be recognized as online_kernel, which is incorrect.
> 
>  329                 online_type = ONLINE_KERNEL;
>  330         else if (!strncmp(buf, "online_movable", min_t(int, count, 14)))
> 
> We have the same problem here,
> 
>  331                 online_type = ONLINE_MOVABLE;
>  332         else if (!strncmp(buf, "online", min_t(int, count, 6)))
> 
> here,
> 
> (Here is more problematic. If we give online_movalbe, which is a typo of online_movable,
>  it will be recognized as online without noticing the author.)
> 
>  333                 online_type = ONLINE_KEEP;
>  334         else if (!strncmp(buf, "offline", min_t(int, count, 7)))
> 
> and here.
> 
>  335                 online_type = -1;
>  336         else {
>  337                 ret = -EINVAL;
>  338                 goto err;
>  339         }
> 	......
> }
> 
> This patch fix this problem by using sysfs_streq() to compare the whole string.
> 
> Reported-by: Hu Tao <hutao@cn.fujitsu.com>
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>

Acked-by: Toshi Kani <toshi.kani@hp.com>

Thanks,
-Toshi


--
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 v2 RESEND 2/2] mem-hotplug: Introduce MMOP_OFFLINE to replace the hard coding -1.

[Toshi Kani]

On Fri, 2014-06-06 at 13:33 +0800, Tang Chen wrote:
> In store_mem_state(), we have:
> ......
>  334         else if (!strncmp(buf, "offline", min_t(int, count, 7)))
>  335                 online_type = -1;
> ......
>  355         case -1:
>  356                 ret = device_offline(&mem->dev);
>  357                 break;
> ......
> 
> Here, "offline" is hard coded as -1.
> 
> This patch does the following renaming:
>  ONLINE_KEEP     ->  MMOP_ONLINE_KEEP
>  ONLINE_KERNEL   ->  MMOP_ONLINE_KERNEL
>  ONLINE_MOVABLE  ->  MMOP_ONLINE_MOVABLE
> 
> and introduce MMOP_OFFLINE = -1 to avoid hard coding.
> 
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> ---
>  drivers/base/memory.c          | 18 +++++++++---------
>  include/linux/memory_hotplug.h |  9 +++++----
>  mm/memory_hotplug.c            |  9 ++++++---
>  3 files changed, 20 insertions(+), 16 deletions(-)

The patch does not apply cleanly to the current top of the tree.  Can
you rebase the patch?

Thanks,
-Toshi

Re: [PATCH v2 RESEND 2/2] mem-hotplug: Introduce MMOP_OFFLINE to replace the hard coding -1.

[Toshi Kani]

On Fri, 2014-06-06 at 13:33 +0800, Tang Chen wrote:
> In store_mem_state(), we have:
> ......
>  334         else if (!strncmp(buf, "offline", min_t(int, count, 7)))
>  335                 online_type = -1;
> ......
>  355         case -1:
>  356                 ret = device_offline(&mem->dev);
>  357                 break;
> ......
> 
> Here, "offline" is hard coded as -1.
> 
> This patch does the following renaming:
>  ONLINE_KEEP     ->  MMOP_ONLINE_KEEP
>  ONLINE_KERNEL   ->  MMOP_ONLINE_KERNEL
>  ONLINE_MOVABLE  ->  MMOP_ONLINE_MOVABLE
> 
> and introduce MMOP_OFFLINE = -1 to avoid hard coding.
> 
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> ---
>  drivers/base/memory.c          | 18 +++++++++---------
>  include/linux/memory_hotplug.h |  9 +++++----
>  mm/memory_hotplug.c            |  9 ++++++---
>  3 files changed, 20 insertions(+), 16 deletions(-)

The patch does not apply cleanly to the current top of the tree.  Can
you rebase the patch?

Thanks,
-Toshi

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