Skip to contentSkip to conversation

ACPI/OSL: Rework of ACPICA memory OSLs to improve performance.

PATCH In review Lv Zheng 1 participant 8 messages Oct 23, 2014 – Oct 28, 2014 ACPI

lore
Share & tools
lore
author
#1 lore raw

PATCH 1/6 ACPI/OSL: Split memory operation region implementations to a seperate file.

In reply to Lv Zheng

This patch moves SystemMemory operation region implementations to a seperate file before doing cleanups. No functional changes.

Signed-off-by Lv Zheng <[email protected]>
Tested-by Fei Yang <[email protected]>
---
 drivers/acpi/Makefile |    2 +-
 drivers/acpi/mem.c    |  395 +++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/osl.c    |  381 -----------------------------------------------
 3 files changed, 396 insertions(+), 382 deletions(-)
 create mode 100644 drivers/acpi/mem.c
drivers/acpi/Makefile +1 -1
index 505d4d7..802b887 100644
@@ -18,7 +18,7 @@ obj-y				+= acpi.o \
 					acpica/
 
 # All the builtin files are in the "acpi." module_param namespace.
-acpi-y				+= osl.o utils.o reboot.o
+acpi-y				+= mem.o osl.o utils.o reboot.o
 acpi-y				+= nvs.o
 
 # Power management related files
drivers/acpi/mem.c new file +395 -0

395 lines not on this page · raw message

drivers/acpi/osl.c +0 -381

411 lines not on this page · raw message

Signature · 1 line
1.7.10

Lv Zheng

author
#0 lore raw

cover ACPI/OSL: Rework of ACPICA memory OSLs to improve performance.

It is reported that there is a performance issue in the ACPICA OSL implementation around memory mappings.

On the reported platforms, there is a debugging facility implemented in the ACPI namespace using circular logging buffer:

        Name (DPTR, 0x3AFEB000)
        Name (EPTR, 0x3AFFB000)
        Name (CPTR, 0x3AFEB010)
        Mutex (MMUT, 0x00)
        Method (MDBG, 1, Serialized)
        {
            Store (Acquire (MMUT, 0x03E8), Local0)
            If (Local0 == Zero)
            {
                OperationRegion (ABLK, SystemMemory, CPTR, 0x10)
                Field (ABLK, ByteAcc, NoLock, Preserve)
                {
                    AAAA,   128
                }
                Store (Arg0, AAAA) /* \MDBG.AAAA */
                CPTR = (CPTR + 0x10) /* \CPTR */
                If (CPTR >= EPTR)
                {
                    CPTR = (DPTR + 0x10) /* \CPTR */
                }
                Release (MMUT)
            }
            Return (Local0)
        }

This function is heavily invoked by other ACPI control methods. The reported platforms suffer from 2-5 minutes kernel stuck in the end of accessing the mapped circular buffer system memory region. By instrumentation, the 2-5 minutes time consumption overhead can be seen to happen on the synchronize_rcu() invoked in the acpi_os_unmap_memory().

This patchset removes the synchronize_rcu() from the hot path to improve the performance.

Lv Zheng (6):
ACPI/OSL: Split memory operation region implementations to a seperate
file.
ACPI/OSL: Rename system memory functions.
ACPI/OSL: Cleanup memory access functions by merging duplicate code. ACPI/OSL: Add acpi_map2virt() to merge duplicate code. ACPI/OSL: Cleanup branch logics.
ACPI/OSL: Fix performance issue in system memory lockings.

 drivers/acpi/Makefile |    2 +-
 drivers/acpi/mem.c    |  414 +++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/osl.c    |  381 ---------------------------------------------
 3 files changed, 415 insertions(+), 382 deletions(-)
 create mode 100644 drivers/acpi/mem.c
Signature · 1 line
1.7.10
author
#2 lore raw

PATCH 2/6 ACPI/OSL: Rename system memory functions.

This patch cleans up system memory functions to make it easier to understand the meaning of such functions.
No functional cleanup.

Signed-off-by Lv Zheng <[email protected]>
Tested-by Fei Yang <[email protected]>
---
 drivers/acpi/mem.c |   38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)
drivers/acpi/mem.c +22 -16
index 722241e..defd317 100644
@@ -33,15 +33,23 @@ struct acpi_ioremap {
 static LIST_HEAD(acpi_ioremaps);
 static DEFINE_MUTEX(acpi_ioremap_lock);
 
-static void acpi_os_drop_map_ref(struct acpi_ioremap *map)
+/*
+ * The following functions must be called with 'acpi_ioremap_lock' or RCU
+ * read lock held.
+ */
+static inline void acpi_map_get(struct acpi_ioremap *map)
+{
+	map->refcount++;
+}
+
+static inline void acpi_map_put(struct acpi_ioremap *map)
 {
 	if (!--map->refcount)
 		list_del_rcu(&map->list);
 }
 
-/* Must be called with 'acpi_ioremap_lock' or RCU read lock held. */
 static struct acpi_ioremap *
-acpi_map_lookup(acpi_physical_address phys, acpi_size size)
+acpi_map_lookup_phys(acpi_physical_address phys, acpi_size size)
 {
 	struct acpi_ioremap *map;
 

@@ -53,7 +61,6 @@ acpi_map_lookup(acpi_physical_address phys, acpi_size size)
 	return NULL;
 }
 
-/* Must be called with 'acpi_ioremap_lock' or RCU read lock held. */
 static struct acpi_ioremap *
 acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
 {

@@ -67,13 +74,12 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
 	return NULL;
 }
 
-/* Must be called with 'acpi_ioremap_lock' or RCU read lock held. */
 static void __iomem *
 acpi_map_vaddr_lookup(acpi_physical_address phys, unsigned int size)
 {
 	struct acpi_ioremap *map;
 
-	map = acpi_map_lookup(phys, size);
+	map = acpi_map_lookup_phys(phys, size);
 	if (map)
 		return map->virt + (phys - map->phys);
 

@@ -111,7 +117,7 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
 		iounmap(vaddr);
 }
 
-static void acpi_os_map_cleanup(struct acpi_ioremap *map)
+static void acpi_map_cleanup(struct acpi_ioremap *map)
 {
 	if (!map->refcount) {
 		synchronize_rcu();

@@ -126,10 +132,10 @@ void __iomem *acpi_os_get_iomem(acpi_physical_address phys, unsigned int size)
 	void __iomem *virt = NULL;
 
 	mutex_lock(&acpi_ioremap_lock);
-	map = acpi_map_lookup(phys, size);
+	map = acpi_map_lookup_phys(phys, size);
 	if (map) {
 		virt = map->virt + (phys - map->phys);
-		map->refcount++;
+		acpi_map_get(map);
 	}
 	mutex_unlock(&acpi_ioremap_lock);
 

@@ -155,9 +161,9 @@ acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
 
 	mutex_lock(&acpi_ioremap_lock);
 	/* Check if there's a suitable mapping already. */
-	map = acpi_map_lookup(phys, size);
+	map = acpi_map_lookup_phys(phys, size);
 	if (map) {
-		map->refcount++;
+		acpi_map_get(map);
 		goto out;
 	}
 

@@ -204,10 +210,10 @@ void __ref acpi_os_unmap_iomem(void __iomem *virt, acpi_size size)
 		WARN(true, PREFIX "%s: bad address %p\n", __func__, virt);
 		return;
 	}
-	acpi_os_drop_map_ref(map);
+	acpi_map_put(map);
 	mutex_unlock(&acpi_ioremap_lock);
 
-	acpi_os_map_cleanup(map);
+	acpi_map_cleanup(map);
 }
 EXPORT_SYMBOL_GPL(acpi_os_unmap_iomem);
 

@@ -265,15 +271,15 @@ void acpi_os_unmap_generic_address(struct acpi_generic_address *gas)
 		return;
 
 	mutex_lock(&acpi_ioremap_lock);
-	map = acpi_map_lookup(addr, gas->bit_width / 8);
+	map = acpi_map_lookup_phys(addr, gas->bit_width / 8);
 	if (!map) {
 		mutex_unlock(&acpi_ioremap_lock);
 		return;
 	}
-	acpi_os_drop_map_ref(map);
+	acpi_map_put(map);
 	mutex_unlock(&acpi_ioremap_lock);
 
-	acpi_os_map_cleanup(map);
+	acpi_map_cleanup(map);
 }
 EXPORT_SYMBOL(acpi_os_unmap_generic_address);
Signature · 1 line
1.7.10
author
#3 lore raw

PATCH 3/6 ACPI/OSL: Cleanup memory access functions by merging duplicate code.

In reply to Lv Zheng

This patch merges duplicate code to cleanup ACPI memory access implementations. No functional changes.

Signed-off-by Lv Zheng <[email protected]>
Tested-by Fei Yang <[email protected]>
---
 drivers/acpi/mem.c |   13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)
drivers/acpi/mem.c +2 -11
index defd317..ba74086 100644
@@ -260,7 +260,6 @@ EXPORT_SYMBOL(acpi_os_map_generic_address);
 void acpi_os_unmap_generic_address(struct acpi_generic_address *gas)
 {
 	u64 addr;
-	struct acpi_ioremap *map;
 
 	if (gas->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY)
 		return;

@@ -270,16 +269,8 @@ void acpi_os_unmap_generic_address(struct acpi_generic_address *gas)
 	if (!addr || !gas->bit_width)
 		return;
 
-	mutex_lock(&acpi_ioremap_lock);
-	map = acpi_map_lookup_phys(addr, gas->bit_width / 8);
-	if (!map) {
-		mutex_unlock(&acpi_ioremap_lock);
-		return;
-	}
-	acpi_map_put(map);
-	mutex_unlock(&acpi_ioremap_lock);
-
-	acpi_map_cleanup(map);
+	acpi_os_unmap_iomem((void __iomem *)((unsigned long)addr),
+			    gas->bit_width / 8);
 }
 EXPORT_SYMBOL(acpi_os_unmap_generic_address);
Signature · 1 line
1.7.10
author
#4 lore raw

PATCH 4/6 ACPI/OSL: Add acpi_map2virt() to merge duplicate code.

In reply to Lv Zheng

This patch adds acpi_map2virt() so that some duplicated code can be merged. No functional changes.

Signed-off-by Lv Zheng <[email protected]>
Tested-by Fei Yang <[email protected]>
---
 drivers/acpi/mem.c |   45 ++++++++++++++++++++-------------------------
 1 file changed, 20 insertions(+), 25 deletions(-)
drivers/acpi/mem.c +20 -25
index ba74086..c472237 100644
@@ -74,16 +74,10 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
 	return NULL;
 }
 
-static void __iomem *
-acpi_map_vaddr_lookup(acpi_physical_address phys, unsigned int size)
+static inline void __iomem *
+acpi_map2virt(struct acpi_ioremap *map, acpi_physical_address phys)
 {
-	struct acpi_ioremap *map;
-
-	map = acpi_map_lookup_phys(phys, size);
-	if (map)
-		return map->virt + (phys - map->phys);
-
-	return NULL;
+	return map ? map->virt + (phys - map->phys) : NULL;
 }
 
 #ifndef CONFIG_IA64

@@ -129,17 +123,14 @@ static void acpi_map_cleanup(struct acpi_ioremap *map)
 void __iomem *acpi_os_get_iomem(acpi_physical_address phys, unsigned int size)
 {
 	struct acpi_ioremap *map;
-	void __iomem *virt = NULL;
 
 	mutex_lock(&acpi_ioremap_lock);
 	map = acpi_map_lookup_phys(phys, size);
-	if (map) {
-		virt = map->virt + (phys - map->phys);
+	if (map)
 		acpi_map_get(map);
-	}
 	mutex_unlock(&acpi_ioremap_lock);
 
-	return virt;
+	return acpi_map2virt(map, phys);
 }
 EXPORT_SYMBOL_GPL(acpi_os_get_iomem);
 

@@ -168,18 +159,16 @@ acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
 	}
 
 	map = kzalloc(sizeof(*map), GFP_KERNEL);
-	if (!map) {
-		mutex_unlock(&acpi_ioremap_lock);
-		return NULL;
-	}
+	if (!map)
+		goto out;
 
 	pg_off = round_down(phys, PAGE_SIZE);
 	pg_sz = round_up(phys + size, PAGE_SIZE) - pg_off;
 	virt = acpi_map(pg_off, pg_sz);
 	if (!virt) {
-		mutex_unlock(&acpi_ioremap_lock);
 		kfree(map);
-		return NULL;
+		map = NULL;
+		goto out;
 	}
 
 	INIT_LIST_HEAD(&map->list);

@@ -190,7 +179,7 @@ acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
 	list_add_tail_rcu(&map->list, &acpi_ioremaps);
 out:
 	mutex_unlock(&acpi_ioremap_lock);
-	return map->virt + (phys - map->phys);
+	return acpi_map2virt(map, phys);
 }
 EXPORT_SYMBOL_GPL(acpi_os_map_iomem);
 

@@ -310,10 +299,13 @@ acpi_os_read_memory(acpi_physical_address phys_addr, u64 *value, u32 width)
 	unsigned int size = width / 8;
 	bool unmap = false;
 	u64 dummy;
+	struct acpi_ioremap *map;
 
 	rcu_read_lock();
-	virt_addr = acpi_map_vaddr_lookup(phys_addr, size);
-	if (!virt_addr) {
+	map = acpi_map_lookup_phys(phys_addr, size);
+	if (map)
+		virt_addr = acpi_map2virt(map, phys_addr);
+	else {
 		rcu_read_unlock();
 		virt_addr = acpi_os_ioremap(phys_addr, size);
 		if (!virt_addr)

@@ -355,10 +347,13 @@ acpi_os_write_memory(acpi_physical_address phys_addr, u64 value, u32 width)
 	void __iomem *virt_addr;
 	unsigned int size = width / 8;
 	bool unmap = false;
+	struct acpi_ioremap *map;
 
 	rcu_read_lock();
-	virt_addr = acpi_map_vaddr_lookup(phys_addr, size);
-	if (!virt_addr) {
+	map = acpi_map_lookup_phys(phys_addr, size);
+	if (map)
+		virt_addr = acpi_map2virt(map, phys_addr);
+	else {
 		rcu_read_unlock();
 		virt_addr = acpi_os_ioremap(phys_addr, size);
 		if (!virt_addr)
Signature · 1 line
1.7.10
author
#5 lore raw

PATCH 5/6 ACPI/OSL: Cleanup branch logics.

In reply to Lv Zheng

This patch cleans up trivial branch logics in acpi_os_unmap_iomem() to eliminate several lines. No functional changes.

Signed-off-by Lv Zheng <[email protected]>
Tested-by Fei Yang <[email protected]>
---
 drivers/acpi/mem.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
drivers/acpi/mem.c +3 -5
index c472237..c7c35f7 100644
@@ -194,12 +194,10 @@ void __ref acpi_os_unmap_iomem(void __iomem *virt, acpi_size size)
 
 	mutex_lock(&acpi_ioremap_lock);
 	map = acpi_map_lookup_virt(virt, size);
-	if (!map) {
-		mutex_unlock(&acpi_ioremap_lock);
+	if (map)
+		acpi_map_put(map);
+	else
 		WARN(true, PREFIX "%s: bad address %p\n", __func__, virt);
-		return;
-	}
-	acpi_map_put(map);
 	mutex_unlock(&acpi_ioremap_lock);
 
 	acpi_map_cleanup(map);
Signature · 1 line
1.7.10
author
#6 lore raw

PATCH 6/6 ACPI/OSL: Fix performance issue in system memory lockings.

In reply to Lv Zheng

It is reported that the synchronize_rcu() used in the hot path is not performance friendly:

 <6>[    3.998532] acpi_ut_update_object_reference: obj:ffff88003c305f78 type:10
 <6>[    4.006137] acpi_ut_update_ref_count:locked count:1 action:1 flags:286
 <6>[    4.013450] acpi_ut_delete_internal_obj: obj:ffff88003c305f78 flag:2
 <6>[    4.020569] acpi_ut_delete_internal_obj:second_desc handler_desc:ffff88003c0784c8 flags:1
 <6>[    4.029729] acpi_ut_delete_internal_obj: going to setup @ ffffffff81489299
 <6>[    4.037431] acpi_ev_system_memory_region_setup: addr:ffffc900000b0070, length:16
*<6>[    4.045716] acpi_os_unmap_memory: locked
*<6>[    4.050111] acpi_os_unmap_memory: found map
*<6>[    4.054798] acpi_os_unmap_memory: map dropped
*<6>[    4.059678] acpi_os_unmap_memory: unlocked
*<6>[  122.761255] acpi_os_map_cleanup: after synchronize_rcu
*<6>[  122.767027] acpi_os_unmap_memory: end
*<6>[  122.771134] After unmap
*<6>[  122.773877] After ACPI_FREE
 <6>[  122.777010] acpi_ut_delete_internal_obj: after setup
 <6>[  122.782576] acpi_ut_update_object_reference: obj:ffff88003c0784c8 type:24
 <6>[  122.790183] acpi_ut_update_ref_count:locked count:1c action:1 flags:282
 <6>[  122.797595] acpi_ut_delete_internal_obj: handler_desc removed
 <6>[  122.804034] acpi_ut_delete_internal_obj: second_desc deleted
 <6>[  122.810376] acpi_ut_delete_internal_obj: object deleted
 <6>[  122.816231] acpi_ns_detach_object: reference removed

Note: The "*" marked acpi_os_unmap_memory() instrumentation messages are
added by the reporter.

The patch converts RCU synchronization into the deferred executed work queue item to eliminate the consumed time from the hot path. By doing so, acpi_os_read_memory()/acpi_os_write_memory() need to hold the reference count and the RCU locks will then be unnecessary. The original acpi_ioremap_lock (renamed to acpi_map_lock) must be converted into spinlock to be used for both the IRQ and the task contexts. But it is still needed that the map operations are serialized because the spinlock cannot be held around ioremap(), so that the parallel map operations can wait for each other instead of inserting duplicated entries into the acpi_ioremaps. This is achieved by introducing another acpi_serial_map_mutex. Note that the unmap serialization is not required due to the reference count protected list removal, thus acpi_os_unmap_iomem() and acpi_map_relinquish() are not protected by the acpi_serial_map_mutex.
The test feedback from the reporter:
"The google reboot stability test passed with your 6 patches. Thanks for
the help."

Known issues:
1. It is better to use a seperate work queue other than the ACPICA dedicated GPE work queue to perform mapping relinquishment. Until a real case can be seen on bugzilla.kernel.org complaining this, no further improvements are done in this patch.

Signed-off-by Lv Zheng <[email protected]>
Reported-and-tested-by Fei Yang <[email protected]>
---
 drivers/acpi/mem.c |   87 ++++++++++++++++++++++++++++++++++------------------
 1 file changed, 58 insertions(+), 29 deletions(-)
drivers/acpi/mem.c +58 -29
index c7c35f7..b631007 100644
@@ -30,13 +30,13 @@ struct acpi_ioremap {
 	unsigned long refcount;
 };
 
+/* ioremap() serialization, no need to serialize iounmap() operations */
+static DEFINE_MUTEX(acpi_serial_map_mutex);
+
 static LIST_HEAD(acpi_ioremaps);
-static DEFINE_MUTEX(acpi_ioremap_lock);
+static DEFINE_SPINLOCK(acpi_map_lock);
 
-/*
- * The following functions must be called with 'acpi_ioremap_lock' or RCU
- * read lock held.
- */
+/* The following functions must be called with 'acpi_map_lock' held. */
 static inline void acpi_map_get(struct acpi_ioremap *map)
 {
 	map->refcount++;

@@ -45,7 +45,7 @@ static inline void acpi_map_get(struct acpi_ioremap *map)
 static inline void acpi_map_put(struct acpi_ioremap *map)
 {
 	if (!--map->refcount)
-		list_del_rcu(&map->list);
+		list_del(&map->list);
 }
 
 static struct acpi_ioremap *

@@ -53,7 +53,7 @@ acpi_map_lookup_phys(acpi_physical_address phys, acpi_size size)
 {
 	struct acpi_ioremap *map;
 
-	list_for_each_entry_rcu(map, &acpi_ioremaps, list)
+	list_for_each_entry(map, &acpi_ioremaps, list)
 		if (map->phys <= phys &&
 		    phys + size <= map->phys + map->size)
 			return map;

@@ -66,7 +66,7 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
 {
 	struct acpi_ioremap *map;
 
-	list_for_each_entry_rcu(map, &acpi_ioremaps, list)
+	list_for_each_entry(map, &acpi_ioremaps, list)
 		if (map->virt <= virt &&
 		    virt + size <= map->virt + map->size)
 			return map;

@@ -80,6 +80,7 @@ acpi_map2virt(struct acpi_ioremap *map, acpi_physical_address phys)
 	return map ? map->virt + (phys - map->phys) : NULL;
 }
 
+/* The following functions must be called without 'acpi_map_lock' held. */
 #ifndef CONFIG_IA64
 #define should_use_kmap(pfn)   page_is_ram(pfn)
 #else

@@ -113,22 +114,33 @@ static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
 
 static void acpi_map_cleanup(struct acpi_ioremap *map)
 {
-	if (!map->refcount) {
-		synchronize_rcu();
+	if (map && !map->refcount) {
 		acpi_unmap(map->phys, map->virt);
 		kfree(map);
 	}
 }
 
+static void acpi_map_relinquish(void *ctx)
+{
+	struct acpi_ioremap *map = ctx;
+	unsigned long flags;
+
+	spin_lock_irqsave(&acpi_map_lock, flags);
+	acpi_map_put(map);
+	spin_unlock_irqrestore(&acpi_map_lock, flags);
+	acpi_map_cleanup(map);
+}
+
 void __iomem *acpi_os_get_iomem(acpi_physical_address phys, unsigned int size)
 {
 	struct acpi_ioremap *map;
+	unsigned long flags;
 
-	mutex_lock(&acpi_ioremap_lock);
+	spin_lock_irqsave(&acpi_map_lock, flags);
 	map = acpi_map_lookup_phys(phys, size);
 	if (map)
 		acpi_map_get(map);
-	mutex_unlock(&acpi_ioremap_lock);
+	spin_unlock_irqrestore(&acpi_map_lock, flags);
 
 	return acpi_map2virt(map, phys);
 }

@@ -141,6 +153,7 @@ acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
 	void __iomem *virt;
 	acpi_physical_address pg_off;
 	acpi_size pg_sz;
+	unsigned long flags;
 
 	if (phys > ULONG_MAX) {
 		pr_err(PREFIX "Cannot map memory that high\n");

@@ -150,17 +163,20 @@ acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
 	if (!acpi_gbl_permanent_mmap)
 		return __acpi_map_table((unsigned long)phys, size);
 
-	mutex_lock(&acpi_ioremap_lock);
+	mutex_lock(&acpi_serial_map_mutex);
+
+	spin_lock_irqsave(&acpi_map_lock, flags);
 	/* Check if there's a suitable mapping already. */
 	map = acpi_map_lookup_phys(phys, size);
 	if (map) {
 		acpi_map_get(map);
 		goto out;
 	}
+	spin_unlock_irqrestore(&acpi_map_lock, flags);
 
 	map = kzalloc(sizeof(*map), GFP_KERNEL);
 	if (!map)
-		goto out;
+		goto err_exit;
 
 	pg_off = round_down(phys, PAGE_SIZE);
 	pg_sz = round_up(phys + size, PAGE_SIZE) - pg_off;

@@ -168,17 +184,21 @@ acpi_os_map_iomem(acpi_physical_address phys, acpi_size size)
 	if (!virt) {
 		kfree(map);
 		map = NULL;
-		goto out;
+		goto err_exit;
 	}
 
+	spin_lock_irqsave(&acpi_map_lock, flags);
 	INIT_LIST_HEAD(&map->list);
 	map->virt = virt;
 	map->phys = pg_off;
 	map->size = pg_sz;
 	map->refcount = 1;
-	list_add_tail_rcu(&map->list, &acpi_ioremaps);
+	list_add_tail(&map->list, &acpi_ioremaps);
 out:
-	mutex_unlock(&acpi_ioremap_lock);
+	spin_unlock_irqrestore(&acpi_map_lock, flags);
+
+err_exit:
+	mutex_unlock(&acpi_serial_map_mutex);
 	return acpi_map2virt(map, phys);
 }
 EXPORT_SYMBOL_GPL(acpi_os_map_iomem);

@@ -186,19 +206,20 @@ EXPORT_SYMBOL_GPL(acpi_os_map_iomem);
 void __ref acpi_os_unmap_iomem(void __iomem *virt, acpi_size size)
 {
 	struct acpi_ioremap *map;
+	unsigned long flags;
 
 	if (!acpi_gbl_permanent_mmap) {
 		__acpi_unmap_table(virt, size);
 		return;
 	}
 
-	mutex_lock(&acpi_ioremap_lock);
+	spin_lock_irqsave(&acpi_map_lock, flags);
 	map = acpi_map_lookup_virt(virt, size);
 	if (map)
 		acpi_map_put(map);
 	else
 		WARN(true, PREFIX "%s: bad address %p\n", __func__, virt);
-	mutex_unlock(&acpi_ioremap_lock);
+	spin_unlock_irqrestore(&acpi_map_lock, flags);
 
 	acpi_map_cleanup(map);
 }

@@ -298,13 +319,16 @@ acpi_os_read_memory(acpi_physical_address phys_addr, u64 *value, u32 width)
 	bool unmap = false;
 	u64 dummy;
 	struct acpi_ioremap *map;
+	unsigned long flags;
 
-	rcu_read_lock();
+	spin_lock_irqsave(&acpi_map_lock, flags);
 	map = acpi_map_lookup_phys(phys_addr, size);
-	if (map)
+	if (map) {
+		acpi_map_get(map);
+		spin_unlock_irqrestore(&acpi_map_lock, flags);
 		virt_addr = acpi_map2virt(map, phys_addr);
-	else {
-		rcu_read_unlock();
+	} else {
+		spin_unlock_irqrestore(&acpi_map_lock, flags);
 		virt_addr = acpi_os_ioremap(phys_addr, size);
 		if (!virt_addr)
 			return AE_BAD_ADDRESS;

@@ -334,7 +358,8 @@ acpi_os_read_memory(acpi_physical_address phys_addr, u64 *value, u32 width)
 	if (unmap)
 		iounmap(virt_addr);
 	else
-		rcu_read_unlock();
+		acpi_os_execute(OSL_GPE_HANDLER,
+				acpi_map_relinquish, map);
 
 	return AE_OK;
 }

@@ -346,13 +371,16 @@ acpi_os_write_memory(acpi_physical_address phys_addr, u64 value, u32 width)
 	unsigned int size = width / 8;
 	bool unmap = false;
 	struct acpi_ioremap *map;
+	unsigned long flags;
 
-	rcu_read_lock();
+	spin_lock_irqsave(&acpi_map_lock, flags);
 	map = acpi_map_lookup_phys(phys_addr, size);
-	if (map)
+	if (map) {
+		acpi_map_get(map);
+		spin_unlock_irqrestore(&acpi_map_lock, flags);
 		virt_addr = acpi_map2virt(map, phys_addr);
-	else {
-		rcu_read_unlock();
+	} else {
+		spin_unlock_irqrestore(&acpi_map_lock, flags);
 		virt_addr = acpi_os_ioremap(phys_addr, size);
 		if (!virt_addr)
 			return AE_BAD_ADDRESS;

@@ -379,7 +407,8 @@ acpi_os_write_memory(acpi_physical_address phys_addr, u64 value, u32 width)
 	if (unmap)
 		iounmap(virt_addr);
 	else
-		rcu_read_unlock();
+		acpi_os_execute(OSL_GPE_HANDLER,
+				acpi_map_relinquish, map);
 
 	return AE_OK;
 }
Signature · 1 line
1.7.10
author
#7 lore raw
In reply to Lv Zheng

There are reasons for why RCU is used here.
Using spinlock here will break specific acpi_os_read_memory()/acpi_os_write_memory() users. So this patchset is not correct, please ignore.

Thanks
-Lv

Quoted 58 lines

From: Zheng, Lv
Sent: Thursday, October 23, 2014 10:12 AM

It is reported that there is a performance issue in the ACPICA OSL implementation around memory mappings.

On the reported platforms, there is a debugging facility implemented in the ACPI namespace using circular logging buffer:

        Name (DPTR, 0x3AFEB000)
        Name (EPTR, 0x3AFFB000)
        Name (CPTR, 0x3AFEB010)
        Mutex (MMUT, 0x00)
        Method (MDBG, 1, Serialized)
        {
            Store (Acquire (MMUT, 0x03E8), Local0)
            If (Local0 == Zero)
            {
                OperationRegion (ABLK, SystemMemory, CPTR, 0x10)
                Field (ABLK, ByteAcc, NoLock, Preserve)
                {
                    AAAA,   128
                }
                Store (Arg0, AAAA) /* \MDBG.AAAA */
                CPTR = (CPTR + 0x10) /* \CPTR */
                If (CPTR >= EPTR)
                {
                    CPTR = (DPTR + 0x10) /* \CPTR */
                }
                Release (MMUT)
            }
            Return (Local0)
        }

This function is heavily invoked by other ACPI control methods. The reported platforms suffer from 2-5 minutes kernel stuck in the end of accessing the mapped circular buffer system memory region. By instrumentation, the 2-5 minutes time consumption overhead can be seen to happen on the synchronize_rcu() invoked in the acpi_os_unmap_memory().

This patchset removes the synchronize_rcu() from the hot path to improve the performance.

Lv Zheng (6):
ACPI/OSL: Split memory operation region implementations to a seperate
file.
ACPI/OSL: Rename system memory functions.
ACPI/OSL: Cleanup memory access functions by merging duplicate code. ACPI/OSL: Add acpi_map2virt() to merge duplicate code. ACPI/OSL: Cleanup branch logics.
ACPI/OSL: Fix performance issue in system memory lockings.

 drivers/acpi/Makefile |    2 +-
 drivers/acpi/mem.c    |  414 +++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/osl.c    |  381 ---------------------------------------------
 3 files changed, 415 insertions(+), 382 deletions(-)
 create mode 100644 drivers/acpi/mem.c

--
1.7.10