2014-06-06 03:57:52

by Tang Chen

[permalink] [raw]
Subject: [PATCH v2 0/2] Fix for memory online/offline.

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


2014-06-06 03:57:55

by Tang Chen

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

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 <[email protected]>
Signed-off-by: Tang Chen <[email protected]>
---

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

2014-06-06 03:57:59

by Tang Chen

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

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 <[email protected]>
---
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

2014-06-06 05:19:23

by Hu Tao

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

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 <[email protected]>
> ---
> 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

2014-06-06 05:32:46

by Tang Chen

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

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 <[email protected]>
---
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

2014-06-12 07:47:54

by Tang Chen

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

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<[email protected]>
> ---
> 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;

2014-06-13 15:09:19

by Toshi Kani

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

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 <[email protected]>
> Signed-off-by: Tang Chen <[email protected]>

Acked-by: Toshi Kani <[email protected]>

Thanks,
-Toshi

2014-06-13 15:10:32

by Toshi Kani

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

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 <[email protected]>
> ---
> 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