acpi,memory-hotplug : implement framework for hot removing memory

PATCH In review Yasuaki Ishimatsu 4 participants 30 messages Oct 3, 2012 – Oct 22, 2012

Subsystems (3)

Memory management ACPI Memory hot(un)plug

lore

Share & tools

lore

Yasuaki Ishimatsu

author

2012-10-03 09:53 UTC #0 lore raw

cover acpi,memory-hotplug : implement framework for hot removing memory

We are trying to implement a physical memory hot removing function as following thread.

https://lkml.org/lkml/2012/9/5/201

But there is not enough review to merge into linux kernel.

I think there are following blockades.
1. no physical memory hot removable system
2. huge patch-set

If you have a KVM system, we can get rid of 1st blockade. Because applying following patch, we can create memory hot removable system on KVM guest.

http://lists.gnu.org/archive/html/qemu-devel/2012-07/msg01389.html

2nd blockade is own problem. So we try to divide huge patch into a small patch in each function as follows:

- bug fix
- acpi framework
- kernel core

We had already sent bug fix patches.
https://lkml.org/lkml/2012/9/27/39
https://lkml.org/lkml/2012/10/2/83

The patch-set implements a framework for hot removing memory.

The memory device can be removed by 2 ways:
1. send eject request by SCI
2. echo 1 >/sys/bus/pci/devices/PNP0C80:XX/eject

In the 1st case, acpi_memory_disable_device() will be called. In the 2nd case, acpi_memory_device_remove() will be called. acpi_memory_device_remove() will also be called when we unbind the memory device from the driver acpi_memhotplug.

acpi_memory_disable_device() has already implemented a code which offlines memory and releases acpi_memory_info struct . But acpi_memory_device_remove() has not implemented it yet.

So the patch prepares the framework for hot removing memory and adds the framework intoacpi_memory_device_remove(). And it prepares remove_memory(). But the function does nothing because we cannot support memory hot remove.

Yasuaki Ishimatsu

author

2012-10-03 09:58 UTC #1 lore raw

PATCH 1/4 acpi,memory-hotplug : add memory offline code to acpi_memory_device_remove()

From: Yasuaki Ishimatsu <[email protected]>

The memory device can be removed by 2 ways:
1. send eject request by SCI
2. echo 1 >/sys/bus/pci/devices/PNP0C80:XX/eject

acpi_memory_disable_device() has already implemented a code which offlines memory and releases acpi_memory_info struct . But acpi_memory_device_remove() has not implemented it yet.

So the patch implements acpi_memory_remove_memory() for offlining memory and releasing acpi_memory_info struct. And it is used by both acpi_memory_device_remove() and acpi_memory_disable_device().

Additionally, if the type is ACPI_BUS_REMOVAL_EJECT in acpi_memory_device_remove() , it means that the user wants to eject the memory device. In this case, acpi_memory_device_remove() calls acpi_memory_remove_memory().

CC David Rientjes <[email protected]>

CC Jiang Liu <[email protected]>

CC Len Brown <[email protected]>

CC Christoph Lameter <[email protected]>

Cc Minchan Kim <[email protected]>

CC Andrew Morton <[email protected]>

CC KOSAKI Motohiro <[email protected]>

Signed-off-by Yasuaki Ishimatsu <[email protected]>

Signed-off-by Wen Congyang <[email protected]>

---
 drivers/acpi/acpi_memhotplug.c |   44 +++++++++++++++++++++++++++++++----------
 1 file changed, 34 insertions(+), 10 deletions(-)

drivers/acpi/acpi_memhotplug.c +34 ~~-10~~

Index: linux-3.6/drivers/acpi/acpi_memhotplug.c
===================================================================
@@ -306,24 +306,37 @@ static int acpi_memory_powerdown_device(
 	return 0;
 }
 
-static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
+static int acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
 {
 	int result;
 	struct acpi_memory_info *info, *n;
 
+	list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
+		if (!info->enabled)
+			return -EBUSY;
+
+		result = remove_memory(info->start_addr, info->length);
+		if (result)
+			return result;
+
+		list_del(&info->list);
+		kfree(info);
+	}
+
+	return 0;
+}
+
+static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
+{
+	int result;
 
 	/*
 	 * Ask the VM to offline this memory range.
 	 * Note: Assume that this function returns zero on success
 	 */
-	list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
-		if (info->enabled) {
-			result = remove_memory(info->start_addr, info->length);
-			if (result)
-				return result;
-		}
-		kfree(info);
-	}
+	result = acpi_memory_remove_memory(mem_device);
+	if (result)
+		return result;
 
 	/* Power-off and eject the device */
 	result = acpi_memory_powerdown_device(mem_device);

@@ -473,12 +486,23 @@ static int acpi_memory_device_add(struct
 static int acpi_memory_device_remove(struct acpi_device *device, int type)
 {
 	struct acpi_memory_device *mem_device = NULL;
-
+	int result;
 
 	if (!device || !acpi_driver_data(device))
 		return -EINVAL;
 
 	mem_device = acpi_driver_data(device);
+
+	if (type == ACPI_BUS_REMOVAL_EJECT) {
+		/*
+		 * offline and remove memory only when the memory device is
+		 * ejected.
+		 */
+		result = acpi_memory_remove_memory(mem_device);
+		if (result)
+			return result;
+	}
+
 	kfree(mem_device);
 
 	return 0;

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

KOSAKI Motohiro

2012-10-04 20:53 UTC #2 lore raw

Re: [PATCH 1/4] acpi,memory-hotplug : add memory offline code to acpi_memory_device_remove()

Quoted 42 lines · Yasuaki Ishimatsu

From: Yasuaki Ishimatsu <[email protected]>
The memory device can be removed by 2 ways:
1. send eject request by SCI
2. echo 1 >/sys/bus/pci/devices/PNP0C80:XX/eject
In the 1st case, acpi_memory_disable_device() will be called. In the 2nd case, acpi_memory_device_remove() will be called. acpi_memory_device_remove() will also be called when we unbind the memory device from the driver acpi_memhotplug.
acpi_memory_disable_device() has already implemented a code which offlines memory and releases acpi_memory_info struct . But acpi_memory_device_remove() has not implemented it yet.
So the patch implements acpi_memory_remove_memory() for offlining memory and releasing acpi_memory_info struct. And it is used by both acpi_memory_device_remove() and acpi_memory_disable_device().
Additionally, if the type is ACPI_BUS_REMOVAL_EJECT in acpi_memory_device_remove() , it means that the user wants to eject the memory device. In this case, acpi_memory_device_remove() calls acpi_memory_remove_memory().
CC David Rientjes <[email protected]>
CC Jiang Liu <[email protected]>
CC Len Brown <[email protected]>
CC Christoph Lameter <[email protected]>
Cc Minchan Kim <[email protected]>
CC Andrew Morton <[email protected]>
CC KOSAKI Motohiro <[email protected]>
Signed-off-by Yasuaki Ishimatsu <[email protected]>
Signed-off-by Wen Congyang <[email protected]>
---
 drivers/acpi/acpi_memhotplug.c |   44 +++++++++++++++++++++++++++++++----------
 1 file changed, 34 insertions(+), 10 deletions(-)
Index: linux-3.6/drivers/acpi/acpi_memhotplug.c
===================================================================

drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 static int acpi_memory_powerdown_device( · line 314

Quoted 10 lines

drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 renamed from drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:33.386378909 +0900 +2 -1

@@ -306,24 +306,37 @@ static int acpi_memory_powerdown_device(
        return 0;
 }
 
-static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
+static int acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
 {
        int result;
        struct acpi_memory_info *info, *n;
 
+       list_for_each_entry_safe(info, n, &mem_device->res_list, list) {

Which lock protect this loop?

drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 static int acpi_memory_powerdown_device( · line 320

Quoted 6 lines

drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 renamed from drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:33.386378909 +0900 +6 -0
+               if (!info->enabled)
+                       return -EBUSY;
+
+               result = remove_memory(info->start_addr, info->length);
+               if (result)
+                       return result;

I suspect you need to implement rollback code instead of just return.

Quoted 6 lines

drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 renamed from drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:33.386378909 +0900 +6 -0
+
+               list_del(&info->list);
+               kfree(info);
+       }
+
+       return 0;

drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 static int acpi_memory_powerdown_device( · line 336

Quoted 10 lines

drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 renamed from drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:33.386378909 +0900 +5 -0
+}
+
+static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
+{
+       int result;
 
        /*
         * Ask the VM to offline this memory range.
         * Note: Assume that this function returns zero on success
         */

Write function comment instead of this silly comment.

Quoted 4 lines

drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 renamed from drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:33.386378909 +0900 +0 -4
-       list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
-               if (info->enabled) {
-                       result = remove_memory(info->start_addr, info->length);
-                       if (result)

drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 static int acpi_memory_powerdown_device( · line 342

Quoted 10 lines

drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 renamed from drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:33.386378909 +0900 +3 -4

-                               return result;
-               }
-               kfree(info);
-       }
+       result = acpi_memory_remove_memory(mem_device);
+       if (result)
+               return result;
 
        /* Power-off and eject the device */
        result = acpi_memory_powerdown_device(mem_device);

This patch move acpi_memory_powerdown_device() from ACPI_NOTIFY_EJECT_REQUEST to release callback, but don't explain why.

Quoted 7 lines

drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 renamed from drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:33.386378909 +0900 +1 -1
@@ -473,12 +486,23 @@ static int acpi_memory_device_add(struct
 static int acpi_memory_device_remove(struct acpi_device *device, int type)
 {
        struct acpi_memory_device *mem_device = NULL;
-
+       int result;
 

drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 static int acpi_memory_device_add(struct · line 500

Quoted 10 lines

drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 renamed from drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:33.386378909 +0900 +6 -0

        if (!device || !acpi_driver_data(device))
                return -EINVAL;
 
        mem_device = acpi_driver_data(device);
+
+       if (type == ACPI_BUS_REMOVAL_EJECT) {
+               /*
+                * offline and remove memory only when the memory device is
+                * ejected.
+                */

This comment explain nothing. A comment should describe _why_ should we do. e.g. Why REMOVAL_NORMAL and REMOVEL_EJECT should be ignored. Why we need remove memory here instead of ACPI_NOTIFY_EJECT_REQUEST.

Quoted 10 lines

drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 renamed from drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:33.386378909 +0900 +5 -1
+               result = acpi_memory_remove_memory(mem_device);
+               if (result)
+                       return result;
+       }
+
        kfree(mem_device);
 
        return 0;
 
--

Quoted 4 lines

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

Wen Congyang

2012-10-08 06:53 UTC #3 lore raw

Re: [PATCH 1/4] acpi,memory-hotplug : add memory offline code to acpi_memory_device_remove()

At 10/05/2012 04:53 AM, KOSAKI Motohiro Wrote:

Quoted 53 lines

Quoted 51 lines · Yasuaki Ishimatsu
From: Yasuaki Ishimatsu <[email protected]>
The memory device can be removed by 2 ways:
1. send eject request by SCI
2. echo 1 >/sys/bus/pci/devices/PNP0C80:XX/eject
In the 1st case, acpi_memory_disable_device() will be called. In the 2nd case, acpi_memory_device_remove() will be called. acpi_memory_device_remove() will also be called when we unbind the memory device from the driver acpi_memhotplug.
acpi_memory_disable_device() has already implemented a code which offlines memory and releases acpi_memory_info struct . But acpi_memory_device_remove() has not implemented it yet.
So the patch implements acpi_memory_remove_memory() for offlining memory and releasing acpi_memory_info struct. And it is used by both acpi_memory_device_remove() and acpi_memory_disable_device().
Additionally, if the type is ACPI_BUS_REMOVAL_EJECT in acpi_memory_device_remove() , it means that the user wants to eject the memory device. In this case, acpi_memory_device_remove() calls acpi_memory_remove_memory().
CC David Rientjes <[email protected]>
CC Jiang Liu <[email protected]>
CC Len Brown <[email protected]>
CC Christoph Lameter <[email protected]>
Cc Minchan Kim <[email protected]>
CC Andrew Morton <[email protected]>
CC KOSAKI Motohiro <[email protected]>
Signed-off-by Yasuaki Ishimatsu <[email protected]>
Signed-off-by Wen Congyang <[email protected]>
---
 drivers/acpi/acpi_memhotplug.c |   44 +++++++++++++++++++++++++++++++----------
 1 file changed, 34 insertions(+), 10 deletions(-)
Index: linux-3.6/drivers/acpi/acpi_memhotplug.c
===================================================================
drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 renamed from drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:33.386378909 +0900 +1 -1
@@ -306,24 +306,37 @@ static int acpi_memory_powerdown_device(
        return 0;
 }
 
-static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
+static int acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
 {
        int result;
        struct acpi_memory_info *info, *n;
 

Quoted 3 lines

drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 static int acpi_memory_powerdown_device( · line 314
Quoted 1 line
drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 renamed from drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:33.386378909 +0900 +1 -0
+       list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
Which lock protect this loop?

There is no any lock to protect it now...

Quoted 43 lines

drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 static int acpi_memory_powerdown_device( · line 320
Quoted 8 lines
drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 renamed from drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:33.386378909 +0900 +6 -0
+               if (!info->enabled)
+                       return -EBUSY;
+
+               result = remove_memory(info->start_addr, info->length);
+               if (result)
+                       return result;
I suspect you need to implement rollback code instead of just return.
Quoted 6 lines
drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 renamed from drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:33.386378909 +0900 +6 -0
+
+               list_del(&info->list);
+               kfree(info);
+       }
+
+       return 0;
drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 static int acpi_memory_powerdown_device( · line 336
Quoted 10 lines
drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 renamed from drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:33.386378909 +0900 +5 -0
+}
+
+static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
+{
+       int result;
 
        /*
         * Ask the VM to offline this memory range.
         * Note: Assume that this function returns zero on success
         */
Write function comment instead of this silly comment.
Quoted 12 lines
drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 renamed from drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:33.386378909 +0900 +3 -8
-       list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
-               if (info->enabled) {
-                       result = remove_memory(info->start_addr, info->length);
-                       if (result)
-                               return result;
-               }
-               kfree(info);
-       }
+       result = acpi_memory_remove_memory(mem_device);
+       if (result)
+               return result;
 

Quoted 5 lines

drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 static int acpi_memory_powerdown_device( · line 342
Quoted 2 lines
drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 renamed from drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:33.386378909 +0900 +0 -0
        /* Power-off and eject the device */
        result = acpi_memory_powerdown_device(mem_device);
This patch move acpi_memory_powerdown_device() from ACPI_NOTIFY_EJECT_REQUEST to release callback, but don't explain why.

Hmm, it doesn't move the code. It just reuse the code in acpi_memory_powerdown_device().

Quoted 15 lines

drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 renamed from drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:33.386378909 +0900 +1 -1

@@ -473,12 +486,23 @@ static int acpi_memory_device_add(struct
 static int acpi_memory_device_remove(struct acpi_device *device, int type)
 {
        struct acpi_memory_device *mem_device = NULL;
-
+       int result;
 
        if (!device || !acpi_driver_data(device))
                return -EINVAL;

Quoted 11 lines

drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 static int acpi_memory_device_add(struct · line 500
Quoted 7 lines
drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 renamed from drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:33.386378909 +0900 +6 -0
        mem_device = acpi_driver_data(device);
+
+       if (type == ACPI_BUS_REMOVAL_EJECT) {
+               /*
+                * offline and remove memory only when the memory device is
+                * ejected.
+                */
This comment explain nothing. A comment should describe _why_ should we do. e.g. Why REMOVAL_NORMAL and REMOVEL_EJECT should be ignored. Why we need remove memory here instead of ACPI_NOTIFY_EJECT_REQUEST.

Hmm, we have 2 ways to remove a memory:
1. SCI
2. echo 1 >/sys/bus/acpi/devices/PNP0C80:XX/eject

In the 2nd case, there is no ACPI_NOTIFY_EJECT_REQUEST. We should offline the memory and remove it from kernel in the release callback. We will poweroff the memory device in acpi_bus_hot_remove_device(), so we must offline and remove it if the type is ACPI_BUS_REMOVAL_EJECT.

I guess we should not poweroff the memory device when we fail to offline it. But device_release_driver() doesn't returns any error...

Thanks
Wen Congyang

Quoted 17 lines

Quoted 16 lines
drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900 renamed from drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:33.386378909 +0900 +5 -1
+               result = acpi_memory_remove_memory(mem_device);
+               if (result)
+                       return result;
+       }
+
        kfree(mem_device);
 
        return 0;
 
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

KOSAKI Motohiro

2012-10-12 19:10 UTC #4 lore raw

Re: [PATCH 1/4] acpi,memory-hotplug : add memory offline code to acpi_memory_device_remove()

Quoted 11 lines

Quoted 9 lines
drivers/acpi/acpi_memhotplug.c static int acpi_memory_powerdown_device( · line 314
Quoted 7 lines
drivers/acpi/acpi_memhotplug.c +2 -1
-static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
+static int acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
 {
        int result;
        struct acpi_memory_info *info, *n;
 
+       list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
Which lock protect this loop?
There is no any lock to protect it now...

When iterate an item removal list, you should use lock for protecting from memory corruption.

Quoted 23 lines

drivers/acpi/acpi_memhotplug.c static int acpi_memory_powerdown_device( · line 336

Quoted 8 lines

drivers/acpi/acpi_memhotplug.c +3 -0

+static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
+{
+       int result;
 
        /*
         * Ask the VM to offline this memory range.
         * Note: Assume that this function returns zero on success
         */

Write function comment instead of this silly comment.

Quoted 12 lines

drivers/acpi/acpi_memhotplug.c +3 -8

-       list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
-               if (info->enabled) {
-                       result = remove_memory(info->start_addr, info->length);
-                       if (result)
-                               return result;
-               }
-               kfree(info);
-       }
+       result = acpi_memory_remove_memory(mem_device);
+       if (result)
+               return result;

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
        /* Power-off and eject the device */
        result = acpi_memory_powerdown_device(mem_device);
This patch move acpi_memory_powerdown_device() from ACPI_NOTIFY_EJECT_REQUEST to release callback, but don't explain why.
Hmm, it doesn't move the code. It just reuse the code in acpi_memory_powerdown_device().

Even if reuse or not reuse, you changed the behavior. If any changes has no good rational, you cannot get an ack.

Quoted 26 lines

Quoted 10 lines
Quoted 10 lines
drivers/acpi/acpi_memhotplug.c +1 -1
@@ -473,12 +486,23 @@ static int acpi_memory_device_add(struct
 static int acpi_memory_device_remove(struct acpi_device *device, int type)
 {
        struct acpi_memory_device *mem_device = NULL;
-
+       int result;
 
        if (!device || !acpi_driver_data(device))
                return -EINVAL;
 
Quoted 11 lines
drivers/acpi/acpi_memhotplug.c static int acpi_memory_device_add(struct · line 500
Quoted 7 lines
drivers/acpi/acpi_memhotplug.c +6 -0
        mem_device = acpi_driver_data(device);
+
+       if (type == ACPI_BUS_REMOVAL_EJECT) {
+               /*
+                * offline and remove memory only when the memory device is
+                * ejected.
+                */
This comment explain nothing. A comment should describe _why_ should we do. e.g. Why REMOVAL_NORMAL and REMOVEL_EJECT should be ignored. Why we need remove memory here instead of ACPI_NOTIFY_EJECT_REQUEST.
Hmm, we have 2 ways to remove a memory:
1. SCI
2. echo 1 >/sys/bus/acpi/devices/PNP0C80:XX/eject

Quoted 7 lines

In the 2nd case, there is no ACPI_NOTIFY_EJECT_REQUEST. We should offline the memory and remove it from kernel in the release callback. We will poweroff the memory device in acpi_bus_hot_remove_device(), so we must offline and remove it if the type is ACPI_BUS_REMOVAL_EJECT.
I guess we should not poweroff the memory device when we fail to offline it. But device_release_driver() doesn't returns any error...

1) I think /sys/bus/acpi/devices/PNP0C80:XX/eject should emulate acpi eject. Can't
you make a pseudo acpi eject event and detach device by acpi regular path?

2) Your explanation didn't explain why we should ignore REMOVAL_NORMAL and REMOVEL_EJECT. As far as reviewers can't track your intention, we can't maintain
the code and can't ack them.

Wen Congyang

2012-10-17 06:42 UTC #5 lore raw

Re: [PATCH 1/4] acpi,memory-hotplug : add memory offline code to acpi_memory_device_remove()

At 10/13/2012 03:10 AM, KOSAKI Motohiro Wrote:

Quoted 44 lines

Quoted 11 lines

Quoted 9 lines
drivers/acpi/acpi_memhotplug.c static int acpi_memory_powerdown_device( · line 314
Quoted 7 lines
drivers/acpi/acpi_memhotplug.c +2 -1
-static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
+static int acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
 {
        int result;
        struct acpi_memory_info *info, *n;
 
+       list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
Which lock protect this loop?
There is no any lock to protect it now...

When iterate an item removal list, you should use lock for protecting from memory corruption.

Quoted 26 lines

drivers/acpi/acpi_memhotplug.c static int acpi_memory_powerdown_device( · line 336

Quoted 8 lines

drivers/acpi/acpi_memhotplug.c +3 -0

+static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
+{
+       int result;
 
        /*
         * Ask the VM to offline this memory range.
         * Note: Assume that this function returns zero on success
         */

Write function comment instead of this silly comment.

Quoted 14 lines

drivers/acpi/acpi_memhotplug.c +3 -8

-       list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
-               if (info->enabled) {
-                       result = remove_memory(info->start_addr, info->length);
-                       if (result)
-                               return result;
-               }
-               kfree(info);
-       }
+       result = acpi_memory_remove_memory(mem_device);
+       if (result)
+               return result;
 
        /* Power-off and eject the device */
        result = acpi_memory_powerdown_device(mem_device);

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
This patch move acpi_memory_powerdown_device() from ACPI_NOTIFY_EJECT_REQUEST to release callback, but don't explain why.
Hmm, it doesn't move the code. It just reuse the code in acpi_memory_powerdown_device().
Even if reuse or not reuse, you changed the behavior. If any changes has no good rational, you cannot get an ack.

I don't understand this? IIRC, the behavior isn't changed.

Thanks
Wen Congyang

Quoted 38 lines

Quoted 30 lines
Quoted 14 lines
Quoted 14 lines
drivers/acpi/acpi_memhotplug.c +1 -1
@@ -473,12 +486,23 @@ static int acpi_memory_device_add(struct
 static int acpi_memory_device_remove(struct acpi_device *device, int type)
 {
        struct acpi_memory_device *mem_device = NULL;
-
+       int result;
 
        if (!device || !acpi_driver_data(device))
                return -EINVAL;
 
Quoted 11 lines
drivers/acpi/acpi_memhotplug.c static int acpi_memory_device_add(struct · line 500
Quoted 7 lines
drivers/acpi/acpi_memhotplug.c +6 -0
        mem_device = acpi_driver_data(device);
+
+       if (type == ACPI_BUS_REMOVAL_EJECT) {
+               /*
+                * offline and remove memory only when the memory device is
+                * ejected.
+                */
This comment explain nothing. A comment should describe _why_ should we do. e.g. Why REMOVAL_NORMAL and REMOVEL_EJECT should be ignored. Why we need remove memory here instead of ACPI_NOTIFY_EJECT_REQUEST.
Hmm, we have 2 ways to remove a memory:
1. SCI
2. echo 1 >/sys/bus/acpi/devices/PNP0C80:XX/eject
Quoted 7 lines
In the 2nd case, there is no ACPI_NOTIFY_EJECT_REQUEST. We should offline the memory and remove it from kernel in the release callback. We will poweroff the memory device in acpi_bus_hot_remove_device(), so we must offline and remove it if the type is ACPI_BUS_REMOVAL_EJECT.
I guess we should not poweroff the memory device when we fail to offline it. But device_release_driver() doesn't returns any error...

Quoted 9 lines

1) I think /sys/bus/acpi/devices/PNP0C80:XX/eject should emulate acpi eject. Can't
you make a pseudo acpi eject event and detach device by acpi regular path?
2) Your explanation didn't explain why we should ignore REMOVAL_NORMAL and REMOVEL_EJECT. As far as reviewers can't track your intention, we can't maintain
the code and can't ack them.

KOSAKI Motohiro

2012-10-17 08:59 UTC #6 lore raw

Re: [PATCH 1/4] acpi,memory-hotplug : add memory offline code to acpi_memory_device_remove()

Quoted 48 lines · Wen Congyang

At 10/13/2012 03:10 AM, KOSAKI Motohiro Wrote:
Quoted 47 lines
Quoted 11 lines
Quoted 9 lines
drivers/acpi/acpi_memhotplug.c static int acpi_memory_powerdown_device( · line 314
Quoted 7 lines
drivers/acpi/acpi_memhotplug.c +2 -1
-static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
+static int acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
 {
        int result;
        struct acpi_memory_info *info, *n;
 
+       list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
Which lock protect this loop?
There is no any lock to protect it now...
When iterate an item removal list, you should use lock for protecting from memory corruption.
Quoted 29 lines
Quoted 28 lines
drivers/acpi/acpi_memhotplug.c static int acpi_memory_powerdown_device( · line 336
Quoted 8 lines
drivers/acpi/acpi_memhotplug.c +3 -0
+static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
+{
+       int result;
 
        /*
         * Ask the VM to offline this memory range.
         * Note: Assume that this function returns zero on success
         */
Write function comment instead of this silly comment.
Quoted 4 lines
drivers/acpi/acpi_memhotplug.c +0 -4
-       list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
-               if (info->enabled) {
-                       result = remove_memory(info->start_addr, info->length);
-                       if (result)
drivers/acpi/acpi_memhotplug.c static int acpi_memory_powerdown_device( · line 342
Quoted 10 lines
drivers/acpi/acpi_memhotplug.c +3 -4
-                               return result;
-               }
-               kfree(info);
-       }
+       result = acpi_memory_remove_memory(mem_device);
+       if (result)
+               return result;
 
        /* Power-off and eject the device */
        result = acpi_memory_powerdown_device(mem_device);
This patch move acpi_memory_powerdown_device() from ACPI_NOTIFY_EJECT_REQUEST to release callback, but don't explain why.

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
Hmm, it doesn't move the code. It just reuse the code in acpi_memory_powerdown_device().
Even if reuse or not reuse, you changed the behavior. If any changes has no good rational, you cannot get an ack.
I don't understand this? IIRC, the behavior isn't changed.

Heh, please explain why do you think so.

Wen Congyang

2012-10-17 09:03 UTC #7 lore raw

Re: [PATCH 1/4] acpi,memory-hotplug : add memory offline code to acpi_memory_device_remove()

At 10/17/2012 04:59 PM, KOSAKI Motohiro Wrote:

Quoted 49 lines · Wen Congyang

Quoted 48 lines
At 10/13/2012 03:10 AM, KOSAKI Motohiro Wrote:
Quoted 47 lines
Quoted 11 lines
Quoted 9 lines
drivers/acpi/acpi_memhotplug.c static int acpi_memory_powerdown_device( · line 314
Quoted 7 lines
drivers/acpi/acpi_memhotplug.c +2 -1
-static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
+static int acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
 {
        int result;
        struct acpi_memory_info *info, *n;
 
+       list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
Which lock protect this loop?
There is no any lock to protect it now...
When iterate an item removal list, you should use lock for protecting from memory corruption.
Quoted 29 lines
Quoted 28 lines
drivers/acpi/acpi_memhotplug.c static int acpi_memory_powerdown_device( · line 336
Quoted 8 lines
drivers/acpi/acpi_memhotplug.c +3 -0
+static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
+{
+       int result;
 
        /*
         * Ask the VM to offline this memory range.
         * Note: Assume that this function returns zero on success
         */
Write function comment instead of this silly comment.
Quoted 4 lines
drivers/acpi/acpi_memhotplug.c +0 -4
-       list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
-               if (info->enabled) {
-                       result = remove_memory(info->start_addr, info->length);
-                       if (result)
drivers/acpi/acpi_memhotplug.c static int acpi_memory_powerdown_device( · line 342
Quoted 10 lines
drivers/acpi/acpi_memhotplug.c +3 -4
-                               return result;
-               }
-               kfree(info);
-       }
+       result = acpi_memory_remove_memory(mem_device);
+       if (result)
+               return result;
 
        /* Power-off and eject the device */
        result = acpi_memory_powerdown_device(mem_device);
This patch move acpi_memory_powerdown_device() from ACPI_NOTIFY_EJECT_REQUEST to release callback, but don't explain why.

Quoted 9 lines

Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Hmm, it doesn't move the code. It just reuse the code in acpi_memory_powerdown_device().
Even if reuse or not reuse, you changed the behavior. If any changes has no good rational, you cannot get an ack.
I don't understand this? IIRC, the behavior isn't changed.
Heh, please explain why do you think so.

We just introduce a function, and move codes from acpi_memory_disable_device() to the new function. We call the new function in acpi_memory_disable_device(), so the function acpi_memory_disable_device()'s behavior isn't changed.

Maybe I don't understand what do you want to say.

Thanks
Wen Congyang

KOSAKI Motohiro

2012-10-17 09:18 UTC #8 lore raw

Re: [PATCH 1/4] acpi,memory-hotplug : add memory offline code to acpi_memory_device_remove()

Quoted 7 lines

Quoted 7 lines
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Hmm, it doesn't move the code. It just reuse the code in acpi_memory_powerdown_device().
Even if reuse or not reuse, you changed the behavior. If any changes has no good rational, you cannot get an ack.
I don't understand this? IIRC, the behavior isn't changed.

Quoted 7 lines

Quoted 1 line
Heh, please explain why do you think so.
We just introduce a function, and move codes from acpi_memory_disable_device() to the new function. We call the new function in acpi_memory_disable_device(), so the function acpi_memory_disable_device()'s behavior isn't changed.
Maybe I don't understand what do you want to say.

Ok, now you agreed you moved the code, yes? So then, you should explain why your code moving makes zero impact other acpi_memory_disable_device() caller.

Wen Congyang

2012-10-17 10:47 UTC #9 lore raw

Re: [PATCH 1/4] acpi,memory-hotplug : add memory offline code to acpi_memory_device_remove()

At 10/17/2012 05:18 PM, KOSAKI Motohiro Wrote:

Quoted 9 lines

Quoted 9 lines
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Hmm, it doesn't move the code. It just reuse the code in acpi_memory_powerdown_device().
Even if reuse or not reuse, you changed the behavior. If any changes has no good rational, you cannot get an ack.
I don't understand this? IIRC, the behavior isn't changed.
Heh, please explain why do you think so.

Quoted 8 lines

Quoted 5 lines
We just introduce a function, and move codes from acpi_memory_disable_device() to the new function. We call the new function in acpi_memory_disable_device(), so the function acpi_memory_disable_device()'s behavior isn't changed.
Maybe I don't understand what do you want to say.
Ok, now you agreed you moved the code, yes? So then, you should explain why your code moving makes zero impact other acpi_memory_disable_device() caller.

We just move the code, and don't change the acpi_memory_disable_device()'s behavior.

I look it the change again, and found some diffs:
1. we treat !info->enabled as error, while it isn't a error without this patch
2. we remove memory info from the list, it is a bug fix because we free the memory that stores memory info.(I have sent a patch to fix this bug, and it is in akpm's tree now)

I guess you mean 1 will change the behavior. In the last version, I don't do it. Ishimatsu changes this and I don't notify this.

To Ishimatsu:

Why do you change this?

Thanks
Wen Congyang

Quoted 6 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Yasuaki Ishimatsu

author

2012-10-18 01:25 UTC #10 lore raw

Re: [PATCH 1/4] acpi,memory-hotplug : add memory offline code to acpi_memory_device_remove()

Hi Wen,

Quoted 26 lines · Wen Congyang

At 10/17/2012 05:18 PM, KOSAKI Motohiro Wrote:
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Hmm, it doesn't move the code. It just reuse the code in acpi_memory_powerdown_device().
Even if reuse or not reuse, you changed the behavior. If any changes has no good rational, you cannot get an ack.
I don't understand this? IIRC, the behavior isn't changed.
Heh, please explain why do you think so.
Quoted 8 lines
Quoted 5 lines
We just introduce a function, and move codes from acpi_memory_disable_device() to the new function. We call the new function in acpi_memory_disable_device(), so the function acpi_memory_disable_device()'s behavior isn't changed.
Maybe I don't understand what do you want to say.
Ok, now you agreed you moved the code, yes? So then, you should explain why your code moving makes zero impact other acpi_memory_disable_device() caller.
We just move the code, and don't change the acpi_memory_disable_device()'s behavior.
I look it the change again, and found some diffs:
1. we treat !info->enabled as error, while it isn't a error without this patch
2. we remove memory info from the list, it is a bug fix because we free the memory that stores memory info.(I have sent a patch to fix this bug, and it is in akpm's tree now)

Quoted 6 lines

I guess you mean 1 will change the behavior. In the last version, I don't do it. Ishimatsu changes this and I don't notify this.
To Ishimatsu:
Why do you change this?

Oops. If so, it's my mistake.
Could you update it in next version?

Thanks,
Yasuaki Ishimatsu

Quoted 11 lines

Thanks
Wen Congyang
Quoted 6 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Wen Congyang

2012-10-19 07:29 UTC #11 lore raw

Re: [PATCH 1/4] acpi,memory-hotplug : add memory offline code to acpi_memory_device_remove()

At 10/18/2012 09:25 AM, Yasuaki Ishimatsu Wrote:

Quoted 42 lines

Hi Wen,
Quoted 39 lines · Wen Congyang
At 10/17/2012 05:18 PM, KOSAKI Motohiro Wrote:
Quoted 16 lines
Quoted 16 lines
Quoted 9 lines
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Hmm, it doesn't move the code. It just reuse the code in acpi_memory_powerdown_device().
Even if reuse or not reuse, you changed the behavior. If any changes has no good rational, you cannot get an ack.
I don't understand this? IIRC, the behavior isn't changed.
Heh, please explain why do you think so.
We just introduce a function, and move codes from acpi_memory_disable_device() to the new
function. We call the new function in acpi_memory_disable_device(), so the function
acpi_memory_disable_device()'s behavior isn't changed.
Quoted 6 lines
Quoted 1 line
Maybe I don't understand what do you want to say.
Ok, now you agreed you moved the code, yes? So then, you should explain why
your code moving makes zero impact other acpi_memory_disable_device() caller.
We just move the code, and don't change the
acpi_memory_disable_device()'s behavior.
I look it the change again, and found some diffs:
1. we treat !info->enabled as error, while it isn't a error without this patch
2. we remove memory info from the list, it is a bug fix because we free the memory
that stores memory info.(I have sent a patch to fix this bug, and
it is in akpm's tree now)
I guess you mean 1 will change the behavior. In the last version, I don't do it.
Ishimatsu changes this and I don't notify this.

Quoted 6 lines

Quoted 3 lines
To Ishimatsu:
Why do you change this?
Oops. If so, it's my mistake.
Could you update it in next version?

Thanks
Wen Congyang

Quoted 16 lines

Thanks,
Yasuaki Ishimatsu

Quoted 12 lines

Thanks
Wen Congyang

Quoted 7 lines

Signature · 5 lines

To unsubscribe from this list: send the line "unsubscribe
linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quoted 13 lines

Quoted 5 lines

Signature · 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Signature · 4 lines

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Wen Congyang

2012-10-17 09:12 UTC #12 lore raw

Re: [PATCH 1/4] acpi,memory-hotplug : add memory offline code to acpi_memory_device_remove()

In reply to KOSAKI Motohiro

At 10/13/2012 03:10 AM, KOSAKI Motohiro Wrote:

Quoted 86 lines

Quoted 11 lines
Quoted 9 lines
drivers/acpi/acpi_memhotplug.c static int acpi_memory_powerdown_device( · line 314
Quoted 7 lines
drivers/acpi/acpi_memhotplug.c +2 -1
-static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
+static int acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
 {
        int result;
        struct acpi_memory_info *info, *n;
 
+       list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
Which lock protect this loop?
There is no any lock to protect it now...
When iterate an item removal list, you should use lock for protecting from memory corruption.
Quoted 23 lines
Quoted 23 lines
drivers/acpi/acpi_memhotplug.c static int acpi_memory_powerdown_device( · line 336
Quoted 8 lines
drivers/acpi/acpi_memhotplug.c +3 -0
+static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
+{
+       int result;
 
        /*
         * Ask the VM to offline this memory range.
         * Note: Assume that this function returns zero on success
         */
Write function comment instead of this silly comment.
Quoted 12 lines
drivers/acpi/acpi_memhotplug.c +3 -8
-       list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
-               if (info->enabled) {
-                       result = remove_memory(info->start_addr, info->length);
-                       if (result)
-                               return result;
-               }
-               kfree(info);
-       }
+       result = acpi_memory_remove_memory(mem_device);
+       if (result)
+               return result;
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
        /* Power-off and eject the device */
        result = acpi_memory_powerdown_device(mem_device);
This patch move acpi_memory_powerdown_device() from ACPI_NOTIFY_EJECT_REQUEST to release callback, but don't explain why.
Hmm, it doesn't move the code. It just reuse the code in acpi_memory_powerdown_device().
Even if reuse or not reuse, you changed the behavior. If any changes has no good rational, you cannot get an ack.
Quoted 31 lines
Quoted 10 lines
Quoted 10 lines
drivers/acpi/acpi_memhotplug.c +1 -1
@@ -473,12 +486,23 @@ static int acpi_memory_device_add(struct
 static int acpi_memory_device_remove(struct acpi_device *device, int type)
 {
        struct acpi_memory_device *mem_device = NULL;
-
+       int result;
 
        if (!device || !acpi_driver_data(device))
                return -EINVAL;
 
Quoted 11 lines
drivers/acpi/acpi_memhotplug.c static int acpi_memory_device_add(struct · line 500
Quoted 7 lines
drivers/acpi/acpi_memhotplug.c +6 -0
        mem_device = acpi_driver_data(device);
+
+       if (type == ACPI_BUS_REMOVAL_EJECT) {
+               /*
+                * offline and remove memory only when the memory device is
+                * ejected.
+                */
This comment explain nothing. A comment should describe _why_ should we do. e.g. Why REMOVAL_NORMAL and REMOVEL_EJECT should be ignored. Why we need remove memory here instead of ACPI_NOTIFY_EJECT_REQUEST.
Hmm, we have 2 ways to remove a memory:
1. SCI
2. echo 1 >/sys/bus/acpi/devices/PNP0C80:XX/eject
In the 2nd case, there is no ACPI_NOTIFY_EJECT_REQUEST. We should offline the memory and remove it from kernel in the release callback. We will poweroff the memory device in acpi_bus_hot_remove_device(), so we must offline and remove it if the type is ACPI_BUS_REMOVAL_EJECT.

Quoted 6 lines

Quoted 2 lines
I guess we should not poweroff the memory device when we fail to offline it. But device_release_driver() doesn't returns any error...
1) I think /sys/bus/acpi/devices/PNP0C80:XX/eject should emulate acpi eject. Can't
you make a pseudo acpi eject event and detach device by acpi regular path?

It is another issue. And we only can implement it here with current acpi implemention. Some other acpi devices(for example: cpu) do it like this.

Quoted 6 lines

2) Your explanation didn't explain why we should ignore REMOVAL_NORMAL and REMOVEL_EJECT. As far as reviewers can't track your intention, we can't maintain
the code and can't ack them.

REMOVAL_NORMAL means the user want to unbind the memory device from this driver. It is no need to eject the device, and we can still use this device after unbinding. So it can be ignored.

REMOVAL_EJECT means the user want to eject and remove the device, and we should not use the device. So we should offline and remove the memory here.

Thanks
Wen Congyang

KOSAKI Motohiro

2012-10-18 19:44 UTC #13 lore raw

Re: [PATCH 1/4] acpi,memory-hotplug : add memory offline code to acpi_memory_device_remove()

Quoted 22 lines

Quoted 22 lines
Quoted 21 lines
Quoted 9 lines
drivers/acpi/acpi_memhotplug.c static int acpi_memory_device_add(struct · line 500
Quoted 5 lines
drivers/acpi/acpi_memhotplug.c +5 -0
+       if (type == ACPI_BUS_REMOVAL_EJECT) {
+               /*
+                * offline and remove memory only when the memory device is
+                * ejected.
+                */
This comment explain nothing. A comment should describe _why_ should we do. e.g. Why REMOVAL_NORMAL and REMOVEL_EJECT should be ignored. Why we need remove memory here instead of ACPI_NOTIFY_EJECT_REQUEST.
Hmm, we have 2 ways to remove a memory:
1. SCI
2. echo 1 >/sys/bus/acpi/devices/PNP0C80:XX/eject
In the 2nd case, there is no ACPI_NOTIFY_EJECT_REQUEST. We should offline the memory and remove it from kernel in the release callback. We will poweroff the memory device in acpi_bus_hot_remove_device(), so we must offline and remove it if the type is ACPI_BUS_REMOVAL_EJECT.
I guess we should not poweroff the memory device when we fail to offline it. But device_release_driver() doesn't returns any error...

Quoted 6 lines

Quoted 3 lines
1) I think /sys/bus/acpi/devices/PNP0C80:XX/eject should emulate acpi eject. Can't
you make a pseudo acpi eject event and detach device by acpi regular path?
It is another issue. And we only can implement it here with current acpi implemention. Some other acpi devices(for example: cpu) do it like this.

Hint: only cpu take like this.

Quoted 6 lines

Quoted 5 lines
2) Your explanation didn't explain why we should ignore REMOVAL_NORMAL and REMOVEL_EJECT. As far as reviewers can't track your intention, we can't maintain
the code and can't ack them.

Quoted 6 lines

REMOVAL_NORMAL means the user want to unbind the memory device from this driver. It is no need to eject the device, and we can still use this device after unbinding. So it can be ignored.
REMOVAL_EJECT means the user want to eject and remove the device, and we should not use the device. So we should offline and remove the memory here.

This is not exactly correct, IMHO. Usually, we must not touch unbinded device because
they are out of OS control. If I understand is correct, the main reason is to distinguish a
rollback of driver initialization failure and true ejection.

REMOVAL_NORMAL is usually used for rollback and REMOVAL_EJECT is used for removal device eject. Typical device don't need to distinguish them because we should
deallocate every resource even when driver initialization failure.

However, cpu and memory are exceptions. They are recognized from kernel before driver initialization. Then even if machine have crappy acpi table and make failure acpi
initialization, disabling memory make no sense.

And, when you make _exceptional_ rule, you should comment verbosely in the code the detail. likes 1) why we need. 2) which device/machine/environment suffer such exception. 2) what affect other subsys.

Even though cpu hotplug has crappy poor comment and document, please don't follow
them.

Wen Congyang

2012-10-19 09:03 UTC #14 lore raw

Re: [PATCH 1/4] acpi,memory-hotplug : add memory offline code to acpi_memory_device_remove()

At 10/19/2012 03:44 AM, KOSAKI Motohiro Wrote:

Quoted 56 lines

Quoted 22 lines
Quoted 22 lines
Quoted 21 lines
Quoted 9 lines
drivers/acpi/acpi_memhotplug.c static int acpi_memory_device_add(struct · line 500
Quoted 5 lines
drivers/acpi/acpi_memhotplug.c +5 -0
+       if (type == ACPI_BUS_REMOVAL_EJECT) {
+               /*
+                * offline and remove memory only when the memory device is
+                * ejected.
+                */
This comment explain nothing. A comment should describe _why_ should we do. e.g. Why REMOVAL_NORMAL and REMOVEL_EJECT should be ignored. Why we need remove memory here instead of ACPI_NOTIFY_EJECT_REQUEST.
Hmm, we have 2 ways to remove a memory:
1. SCI
2. echo 1 >/sys/bus/acpi/devices/PNP0C80:XX/eject
In the 2nd case, there is no ACPI_NOTIFY_EJECT_REQUEST. We should offline the memory and remove it from kernel in the release callback. We will poweroff the memory device in acpi_bus_hot_remove_device(), so we must offline and remove it if the type is ACPI_BUS_REMOVAL_EJECT.
I guess we should not poweroff the memory device when we fail to offline it. But device_release_driver() doesn't returns any error...
Quoted 6 lines
Quoted 3 lines
1) I think /sys/bus/acpi/devices/PNP0C80:XX/eject should emulate acpi eject. Can't
you make a pseudo acpi eject event and detach device by acpi regular path?
It is another issue. And we only can implement it here with current acpi implemention. Some other acpi devices(for example: cpu) do it like this.
Hint: only cpu take like this.
Quoted 6 lines
Quoted 5 lines
2) Your explanation didn't explain why we should ignore REMOVAL_NORMAL and REMOVEL_EJECT. As far as reviewers can't track your intention, we can't maintain
the code and can't ack them.
Quoted 6 lines
REMOVAL_NORMAL means the user want to unbind the memory device from this driver. It is no need to eject the device, and we can still use this device after unbinding. So it can be ignored.
REMOVAL_EJECT means the user want to eject and remove the device, and we should not use the device. So we should offline and remove the memory here.
This is not exactly correct, IMHO. Usually, we must not touch unbinded device because
they are out of OS control. If I understand is correct, the main reason is to distinguish a
rollback of driver initialization failure and true ejection.
REMOVAL_NORMAL is usually used for rollback and REMOVAL_EJECT is used for removal device eject. Typical device don't need to distinguish them because we should
deallocate every resource even when driver initialization failure.

Quoted 4 lines

However, cpu and memory are exceptions. They are recognized from kernel before driver initialization. Then even if machine have crappy acpi table and make failure acpi
initialization, disabling memory make no sense.

Hmm, IIRC, if the memory is recognized from kerenl before driver initialization, the memory device is not managed by the driver acpi_memhotplug.

I think we should also deal with REMOVAL_NORMAL here now. Otherwise it will cause some critical problem: we unbind the device from the driver but we still use it. If we eject it, we have no chance to offline and remove it. It is very dangerous.

Thanks
Wen Congyang

Quoted 10 lines

And, when you make _exceptional_ rule, you should comment verbosely in the code the detail. likes 1) why we need. 2) which device/machine/environment suffer such exception. 2) what affect other subsys.
Even though cpu hotplug has crappy poor comment and document, please don't follow
them.

KOSAKI Motohiro

2012-10-19 18:20 UTC #15 lore raw

Re: [PATCH 1/4] acpi,memory-hotplug : add memory offline code to acpi_memory_device_remove()

Quoted 2 lines

Hmm, IIRC, if the memory is recognized from kerenl before driver initialization, the memory device is not managed by the driver acpi_memhotplug.

Yup.

Quoted 3 lines

I think we should also deal with REMOVAL_NORMAL here now. Otherwise it will cause some critical problem: we unbind the device from the driver but we still use it. If we eject it, we have no chance to offline and remove it. It is very dangerous.

??
If resource was not allocated a driver, a driver doesn't need to deallocate it when
error path. I haven't caught your point.

Wen Congyang

2012-10-20 04:56 UTC #16 lore raw

Re: [PATCH 1/4] acpi,memory-hotplug : add memory offline code to acpi_memory_device_remove()

At 10/20/2012 02:19 AM, KOSAKI Motohiro Wrote:

Quoted 6 lines

Quoted 2 lines
Hmm, IIRC, if the memory is recognized from kerenl before driver initialization, the memory device is not managed by the driver acpi_memhotplug.
Yup.

Quoted 9 lines

Quoted 3 lines
I think we should also deal with REMOVAL_NORMAL here now. Otherwise it will cause some critical problem: we unbind the device from the driver but we still use it. If we eject it, we have no chance to offline and remove it. It is very dangerous.
??
If resource was not allocated a driver, a driver doesn't need to deallocate it when
error path. I haven't caught your point.

REMOVAL_NORMAL can be in 2 cases:
1. error path. If init call fails, we don't call it. We call this function only when something fails after init.

2. unbind the device from the driver.
If we don't offline and remove memory when unbinding the device from the driver, the device may be out of control. When we eject this driver, we don't offline and remove it, but we will eject and poweroff the device. It is very dangerous because the kernel uses the memory but we poweroff it.

acpi_bus_hot_remove_device()
acpi_bus_trim() // this function successes because the device has no driver _PS3 // poweroff
_EJ0 // eject

Thanks
Wen Congyang

KOSAKI Motohiro

2012-10-22 15:11 UTC #17 lore raw

Re: [PATCH 1/4] acpi,memory-hotplug : add memory offline code to acpi_memory_device_remove()

Quoted 6 lines

Quoted 5 lines
??
If resource was not allocated a driver, a driver doesn't need to deallocate it when
error path. I haven't caught your point.

Quoted 6 lines

REMOVAL_NORMAL can be in 2 cases:
1. error path. If init call fails, we don't call it. We call this function only when something fails after init.
2. unbind the device from the driver.
If we don't offline and remove memory when unbinding the device from the driver, the device may be out of control. When we eject this driver, we don't offline and

Memory never be out of control by driver unloading. It is controled from kernel core. It is an exception from regular linux driver model.

Quoted 7 lines

remove it, but we will eject and poweroff the device. It is very dangerous because the kernel uses the memory but we poweroff it.
acpi_bus_hot_remove_device()
acpi_bus_trim() // this function successes because the device has no driver _PS3 // poweroff
_EJ0 // eject

KOSAKI Motohiro

2012-10-22 15:34 UTC #18 lore raw

Re: [PATCH 1/4] acpi,memory-hotplug : add memory offline code to acpi_memory_device_remove()

Quoted 13 lines · KOSAKI Motohiro

Quoted 6 lines
Quoted 5 lines
??
If resource was not allocated a driver, a driver doesn't need to deallocate it when
error path. I haven't caught your point.
Quoted 6 lines
REMOVAL_NORMAL can be in 2 cases:
1. error path. If init call fails, we don't call it. We call this function only when something fails after init.
2. unbind the device from the driver.
If we don't offline and remove memory when unbinding the device from the driver, the device may be out of control. When we eject this driver, we don't offline and

Quoted 2 lines

Memory never be out of control by driver unloading. It is controled from kernel core. It is an exception from regular linux driver model.

Ah, got it.
acpi_bus_hot_remove_device() evaluate PS3 before EJ0. Then your first patch may cause memory lost.

Yasuaki Ishimatsu

author

2012-10-03 10:02 UTC #19 lore raw

PATCH 2/4 acpi,memory-hotplug : rename remove_memory() to offline_memory()

In reply to Yasuaki Ishimatsu

From: Yasuaki Ishimatsu <[email protected]>

add_memory() hot adds a physical memory. But remove_memory does not hot remove a phsical memory. It only offlines memory. The name confuse us.

So the patch renames remove_memory() to offline_memory(). We will use rename_memory() for hot removing memory.

CC David Rientjes <[email protected]>

CC Jiang Liu <[email protected]>

CC Len Brown <[email protected]>

CC Christoph Lameter <[email protected]>

Cc Minchan Kim <[email protected]>

CC Andrew Morton <[email protected]>

CC KOSAKI Motohiro <[email protected]>

Signed-off-by Yasuaki Ishimatsu <[email protected]>

Signed-off-by Wen Congyang <[email protected]>

---
 drivers/acpi/acpi_memhotplug.c |    2 +-
 include/linux/memory_hotplug.h |    2 +-
 mm/memory_hotplug.c            |    6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

drivers/acpi/acpi_memhotplug.c +1 -1

Index: linux-3.6/drivers/acpi/acpi_memhotplug.c
===================================================================
@@ -316,7 +316,7 @@ acpi_memory_remove_memory(struct acpi_me
 		if (!info->enabled)
 			return -EBUSY;
 
-		result = remove_memory(info->start_addr, info->length);
+		result = offline_memory(info->start_addr, info->length);
 		if (result)
 			return result;

include/linux/memory_hotplug.h +1 -1

Index: linux-3.6/include/linux/memory_hotplug.h
===================================================================
@@ -236,7 +236,7 @@ extern int add_memory(int nid, u64 start
 extern int arch_add_memory(int nid, u64 start, u64 size);
 extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages);
 extern int offline_memory_block(struct memory_block *mem);
-extern int remove_memory(u64 start, u64 size);
+extern int offline_memory(u64 start, u64 size);
 extern int sparse_add_one_section(struct zone *zone, unsigned long start_pfn,
 								int nr_pages);
 extern void sparse_remove_one_section(struct zone *zone, struct mem_section *ms);

mm/memory_hotplug.c +3 -3

Index: linux-3.6/mm/memory_hotplug.c
===================================================================
@@ -1003,7 +1003,7 @@ int offline_pages(unsigned long start_pf
 	return __offline_pages(start_pfn, start_pfn + nr_pages, 120 * HZ);
 }
 
-int remove_memory(u64 start, u64 size)
+int offline_memory(u64 start, u64 size)
 {
 	struct memory_block *mem = NULL;
 	struct mem_section *section;

@@ -1047,9 +1047,9 @@ int offline_pages(unsigned long start_pf
 {
 	return -EINVAL;
 }
-int remove_memory(u64 start, u64 size)
+int offline_memory(u64 start, u64 size)
 {
 	return -EINVAL;
 }
 #endif /* CONFIG_MEMORY_HOTREMOVE */
-EXPORT_SYMBOL_GPL(remove_memory);
+EXPORT_SYMBOL_GPL(offline_memory);

KOSAKI Motohiro

2012-10-04 21:31 UTC #20 lore raw

Re: [PATCH 2/4] acpi,memory-hotplug : rename remove_memory() to offline_memory()

Quoted 18 lines · Yasuaki Ishimatsu

From: Yasuaki Ishimatsu <[email protected]>
add_memory() hot adds a physical memory. But remove_memory does not hot remove a phsical memory. It only offlines memory. The name confuse us.
So the patch renames remove_memory() to offline_memory(). We will use rename_memory() for hot removing memory.
CC David Rientjes <[email protected]>
CC Jiang Liu <[email protected]>
CC Len Brown <[email protected]>
CC Christoph Lameter <[email protected]>
Cc Minchan Kim <[email protected]>
CC Andrew Morton <[email protected]>
CC KOSAKI Motohiro <[email protected]>
Signed-off-by Yasuaki Ishimatsu <[email protected]>
Signed-off-by Wen Congyang <[email protected]>

Quoted 5 lines

---
 drivers/acpi/acpi_memhotplug.c |    2 +-
 include/linux/memory_hotplug.h |    2 +-
 mm/memory_hotplug.c            |    6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

Probably, the better way is to just remove remove_memory() and use offline_pages().

btw, current remove_memory() pfn calculation is just buggy.

Quoted 6 lines

int remove_memory(u64 start, u64 size)

{
	unsigned long start_pfn, end_pfn;

	start_pfn = PFN_DOWN(start);
	end_pfn = start_pfn + PFN_DOWN(size);

It should be:

	start_pfn = PFN_DOWN(start);
	end_pfn = PFN_UP(start + size)

	start_pfn = PFN_UP(start);
	end_pfn = PFN_DOWN(start + size)

Wen Congyang

2012-10-08 06:40 UTC #21 lore raw

Re: [PATCH 2/4] acpi,memory-hotplug : rename remove_memory() to offline_memory()

At 10/05/2012 05:31 AM, KOSAKI Motohiro Wrote:

Quoted 26 lines

Quoted 18 lines · Yasuaki Ishimatsu
From: Yasuaki Ishimatsu <[email protected]>
add_memory() hot adds a physical memory. But remove_memory does not hot remove a phsical memory. It only offlines memory. The name confuse us.
So the patch renames remove_memory() to offline_memory(). We will use rename_memory() for hot removing memory.
CC David Rientjes <[email protected]>
CC Jiang Liu <[email protected]>
CC Len Brown <[email protected]>
CC Christoph Lameter <[email protected]>
Cc Minchan Kim <[email protected]>
CC Andrew Morton <[email protected]>
CC KOSAKI Motohiro <[email protected]>
Signed-off-by Yasuaki Ishimatsu <[email protected]>
Signed-off-by Wen Congyang <[email protected]>
Quoted 5 lines
---
 drivers/acpi/acpi_memhotplug.c |    2 +-
 include/linux/memory_hotplug.h |    2 +-
 mm/memory_hotplug.c            |    6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

Quoted 2 lines

Probably, the better way is to just remove remove_memory() and use offline_pages().

we don't notify the userspace that the memory is offlined in offline_pages(). We reimplement offline_memory(), but ishimatsu doesn't include that patch to this series.

Thanks
Wen Congyang

Quoted 13 lines

btw, current remove_memory() pfn calculation is just buggy.
Quoted 6 lines
int remove_memory(u64 start, u64 size)
{
	unsigned long start_pfn, end_pfn;

	start_pfn = PFN_DOWN(start);
	end_pfn = start_pfn + PFN_DOWN(size);
It should be:

Quoted 8 lines

	start_pfn = PFN_DOWN(start);
	end_pfn = PFN_UP(start + size)

	start_pfn = PFN_UP(start);
	end_pfn = PFN_DOWN(start + size)

KOSAKI Motohiro

2012-10-12 18:57 UTC #22 lore raw

Re: [PATCH 2/4] acpi,memory-hotplug : rename remove_memory() to offline_memory()

Quoted 27 lines · Wen Congyang

At 10/05/2012 05:31 AM, KOSAKI Motohiro Wrote:
Quoted 26 lines
Quoted 23 lines · Yasuaki Ishimatsu
From: Yasuaki Ishimatsu <[email protected]>
add_memory() hot adds a physical memory. But remove_memory does not hot remove a phsical memory. It only offlines memory. The name confuse us.
So the patch renames remove_memory() to offline_memory(). We will use rename_memory() for hot removing memory.
CC David Rientjes <[email protected]>
CC Jiang Liu <[email protected]>
CC Len Brown <[email protected]>
CC Christoph Lameter <[email protected]>
Cc Minchan Kim <[email protected]>
CC Andrew Morton <[email protected]>
CC KOSAKI Motohiro <[email protected]>
Signed-off-by Yasuaki Ishimatsu <[email protected]>
Signed-off-by Wen Congyang <[email protected]>
---
 drivers/acpi/acpi_memhotplug.c |    2 +-
 include/linux/memory_hotplug.h |    2 +-
 mm/memory_hotplug.c            |    6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

Quoted 6 lines

Quoted 2 lines
Probably, the better way is to just remove remove_memory() and use offline_pages().
we don't notify the userspace that the memory is offlined in offline_pages(). We reimplement offline_memory(), but ishimatsu doesn't include that patch to this series.

Quote from Documentation/SubmittingPatch

Quoted 2 lines

3) Separate your changes.
Separate _logical changes_ into a single patch file.

Please send us _logical_ meaningful patch set.

Yasuaki Ishimatsu

author

2012-10-03 10:09 UTC #23 lore raw

PATCH 3/6 acpi,memory-hotplug : add physical memory hotplug code to acpi_memhotplug.c

In reply to Yasuaki Ishimatsu

From: Yasuaki Ishimatsu <[email protected]>

For hot removing physical memory, the patch adds remove_memory() into acpi_memory_remove_memory(). But we cannot support physical memory hot remove. So remove_memory() do nothinig.

CC David Rientjes <[email protected]>

CC Jiang Liu <[email protected]>

CC Len Brown <[email protected]>

CC Christoph Lameter <[email protected]>

Cc Minchan Kim <[email protected]>

CC Andrew Morton <[email protected]>

CC KOSAKI Motohiro <[email protected]>

Signed-off-by Yasuaki Ishimatsu <[email protected]>

Signed-off-by Wen Congyang <[email protected]>

---
 drivers/acpi/acpi_memhotplug.c |   10 ++++++++++
 include/linux/memory_hotplug.h |    5 +++++
 mm/memory_hotplug.c            |    7 +++++++
 3 files changed, 22 insertions(+)

drivers/acpi/acpi_memhotplug.c +10 -0

Index: linux-3.6/drivers/acpi/acpi_memhotplug.c
===================================================================
@@ -310,6 +310,9 @@ static int acpi_memory_remove_memory(str
 {
 	int result;
 	struct acpi_memory_info *info, *n;
+	int node;
+
+	node = acpi_get_node(mem_device->device->handle);
 
 	list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
 		if (!info->enabled)

@@ -319,6 +322,13 @@ static int acpi_memory_remove_memory(str
 		if (result)
 			return result;
 
+		if (node < 0)
+			node = memory_add_physaddr_to_nid(info->start_addr);
+
+		result = remove_memory(node, info->start_addr, info->length);
+		if (result)
+			return result;
+
 		list_del(&info->list);
 		kfree(info);
 	}

include/linux/memory_hotplug.h +5 -0

Index: linux-3.6/include/linux/memory_hotplug.h
===================================================================
@@ -222,6 +222,7 @@ static inline void unlock_memory_hotplug
 #ifdef CONFIG_MEMORY_HOTREMOVE
 
 extern int is_mem_section_removable(unsigned long pfn, unsigned long nr_pages);
+extern int remove_memory(int nid, u64 start, u64 size);
 
 #else
 static inline int is_mem_section_removable(unsigned long pfn,

@@ -229,6 +230,10 @@ static inline int is_mem_section_removab
 {
 	return 0;
 }
+static inline int remove_memory(int nid, u64 start, u64 size)
+{
+	return -EBUSY;
+}
 #endif /* CONFIG_MEMORY_HOTREMOVE */
 
 extern int mem_online_node(int nid);

mm/memory_hotplug.c +7 -0

Index: linux-3.6/mm/memory_hotplug.c
===================================================================
@@ -1042,6 +1042,13 @@ int offline_memory(u64 start, u64 size)
 
 	return 0;
 }
+
+int remove_memory(int nid, u64 start, u64 size)
+{
+	/* It is not implemented yet*/
+	return 0;
+}
+EXPORT_SYMBOL_GPL(remove_memory);
 #else
 int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
 {

KOSAKI Motohiro

2012-10-05 18:54 UTC #24 lore raw

Re: [PATCH 3/6] acpi,memory-hotplug : add physical memory hotplug code to acpi_memhotplug.c

Quoted 5 lines · Yasuaki Ishimatsu

From: Yasuaki Ishimatsu <[email protected]>
For hot removing physical memory, the patch adds remove_memory() into acpi_memory_remove_memory(). But we cannot support physical memory hot remove. So remove_memory() do nothinig.

I don't understand this explanation. Why do we need do nothing change? I guess you need to fold this patch into another meaningful fix.

Yasuaki Ishimatsu

author

2012-10-03 10:11 UTC #25 lore raw

PATCH 4/4 acpi,memory-hotplug : store the node id in acpi_memory_device

In reply to Yasuaki Ishimatsu

From: Wen Congyang <[email protected]>

The memory device has only one node id. Store the node id when enable the memory device, and we can reuse it when removing the memory device.

CC David Rientjes <[email protected]>

CC Jiang Liu <[email protected]>

CC Len Brown <[email protected]>

CC Benjamin Herrenschmidt <[email protected]>

CC Paul Mackerras <[email protected]>

CC Christoph Lameter <[email protected]>

Cc Minchan Kim <[email protected]>

CC Andrew Morton <[email protected]>

CC KOSAKI Motohiro <[email protected]>

CC Yasuaki Ishimatsu <[email protected]>

Signed-off-by Wen Congyang <[email protected]>

Signed-off-by Yasuaki Ishimatsu <[email protected]>

---
 drivers/acpi/acpi_memhotplug.c |   11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

drivers/acpi/acpi_memhotplug.c +5 -6

Index: linux-3.6/drivers/acpi/acpi_memhotplug.c
===================================================================
@@ -83,6 +83,7 @@ struct acpi_memory_info {
 struct acpi_memory_device {
 	struct acpi_device * device;
 	unsigned int state;	/* State of the memory device */
+	int nid;
 	struct list_head res_list;
 };

@@ -256,6 +257,9 @@ static int acpi_memory_enable_device(str
 		info->enabled = 1;
 		num_enabled++;
 	}
+
+	mem_device->nid = node;
+
 	if (!num_enabled) {
 		printk(KERN_ERR PREFIX "add_memory failed\n");
 		mem_device->state = MEMORY_INVALID_STATE;

@@ -310,9 +314,7 @@ static int acpi_memory_remove_memory(str
 {
 	int result;
 	struct acpi_memory_info *info, *n;
-	int node;
-
-	node = acpi_get_node(mem_device->device->handle);
+	int node = mem_device->nid;
 
 	list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
 		if (!info->enabled)

@@ -322,9 +324,6 @@ static int acpi_memory_remove_memory(str
 		if (result)
 			return result;
 
-		if (node < 0)
-			node = memory_add_physaddr_to_nid(info->start_addr);
-
 		result = remove_memory(node, info->start_addr, info->length);
 		if (result)
 			return result;

KOSAKI Motohiro

2012-10-05 18:56 UTC #26 lore raw

Re: [PATCH 4/4] acpi,memory-hotplug : store the node id in acpi_memory_device

Quoted 5 lines · Yasuaki Ishimatsu

From: Wen Congyang <[email protected]>
The memory device has only one node id. Store the node id when enable the memory device, and we can reuse it when removing the memory device.

You don't explain why we need this. Then nobody can review nor ack.

Wen Congyang

2012-10-08 06:41 UTC #27 lore raw

Re: [PATCH 4/4] acpi,memory-hotplug : store the node id in acpi_memory_device

At 10/06/2012 02:56 AM, KOSAKI Motohiro Wrote:

Quoted 10 lines

Quoted 5 lines · Yasuaki Ishimatsu
From: Wen Congyang <[email protected]>
The memory device has only one node id. Store the node id when enable the memory device, and we can reuse it when removing the memory device.
You don't explain why we need this. Then nobody can review nor ack.

This patch doesn't fix any problem. Its purpose is: avoid to calculate the node id twice.

Thanks
Wen Congyang

KOSAKI Motohiro

2012-10-12 19:00 UTC #28 lore raw

Re: [PATCH 4/4] acpi,memory-hotplug : store the node id in acpi_memory_device

Quoted 9 lines · Wen Congyang

At 10/06/2012 02:56 AM, KOSAKI Motohiro Wrote:
Quoted 8 lines
Quoted 5 lines · Yasuaki Ishimatsu
From: Wen Congyang <[email protected]>
The memory device has only one node id. Store the node id when enable the memory device, and we can reuse it when removing the memory device.

Quoted 5 lines

Quoted 2 lines
You don't explain why we need this. Then nobody can review nor ack.
This patch doesn't fix any problem. Its purpose is: avoid to calculate the node id twice.

ditto.

Please separate patches as logical change. You should make problem fix patch set.

Ni zhan Chen

2012-10-06 14:22 UTC #29 lore raw

In reply to Yasuaki Ishimatsu

Quoted 9 lines · Yasuaki Ishimatsu

We are trying to implement a physical memory hot removing function as following thread.
https://lkml.org/lkml/2012/9/5/201
But there is not enough review to merge into linux kernel.
I think there are following blockades.
1. no physical memory hot removable system

Which kind of special machine support physical memory hot-remove now?

Quoted 39 lines

2. huge patch-set
If you have a KVM system, we can get rid of 1st blockade. Because applying following patch, we can create memory hot removable system on KVM guest.
http://lists.gnu.org/archive/html/qemu-devel/2012-07/msg01389.html
2nd blockade is own problem. So we try to divide huge patch into a small patch in each function as follows:
- bug fix
- acpi framework
- kernel core
We had already sent bug fix patches.
https://lkml.org/lkml/2012/9/27/39
https://lkml.org/lkml/2012/10/2/83
The patch-set implements a framework for hot removing memory.
The memory device can be removed by 2 ways:
1. send eject request by SCI
2. echo 1 >/sys/bus/pci/devices/PNP0C80:XX/eject
In the 1st case, acpi_memory_disable_device() will be called. In the 2nd case, acpi_memory_device_remove() will be called. acpi_memory_device_remove() will also be called when we unbind the memory device from the driver acpi_memhotplug.
acpi_memory_disable_device() has already implemented a code which offlines memory and releases acpi_memory_info struct . But acpi_memory_device_remove() has not implemented it yet.
So the patch prepares the framework for hot removing memory and adds the framework intoacpi_memory_device_remove(). And it prepares remove_memory(). But the function does nothing because we cannot support memory hot remove.

Quoted 6 lines

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