From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 056B0C433DF for ; Fri, 12 Jun 2020 12:05:21 +0000 (UTC) Received: from ml01.01.org (ml01.01.org [198.145.21.10]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id D95D120801 for ; Fri, 12 Jun 2020 12:05:20 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D95D120801 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-nvdimm-bounces@lists.01.org Received: from ml01.vlan13.01.org (localhost [IPv6:::1]) by ml01.01.org (Postfix) with ESMTP id AB7521007A3DB; Fri, 12 Jun 2020 05:05:20 -0700 (PDT) Received-SPF: Pass (mailfrom) identity=mailfrom; client-ip=209.85.167.194; helo=mail-oi1-f194.google.com; envelope-from=rjwysocki@gmail.com; receiver= Received: from mail-oi1-f194.google.com (mail-oi1-f194.google.com [209.85.167.194]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 3A9DA1007A829 for ; Fri, 12 Jun 2020 05:05:18 -0700 (PDT) Received: by mail-oi1-f194.google.com with SMTP id x202so8411749oix.11 for ; Fri, 12 Jun 2020 05:05:18 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=vFwhiYEH8oFfrk8TNiw5uMiuVfFehhn4UASR2l6vNwU=; b=VM9MrB3Nob1DPttEU4bfsF4jxj1FWcyTfpUJSK0qsogfxaDrq8ok5d9hKv7aWeZVtW QUbYXAgBC5icXRjf76iCjDl/DHdyipLd/dlSmu/6fGbNCeATAdtVr6APta3LB22AaGwB CmsxjzrIpeqqQJWVaJZzpG7zY3eoAhQ5EJzvO5ZPUnd/LCfVkyXuQPpPwdqwRwvRTTLT PJizmDqMdqKK8frSGxykqAyd6kS3NQRtKGwNtG0fgKGJmFeJN6Yz4Smtz4Uf609TupRi Ed++rNDE4WA7Oe8DTmcSd6jg6094uMUB3qfpNRbmSENRAG0JeOimrOtiI2KaYlBkkbf+ C07g== X-Gm-Message-State: AOAM530x6A1iYBY0WXFA5y5Gobf5eHQBojII6+HZMrAqLnw+iNXnkVAO qMgLh1eREny4g/klOUSnurbRn0amtYZOhTdyIPc= X-Google-Smtp-Source: ABdhPJzzratWFEFnCI62Qa2Ya16bGmtMS72/S8i52Q1nskzw+ZVCcqsmcmX3yUortj+kqoj/uHbWSu+vHMfgewzhpGs= X-Received: by 2002:aca:ab92:: with SMTP id u140mr1819440oie.68.1591963516898; Fri, 12 Jun 2020 05:05:16 -0700 (PDT) MIME-Version: 1.0 References: <158889473309.2292982.18007035454673387731.stgit@dwillia2-desk3.amr.corp.intel.com> <318372766.6LKUBsbRXE@kreacher> <3974162.pZLctmZ5Iv@kreacher> In-Reply-To: From: "Rafael J. Wysocki" Date: Fri, 12 Jun 2020 14:05:01 +0200 Message-ID: Subject: Re: [RFT][PATCH 2/3] ACPICA: Remove unused memory mappings on interpreter exit To: "Kaneda, Erik" Message-ID-Hash: 4TDPDVRTOYBWQA4Z66MJKZHI76N6G2Z2 X-Message-ID-Hash: 4TDPDVRTOYBWQA4Z66MJKZHI76N6G2Z2 X-MailFrom: rjwysocki@gmail.com X-Mailman-Rule-Hits: nonmember-moderation X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation CC: "Rafael J. Wysocki" , "Wysocki, Rafael J" , Len Brown , Borislav Petkov , James Morse , Myron Stowe , Andy Shevchenko , "linux-kernel@vger.kernel.org" , "linux-acpi@vger.kernel.org" , "linux-nvdimm@lists.01.org" , "Moore, Robert" X-Mailman-Version: 3.1.1 Precedence: list List-Id: "Linux-nvdimm developer list." Archived-At: List-Archive: List-Help: List-Post: List-Subscribe: List-Unsubscribe: Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit On Fri, Jun 12, 2020 at 2:12 AM Kaneda, Erik wrote: > > > > > -----Original Message----- > > From: Rafael J. Wysocki > > Sent: Wednesday, June 10, 2020 5:22 AM > > To: Williams, Dan J > > Cc: Kaneda, Erik ; Wysocki, Rafael J > > ; Len Brown ; Borislav > > Petkov ; Weiny, Ira ; James Morse > > ; Myron Stowe ; > > Andy Shevchenko ; linux- > > kernel@vger.kernel.org; linux-acpi@vger.kernel.org; linux- > > nvdimm@lists.01.org; Moore, Robert > > Subject: [RFT][PATCH 2/3] ACPICA: Remove unused memory mappings on > > interpreter exit > > > > From: "Rafael J. Wysocki" > > > > 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 > > --- > > 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 > > #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! _______________________________________________ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-leave@lists.01.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.1 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 424F7C433E0 for ; Fri, 12 Jun 2020 12:05:20 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 1785420792 for ; Fri, 12 Jun 2020 12:05:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1591963520; bh=i7FPVGv7sjfZ05AaWsOh5GrgB9VghseMEJeKu9iOL/g=; h=References:In-Reply-To:From:Date:Subject:To:Cc:List-ID:From; b=lQA7AiN1pujb0FTJRa8W5S3TXckPmtOOdlaDElXWxsujJ2O/N3qx9NuoajRwpDHSZ zmMhSGxQ0GgIJpGn35qiW7unTpBlKvohZFXp7JTsL3zUJcHC2al0upmwyphSlpAg5M ZgxTjLgbbmSJzJ0Vbbv16QrGdsQfTPZ6Hd3LQe3Q= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726053AbgFLMFT (ORCPT ); Fri, 12 Jun 2020 08:05:19 -0400 Received: from mail-oi1-f193.google.com ([209.85.167.193]:37380 "EHLO mail-oi1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725886AbgFLMFT (ORCPT ); Fri, 12 Jun 2020 08:05:19 -0400 Received: by mail-oi1-f193.google.com with SMTP id a3so8447355oid.4; Fri, 12 Jun 2020 05:05:17 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=vFwhiYEH8oFfrk8TNiw5uMiuVfFehhn4UASR2l6vNwU=; b=rpXwuNrIUBEsysoJlKwBAzay84x/N815Qgp7/4UpAemlXPmp6bCxcckcMlVeBwowyc fYr21lgigZZt9Rkr/Hy4P12NnOm3SL2VaerWP0c+C9LkHOO73YWBXKG+fo476Svtbem1 ko+XxWYHnvz4irQigN7fROn2XDq/ULd4GvAcq+ZHVfdRH8SEUMsjjvefGqNoqdd+L65c iAoMdu2OzoTvk064C3l6g1WBFWKWtQFL80xG6KbB6hVoe04MnztMGTw29t1v/UFBhD7S J1akxtEzqG0MWGHIj5FMDUsdFqcxHpeRfID/PQ0ga1iasjiOtMl6UuQDEAgzWFz8YJcA Fvbg== X-Gm-Message-State: AOAM532fMqdUKBmNczIkIUP+1VDRD4I7vptMHFir1C4WVN16GioL/OGL aQtXwEGgnTUppMTBk1gRGIPW3YpmTarwu2oCdzI= X-Google-Smtp-Source: ABdhPJzzratWFEFnCI62Qa2Ya16bGmtMS72/S8i52Q1nskzw+ZVCcqsmcmX3yUortj+kqoj/uHbWSu+vHMfgewzhpGs= X-Received: by 2002:aca:ab92:: with SMTP id u140mr1819440oie.68.1591963516898; Fri, 12 Jun 2020 05:05:16 -0700 (PDT) MIME-Version: 1.0 References: <158889473309.2292982.18007035454673387731.stgit@dwillia2-desk3.amr.corp.intel.com> <318372766.6LKUBsbRXE@kreacher> <3974162.pZLctmZ5Iv@kreacher> In-Reply-To: From: "Rafael J. Wysocki" Date: Fri, 12 Jun 2020 14:05:01 +0200 Message-ID: Subject: Re: [RFT][PATCH 2/3] ACPICA: Remove unused memory mappings on interpreter exit To: "Kaneda, Erik" Cc: "Rafael J. Wysocki" , "Williams, Dan J" , "Wysocki, Rafael J" , Len Brown , Borislav Petkov , "Weiny, Ira" , James Morse , Myron Stowe , Andy Shevchenko , "linux-kernel@vger.kernel.org" , "linux-acpi@vger.kernel.org" , "linux-nvdimm@lists.01.org" , "Moore, Robert" Content-Type: text/plain; charset="UTF-8" Sender: linux-acpi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org On Fri, Jun 12, 2020 at 2:12 AM Kaneda, Erik wrote: > > > > > -----Original Message----- > > From: Rafael J. Wysocki > > Sent: Wednesday, June 10, 2020 5:22 AM > > To: Williams, Dan J > > Cc: Kaneda, Erik ; Wysocki, Rafael J > > ; Len Brown ; Borislav > > Petkov ; Weiny, Ira ; James Morse > > ; Myron Stowe ; > > Andy Shevchenko ; linux- > > kernel@vger.kernel.org; linux-acpi@vger.kernel.org; linux- > > nvdimm@lists.01.org; Moore, Robert > > Subject: [RFT][PATCH 2/3] ACPICA: Remove unused memory mappings on > > interpreter exit > > > > From: "Rafael J. Wysocki" > > > > 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 > > --- > > 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 > > #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!