[RFT,2/3] ACPICA: Remove unused memory mappings on interpreter exit

Commit Message

Rafael J. Wysocki June 10, 2020, 12:21 p.m. UTC

From: "Rafael J. Wysocki" <[e-mail]>

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 <[e-mail]>
---
 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(-)

Comments

Erik Kaneda June 12, 2020, 12:12 a.m. UTC | #1

> -----Original Message-----
> From: Rafael J. Wysocki <[e-mail]>
> Sent: Wednesday, June 10, 2020 5:22 AM
> To: Williams, Dan J <[e-mail]>
> Cc: Kaneda, Erik <[e-mail]>; Wysocki, Rafael J
> <[e-mail]>; Len Brown <[e-mail]>; Borislav
> Petkov <[e-mail]>; Weiny, Ira <[e-mail]>; James Morse
> <[e-mail]>; Myron Stowe <[e-mail]>;
> Andy Shevchenko <[e-mail]>; linux-
> [e-mail]; [e-mail]; linux-
> [e-mail]; Moore, Robert <[e-mail]>
> Subject: [RFT][PATCH 2/3] ACPICA: Remove unused memory mappings on
> interpreter exit
> 
> From: "Rafael J. Wysocki" <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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.


> +		ACPI_FREE(mapping);
> +
> +		acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE);
> +	}
> +
> +	acpi_ut_release_mutex(ACPI_MTX_NAMESPACE);
> +
> +	return_VOID;
> +}
> +#else /* !ACPI_OS_MAP_MEMORY_FAST_PATH */
> +void acpi_ev_system_release_memory_mappings(void)
> +{
> +	return_VOID;
> +}
> +#endif /* !ACPI_OS_MAP_MEMORY_FAST_PATH */
> +
> 
> /**********************************************************
> *********************
>   *
>   * FUNCTION:    acpi_ev_system_memory_region_setup
> @@ -60,9 +106,8 @@ acpi_ev_system_memory_region_setup(acpi_handle
> handle,
>  				while (local_region_context->first_mapping)
> {
>  					mapping = local_region_context-
> >first_mapping;
>  					local_region_context->first_mapping
> = mapping->next;
> -					acpi_os_unmap_memory(mapping-
> >logical_address,
> -							     mapping->length);
> -					ACPI_FREE(mapping);
> +					mapping->next =
> unused_memory_mappings;
> +					unused_memory_mappings =
> mapping;
>  				}
>  #endif
>  			}
> diff --git a/drivers/acpi/acpica/exutils.c b/drivers/acpi/acpica/exutils.c
> index 8fefa6feac2f..516d67664392 100644
> --- a/drivers/acpi/acpica/exutils.c
> +++ b/drivers/acpi/acpica/exutils.c
> @@ -25,6 +25,7 @@
> 
>  #include <acpi/acpi.h>
>  #include "accommon.h"
> +#include "acevents.h"
>  #include "acinterp.h"
>  #include "amlcode.h"
> 
> @@ -106,6 +107,8 @@ void acpi_ex_exit_interpreter(void)
>  			    "Could not release AML Interpreter mutex"));
>  	}
> 
> +	acpi_ev_system_release_memory_mappings();
> +
>  	return_VOID;
>  }
> 
> diff --git a/drivers/acpi/acpica/utxface.c b/drivers/acpi/acpica/utxface.c
> index ca7c9f0144ef..d972696be846 100644
> --- a/drivers/acpi/acpica/utxface.c
> +++ b/drivers/acpi/acpica/utxface.c
> @@ -11,6 +11,7 @@
> 
>  #include <acpi/acpi.h>
>  #include "accommon.h"
> +#include "acevents.h"
>  #include "acdebug.h"
> 
>  #define _COMPONENT          ACPI_UTILITIES
> @@ -244,6 +245,28 @@ acpi_status acpi_purge_cached_objects(void)
> 
>  ACPI_EXPORT_SYMBOL(acpi_purge_cached_objects)
> 
> +/*********************************************************
> ********************
> + *
> + * FUNCTION:    acpi_release_unused_memory_mappings
> + *
> + * PARAMETERS:  None
> + *
> + * RETURN:      None
> + *
> + * DESCRIPTION: Remove memory mappings that are not used any more.
> + *
> +
> **********************************************************
> ******************/
> +void acpi_release_unused_memory_mappings(void)
> +{
> +	ACPI_FUNCTION_TRACE(acpi_release_unused_memory_mappings);
> +
> +	acpi_ev_system_release_memory_mappings();
> +
> +	return_VOID;
> +}
> +
> +ACPI_EXPORT_SYMBOL(acpi_release_unused_memory_mappings)
> +
> 
> /**********************************************************
> *******************
>   *
>   * FUNCTION:    acpi_install_interface
> diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
> index 1dc8d262035b..8d2cc02257ed 100644
> --- a/include/acpi/acpixf.h
> +++ b/include/acpi/acpixf.h
> @@ -449,6 +449,7 @@ ACPI_EXTERNAL_RETURN_STATUS(acpi_status
>  						    acpi_size length,
>  						    struct acpi_pld_info
>  						    **return_buffer))
> +ACPI_EXTERNAL_RETURN_VOID(void
> acpi_release_unused_memory_mappings(void))
> 
>  /*
>   * ACPI table load/unload interfaces
> --
> 2.26.2
> 
> 
>

Rafael J. Wysocki June 12, 2020, 12:05 p.m. UTC | #2

On Fri, Jun 12, 2020 at 2:12 AM Kaneda, Erik <[e-mail]> wrote:
>
>
>
> > -----Original Message-----
> > From: Rafael J. Wysocki <[e-mail]>
> > Sent: Wednesday, June 10, 2020 5:22 AM
> > To: Williams, Dan J <[e-mail]>
> > Cc: Kaneda, Erik <[e-mail]>; Wysocki, Rafael J
> > <[e-mail]>; Len Brown <[e-mail]>; Borislav
> > Petkov <[e-mail]>; Weiny, Ira <[e-mail]>; James Morse
> > <[e-mail]>; Myron Stowe <[e-mail]>;
> > Andy Shevchenko <[e-mail]>; linux-
> > [e-mail]; [e-mail]; linux-
> > [e-mail]; Moore, Robert <[e-mail]>
> > Subject: [RFT][PATCH 2/3] ACPICA: Remove unused memory mappings on
> > interpreter exit
> >
> > From: "Rafael J. Wysocki" <[e-mail]>
> >
> > 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 <[e-mail]>
> > ---
> >  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!

Rafael J. Wysocki June 13, 2020, 7:28 p.m. UTC | #3

On Friday, June 12, 2020 2:05:01 PM CEST Rafael J. Wysocki wrote:
> On Fri, Jun 12, 2020 at 2:12 AM Kaneda, Erik <[e-mail]> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Rafael J. Wysocki <[e-mail]>
> > > Sent: Wednesday, June 10, 2020 5:22 AM
> > > To: Williams, Dan J <[e-mail]>
> > > Cc: Kaneda, Erik <[e-mail]>; Wysocki, Rafael J
> > > <[e-mail]>; Len Brown <[e-mail]>; Borislav
> > > Petkov <[e-mail]>; Weiny, Ira <[e-mail]>; James Morse
> > > <[e-mail]>; Myron Stowe <[e-mail]>;
> > > Andy Shevchenko <[e-mail]>; linux-
> > > [e-mail]; [e-mail]; linux-
> > > [e-mail]; Moore, Robert <[e-mail]>
> > > Subject: [RFT][PATCH 2/3] ACPICA: Remove unused memory mappings on
> > > interpreter exit
> > >
> > > From: "Rafael J. Wysocki" <[e-mail]>
> > >
> > > 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 <[e-mail]>
> > > ---
> > >  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.

After writing the above I started to think about the extra changes needed
to improve that and I realized that it would take making the OS layer
support deferred memory unmapping, such that the unused mappings would be
queued up for later removal and then released in one go at a suitable time.

However, that would be sufficient to address the issue addressed by this
series, because the deferred unmapping could be used in
acpi_ev_system_memory_region_setup() right away and that would be a much
simpler change than the one made in patch [1/3].

So I went ahead and implemented this and the result is there in the
acpica-osl branch in my tree, but it hasn't been built yet, so caveat
emptor.  Anyway, please feel free to have a look at it still.

Cheers!

Dan Williams June 15, 2020, 7:06 p.m. UTC | #4

On Sat, Jun 13, 2020 at 12:29 PM Rafael J. Wysocki <[e-mail]> wrote:
[,,]
> > While I agree that this is still somewhat suboptimal, improving this
> > would require more changes in the OSL code.
>
> After writing the above I started to think about the extra changes needed
> to improve that and I realized that it would take making the OS layer
> support deferred memory unmapping, such that the unused mappings would be
> queued up for later removal and then released in one go at a suitable time.
>
> However, that would be sufficient to address the issue addressed by this
> series, because the deferred unmapping could be used in
> acpi_ev_system_memory_region_setup() right away and that would be a much
> simpler change than the one made in patch [1/3].
>
> So I went ahead and implemented this and the result is there in the
> acpica-osl branch in my tree, but it hasn't been built yet, so caveat
> emptor.  Anyway, please feel free to have a look at it still.

I'll have a look. However, I was just about to build a test kernel for
the original reporter of this problem with this patch set. Do you want
test feedback on that branch, or this set as is?

Patch

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_mappings);
+
+	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_FREE(mapping);
+
+		acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE);
+	}
+
+	acpi_ut_release_mutex(ACPI_MTX_NAMESPACE);
+
+	return_VOID;
+}
+#else /* !ACPI_OS_MAP_MEMORY_FAST_PATH */
+void acpi_ev_system_release_memory_mappings(void)
+{
+	return_VOID;
+}
+#endif /* !ACPI_OS_MAP_MEMORY_FAST_PATH */
+
 /*******************************************************************************
  *
  * FUNCTION:    acpi_ev_system_memory_region_setup
@@ -60,9 +106,8 @@  acpi_ev_system_memory_region_setup(acpi_handle handle,
 				while (local_region_context->first_mapping) {
 					mapping = local_region_context->first_mapping;
 					local_region_context->first_mapping = mapping->next;
-					acpi_os_unmap_memory(mapping->logical_address,
-							     mapping->length);
-					ACPI_FREE(mapping);
+					mapping->next = unused_memory_mappings;
+					unused_memory_mappings = mapping;
 				}
 #endif
 			}
diff --git a/drivers/acpi/acpica/exutils.c b/drivers/acpi/acpica/exutils.c
index 8fefa6feac2f..516d67664392 100644
--- a/drivers/acpi/acpica/exutils.c
+++ b/drivers/acpi/acpica/exutils.c
@@ -25,6 +25,7 @@ 
 
 #include <acpi/acpi.h>
 #include "accommon.h"
+#include "acevents.h"
 #include "acinterp.h"
 #include "amlcode.h"
 
@@ -106,6 +107,8 @@  void acpi_ex_exit_interpreter(void)
 			    "Could not release AML Interpreter mutex"));
 	}
 
+	acpi_ev_system_release_memory_mappings();
+
 	return_VOID;
 }
 
diff --git a/drivers/acpi/acpica/utxface.c b/drivers/acpi/acpica/utxface.c
index ca7c9f0144ef..d972696be846 100644
--- a/drivers/acpi/acpica/utxface.c
+++ b/drivers/acpi/acpica/utxface.c
@@ -11,6 +11,7 @@ 
 
 #include <acpi/acpi.h>
 #include "accommon.h"
+#include "acevents.h"
 #include "acdebug.h"
 
 #define _COMPONENT          ACPI_UTILITIES
@@ -244,6 +245,28 @@  acpi_status acpi_purge_cached_objects(void)
 
 ACPI_EXPORT_SYMBOL(acpi_purge_cached_objects)
 
+/*****************************************************************************
+ *
+ * FUNCTION:    acpi_release_unused_memory_mappings
+ *
+ * PARAMETERS:  None
+ *
+ * RETURN:      None
+ *
+ * DESCRIPTION: Remove memory mappings that are not used any more.
+ *
+ ****************************************************************************/
+void acpi_release_unused_memory_mappings(void)
+{
+	ACPI_FUNCTION_TRACE(acpi_release_unused_memory_mappings);
+
+	acpi_ev_system_release_memory_mappings();
+
+	return_VOID;
+}
+
+ACPI_EXPORT_SYMBOL(acpi_release_unused_memory_mappings)
+
 /*****************************************************************************
  *
  * FUNCTION:    acpi_install_interface
diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
index 1dc8d262035b..8d2cc02257ed 100644
--- a/include/acpi/acpixf.h
+++ b/include/acpi/acpixf.h
@@ -449,6 +449,7 @@  ACPI_EXTERNAL_RETURN_STATUS(acpi_status
 						    acpi_size length,
 						    struct acpi_pld_info
 						    **return_buffer))
+ACPI_EXTERNAL_RETURN_VOID(void acpi_release_unused_memory_mappings(void))
 
 /*
  * ACPI table load/unload interfaces