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.
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
===================================================================
--- linux-3.6.orig/drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:33.386378909 +0900
+++ linux-3.6/drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900
@@ -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;
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(-)
Index: linux-3.6/drivers/acpi/acpi_memhotplug.c
===================================================================
--- linux-3.6.orig/drivers/acpi/acpi_memhotplug.c 2012-10-03 18:17:29.291244669 +0900
+++ linux-3.6/drivers/acpi/acpi_memhotplug.c 2012-10-03 18:17:41.494247869 +0900
@@ -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;
Index: linux-3.6/include/linux/memory_hotplug.h
===================================================================
--- linux-3.6.orig/include/linux/memory_hotplug.h 2012-10-03 18:17:01.863247694 +0900
+++ linux-3.6/include/linux/memory_hotplug.h 2012-10-03 18:17:41.496247872 +0900
@@ -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);
Index: linux-3.6/mm/memory_hotplug.c
===================================================================
--- linux-3.6.orig/mm/memory_hotplug.c 2012-10-03 18:17:01.861247692 +0900
+++ linux-3.6/mm/memory_hotplug.c 2012-10-03 18:17:41.503247876 +0900
@@ -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);
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(+)
Index: linux-3.6/drivers/acpi/acpi_memhotplug.c
===================================================================
--- linux-3.6.orig/drivers/acpi/acpi_memhotplug.c 2012-10-03 19:03:10.960400793 +0900
+++ linux-3.6/drivers/acpi/acpi_memhotplug.c 2012-10-03 19:03:26.818401966 +0900
@@ -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);
}
Index: linux-3.6/include/linux/memory_hotplug.h
===================================================================
--- linux-3.6.orig/include/linux/memory_hotplug.h 2012-10-03 19:03:10.963400796 +0900
+++ linux-3.6/include/linux/memory_hotplug.h 2012-10-03 19:03:26.820401968 +0900
@@ -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);
Index: linux-3.6/mm/memory_hotplug.c
===================================================================
--- linux-3.6.orig/mm/memory_hotplug.c 2012-10-03 19:03:10.962400795 +0900
+++ linux-3.6/mm/memory_hotplug.c 2012-10-03 19:04:15.493404911 +0900
@@ -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)
{
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(-)
Index: linux-3.6/drivers/acpi/acpi_memhotplug.c
===================================================================
--- linux-3.6.orig/drivers/acpi/acpi_memhotplug.c 2012-10-03 19:03:26.818401966 +0900
+++ linux-3.6/drivers/acpi/acpi_memhotplug.c 2012-10-03 19:08:38.804604700 +0900
@@ -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;
On Wed, Oct 3, 2012 at 5:58 AM, Yasuaki Ishimatsu
<[email protected]> wrote:
> 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
> ===================================================================
> --- linux-3.6.orig/drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:33.386378909 +0900
> +++ linux-3.6/drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900
> @@ -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?
> + 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.
> +
> + 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
> */
Write function comment instead of this silly comment.
> - 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);
This patch move acpi_memory_powerdown_device() from ACPI_NOTIFY_EJECT_REQUEST
to release callback, but don't explain why.
> @@ -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.
> + */
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.
> + 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>
On Wed, Oct 3, 2012 at 6:02 AM, Yasuaki Ishimatsu
<[email protected]> wrote:
> 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(-)
Probably, the better way is to just remove remove_memory() and use
offline_pages().
btw, current remove_memory() pfn calculation is just buggy.
> 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)
or
start_pfn = PFN_UP(start);
end_pfn = PFN_DOWN(start + size)
On Wed, Oct 3, 2012 at 6:09 AM, Yasuaki Ishimatsu
<[email protected]> wrote:
> 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.
On Wed, Oct 3, 2012 at 6:11 AM, Yasuaki Ishimatsu
<[email protected]> wrote:
> 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.
On 10/03/2012 05:52 PM, Yasuaki Ishimatsu wrote:
> 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?
> 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.
>
> --
> 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>
>
At 10/05/2012 05:31 AM, KOSAKI Motohiro Wrote:
> On Wed, Oct 3, 2012 at 6:02 AM, Yasuaki Ishimatsu
> <[email protected]> wrote:
>> 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(-)
>
> 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
>
> btw, current remove_memory() pfn calculation is just buggy.
>
>
>> 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)
>
> or
>
> start_pfn = PFN_UP(start);
> end_pfn = PFN_DOWN(start + size)
>
At 10/06/2012 02:56 AM, KOSAKI Motohiro Wrote:
> On Wed, Oct 3, 2012 at 6:11 AM, Yasuaki Ishimatsu
> <[email protected]> wrote:
>> 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
At 10/05/2012 04:53 AM, KOSAKI Motohiro Wrote:
> On Wed, Oct 3, 2012 at 5:58 AM, Yasuaki Ishimatsu
> <[email protected]> wrote:
>> 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
>> ===================================================================
>> --- linux-3.6.orig/drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:33.386378909 +0900
>> +++ linux-3.6/drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900
>> @@ -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?
There is no any lock to protect it now...
>
>
>> + 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.
>
>
>> +
>> + 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
>> */
>
> Write function comment instead of this silly comment.
>
>> - 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);
>
> 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().
>
>
>
>
>
>> @@ -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.
>> + */
>
> 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
>
>
>> + 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>
>
On Mon, Oct 8, 2012 at 2:45 AM, Wen Congyang <[email protected]> wrote:
> At 10/05/2012 05:31 AM, KOSAKI Motohiro Wrote:
>> On Wed, Oct 3, 2012 at 6:02 AM, Yasuaki Ishimatsu
>> <[email protected]> wrote:
>>> 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(-)
>>
>> 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
> 3) Separate your changes.
> Separate _logical changes_ into a single patch file.
Please send us _logical_ meaningful patch set.
On Mon, Oct 8, 2012 at 2:47 AM, Wen Congyang <[email protected]> wrote:
> At 10/06/2012 02:56 AM, KOSAKI Motohiro Wrote:
>> On Wed, Oct 3, 2012 at 6:11 AM, Yasuaki Ishimatsu
>> <[email protected]> wrote:
>>> 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.
ditto.
Please separate patches as logical change. You should make problem fix
patch set.
>>> -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.
>>> +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.
>>
>>> - 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);
>>
>> 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.
>>> @@ -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.
>>> + */
>>
>> 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...
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.
At 10/13/2012 03:10 AM, KOSAKI Motohiro Wrote:
>>>> -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.
>
>
>
>
>>>> +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.
>>>
>>>> - 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);
>>>
>>> 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
>
>
>
>
>>>> @@ -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.
>>>> + */
>>>
>>> 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...
>
> 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.
>
On Wed, Oct 17, 2012 at 2:48 AM, Wen Congyang <[email protected]> wrote:
> At 10/13/2012 03:10 AM, KOSAKI Motohiro Wrote:
>>>>> -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.
>>
>>
>>
>>
>>>>> +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.
>>>>
>>>>> - 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);
>>>>
>>>> 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.
Heh, please explain why do you think so.
At 10/17/2012 04:59 PM, KOSAKI Motohiro Wrote:
> On Wed, Oct 17, 2012 at 2:48 AM, Wen Congyang <[email protected]> wrote:
>> At 10/13/2012 03:10 AM, KOSAKI Motohiro Wrote:
>>>>>> -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.
>>>
>>>
>>>
>>>
>>>>>> +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.
>>>>>
>>>>>> - 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);
>>>>>
>>>>> 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.
>
> 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
At 10/13/2012 03:10 AM, KOSAKI Motohiro Wrote:
>>>> -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.
>
>
>
>
>>>> +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.
>>>
>>>> - 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);
>>>
>>> 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.
>
>
>
>
>>>> @@ -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.
>>>> + */
>>>
>>> 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...
>
> 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.
>
> 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
>>>>> 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.
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.
At 10/17/2012 05:18 PM, KOSAKI Motohiro Wrote:
>>>>>> 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.
>
> 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
> --
> 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/
>
Hi Wen,
2012/10/17 18:52, Wen Congyang wrote:
> At 10/17/2012 05:18 PM, KOSAKI Motohiro Wrote:
>>>>>>> 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.
>>
>> 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?
Oops. If so, it's my mistake.
Could you update it in next version?
Thanks,
Yasuaki Ishimatsu
>
> Thanks
> Wen Congyang
>
>> --
>> 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/
>>
>
> --
> 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
>
>>>>> + 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...
>>
>> 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.
>> 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.
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.
At 10/18/2012 09:25 AM, Yasuaki Ishimatsu Wrote:
> Hi Wen,
>
> 2012/10/17 18:52, Wen Congyang wrote:
>> At 10/17/2012 05:18 PM, KOSAKI Motohiro Wrote:
>>>>>>>> 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.
>>>
>>> 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?
>
> Oops. If so, it's my mistake.
> Could you update it in next version?
OK
Thanks
Wen Congyang
>
> Thanks,
> Yasuaki Ishimatsu
>
>>
>> Thanks
>> Wen Congyang
>>
>>> --
>>> 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/
>>>
>>
>> --
>> 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
>>
>
>
> --
> 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/
>
At 10/19/2012 03:44 AM, KOSAKI Motohiro Wrote:
>>>>>> + 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...
>>>
>>> 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.
>
>
>>> 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.
>
> 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.
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
>
> 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.
>
> Hmm, IIRC, if the memory is recognized from kerenl before driver initialization,
> the memory device is not managed by the driver acpi_memhotplug.
Yup.
> 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.
At 10/20/2012 02:19 AM, KOSAKI Motohiro Wrote:
>> Hmm, IIRC, if the memory is recognized from kerenl before driver initialization,
>> the memory device is not managed by the driver acpi_memhotplug.
>
> Yup.
>
>
>> 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
>> ??
>> 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
Memory never be out of control by driver unloading. It is controled
from kernel core. It is an exception from regular linux driver model.
> 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
On Mon, Oct 22, 2012 at 11:11 AM, KOSAKI Motohiro
<[email protected]> wrote:
>>> ??
>>> 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
>
> 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.