Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
From: Al Stone <ahs3@redhat.com>
To: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Dan Williams <dan.j.williams@intel.com>,
	Erik Kaneda <erik.kaneda@intel.com>,
	Rafael Wysocki <rafael.j.wysocki@intel.com>,
	Len Brown <lenb@kernel.org>, Borislav Petkov <bp@alien8.de>,
	Ira Weiny <ira.weiny@intel.com>,
	James Morse <james.morse@arm.com>,
	Myron Stowe <myron.stowe@redhat.com>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
	"linux-nvdimm@lists.01.org" <linux-nvdimm@lists.01.org>,
	Bob Moore <robert.moore@intel.com>
Subject: Re: [PATCH v4 2/2] ACPICA: Preserve memory opregion mappings
Date: Tue, 30 Jun 2020 09:31:27 -0600
Message-ID: <20200630153127.GP1237914@redhat.com> (raw)
In-Reply-To: <CAJZ5v0hiAVfgWTLcP2N5PWLsqL7mpHbuL1_de79svYYhd3R57A@mail.gmail.com>

On 30 Jun 2020 13:44, Rafael J. Wysocki wrote:
> On Mon, Jun 29, 2020 at 10:57 PM Al Stone <ahs3@redhat.com> wrote:
> >
> > On 29 Jun 2020 18:33, Rafael J. Wysocki wrote:
> > > From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> > >
> > > The ACPICA's strategy with respect to the handling of memory mappings
> > > associated with memory operation regions is to avoid mapping the
> > > entire region at once which may be problematic at least in principle
> > > (for example, it may lead to conflicts with overlapping mappings
> > > having different attributes created by drivers).  It may also be
> > > wasteful, because memory opregions on some systems take up vast
> > > chunks of address space while the fields in those regions actually
> > > accessed by AML are sparsely distributed.
> > >
> > > For this reason, a one-page "window" is mapped for a given opregion
> > > on the first memory access through it and if that "window" does not
> > > cover an address range accessed through that opregion subsequently,
> > > it is unmapped and a new "window" is mapped to replace it.  Next,
> > > if the new "window" is not sufficient to acess memory through the
> > > opregion in question in the future, it will be replaced with yet
> > > another "window" and so on.  That may lead to a suboptimal sequence
> > > of memory mapping and unmapping operations, for example if two fields
> > > in one opregion separated from each other by a sufficiently wide
> > > chunk of unused address space are accessed in an alternating pattern.
> > >
> > > The situation may still be suboptimal if the deferred unmapping
> > > introduced previously is supported by the OS layer.  For instance,
> > > the alternating memory access pattern mentioned above may produce
> > > a relatively long list of mappings to release with substantial
> > > duplication among the entries in it, which could be avoided if
> > > acpi_ex_system_memory_space_handler() did not release the mapping
> > > used by it previously as soon as the current access was not covered
> > > by it.
> > >
> > > In order to improve that, modify acpi_ex_system_memory_space_handler()
> > > to preserve all of the memory mappings created by it until the memory
> > > regions associated with them go away.
> > >
> > > Accordingly, update acpi_ev_system_memory_region_setup() to unmap all
> > > memory associated with memory opregions that go away.
> > >
> > > Reported-by: Dan Williams <dan.j.williams@intel.com>
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >  drivers/acpi/acpica/evrgnini.c | 14 ++++----
> > >  drivers/acpi/acpica/exregion.c | 65 ++++++++++++++++++++++++----------
> > >  include/acpi/actypes.h         | 12 +++++--
> > >  3 files changed, 64 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/drivers/acpi/acpica/evrgnini.c b/drivers/acpi/acpica/evrgnini.c
> > > index aefc0145e583..89be3ccdad53 100644
> > > --- a/drivers/acpi/acpica/evrgnini.c
> > > +++ b/drivers/acpi/acpica/evrgnini.c
> > > @@ -38,6 +38,7 @@ acpi_ev_system_memory_region_setup(acpi_handle handle,
> > >       union acpi_operand_object *region_desc =
> > >           (union acpi_operand_object *)handle;
> > >       struct acpi_mem_space_context *local_region_context;
> > > +     struct acpi_mem_mapping *mm;
> > >
> > >       ACPI_FUNCTION_TRACE(ev_system_memory_region_setup);
> > >
> > > @@ -46,13 +47,14 @@ acpi_ev_system_memory_region_setup(acpi_handle handle,
> > >                       local_region_context =
> > >                           (struct acpi_mem_space_context *)*region_context;
> > >
> > > -                     /* Delete a cached mapping if present */
> > > +                     /* Delete memory mappings if present */
> > >
> > > -                     if (local_region_context->mapped_length) {
> > > -                             acpi_os_unmap_memory(local_region_context->
> > > -                                                  mapped_logical_address,
> > > -                                                  local_region_context->
> > > -                                                  mapped_length);
> > > +                     while (local_region_context->first_mm) {
> > > +                             mm = local_region_context->first_mm;
> > > +                             local_region_context->first_mm = mm->next_mm;
> > > +                             acpi_os_unmap_memory(mm->logical_address,
> > > +                                                  mm->length);
> > > +                             ACPI_FREE(mm);
> > >                       }
> > >                       ACPI_FREE(local_region_context);
> > >                       *region_context = NULL;
> > > diff --git a/drivers/acpi/acpica/exregion.c b/drivers/acpi/acpica/exregion.c
> > > index d15a66de26c0..fd68f2134804 100644
> > > --- a/drivers/acpi/acpica/exregion.c
> > > +++ b/drivers/acpi/acpica/exregion.c
> > > @@ -41,6 +41,7 @@ acpi_ex_system_memory_space_handler(u32 function,
> > >       acpi_status status = AE_OK;
> > >       void *logical_addr_ptr = NULL;
> > >       struct acpi_mem_space_context *mem_info = region_context;
> > > +     struct acpi_mem_mapping *mm = mem_info->cur_mm;
> > >       u32 length;
> > >       acpi_size map_length;
> >
> > I think this needs to be:
> >
> >         acpi_size map_length = mem_info->length;
> >
> > since it now gets used in the ACPI_ERROR() call below.
> 
> No, it's better to print the length value in the message.

Yeah, that was the other option.

> >  I'm getting a "maybe used unitialized" error on compilation.
> 
> Thanks for reporting!
> 
> I've updated the commit in the acpica-osl branch with the fix.

Thanks, Rafael.

Do you have a generic way of testing this?  I can see a way to do it
-- timing a call of a method in a dynamically loaded SSDT -- but if
you had a test case laying around, I could continue to be lazy :).

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------


  reply index

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-07 23:39 [PATCH v2] ACPI: Drop rcu usage for MMIO mappings Dan Williams
2020-06-05 13:32 ` Rafael J. Wysocki
2020-06-05 16:18   ` Dan Williams
2020-06-05 16:21     ` Rafael J. Wysocki
2020-06-05 16:39       ` Dan Williams
2020-06-05 17:02         ` Rafael J. Wysocki
2020-06-05 14:06 ` [RFT][PATCH] ACPI: OSL: Use rwlock instead of RCU for memory management Rafael J. Wysocki
2020-06-05 17:08   ` Dan Williams
2020-06-06  6:56     ` Rafael J. Wysocki
2020-06-08 15:33       ` Rafael J. Wysocki
2020-06-08 16:29         ` Rafael J. Wysocki
2020-06-05 19:40   ` Andy Shevchenko
2020-06-06  6:48     ` Rafael J. Wysocki
2020-06-10 12:17 ` [RFT][PATCH 0/3] ACPI: ACPICA / OSL: Avoid unmapping ACPI memory inside of the AML interpreter Rafael J. Wysocki
2020-06-10 12:20   ` [RFT][PATCH 1/3] ACPICA: Defer unmapping of memory used in memory opregions Rafael J. Wysocki
2020-06-10 12:21   ` [RFT][PATCH 2/3] ACPICA: Remove unused memory mappings on interpreter exit Rafael J. Wysocki
2020-06-12  0:12     ` Kaneda, Erik
2020-06-12 12:05       ` Rafael J. Wysocki
2020-06-13 19:28         ` Rafael J. Wysocki
2020-06-15 19:06           ` Dan Williams
2020-06-10 12:22   ` [RFT][PATCH 3/3] ACPI: OSL: Define ACPI_OS_MAP_MEMORY_FAST_PATH() Rafael J. Wysocki
2020-06-13 19:19   ` [RFT][PATCH 0/3] ACPI: ACPICA / OSL: Avoid unmapping ACPI memory inside of the AML interpreter Rafael J. Wysocki
2020-06-22 13:50 ` [RFT][PATCH v2 0/4] " Rafael J. Wysocki
2020-06-22 13:52   ` [RFT][PATCH v2 1/4] ACPICA: Defer unmapping of opregion memory if supported by OS Rafael J. Wysocki
2020-06-22 13:53   ` [RFT][PATCH v2 2/4] ACPI: OSL: Add support for deferred unmapping of ACPI memory Rafael J. Wysocki
2020-06-22 14:56     ` Andy Shevchenko
2020-06-22 15:27       ` Rafael J. Wysocki
2020-06-22 15:46         ` Andy Shevchenko
2020-06-22 14:01   ` [RFT][PATCH v2 3/4] ACPICA: Preserve memory opregion mappings if supported by OS Rafael J. Wysocki
2020-06-26 22:53     ` Kaneda, Erik
2020-06-29 13:02       ` Rafael J. Wysocki
2020-06-22 14:02   ` [RFT][PATCH v2 4/4] ACPI: OSL: Implement acpi_os_map_memory_fast_path() Rafael J. Wysocki
2020-06-26 17:28   ` [RFT][PATCH v3 0/4] ACPI: ACPICA / OSL: Avoid unmapping ACPI memory inside of the AML interpreter Rafael J. Wysocki
2020-06-26 17:31     ` [RFT][PATCH v3 1/4] ACPICA: Take deferred unmapping of memory into account Rafael J. Wysocki
2020-06-26 17:31     ` [RFT][PATCH v3 2/4] ACPI: OSL: Implement deferred unmapping of ACPI memory Rafael J. Wysocki
2020-06-26 17:32     ` [RFT][PATCH v3 3/4] ACPICA: Preserve memory opregion mappings if supported by OS Rafael J. Wysocki
2020-06-26 17:33     ` [RFT][PATCH v3 4/4] ACPI: OSL: Implement acpi_os_map_memory_fast_path() Rafael J. Wysocki
2020-06-26 18:41     ` [RFT][PATCH v3 0/4] ACPI: ACPICA / OSL: Avoid unmapping ACPI memory inside of the AML interpreter Dan Williams
2020-06-28 17:09       ` Rafael J. Wysocki
2020-06-29 20:46         ` Dan Williams
2020-06-30 11:04           ` Rafael J. Wysocki
2020-06-29 16:31     ` [PATCH v4 0/2] " Rafael J. Wysocki
2020-06-29 16:33       ` [PATCH v4 1/2] ACPI: OSL: Implement deferred unmapping of ACPI memory Rafael J. Wysocki
2020-06-29 16:33       ` [PATCH v4 2/2] ACPICA: Preserve memory opregion mappings Rafael J. Wysocki
2020-06-29 20:57         ` Al Stone
2020-06-30 11:44           ` Rafael J. Wysocki
2020-06-30 15:31             ` Al Stone [this message]
2020-06-30 15:52               ` Rafael J. Wysocki
2020-06-30 19:57                 ` Al Stone

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200630153127.GP1237914@redhat.com \
    --to=ahs3@redhat.com \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=bp@alien8.de \
    --cc=dan.j.williams@intel.com \
    --cc=erik.kaneda@intel.com \
    --cc=ira.weiny@intel.com \
    --cc=james.morse@arm.com \
    --cc=lenb@kernel.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nvdimm@lists.01.org \
    --cc=myron.stowe@redhat.com \
    --cc=rafael.j.wysocki@intel.com \
    --cc=rafael@kernel.org \
    --cc=rjw@rjwysocki.net \
    --cc=robert.moore@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ACPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-acpi/0 linux-acpi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-acpi linux-acpi/ https://lore.kernel.org/linux-acpi \
		linux-acpi@vger.kernel.org
	public-inbox-index linux-acpi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-acpi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git