On Fri, Jun 12, 2020 at 2:12 AM Kaneda, Erik <[email protected]> wrote: > > > > > -----Original Message----- > > From: Rafael J. Wysocki <[email protected]> > > Sent: Wednesday, June 10, 2020 5:22 AM > > To: Williams, Dan J <[email protected]> > > Cc: Kaneda, Erik <[email protected]>; Wysocki, Rafael J > > <[email protected]>; Len Brown <[email protected]>; Borislav > > Petkov <[email protected]>; Weiny, Ira <[email protected]>; James Morse > > <[email protected]>; Myron Stowe <[email protected]>; > > Andy Shevchenko <[email protected]>; linux- > > [email protected]; [email protected]; linux- > > [email protected]; Moore, Robert <[email protected]> > > Subject: [RFT][PATCH 2/3] ACPICA: Remove unused memory mappings on > > interpreter exit > > > > From: "Rafael J. Wysocki" <[email protected]> > > > > For transient memory opregions that are created dynamically under > > the namespace and interpreter mutexes and go away quickly, there > > still is the problem that removing their memory mappings may take > > significant time and so doing that while holding the mutexes should > > be avoided. > > > > For example, unmapping a chunk of memory associated with a memory > > opregion in Linux involves running synchronize_rcu_expedited() > > which really should not be done with the namespace mutex held. > > > > To address that problem, notice that the unused memory mappings left > > behind by the "dynamic" opregions that went away need not be unmapped > > right away when the opregion is deactivated. Instead, they may be > > unmapped when exiting the interpreter, after the namespace and > > interpreter mutexes have been dropped (there's one more place dealing > > with opregions in the debug code that can be treated analogously). > > > > Accordingly, change acpi_ev_system_memory_region_setup() to put > > the unused mappings into a global list instead of unmapping them > > right away and add acpi_ev_system_release_memory_mappings() to > > be called when leaving the interpreter in order to unmap the > > unused memory mappings in the global list (which is protected > > by the namespace mutex). > > > > Signed-off-by: Rafael J. Wysocki <[email protected]> > > --- > > drivers/acpi/acpica/acevents.h | 2 ++ > > drivers/acpi/acpica/dbtest.c | 3 ++ > > drivers/acpi/acpica/evrgnini.c | 51 > > ++++++++++++++++++++++++++++++++-- > > drivers/acpi/acpica/exutils.c | 3 ++ > > drivers/acpi/acpica/utxface.c | 23 +++++++++++++++ > > include/acpi/acpixf.h | 1 + > > 6 files changed, 80 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/acpi/acpica/acevents.h b/drivers/acpi/acpica/acevents.h > > index 79f292687bd6..463eb9124765 100644 > > --- a/drivers/acpi/acpica/acevents.h > > +++ b/drivers/acpi/acpica/acevents.h > > @@ -197,6 +197,8 @@ acpi_ev_execute_reg_method(union > > acpi_operand_object *region_obj, u32 function); > > /* > > * evregini - Region initialization and setup > > */ > > +void acpi_ev_system_release_memory_mappings(void); > > + > > acpi_status > > acpi_ev_system_memory_region_setup(acpi_handle handle, > > u32 function, > > diff --git a/drivers/acpi/acpica/dbtest.c b/drivers/acpi/acpica/dbtest.c > > index 6db44a5ac786..7dac6dae5c48 100644 > > --- a/drivers/acpi/acpica/dbtest.c > > +++ b/drivers/acpi/acpica/dbtest.c > > @@ -8,6 +8,7 @@ > > #include <acpi/acpi.h> > > #include "accommon.h" > > #include "acdebug.h" > > +#include "acevents.h" > > #include "acnamesp.h" > > #include "acpredef.h" > > #include "acinterp.h" > > @@ -768,6 +769,8 @@ acpi_db_test_field_unit_type(union > > acpi_operand_object *obj_desc) > > acpi_ut_release_mutex(ACPI_MTX_NAMESPACE); > > acpi_ut_release_mutex(ACPI_MTX_INTERPRETER); > > > > + acpi_ev_system_release_memory_mappings(); > > + > > bit_length = obj_desc->common_field.bit_length; > > byte_length = > > ACPI_ROUND_BITS_UP_TO_BYTES(bit_length); > > > > diff --git a/drivers/acpi/acpica/evrgnini.c b/drivers/acpi/acpica/evrgnini.c > > index 48a5e6eaf9b9..946c4eef054d 100644 > > --- a/drivers/acpi/acpica/evrgnini.c > > +++ b/drivers/acpi/acpica/evrgnini.c > > @@ -16,6 +16,52 @@ > > #define _COMPONENT ACPI_EVENTS > > ACPI_MODULE_NAME("evrgnini") > > > > +#ifdef ACPI_OS_MAP_MEMORY_FAST_PATH > > +static struct acpi_mem_mapping *unused_memory_mappings; > > + > > +/********************************************************* > > ********************** > > + * > > + * FUNCTION: acpi_ev_system_release_memory_mappings > > + * > > + * PARAMETERS: None > > + * > > + * RETURN: None > > + * > > + * DESCRIPTION: Release all of the unused memory mappings in the queue > > + * under the interpreter mutex. > > + * > > + > > ********************************************************** > > ********************/ > > +void acpi_ev_system_release_memory_mappings(void) > > +{ > > + struct acpi_mem_mapping *mapping; > > + > > + > > ACPI_FUNCTION_TRACE(acpi_ev_system_release_memory_mappin > > gs); > > + > > + acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE); > > + > > + while (unused_memory_mappings) { > > + mapping = unused_memory_mappings; > > + unused_memory_mappings = mapping->next; > > + > > + acpi_ut_release_mutex(ACPI_MTX_NAMESPACE); > > + > > + acpi_os_unmap_memory(mapping->logical_address, > > mapping->length); > > acpi_os_unmap_memory calls synchronize_rcu_expedited(). I'm no RCU expert but > the > definition of this function states: > > * Although this is a great improvement over previous expedited > * implementations, it is still unfriendly to real-time workloads, so is > * thus not recommended for any sort of common-case code. In fact, if > * you are using synchronize_rcu_expedited() in a loop, please restructure > * your code to batch your updates, and then use a single synchronize_rcu() > * instead.
If this really ends up being a loop, the code without this patch will also call synchronize_rcu_expedited() in a loop, but indirectly and under the namespace and interpreter mutexes. While I agree that this is still somewhat suboptimal, improving this would require more changes in the OSL code. Cheers!
