Hi,
This series contains changes that are possible on top of the linux-pm.git
tree's acpi-hotplug branch. They touch ACPI, driver core and the core memory
hotplug code and the majority of them are about removing code that's not
necessary any more.
Please review and let me know if there's anything wrong with any of them.
[1/5] Drop the struct acpi_device's removal_type field that's not used any more.
[2/5] Pass processor object handle to acpi_bind_one()
[3/5] Replace offline_memory_block() with device_offline().
[4/5] Add second pass of companion offlining to acpi_scan_hot_remove().
[5/5] Drop ACPI memory hotplug code that's not necessary any more.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
From: Rafael J. Wysocki <[email protected]>
Since offline_memory_block(mem) is functionally equivalent to
device_offline(&mem->dev), make the only caller of the former use
the latter instead and drop offline_memory_block() entirely.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/base/memory.c | 21 ---------------------
include/linux/memory_hotplug.h | 1 -
mm/memory_hotplug.c | 2 +-
3 files changed, 1 insertion(+), 23 deletions(-)
Index: linux-pm/drivers/base/memory.c
===================================================================
--- linux-pm.orig/drivers/base/memory.c
+++ linux-pm/drivers/base/memory.c
@@ -735,27 +735,6 @@ int unregister_memory_section(struct mem
}
#endif /* CONFIG_MEMORY_HOTREMOVE */
-/*
- * offline one memory block. If the memory block has been offlined, do nothing.
- *
- * Call under device_hotplug_lock.
- */
-int offline_memory_block(struct memory_block *mem)
-{
- int ret = 0;
-
- mutex_lock(&mem->state_mutex);
- if (mem->state != MEM_OFFLINE) {
- ret = __memory_block_change_state_uevent(mem, MEM_OFFLINE,
- MEM_ONLINE, -1);
- if (!ret)
- mem->dev.offline = true;
- }
- mutex_unlock(&mem->state_mutex);
-
- return ret;
-}
-
/* return true if the memory block is offlined, otherwise, return false */
bool is_memblock_offlined(struct memory_block *mem)
{
Index: linux-pm/include/linux/memory_hotplug.h
===================================================================
--- linux-pm.orig/include/linux/memory_hotplug.h
+++ linux-pm/include/linux/memory_hotplug.h
@@ -251,7 +251,6 @@ extern int mem_online_node(int nid);
extern int add_memory(int nid, u64 start, u64 size);
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 bool is_memblock_offlined(struct memory_block *mem);
extern int remove_memory(int nid, u64 start, u64 size);
extern int sparse_add_one_section(struct zone *zone, unsigned long start_pfn,
Index: linux-pm/mm/memory_hotplug.c
===================================================================
--- linux-pm.orig/mm/memory_hotplug.c
+++ linux-pm/mm/memory_hotplug.c
@@ -1680,7 +1680,7 @@ int walk_memory_range(unsigned long star
static int offline_memory_block_cb(struct memory_block *mem, void *arg)
{
int *ret = arg;
- int error = offline_memory_block(mem);
+ int error = device_offline(&mem->dev);
if (error != 0 && *ret == 0)
*ret = error;
From: Rafael J. Wysocki <[email protected]>
Now that the memory offlining should be taken care of by the
companion device offlining code in acpi_scan_hot_remove(), the
ACPI memory hotplug driver doesn't need to offline it in
acpi_memory_remove_memory() any more. Consequently, it doesn't
need to call remove_memory() any more, which means that that
funtion may be dropped entirely, because acpi_memory_remove_memory()
is the only user of it.
Make the changes described above to get rid of the dead code.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/acpi_memhotplug.c | 15 ------
include/linux/memory_hotplug.h | 1
mm/memory_hotplug.c | 102 -----------------------------------------
3 files changed, 2 insertions(+), 116 deletions(-)
Index: linux-pm/drivers/acpi/acpi_memhotplug.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpi_memhotplug.c
+++ linux-pm/drivers/acpi/acpi_memhotplug.c
@@ -271,31 +271,20 @@ static int acpi_memory_enable_device(str
return 0;
}
-static int acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
+static void acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
{
acpi_handle handle = mem_device->device->handle;
- int result = 0, nid;
struct acpi_memory_info *info, *n;
- nid = acpi_get_node(handle);
-
list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
if (!info->enabled)
continue;
- if (nid < 0)
- nid = memory_add_physaddr_to_nid(info->start_addr);
-
+ /* All of the memory blocks are offline at this point. */
acpi_unbind_memory_blocks(info, handle);
- result = remove_memory(nid, info->start_addr, info->length);
- if (result)
- return result;
-
list_del(&info->list);
kfree(info);
}
-
- return result;
}
static void acpi_memory_device_free(struct acpi_memory_device *mem_device)
Index: linux-pm/include/linux/memory_hotplug.h
===================================================================
--- linux-pm.orig/include/linux/memory_hotplug.h
+++ linux-pm/include/linux/memory_hotplug.h
@@ -252,7 +252,6 @@ 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 bool is_memblock_offlined(struct memory_block *mem);
-extern int remove_memory(int nid, 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-pm/mm/memory_hotplug.c
===================================================================
--- linux-pm.orig/mm/memory_hotplug.c
+++ linux-pm/mm/memory_hotplug.c
@@ -1670,41 +1670,6 @@ int walk_memory_range(unsigned long star
}
#ifdef CONFIG_MEMORY_HOTREMOVE
-/**
- * offline_memory_block_cb - callback function for offlining memory block
- * @mem: the memory block to be offlined
- * @arg: buffer to hold error msg
- *
- * Always return 0, and put the error msg in arg if any.
- */
-static int offline_memory_block_cb(struct memory_block *mem, void *arg)
-{
- int *ret = arg;
- int error = device_offline(&mem->dev);
-
- if (error != 0 && *ret == 0)
- *ret = error;
-
- return 0;
-}
-
-static int is_memblock_offlined_cb(struct memory_block *mem, void *arg)
-{
- int ret = !is_memblock_offlined(mem);
-
- if (unlikely(ret)) {
- phys_addr_t beginpa, endpa;
-
- beginpa = PFN_PHYS(section_nr_to_pfn(mem->start_section_nr));
- endpa = PFN_PHYS(section_nr_to_pfn(mem->end_section_nr + 1))-1;
- pr_warn("removing memory fails, because memory "
- "[%pa-%pa] is onlined\n",
- &beginpa, &endpa);
- }
-
- return ret;
-}
-
static int check_cpu_on_node(void *data)
{
struct pglist_data *pgdat = data;
@@ -1812,76 +1777,9 @@ void try_offline_node(int nid)
memset(pgdat, 0, sizeof(*pgdat));
}
EXPORT_SYMBOL(try_offline_node);
-
-int __ref remove_memory(int nid, u64 start, u64 size)
-{
- unsigned long start_pfn, end_pfn;
- int ret = 0;
- int retry = 1;
-
- start_pfn = PFN_DOWN(start);
- end_pfn = PFN_UP(start + size - 1);
-
- /*
- * When CONFIG_MEMCG is on, one memory block may be used by other
- * blocks to store page cgroup when onlining pages. But we don't know
- * in what order pages are onlined. So we iterate twice to offline
- * memory:
- * 1st iterate: offline every non primary memory block.
- * 2nd iterate: offline primary (i.e. first added) memory block.
- */
-repeat:
- walk_memory_range(start_pfn, end_pfn, &ret,
- offline_memory_block_cb);
- if (ret) {
- if (!retry)
- return ret;
-
- retry = 0;
- ret = 0;
- goto repeat;
- }
-
- lock_memory_hotplug();
-
- /*
- * we have offlined all memory blocks like this:
- * 1. lock memory hotplug
- * 2. offline a memory block
- * 3. unlock memory hotplug
- *
- * repeat step1-3 to offline the memory block. All memory blocks
- * must be offlined before removing memory. But we don't hold the
- * lock in the whole operation. So we should check whether all
- * memory blocks are offlined.
- */
-
- ret = walk_memory_range(start_pfn, end_pfn, NULL,
- is_memblock_offlined_cb);
- if (ret) {
- unlock_memory_hotplug();
- return ret;
- }
-
- /* remove memmap entry */
- firmware_map_remove(start, start + size, "System RAM");
-
- arch_remove_memory(start, size);
-
- try_offline_node(nid);
-
- unlock_memory_hotplug();
-
- return 0;
-}
#else
int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
{
return -EINVAL;
}
-int remove_memory(int nid, u64 start, u64 size)
-{
- return -EINVAL;
-}
#endif /* CONFIG_MEMORY_HOTREMOVE */
-EXPORT_SYMBOL_GPL(remove_memory);
From: Rafael J. Wysocki <[email protected]>
Make acpi_processor_add() pass the ACPI handle of the processor
namespace object to acpi_bind_one() instead of setting it directly
to allow acpi_bind_one() to catch possible bugs causing the ACPI
handle of the processor device to be set earlier.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/acpi_processor.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
Index: linux-pm/drivers/acpi/acpi_processor.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpi_processor.c
+++ linux-pm/drivers/acpi/acpi_processor.c
@@ -389,8 +389,7 @@ static int __cpuinit acpi_processor_add(
per_cpu(processor_device_array, pr->id) = device;
dev = get_cpu_device(pr->id);
- ACPI_HANDLE_SET(dev, pr->handle);
- result = acpi_bind_one(dev, NULL);
+ result = acpi_bind_one(dev, pr->handle);
if (result)
goto err;
From: Rafael J. Wysocki <[email protected]>
As indicated by comments in mm/memory_hotplug.c:remove_memory(),
if CONFIG_MEMCG is set, it may not be possible to offline all of the
memory blocks held by one module (FRU) in one pass (because one of
them may be used by the others to store page cgroup in that case
and that block has to be offlined before the other ones).
To handle that arguably corner case, add a second pass of companion
device offlining to acpi_scan_hot_remove() and make it ignore errors
returned in the first pass (and make it skip the second pass if the
first one is successful).
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/scan.c | 67 ++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 50 insertions(+), 17 deletions(-)
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -131,6 +131,7 @@ static acpi_status acpi_bus_offline_comp
{
struct acpi_device *device = NULL;
struct acpi_device_physical_node *pn;
+ bool second_pass = (bool)data;
acpi_status status = AE_OK;
if (acpi_bus_get_device(handle, &device))
@@ -141,15 +142,26 @@ static acpi_status acpi_bus_offline_comp
list_for_each_entry(pn, &device->physical_node_list, node) {
int ret;
+ if (second_pass) {
+ /* Skip devices offlined by the first pass. */
+ if (pn->put_online)
+ continue;
+ } else {
+ pn->put_online = false;
+ }
ret = device_offline(pn->dev);
if (acpi_force_hot_remove)
continue;
- if (ret < 0) {
- status = AE_ERROR;
- break;
+ if (ret >= 0) {
+ pn->put_online = !ret;
+ } else {
+ *ret_p = pn->dev;
+ if (second_pass) {
+ status = AE_ERROR;
+ break;
+ }
}
- pn->put_online = !ret;
}
mutex_unlock(&device->physical_node_lock);
@@ -185,6 +197,7 @@ static int acpi_scan_hot_remove(struct a
acpi_handle not_used;
struct acpi_object_list arg_list;
union acpi_object arg;
+ struct device *errdev;
acpi_status status;
unsigned long long sta;
@@ -197,22 +210,42 @@ static int acpi_scan_hot_remove(struct a
lock_device_hotplug();
- status = acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
- NULL, acpi_bus_offline_companions, NULL,
- NULL);
- if (ACPI_SUCCESS(status) || acpi_force_hot_remove)
- status = acpi_bus_offline_companions(handle, 0, NULL, NULL);
-
- if (ACPI_FAILURE(status) && !acpi_force_hot_remove) {
- acpi_bus_online_companions(handle, 0, NULL, NULL);
+ /*
+ * Carry out two passes here and ignore errors in the first pass,
+ * because if the devices in question are memory blocks and
+ * CONFIG_MEMCG is set, one of the blocks may hold data structures
+ * that the other blocks depend on, but it is not known in advance which
+ * block holds them.
+ *
+ * If the first pass is successful, the second one isn't needed, though.
+ */
+ errdev = NULL;
+ acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
+ NULL, acpi_bus_offline_companions,
+ (void *)false, (void **)&errdev);
+ acpi_bus_offline_companions(handle, 0, (void *)false, (void **)&errdev);
+ if (errdev) {
+ errdev = NULL;
acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
- acpi_bus_online_companions, NULL, NULL,
- NULL);
+ NULL, acpi_bus_offline_companions,
+ (void *)true , (void **)&errdev);
+ if (!errdev || acpi_force_hot_remove)
+ acpi_bus_offline_companions(handle, 0, (void *)true,
+ (void **)&errdev);
+
+ if (errdev && !acpi_force_hot_remove) {
+ dev_warn(errdev, "Offline failed.\n");
+ acpi_bus_online_companions(handle, 0, NULL, NULL);
+ acpi_walk_namespace(ACPI_TYPE_ANY, handle,
+ ACPI_UINT32_MAX,
+ acpi_bus_online_companions, NULL,
+ NULL, NULL);
- unlock_device_hotplug();
+ unlock_device_hotplug();
- put_device(&device->dev);
- return -EBUSY;
+ put_device(&device->dev);
+ return -EBUSY;
+ }
}
ACPI_DEBUG_PRINT((ACPI_DB_INFO,
From: Rafael J. Wysocki <[email protected]>
The ACPI processor driver was the only user of the removal_type
field in struct acpi_device, but it doesn't use that field any more
after recent changes. Thus, removal_type has no more users, so drop
it along with the associated data type.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/scan.c | 2 --
include/acpi/acpi_bus.h | 8 --------
2 files changed, 10 deletions(-)
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -63,13 +63,6 @@ acpi_get_physical_device_location(acpi_h
#define ACPI_BUS_FILE_ROOT "acpi"
extern struct proc_dir_entry *acpi_root_dir;
-enum acpi_bus_removal_type {
- ACPI_BUS_REMOVAL_NORMAL = 0,
- ACPI_BUS_REMOVAL_EJECT,
- ACPI_BUS_REMOVAL_SUPRISE,
- ACPI_BUS_REMOVAL_TYPE_COUNT
-};
-
enum acpi_bus_device_type {
ACPI_BUS_TYPE_DEVICE = 0,
ACPI_BUS_TYPE_POWER,
@@ -311,7 +304,6 @@ struct acpi_device {
struct acpi_driver *driver;
void *driver_data;
struct device dev;
- enum acpi_bus_removal_type removal_type; /* indicate for different removal type */
u8 physical_node_count;
struct list_head physical_node_list;
struct mutex physical_node_lock;
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1036,7 +1036,6 @@ int acpi_device_add(struct acpi_device *
printk(KERN_ERR PREFIX "Error creating sysfs interface for device %s\n",
dev_name(&device->dev));
- device->removal_type = ACPI_BUS_REMOVAL_NORMAL;
return 0;
err:
@@ -2026,7 +2025,6 @@ static acpi_status acpi_bus_device_detac
if (!acpi_bus_get_device(handle, &device)) {
struct acpi_scan_handler *dev_handler = device->handler;
- device->removal_type = ACPI_BUS_REMOVAL_EJECT;
if (dev_handler) {
if (dev_handler->detach)
dev_handler->detach(device);
On Sun, May 19, 2013 at 01:33:02AM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Since offline_memory_block(mem) is functionally equivalent to
> device_offline(&mem->dev), make the only caller of the former use
> the latter instead and drop offline_memory_block() entirely.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
On Sun, 2013-05-19 at 01:34 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Now that the memory offlining should be taken care of by the
> companion device offlining code in acpi_scan_hot_remove(), the
> ACPI memory hotplug driver doesn't need to offline it in
> acpi_memory_remove_memory() any more. Consequently, it doesn't
> need to call remove_memory() any more, which means that that
> funtion may be dropped entirely, because acpi_memory_remove_memory()
> is the only user of it.
The off-lining part of remove_memory() can be removed, but not the
hot-delete part. Please see my comments below.
> Make the changes described above to get rid of the dead code.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> drivers/acpi/acpi_memhotplug.c | 15 ------
> include/linux/memory_hotplug.h | 1
> mm/memory_hotplug.c | 102 -----------------------------------------
> 3 files changed, 2 insertions(+), 116 deletions(-)
>
> Index: linux-pm/drivers/acpi/acpi_memhotplug.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpi_memhotplug.c
> +++ linux-pm/drivers/acpi/acpi_memhotplug.c
> @@ -271,31 +271,20 @@ static int acpi_memory_enable_device(str
> return 0;
> }
>
> -static int acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
> +static void acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
> {
> acpi_handle handle = mem_device->device->handle;
> - int result = 0, nid;
> struct acpi_memory_info *info, *n;
>
> - nid = acpi_get_node(handle);
> -
> list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
> if (!info->enabled)
> continue;
>
> - if (nid < 0)
> - nid = memory_add_physaddr_to_nid(info->start_addr);
> -
> + /* All of the memory blocks are offline at this point. */
> acpi_unbind_memory_blocks(info, handle);
> - result = remove_memory(nid, info->start_addr, info->length);
We still need to call remove_memory().
> - if (result)
> - return result;
> -
> list_del(&info->list);
> kfree(info);
> }
> -
> - return result;
> }
>
> static void acpi_memory_device_free(struct acpi_memory_device *mem_device)
> Index: linux-pm/include/linux/memory_hotplug.h
> ===================================================================
> --- linux-pm.orig/include/linux/memory_hotplug.h
> +++ linux-pm/include/linux/memory_hotplug.h
> @@ -252,7 +252,6 @@ 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 bool is_memblock_offlined(struct memory_block *mem);
> -extern int remove_memory(int nid, 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-pm/mm/memory_hotplug.c
> ===================================================================
> --- linux-pm.orig/mm/memory_hotplug.c
> +++ linux-pm/mm/memory_hotplug.c
> @@ -1670,41 +1670,6 @@ int walk_memory_range(unsigned long star
> }
:
> -
> -int __ref remove_memory(int nid, u64 start, u64 size)
> -{
> - unsigned long start_pfn, end_pfn;
> - int ret = 0;
> - int retry = 1;
> -
> - start_pfn = PFN_DOWN(start);
> - end_pfn = PFN_UP(start + size - 1);
> -
> - /*
> - * When CONFIG_MEMCG is on, one memory block may be used by other
> - * blocks to store page cgroup when onlining pages. But we don't know
> - * in what order pages are onlined. So we iterate twice to offline
> - * memory:
> - * 1st iterate: offline every non primary memory block.
> - * 2nd iterate: offline primary (i.e. first added) memory block.
> - */
> -repeat:
> - walk_memory_range(start_pfn, end_pfn, &ret,
> - offline_memory_block_cb);
> - if (ret) {
> - if (!retry)
> - return ret;
> -
> - retry = 0;
> - ret = 0;
> - goto repeat;
> - }
The above procedure can be removed as it is for off-lining.
> - lock_memory_hotplug();
> -
> - /*
> - * we have offlined all memory blocks like this:
> - * 1. lock memory hotplug
> - * 2. offline a memory block
> - * 3. unlock memory hotplug
> - *
> - * repeat step1-3 to offline the memory block. All memory blocks
> - * must be offlined before removing memory. But we don't hold the
> - * lock in the whole operation. So we should check whether all
> - * memory blocks are offlined.
> - */
> -
> - ret = walk_memory_range(start_pfn, end_pfn, NULL,
> - is_memblock_offlined_cb);
> - if (ret) {
> - unlock_memory_hotplug();
> - return ret;
> - }
> -
I think the above procedure is still useful for safe guard.
> - /* remove memmap entry */
> - firmware_map_remove(start, start + size, "System RAM");
> -
> - arch_remove_memory(start, size);
> -
> - try_offline_node(nid);
The above procedure performs memory hot-delete specific operations and
is necessary.
Thanks,
-Toshi
> - unlock_memory_hotplug();
> -
> - return 0;
> -}
On Monday, May 20, 2013 11:27:56 AM Toshi Kani wrote:
> On Sun, 2013-05-19 at 01:34 +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > Now that the memory offlining should be taken care of by the
> > companion device offlining code in acpi_scan_hot_remove(), the
> > ACPI memory hotplug driver doesn't need to offline it in
> > acpi_memory_remove_memory() any more. Consequently, it doesn't
> > need to call remove_memory() any more, which means that that
> > funtion may be dropped entirely, because acpi_memory_remove_memory()
> > is the only user of it.
>
> The off-lining part of remove_memory() can be removed, but not the
> hot-delete part. Please see my comments below.
>
> > Make the changes described above to get rid of the dead code.
> >
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > ---
> > drivers/acpi/acpi_memhotplug.c | 15 ------
> > include/linux/memory_hotplug.h | 1
> > mm/memory_hotplug.c | 102 -----------------------------------------
> > 3 files changed, 2 insertions(+), 116 deletions(-)
> >
> > Index: linux-pm/drivers/acpi/acpi_memhotplug.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/acpi_memhotplug.c
> > +++ linux-pm/drivers/acpi/acpi_memhotplug.c
> > @@ -271,31 +271,20 @@ static int acpi_memory_enable_device(str
> > return 0;
> > }
> >
> > -static int acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
> > +static void acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
> > {
> > acpi_handle handle = mem_device->device->handle;
> > - int result = 0, nid;
> > struct acpi_memory_info *info, *n;
> >
> > - nid = acpi_get_node(handle);
> > -
> > list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
> > if (!info->enabled)
> > continue;
> >
> > - if (nid < 0)
> > - nid = memory_add_physaddr_to_nid(info->start_addr);
> > -
> > + /* All of the memory blocks are offline at this point. */
> > acpi_unbind_memory_blocks(info, handle);
> > - result = remove_memory(nid, info->start_addr, info->length);
>
> We still need to call remove_memory().
>
> > - if (result)
> > - return result;
> > -
> > list_del(&info->list);
> > kfree(info);
> > }
> > -
> > - return result;
> > }
> >
> > static void acpi_memory_device_free(struct acpi_memory_device *mem_device)
> > Index: linux-pm/include/linux/memory_hotplug.h
> > ===================================================================
> > --- linux-pm.orig/include/linux/memory_hotplug.h
> > +++ linux-pm/include/linux/memory_hotplug.h
> > @@ -252,7 +252,6 @@ 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 bool is_memblock_offlined(struct memory_block *mem);
> > -extern int remove_memory(int nid, 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-pm/mm/memory_hotplug.c
> > ===================================================================
> > --- linux-pm.orig/mm/memory_hotplug.c
> > +++ linux-pm/mm/memory_hotplug.c
> > @@ -1670,41 +1670,6 @@ int walk_memory_range(unsigned long star
> > }
>
> :
>
> > -
> > -int __ref remove_memory(int nid, u64 start, u64 size)
> > -{
> > - unsigned long start_pfn, end_pfn;
> > - int ret = 0;
> > - int retry = 1;
> > -
> > - start_pfn = PFN_DOWN(start);
> > - end_pfn = PFN_UP(start + size - 1);
> > -
> > - /*
> > - * When CONFIG_MEMCG is on, one memory block may be used by other
> > - * blocks to store page cgroup when onlining pages. But we don't know
> > - * in what order pages are onlined. So we iterate twice to offline
> > - * memory:
> > - * 1st iterate: offline every non primary memory block.
> > - * 2nd iterate: offline primary (i.e. first added) memory block.
> > - */
> > -repeat:
> > - walk_memory_range(start_pfn, end_pfn, &ret,
> > - offline_memory_block_cb);
> > - if (ret) {
> > - if (!retry)
> > - return ret;
> > -
> > - retry = 0;
> > - ret = 0;
> > - goto repeat;
> > - }
>
> The above procedure can be removed as it is for off-lining.
>
> > - lock_memory_hotplug();
> > -
> > - /*
> > - * we have offlined all memory blocks like this:
> > - * 1. lock memory hotplug
> > - * 2. offline a memory block
> > - * 3. unlock memory hotplug
> > - *
> > - * repeat step1-3 to offline the memory block. All memory blocks
> > - * must be offlined before removing memory. But we don't hold the
> > - * lock in the whole operation. So we should check whether all
> > - * memory blocks are offlined.
> > - */
> > -
> > - ret = walk_memory_range(start_pfn, end_pfn, NULL,
> > - is_memblock_offlined_cb);
> > - if (ret) {
> > - unlock_memory_hotplug();
> > - return ret;
> > - }
> > -
>
> I think the above procedure is still useful for safe guard.
But then it shoud to BUG_ON() instead of returning an error (which isn't very
useful for anything now).
> > - /* remove memmap entry */
> > - firmware_map_remove(start, start + size, "System RAM");
> > -
> > - arch_remove_memory(start, size);
> > -
> > - try_offline_node(nid);
>
> The above procedure performs memory hot-delete specific operations and
> is necessary.
OK, I see. I'll replace this patch with something simpler, then.
What about the other patches in the series?
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Mon, 2013-05-20 at 21:47 +0200, Rafael J. Wysocki wrote:
> On Monday, May 20, 2013 11:27:56 AM Toshi Kani wrote:
> > On Sun, 2013-05-19 at 01:34 +0200, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <[email protected]>
:
> > > - lock_memory_hotplug();
> > > -
> > > - /*
> > > - * we have offlined all memory blocks like this:
> > > - * 1. lock memory hotplug
> > > - * 2. offline a memory block
> > > - * 3. unlock memory hotplug
> > > - *
> > > - * repeat step1-3 to offline the memory block. All memory blocks
> > > - * must be offlined before removing memory. But we don't hold the
> > > - * lock in the whole operation. So we should check whether all
> > > - * memory blocks are offlined.
> > > - */
> > > -
> > > - ret = walk_memory_range(start_pfn, end_pfn, NULL,
> > > - is_memblock_offlined_cb);
> > > - if (ret) {
> > > - unlock_memory_hotplug();
> > > - return ret;
> > > - }
> > > -
> >
> > I think the above procedure is still useful for safe guard.
>
> But then it shoud to BUG_ON() instead of returning an error (which isn't very
> useful for anything now).
Right since we cannot fail at that state.
> > > - /* remove memmap entry */
> > > - firmware_map_remove(start, start + size, "System RAM");
> > > -
> > > - arch_remove_memory(start, size);
> > > -
> > > - try_offline_node(nid);
> >
> > The above procedure performs memory hot-delete specific operations and
> > is necessary.
>
> OK, I see. I'll replace this patch with something simpler, then.
Thanks.
> What about the other patches in the series?
I will send you my comments later (a bit interrupted for other thing
now).
-Toshi
On Mon, 2013-05-20 at 13:55 -0600, Toshi Kani wrote:
> On Mon, 2013-05-20 at 21:47 +0200, Rafael J. Wysocki wrote:
> > On Monday, May 20, 2013 11:27:56 AM Toshi Kani wrote:
> > > On Sun, 2013-05-19 at 01:34 +0200, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <[email protected]>
>
> :
>
> > > > - lock_memory_hotplug();
> > > > -
> > > > - /*
> > > > - * we have offlined all memory blocks like this:
> > > > - * 1. lock memory hotplug
> > > > - * 2. offline a memory block
> > > > - * 3. unlock memory hotplug
> > > > - *
> > > > - * repeat step1-3 to offline the memory block. All memory blocks
> > > > - * must be offlined before removing memory. But we don't hold the
> > > > - * lock in the whole operation. So we should check whether all
> > > > - * memory blocks are offlined.
> > > > - */
> > > > -
> > > > - ret = walk_memory_range(start_pfn, end_pfn, NULL,
> > > > - is_memblock_offlined_cb);
> > > > - if (ret) {
> > > > - unlock_memory_hotplug();
> > > > - return ret;
> > > > - }
> > > > -
> > >
> > > I think the above procedure is still useful for safe guard.
> >
> > But then it shoud to BUG_ON() instead of returning an error (which isn't very
> > useful for anything now).
>
> Right since we cannot fail at that state.
>
> > > > - /* remove memmap entry */
> > > > - firmware_map_remove(start, start + size, "System RAM");
> > > > -
> > > > - arch_remove_memory(start, size);
> > > > -
> > > > - try_offline_node(nid);
> > >
> > > The above procedure performs memory hot-delete specific operations and
> > > is necessary.
> >
> > OK, I see. I'll replace this patch with something simpler, then.
>
> Thanks.
>
> > What about the other patches in the series?
>
> I will send you my comments later (a bit interrupted for other thing
> now).
Other patches look good. For patch 1/5 to 4/5:
Acked-by: Toshi Kani <[email protected]>
Thanks,
-Toshi
On 2013/5/19 7:34, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> As indicated by comments in mm/memory_hotplug.c:remove_memory(),
> if CONFIG_MEMCG is set, it may not be possible to offline all of the
> memory blocks held by one module (FRU) in one pass (because one of
> them may be used by the others to store page cgroup in that case
> and that block has to be offlined before the other ones).
>
> To handle that arguably corner case, add a second pass of companion
> device offlining to acpi_scan_hot_remove() and make it ignore errors
> returned in the first pass (and make it skip the second pass if the
> first one is successful).
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> drivers/acpi/scan.c | 67 ++++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 50 insertions(+), 17 deletions(-)
>
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -131,6 +131,7 @@ static acpi_status acpi_bus_offline_comp
> {
> struct acpi_device *device = NULL;
> struct acpi_device_physical_node *pn;
> + bool second_pass = (bool)data;
> acpi_status status = AE_OK;
>
> if (acpi_bus_get_device(handle, &device))
> @@ -141,15 +142,26 @@ static acpi_status acpi_bus_offline_comp
> list_for_each_entry(pn, &device->physical_node_list, node) {
> int ret;
>
> + if (second_pass) {
> + /* Skip devices offlined by the first pass. */
> + if (pn->put_online)
should it be "if (!pn->put_online)" ?
Thanks
Xishi Qiu
> + continue;
> + } else {
> + pn->put_online = false;
> + }
> ret = device_offline(pn->dev);
> if (acpi_force_hot_remove)
> continue;
>
> - if (ret < 0) {
> - status = AE_ERROR;
> - break;
> + if (ret >= 0) {
> + pn->put_online = !ret;
> + } else {
> + *ret_p = pn->dev;
> + if (second_pass) {
> + status = AE_ERROR;
> + break;
> + }
> }
> - pn->put_online = !ret;
> }
>
> mutex_unlock(&device->physical_node_lock);
On Tuesday, May 21, 2013 03:34:37 PM Xishi Qiu wrote:
> On 2013/5/19 7:34, Rafael J. Wysocki wrote:
>
> > From: Rafael J. Wysocki <[email protected]>
> >
> > As indicated by comments in mm/memory_hotplug.c:remove_memory(),
> > if CONFIG_MEMCG is set, it may not be possible to offline all of the
> > memory blocks held by one module (FRU) in one pass (because one of
> > them may be used by the others to store page cgroup in that case
> > and that block has to be offlined before the other ones).
> >
> > To handle that arguably corner case, add a second pass of companion
> > device offlining to acpi_scan_hot_remove() and make it ignore errors
> > returned in the first pass (and make it skip the second pass if the
> > first one is successful).
> >
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > ---
> > drivers/acpi/scan.c | 67 ++++++++++++++++++++++++++++++++++++++--------------
> > 1 file changed, 50 insertions(+), 17 deletions(-)
> >
> > Index: linux-pm/drivers/acpi/scan.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/scan.c
> > +++ linux-pm/drivers/acpi/scan.c
> > @@ -131,6 +131,7 @@ static acpi_status acpi_bus_offline_comp
> > {
> > struct acpi_device *device = NULL;
> > struct acpi_device_physical_node *pn;
> > + bool second_pass = (bool)data;
> > acpi_status status = AE_OK;
> >
> > if (acpi_bus_get_device(handle, &device))
> > @@ -141,15 +142,26 @@ static acpi_status acpi_bus_offline_comp
> > list_for_each_entry(pn, &device->physical_node_list, node) {
> > int ret;
> >
> > + if (second_pass) {
> > + /* Skip devices offlined by the first pass. */
> > + if (pn->put_online)
>
> should it be "if (!pn->put_online)" ?
No, I don't think so.
pn->put_online set means that the device has been offlined by the first pass,
so we don't need to try it in the second one.
Thanks,
Rafael
> > + continue;
> > + } else {
> > + pn->put_online = false;
> > + }
> > ret = device_offline(pn->dev);
> > if (acpi_force_hot_remove)
> > continue;
> >
> > - if (ret < 0) {
> > - status = AE_ERROR;
> > - break;
> > + if (ret >= 0) {
> > + pn->put_online = !ret;
> > + } else {
> > + *ret_p = pn->dev;
> > + if (second_pass) {
> > + status = AE_ERROR;
> > + break;
> > + }
> > }
> > - pn->put_online = !ret;
> > }
> >
> > mutex_unlock(&device->physical_node_lock);
>
>
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Monday, May 20, 2013 01:55:33 PM Toshi Kani wrote:
> On Mon, 2013-05-20 at 21:47 +0200, Rafael J. Wysocki wrote:
> > On Monday, May 20, 2013 11:27:56 AM Toshi Kani wrote:
> > > On Sun, 2013-05-19 at 01:34 +0200, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <[email protected]>
>
> :
>
> > > > - lock_memory_hotplug();
> > > > -
> > > > - /*
> > > > - * we have offlined all memory blocks like this:
> > > > - * 1. lock memory hotplug
> > > > - * 2. offline a memory block
> > > > - * 3. unlock memory hotplug
> > > > - *
> > > > - * repeat step1-3 to offline the memory block. All memory blocks
> > > > - * must be offlined before removing memory. But we don't hold the
> > > > - * lock in the whole operation. So we should check whether all
> > > > - * memory blocks are offlined.
> > > > - */
> > > > -
> > > > - ret = walk_memory_range(start_pfn, end_pfn, NULL,
> > > > - is_memblock_offlined_cb);
> > > > - if (ret) {
> > > > - unlock_memory_hotplug();
> > > > - return ret;
> > > > - }
> > > > -
> > >
> > > I think the above procedure is still useful for safe guard.
> >
> > But then it shoud to BUG_ON() instead of returning an error (which isn't very
> > useful for anything now).
>
> Right since we cannot fail at that state.
>
> > > > - /* remove memmap entry */
> > > > - firmware_map_remove(start, start + size, "System RAM");
> > > > -
> > > > - arch_remove_memory(start, size);
> > > > -
> > > > - try_offline_node(nid);
> > >
> > > The above procedure performs memory hot-delete specific operations and
> > > is necessary.
> >
> > OK, I see. I'll replace this patch with something simpler, then.
>
> Thanks.
The replacement patch is appended.
Thanks,
Rafael
---
From: Rafael J. Wysocki <[email protected]>
Subject: Memory hotplug / ACPI: Simplify memory removal
Now that the memory offlining should be taken care of by the
companion device offlining code in acpi_scan_hot_remove(), the
ACPI memory hotplug driver doesn't need to offline it in
remove_memory() any more. Moreover, since the return value of
remove_memory() is not used, it's better to make it be a void
function and trigger a BUG() if the memory scheduled for removal is
not offline.
Change the code in accordance with the above observations.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/acpi_memhotplug.c | 13 +------
include/linux/memory_hotplug.h | 2 -
mm/memory_hotplug.c | 71 ++++-------------------------------------
3 files changed, 12 insertions(+), 74 deletions(-)
Index: linux-pm/include/linux/memory_hotplug.h
===================================================================
--- linux-pm.orig/include/linux/memory_hotplug.h
+++ linux-pm/include/linux/memory_hotplug.h
@@ -252,7 +252,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 bool is_memblock_offlined(struct memory_block *mem);
-extern int remove_memory(int nid, u64 start, u64 size);
+extern void remove_memory(int nid, 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-pm/drivers/acpi/acpi_memhotplug.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpi_memhotplug.c
+++ linux-pm/drivers/acpi/acpi_memhotplug.c
@@ -271,13 +271,11 @@ static int acpi_memory_enable_device(str
return 0;
}
-static int acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
+static void acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
{
acpi_handle handle = mem_device->device->handle;
- int result = 0, nid;
struct acpi_memory_info *info, *n;
-
- nid = acpi_get_node(handle);
+ int nid = acpi_get_node(handle);
list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
if (!info->enabled)
@@ -287,15 +285,10 @@ static int acpi_memory_remove_memory(str
nid = memory_add_physaddr_to_nid(info->start_addr);
acpi_unbind_memory_blocks(info, handle);
- result = remove_memory(nid, info->start_addr, info->length);
- if (result)
- return result;
-
+ remove_memory(nid, info->start_addr, info->length);
list_del(&info->list);
kfree(info);
}
-
- return result;
}
static void acpi_memory_device_free(struct acpi_memory_device *mem_device)
Index: linux-pm/mm/memory_hotplug.c
===================================================================
--- linux-pm.orig/mm/memory_hotplug.c
+++ linux-pm/mm/memory_hotplug.c
@@ -1670,24 +1670,6 @@ int walk_memory_range(unsigned long star
}
#ifdef CONFIG_MEMORY_HOTREMOVE
-/**
- * offline_memory_block_cb - callback function for offlining memory block
- * @mem: the memory block to be offlined
- * @arg: buffer to hold error msg
- *
- * Always return 0, and put the error msg in arg if any.
- */
-static int offline_memory_block_cb(struct memory_block *mem, void *arg)
-{
- int *ret = arg;
- int error = device_offline(&mem->dev);
-
- if (error != 0 && *ret == 0)
- *ret = error;
-
- return 0;
-}
-
static int is_memblock_offlined_cb(struct memory_block *mem, void *arg)
{
int ret = !is_memblock_offlined(mem);
@@ -1813,54 +1795,22 @@ void try_offline_node(int nid)
}
EXPORT_SYMBOL(try_offline_node);
-int __ref remove_memory(int nid, u64 start, u64 size)
+void __ref remove_memory(int nid, u64 start, u64 size)
{
- unsigned long start_pfn, end_pfn;
- int ret = 0;
- int retry = 1;
-
- start_pfn = PFN_DOWN(start);
- end_pfn = PFN_UP(start + size - 1);
-
- /*
- * When CONFIG_MEMCG is on, one memory block may be used by other
- * blocks to store page cgroup when onlining pages. But we don't know
- * in what order pages are onlined. So we iterate twice to offline
- * memory:
- * 1st iterate: offline every non primary memory block.
- * 2nd iterate: offline primary (i.e. first added) memory block.
- */
-repeat:
- walk_memory_range(start_pfn, end_pfn, &ret,
- offline_memory_block_cb);
- if (ret) {
- if (!retry)
- return ret;
-
- retry = 0;
- ret = 0;
- goto repeat;
- }
+ int ret;
lock_memory_hotplug();
/*
- * we have offlined all memory blocks like this:
- * 1. lock memory hotplug
- * 2. offline a memory block
- * 3. unlock memory hotplug
- *
- * repeat step1-3 to offline the memory block. All memory blocks
- * must be offlined before removing memory. But we don't hold the
- * lock in the whole operation. So we should check whether all
- * memory blocks are offlined.
+ * All memory blocks must be offlined before removing memory. Check
+ * whether all memory blocks in question are offline and trigger a BUG()
+ * if this is not the case.
*/
-
- ret = walk_memory_range(start_pfn, end_pfn, NULL,
+ ret = walk_memory_range(PFN_DOWN(start), PFN_UP(start + size - 1), NULL,
is_memblock_offlined_cb);
if (ret) {
unlock_memory_hotplug();
- return ret;
+ BUG();
}
/* remove memmap entry */
@@ -1871,17 +1821,12 @@ repeat:
try_offline_node(nid);
unlock_memory_hotplug();
-
- return 0;
}
#else
int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
{
return -EINVAL;
}
-int remove_memory(int nid, u64 start, u64 size)
-{
- return -EINVAL;
-}
+void remove_memory(int nid, u64 start, u64 size) {}
#endif /* CONFIG_MEMORY_HOTREMOVE */
EXPORT_SYMBOL_GPL(remove_memory);
On Thu, 2013-05-23 at 00:09 +0200, Rafael J. Wysocki wrote:
> On Monday, May 20, 2013 01:55:33 PM Toshi Kani wrote:
> > On Mon, 2013-05-20 at 21:47 +0200, Rafael J. Wysocki wrote:
> > > On Monday, May 20, 2013 11:27:56 AM Toshi Kani wrote:
> > > > On Sun, 2013-05-19 at 01:34 +0200, Rafael J. Wysocki wrote:
> > > > > From: Rafael J. Wysocki <[email protected]>
> >
> > :
> >
> > > > > - lock_memory_hotplug();
> > > > > -
> > > > > - /*
> > > > > - * we have offlined all memory blocks like this:
> > > > > - * 1. lock memory hotplug
> > > > > - * 2. offline a memory block
> > > > > - * 3. unlock memory hotplug
> > > > > - *
> > > > > - * repeat step1-3 to offline the memory block. All memory blocks
> > > > > - * must be offlined before removing memory. But we don't hold the
> > > > > - * lock in the whole operation. So we should check whether all
> > > > > - * memory blocks are offlined.
> > > > > - */
> > > > > -
> > > > > - ret = walk_memory_range(start_pfn, end_pfn, NULL,
> > > > > - is_memblock_offlined_cb);
> > > > > - if (ret) {
> > > > > - unlock_memory_hotplug();
> > > > > - return ret;
> > > > > - }
> > > > > -
> > > >
> > > > I think the above procedure is still useful for safe guard.
> > >
> > > But then it shoud to BUG_ON() instead of returning an error (which isn't very
> > > useful for anything now).
> >
> > Right since we cannot fail at that state.
> >
> > > > > - /* remove memmap entry */
> > > > > - firmware_map_remove(start, start + size, "System RAM");
> > > > > -
> > > > > - arch_remove_memory(start, size);
> > > > > -
> > > > > - try_offline_node(nid);
> > > >
> > > > The above procedure performs memory hot-delete specific operations and
> > > > is necessary.
> > >
> > > OK, I see. I'll replace this patch with something simpler, then.
> >
> > Thanks.
>
> The replacement patch is appended.
The updated patch looks good.
Reviewed-by: Toshi Kani <[email protected]>
Thanks,
-Toshi
>
> Thanks,
> Rafael
>
> ---
> From: Rafael J. Wysocki <[email protected]>
> Subject: Memory hotplug / ACPI: Simplify memory removal
>
> Now that the memory offlining should be taken care of by the
> companion device offlining code in acpi_scan_hot_remove(), the
> ACPI memory hotplug driver doesn't need to offline it in
> remove_memory() any more. Moreover, since the return value of
> remove_memory() is not used, it's better to make it be a void
> function and trigger a BUG() if the memory scheduled for removal is
> not offline.
>
> Change the code in accordance with the above observations.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> drivers/acpi/acpi_memhotplug.c | 13 +------
> include/linux/memory_hotplug.h | 2 -
> mm/memory_hotplug.c | 71 ++++-------------------------------------
> 3 files changed, 12 insertions(+), 74 deletions(-)