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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0C2D5C433F5 for ; Fri, 20 May 2022 15:40:18 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8F9666B0071; Fri, 20 May 2022 11:40:17 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 881876B0072; Fri, 20 May 2022 11:40:17 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6F9E16B0073; Fri, 20 May 2022 11:40:17 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 5A79E6B0071 for ; Fri, 20 May 2022 11:40:17 -0400 (EDT) Received: from smtpin09.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 1E9CA21958 for ; Fri, 20 May 2022 15:40:17 +0000 (UTC) X-FDA: 79486532874.09.9BDEE2E Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by imf10.hostedemail.com (Postfix) with ESMTP id A0E0EC00AE for ; Fri, 20 May 2022 15:39:42 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1653061214; x=1684597214; h=message-id:subject:from:to:cc:date:in-reply-to: references:mime-version:content-transfer-encoding; bh=zZJc+cwhsja9y7lERlCJUaB0/lAi8V2cU4FnxtzqUfk=; b=goL2nMrnuC/txpPd0whZOcUFf0851ttwZW79/NlB8FdyDoWRz01CrVwO c35spzpCfixZx58jadzEVDXPfJjwPM/8nWvHDsfuvcGOMH71VzQSC/GjF sqlbDUpYAAj36LtGxp7VNCEQmf2h+D4Ek4KYOd8TIuTofVSjNxSYslekS m5M5ndnWxvOF4LhRTxwikpxUoa/Vm8pCFMpos+RPkUAq6Y1v1b25OQ2bV oodTD76tQTfBQsvcd6l6woBBr53hRMMOzGML0V/ctx9HEhMD7ONFU45Hv 77jUPhzMnpuMGXzW5xrxgHBdyH30tsjerezugYWaRV84pV7jccrklXscu w==; X-IronPort-AV: E=McAfee;i="6400,9594,10353"; a="260225555" X-IronPort-AV: E=Sophos;i="5.91,239,1647327600"; d="scan'208";a="260225555" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 May 2022 08:40:05 -0700 X-IronPort-AV: E=Sophos;i="5.91,239,1647327600"; d="scan'208";a="607066921" Received: from kcaccard-mobl.amr.corp.intel.com (HELO kcaccard-mobl1.jf.intel.com) ([10.209.83.65]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 May 2022 08:40:04 -0700 Message-ID: <5c12487f71ff84c795730575bc083f88c668ba4c.camel@linux.intel.com> Subject: Re: [PATCH v2] x86/sgx: Set active memcg prior to shmem allocation From: Kristen Carlson Accardi To: Roman Gushchin Cc: linux-sgx@vger.kernel.org, Jarkko Sakkinen , Dave Hansen , Thomas Gleixner , Ingo Molnar , Borislav Petkov , x86@kernel.org, "H. Peter Anvin" , stable@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, mhocko@suse.com, hannes@cmpxchg.org, shakeelb@google.com Date: Fri, 20 May 2022 08:40:03 -0700 In-Reply-To: References: <20220519210445.5310-1-kristen@linux.intel.com> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.30.5 (3.30.5-1.fc29) MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: A0E0EC00AE X-Stat-Signature: 1gqwmezeecjybs85zqrrcepz6wgwjtm4 Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=goL2nMrn; dmarc=pass (policy=none) header.from=intel.com; spf=none (imf10.hostedemail.com: domain of kristen@linux.intel.com has no SPF policy when checking 134.134.136.20) smtp.mailfrom=kristen@linux.intel.com X-Rspam-User: X-HE-Tag: 1653061182-234112 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu, 2022-05-19 at 18:24 -0700, Roman Gushchin wrote: > On Thu, May 19, 2022 at 02:04:45PM -0700, Kristen Carlson Accardi > wrote: > > When the system runs out of enclave memory, SGX can reclaim EPC > > pages > > by swapping to normal RAM. These backing pages are allocated via a > > per-enclave shared memory area. Since SGX allows unlimited over > > commit on EPC memory, the reclaimer thread can allocate a large > > number of backing RAM pages in response to EPC memory pressure. > > > > When the shared memory backing RAM allocation occurs during > > the reclaimer thread context, the shared memory is charged to > > the root memory control group, and the shmem usage of the enclave > > is not properly accounted for, making cgroups ineffective at > > limiting the amount of RAM an enclave can consume. > > > > For example, when using a cgroup to launch a set of test > > enclaves, the kernel does not properly account for 50% - 75% of > > shmem page allocations on average. In the worst case, when > > nearly all allocations occur during the reclaimer thread, the > > kernel accounts less than a percent of the amount of shmem used > > by the enclave's cgroup to the correct cgroup. > > > > SGX stores a list of mm_structs that are associated with > > an enclave. Pick one of them during reclaim and charge that > > mm's memcg with the shmem allocation. The one that gets picked > > is arbitrary, but this list almost always only has one mm. The > > cases where there is more than one mm with different memcg's > > are not worth considering. > > > > Create a new function - sgx_encl_alloc_backing(). This function > > is used whenever a new backing storage page needs to be > > allocated. Previously the same function was used for page > > allocation as well as retrieving a previously allocated page. > > Prior to backing page allocation, if there is a mm_struct > > associated > > with the enclave that is requesting the allocation, it is set > > as the active memory control group. > > > > Signed-off-by: Kristen Carlson Accardi > > --- > > V1 -> V2: > > Changed sgx_encl_set_active_memcg() to simply return the correct > > memcg for the enclave and renamed to sgx_encl_get_mem_cgroup(). > > > > Created helper function current_is_ksgxd() to improve readability. > > > > Use mmget_not_zero()/mmput_async() when searching mm_list. > > > > Move call to set_active_memcg() to sgx_encl_alloc_backing() and > > use mem_cgroup_put() to avoid leaking a memcg reference. > > > > Address review feedback regarding comments and commit log. > > > > arch/x86/kernel/cpu/sgx/encl.c | 109 > > ++++++++++++++++++++++++++++++++- > > arch/x86/kernel/cpu/sgx/encl.h | 11 +++- > > arch/x86/kernel/cpu/sgx/main.c | 4 +- > > 3 files changed, 118 insertions(+), 6 deletions(-) > > > > diff --git a/arch/x86/kernel/cpu/sgx/encl.c > > b/arch/x86/kernel/cpu/sgx/encl.c > > index 001808e3901c..6d10202612d6 100644 > > --- a/arch/x86/kernel/cpu/sgx/encl.c > > +++ b/arch/x86/kernel/cpu/sgx/encl.c > > @@ -32,7 +32,7 @@ static int __sgx_encl_eldu(struct sgx_encl_page > > *encl_page, > > else > > page_index = PFN_DOWN(encl->size); > > > > - ret = sgx_encl_get_backing(encl, page_index, &b); > > + ret = sgx_encl_lookup_backing(encl, page_index, &b); > > if (ret) > > return ret; > > > > @@ -574,7 +574,7 @@ static struct page > > *sgx_encl_get_backing_page(struct sgx_encl *encl, > > * 0 on success, > > * -errno otherwise. > > */ > > -int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long > > page_index, > > +static int sgx_encl_get_backing(struct sgx_encl *encl, unsigned > > long page_index, > > struct sgx_backing *backing) > > { > > pgoff_t pcmd_index = PFN_DOWN(encl->size) + 1 + (page_index >> > > 5); > > @@ -601,6 +601,111 @@ int sgx_encl_get_backing(struct sgx_encl > > *encl, unsigned long page_index, > > return 0; > > } > > > > +/* > > + * When called from ksgxd, returns the mem_cgroup of a struct mm > > stored > > + * in the enclave's mm_list. When not called from ksgxd, just > > returns > > + * the mem_cgroup of the current task. > > + */ > > +static struct mem_cgroup *sgx_encl_get_mem_cgroup(struct sgx_encl > > *encl) > > +{ > > + struct mem_cgroup *memcg = NULL; > > + struct sgx_encl_mm *encl_mm; > > + int idx; > > + > > + /* > > + * If called from normal task context, return the mem_cgroup > > + * of the current task's mm. The remainder of the handling is > > for > > + * ksgxd. > > + */ > > + if (!current_is_ksgxd()) > > + return get_mem_cgroup_from_mm(current->mm); > > + > > + /* > > + * Search the enclave's mm_list to find an mm associated with > > + * this enclave to charge the allocation to. > > + */ > > + idx = srcu_read_lock(&encl->srcu); > > + > > + list_for_each_entry_rcu(encl_mm, &encl->mm_list, list) { > > + if (!mmget_not_zero(encl_mm->mm)) > > + continue; > > + > > + memcg = get_mem_cgroup_from_mm(encl_mm->mm); > > + > > + mmput_async(encl_mm->mm); > > + > > + break; > > + } > > + > > + srcu_read_unlock(&encl->srcu, idx); > > + > > + /* > > + * In the rare case that there isn't an mm associated with > > + * the enclave, set memcg to the current active mem_cgroup. > > + * This will be the root mem_cgroup if there is no active > > + * mem_cgroup. > > + */ > > + if (!memcg) > > + return get_mem_cgroup_from_mm(NULL); > > + > > + return memcg; > > +} > > You can simplify the function a bit. But it's up to you, not a strong > opinion. > > static struct mem_cgroup *sgx_encl_get_mem_cgroup(struct sgx_encl > *encl) > { > struct mem_cgroup *memcg = NULL; > struct sgx_encl_mm *encl_mm; > int idx; > > if (current_is_ksgxd()) { > /* > * Search the enclave's mm_list to find an mm > associated with > * this enclave to charge the allocation to. > */ > idx = srcu_read_lock(&encl->srcu); > list_for_each_entry_rcu(encl_mm, &encl->mm_list, list) > { > if (!mmget_not_zero(encl_mm->mm)) > continue; > > memcg = get_mem_cgroup_from_mm(encl_mm->mm); > mmput_async(encl_mm->mm); > break; > } > srcu_read_unlock(&encl->srcu, idx); > } > > return memcg ? memcg : get_mem_cgroup_from_mm(current->mm); > } > I don't have strong opinions on this either - I actually had it written this way originally but then decided maybe other people would find it more readable the other way. I definitely don't care either way. > -- > > The rest of the patch looks good to me. Please, feel free to add: > > Acked-by: Roman Gushchin > > Thanks! Thanks so much for your review, I will add this to v3. Kristen