Message ID | 20230728164254.27562-7-james.morse@arm.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:918b:0:b0:3e4:2afc:c1 with SMTP id s11csp579612vqg; Fri, 28 Jul 2023 10:15:12 -0700 (PDT) X-Google-Smtp-Source: APBJJlGGnq2aIGJvV7a87J1w9wmAJ+c1GCgFsxN73voguTvdqdbOQ4ZE46SdvWjLGwMjZvSwIa7R X-Received: by 2002:a05:6a20:2c98:b0:13b:a2c9:922e with SMTP id g24-20020a056a202c9800b0013ba2c9922emr1580216pzj.27.1690564512171; Fri, 28 Jul 2023 10:15:12 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1690564512; cv=none; d=google.com; s=arc-20160816; b=tLuosdIh50mNWoaZIXkq+k4ShrPDQ3rPU7cUOlN385D0yQKwGLh1kOJH86QcF8Epj+ MqUVS/mQLsVc0ryjlTBs9XtwfetGWIF6eKeCcs6Xw9irGW24OTMte8tYq2R3peSXyIQb V6Qn3Uw/3rwNEqgJy03n1bZ5r1TxfMxJibNxTEuYG4Z8zf5tvF726GCgflYC9DUgLrpq qikez78C2jYygze9P6JvYolrWoI5DO9H2f9UXxzePOdcX1LBm98G83yhBEGE+FxJS5R6 y/ZQys+cmq+R4xNcndoUsXsF54hII738niqj6L8ygZeS2yasAw7YJqiOCoXMRZeQvpIP rg/Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from; bh=vgJWjfsj4iSbRl+JL1kwq0L+XTc/Da8yZdgkJG8ZFGw=; fh=n9L+e/Mo/wOalMjVbltm4zKnQoPh0Qbxoe+x3eznw8A=; b=hZNkIfnN+VXe2ryqdf+9ahxn31YpWV4NYUM5Tp7ts8v154jp7Z1vqn6yfm5kyQ9q5w QJGyiFALWUL+Evs1lFZunn8fqcvLIzHDgeIiclWm7sQf9uJzwuyvJULVsB5ajwFMlxQa Pz3U6SXN8oZ1UTVQPJa4XxSXhkq7azCeuq20+FHpHYukP91yC0jW7l0VTpiFjwaF+rJH dPboqIZuX/r2zrMTguNgPkAwmhsvNHru9bR/FzZKqdbl3t/J5/0d8LTLMYxLH9g0yvZ0 h9at/d+UZGQaZfmlGuBO1ewYtx9Q/s0UyYRgoV9w1teUtov7rRHb8ulVzvnfSQ39HK58 EfwA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id cp24-20020a056a00349800b0068255c2b8a8si3369975pfb.151.2023.07.28.10.14.55; Fri, 28 Jul 2023 10:15:12 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235146AbjG1Qoq (ORCPT <rfc822;hanasaki@gmail.com> + 99 others); Fri, 28 Jul 2023 12:44:46 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46948 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234089AbjG1Qnh (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 28 Jul 2023 12:43:37 -0400 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 37FB64208 for <linux-kernel@vger.kernel.org>; Fri, 28 Jul 2023 09:43:36 -0700 (PDT) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 12542D75; Fri, 28 Jul 2023 09:44:19 -0700 (PDT) Received: from merodach.members.linode.com (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 4624C3F67D; Fri, 28 Jul 2023 09:43:33 -0700 (PDT) From: James Morse <james.morse@arm.com> To: x86@kernel.org, linux-kernel@vger.kernel.org Cc: Fenghua Yu <fenghua.yu@intel.com>, Reinette Chatre <reinette.chatre@intel.com>, Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, H Peter Anvin <hpa@zytor.com>, Babu Moger <Babu.Moger@amd.com>, James Morse <james.morse@arm.com>, shameerali.kolothum.thodi@huawei.com, D Scott Phillips OS <scott@os.amperecomputing.com>, carl@os.amperecomputing.com, lcherian@marvell.com, bobo.shaobowang@huawei.com, tan.shaopeng@fujitsu.com, xingxin.hx@openanolis.org, baolin.wang@linux.alibaba.com, Jamie Iles <quic_jiles@quicinc.com>, Xin Hao <xhao@linux.alibaba.com>, peternewman@google.com, dfustini@baylibre.com Subject: [PATCH v5 06/24] x86/resctrl: Track the number of dirty RMID a CLOSID has Date: Fri, 28 Jul 2023 16:42:36 +0000 Message-Id: <20230728164254.27562-7-james.morse@arm.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20230728164254.27562-1-james.morse@arm.com> References: <20230728164254.27562-1-james.morse@arm.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1772685373812618943 X-GMAIL-MSGID: 1772685373812618943 |
Series |
x86/resctrl: monitored closid+rmid together, separate arch/fs locking
|
|
Commit Message
James Morse
July 28, 2023, 4:42 p.m. UTC
MPAM's PMG bits extend its PARTID space, meaning the same PMG value can be
used for different control groups.
This means once a CLOSID is allocated, all its monitoring ids may still be
dirty, and held in limbo.
Keep track of the number of RMID held in limbo each CLOSID has. This will
allow a future helper to find the 'cleanest' CLOSID when allocating.
The array is only needed when CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID is
defined. This will never be the case on x86.
Signed-off-by: James Morse <james.morse@arm.com>
---
Changes since v4:
* Moved closid_num_dirty_rmid[] update under entry->busy check
* Take the mutex in dom_data_init() as the caller doesn't.
---
arch/x86/kernel/cpu/resctrl/monitor.c | 49 +++++++++++++++++++++++----
1 file changed, 42 insertions(+), 7 deletions(-)
Comments
Hi James, On 7/28/2023 9:42 AM, James Morse wrote: > diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c > index de91ca781d9f..44addc0126fc 100644 > --- a/arch/x86/kernel/cpu/resctrl/monitor.c > +++ b/arch/x86/kernel/cpu/resctrl/monitor.c > @@ -43,6 +43,13 @@ struct rmid_entry { > */ > static LIST_HEAD(rmid_free_lru); > > +/** > + * @closid_num_dirty_rmid The number of dirty RMID each CLOSID has. > + * Only allocated when CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID is defined. > + * Indexed by CLOSID. Protected by rdtgroup_mutex. > + */ > +static int *closid_num_dirty_rmid; > + Will the values ever be negative? > /** > * @rmid_limbo_count count of currently unused but (potentially) > * dirty RMIDs. > @@ -285,6 +292,17 @@ int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain *d, > return 0; > } > > +static void limbo_release_entry(struct rmid_entry *entry) > +{ > + lockdep_assert_held(&rdtgroup_mutex); > + > + rmid_limbo_count--; > + list_add_tail(&entry->list, &rmid_free_lru); > + > + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) > + closid_num_dirty_rmid[entry->closid]--; > +} > + > /* > * Check the RMIDs that are marked as busy for this domain. If the > * reported LLC occupancy is below the threshold clear the busy bit and > @@ -321,10 +339,8 @@ void __check_limbo(struct rdt_domain *d, bool force_free) > > if (force_free || !rmid_dirty) { > clear_bit(idx, d->rmid_busy_llc); > - if (!--entry->busy) { > - rmid_limbo_count--; > - list_add_tail(&entry->list, &rmid_free_lru); > - } > + if (!--entry->busy) > + limbo_release_entry(entry); > } > cur_idx = idx + 1; > } > @@ -391,6 +407,8 @@ static void add_rmid_to_limbo(struct rmid_entry *entry) > u64 val = 0; > u32 idx; > > + lockdep_assert_held(&rdtgroup_mutex); > + > idx = resctrl_arch_rmid_idx_encode(entry->closid, entry->rmid); > > entry->busy = 0; > @@ -416,9 +434,11 @@ static void add_rmid_to_limbo(struct rmid_entry *entry) > } > put_cpu(); > > - if (entry->busy) > + if (entry->busy) { > rmid_limbo_count++; > - else > + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) > + closid_num_dirty_rmid[entry->closid]++; > + } else > list_add_tail(&entry->list, &rmid_free_lru); > } This new addition breaks the coding style with the last statement now also needing a brace. > > @@ -782,13 +802,28 @@ void mbm_setup_overflow_handler(struct rdt_domain *dom, unsigned long delay_ms) > static int dom_data_init(struct rdt_resource *r) > { > u32 idx_limit = resctrl_arch_system_num_rmid_idx(); > + u32 num_closid = resctrl_arch_get_num_closid(r); > struct rmid_entry *entry = NULL; > u32 idx; > int i; > > + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) { > + int *tmp; > + > + tmp = kcalloc(num_closid, sizeof(int), GFP_KERNEL); > + if (!tmp) > + return -ENOMEM; > + > + mutex_lock(&rdtgroup_mutex); > + closid_num_dirty_rmid = tmp; > + mutex_unlock(&rdtgroup_mutex); > + } > + It does no harm but I cannot see why the mutex is needed here. > rmid_ptrs = kcalloc(idx_limit, sizeof(struct rmid_entry), GFP_KERNEL); > - if (!rmid_ptrs) > + if (!rmid_ptrs) { > + kfree(closid_num_dirty_rmid); > return -ENOMEM; > + } > > for (i = 0; i < idx_limit; i++) { > entry = &rmid_ptrs[i]; How will this new memory be freed? Actually I cannot find where rmid_ptrs is freed either .... is a "dom_data_free()" needed? Reinette
Hi, James, On 7/28/23 09:42, James Morse wrote: > MPAM's PMG bits extend its PARTID space, meaning the same PMG value can be > used for different control groups. > > This means once a CLOSID is allocated, all its monitoring ids may still be > dirty, and held in limbo. > > Keep track of the number of RMID held in limbo each CLOSID has. This will > allow a future helper to find the 'cleanest' CLOSID when allocating. > > The array is only needed when CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID is > defined. This will never be the case on x86. > > Signed-off-by: James Morse <james.morse@arm.com> > --- > Changes since v4: > * Moved closid_num_dirty_rmid[] update under entry->busy check > * Take the mutex in dom_data_init() as the caller doesn't. > --- > arch/x86/kernel/cpu/resctrl/monitor.c | 49 +++++++++++++++++++++++---- > 1 file changed, 42 insertions(+), 7 deletions(-) > > diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c > index de91ca781d9f..44addc0126fc 100644 > --- a/arch/x86/kernel/cpu/resctrl/monitor.c > +++ b/arch/x86/kernel/cpu/resctrl/monitor.c > @@ -43,6 +43,13 @@ struct rmid_entry { > */ > static LIST_HEAD(rmid_free_lru); > > +/** > + * @closid_num_dirty_rmid The number of dirty RMID each CLOSID has. > + * Only allocated when CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID is defined. > + * Indexed by CLOSID. Protected by rdtgroup_mutex. > + */ > +static int *closid_num_dirty_rmid; > + > /** > * @rmid_limbo_count count of currently unused but (potentially) > * dirty RMIDs. > @@ -285,6 +292,17 @@ int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain *d, > return 0; > } > > +static void limbo_release_entry(struct rmid_entry *entry) > +{ > + lockdep_assert_held(&rdtgroup_mutex); > + > + rmid_limbo_count--; > + list_add_tail(&entry->list, &rmid_free_lru); > + > + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) > + closid_num_dirty_rmid[entry->closid]--; > +} > + > /* > * Check the RMIDs that are marked as busy for this domain. If the > * reported LLC occupancy is below the threshold clear the busy bit and > @@ -321,10 +339,8 @@ void __check_limbo(struct rdt_domain *d, bool force_free) > > if (force_free || !rmid_dirty) { > clear_bit(idx, d->rmid_busy_llc); > - if (!--entry->busy) { > - rmid_limbo_count--; > - list_add_tail(&entry->list, &rmid_free_lru); > - } > + if (!--entry->busy) > + limbo_release_entry(entry); > } > cur_idx = idx + 1; > } > @@ -391,6 +407,8 @@ static void add_rmid_to_limbo(struct rmid_entry *entry) > u64 val = 0; > u32 idx; > > + lockdep_assert_held(&rdtgroup_mutex); > + > idx = resctrl_arch_rmid_idx_encode(entry->closid, entry->rmid); > > entry->busy = 0; > @@ -416,9 +434,11 @@ static void add_rmid_to_limbo(struct rmid_entry *entry) > } > put_cpu(); > > - if (entry->busy) > + if (entry->busy) { > rmid_limbo_count++; > - else > + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) > + closid_num_dirty_rmid[entry->closid]++; > + } else > list_add_tail(&entry->list, &rmid_free_lru); Unbalanced braces in if-else. Need to add braces in "else". > } > > @@ -782,13 +802,28 @@ void mbm_setup_overflow_handler(struct rdt_domain *dom, unsigned long delay_ms) > static int dom_data_init(struct rdt_resource *r) > { > u32 idx_limit = resctrl_arch_system_num_rmid_idx(); > + u32 num_closid = resctrl_arch_get_num_closid(r); > struct rmid_entry *entry = NULL; > u32 idx; > int i; > > + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) { > + int *tmp; > + > + tmp = kcalloc(num_closid, sizeof(int), GFP_KERNEL); > + if (!tmp) > + return -ENOMEM; > + > + mutex_lock(&rdtgroup_mutex); > + closid_num_dirty_rmid = tmp; > + mutex_unlock(&rdtgroup_mutex); > + } > + > rmid_ptrs = kcalloc(idx_limit, sizeof(struct rmid_entry), GFP_KERNEL); > - if (!rmid_ptrs) > + if (!rmid_ptrs) { > + kfree(closid_num_dirty_rmid); > return -ENOMEM; > + } > > for (i = 0; i < idx_limit; i++) { > entry = &rmid_ptrs[i]; Thanks. -Fenghua
Hi Reinette, On 09/08/2023 23:33, Reinette Chatre wrote: > On 7/28/2023 9:42 AM, James Morse wrote: >> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c >> index de91ca781d9f..44addc0126fc 100644 >> --- a/arch/x86/kernel/cpu/resctrl/monitor.c >> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c >> @@ -43,6 +43,13 @@ struct rmid_entry { >> */ >> static LIST_HEAD(rmid_free_lru); >> >> +/** >> + * @closid_num_dirty_rmid The number of dirty RMID each CLOSID has. >> + * Only allocated when CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID is defined. >> + * Indexed by CLOSID. Protected by rdtgroup_mutex. >> + */ >> +static int *closid_num_dirty_rmid; >> + > > Will the values ever be negative? Nope, int is just fewer keystrokes. I'll change it to unsigned int. >> /** >> * @rmid_limbo_count count of currently unused but (potentially) >> * dirty RMIDs. >> @@ -782,13 +802,28 @@ void mbm_setup_overflow_handler(struct rdt_domain *dom, unsigned long delay_ms) >> static int dom_data_init(struct rdt_resource *r) >> { >> u32 idx_limit = resctrl_arch_system_num_rmid_idx(); >> + u32 num_closid = resctrl_arch_get_num_closid(r); >> struct rmid_entry *entry = NULL; >> u32 idx; >> int i; >> >> + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) { >> + int *tmp; >> + >> + tmp = kcalloc(num_closid, sizeof(int), GFP_KERNEL); >> + if (!tmp) >> + return -ENOMEM; >> + >> + mutex_lock(&rdtgroup_mutex); >> + closid_num_dirty_rmid = tmp; >> + mutex_unlock(&rdtgroup_mutex); >> + } >> + > > It does no harm but I cannot see why the mutex is needed here. It's belt-and-braces to ensure that all accesses to that global variable are protected by that lock. This avoids giving me a memory ordering headache. rmid_ptrs and the call to __rmid_entry() that dereferences it should probably get the same treatment. I'll move the locking to the caller as the least-churny way of covering both. >> rmid_ptrs = kcalloc(idx_limit, sizeof(struct rmid_entry), GFP_KERNEL); >> - if (!rmid_ptrs) >> + if (!rmid_ptrs) { >> + kfree(closid_num_dirty_rmid); >> return -ENOMEM; >> + } >> >> for (i = 0; i < idx_limit; i++) { >> entry = &rmid_ptrs[i]; > > How will this new memory be freed? Actually I cannot find where > rmid_ptrs is freed either .... is a "dom_data_free()" needed? Oh that's not deliberate? :P rmid_ptrs has been immortal since the beginning. The good news is resctrl_exit() goes in the exitcall section, which is in the DISCARDS section of the linker script as resctrl can't be built as a module. It isn't possible to tear resctrl down, so no-one will notice this leak. Something on my eternal-todo-list is to make the filesystem parts of resctrl a loadable module (if Tony doesn't get there first!). That would flush this sort of thing out. Last time I triggered resctrl_exit() manually not all of the files got cleaned up - I haven't investigated it further. I agree it should probably have a kfree() call somewhere under rdtgroup_exit(), as its only the L3 that needs any of this, I'll add resctrl_exit_mon_l3_config() for rdtgroup_exit() to call. Another option is to rip out all the __exit text as its discarded anyway. But if loadable modules is the direction of travel, it probably make more sense to fix it. Thanks, James
Hi Fenghua On 15/08/2023 03:37, Fenghua Yu wrote: > On 7/28/23 09:42, James Morse wrote: >> MPAM's PMG bits extend its PARTID space, meaning the same PMG value can be >> used for different control groups. >> >> This means once a CLOSID is allocated, all its monitoring ids may still be >> dirty, and held in limbo. >> >> Keep track of the number of RMID held in limbo each CLOSID has. This will >> allow a future helper to find the 'cleanest' CLOSID when allocating. >> >> The array is only needed when CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID is >> defined. This will never be the case on x86. >> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c >> index de91ca781d9f..44addc0126fc 100644 >> --- a/arch/x86/kernel/cpu/resctrl/monitor.c >> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c >> @@ -43,6 +43,13 @@ struct rmid_entry { >> */ >> static LIST_HEAD(rmid_free_lru); >> > > Better to add: > > #if CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID >> +/** >> + * @closid_num_dirty_rmid The number of dirty RMID each CLOSID has. >> + * Only allocated when CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID is defined. >> + * Indexed by CLOSID. Protected by rdtgroup_mutex. >> + */ >> +static int *closid_num_dirty_rmid; > #endif > > Then the global variable won't exist on x86 to avoid confusion and space. > > Some code related to the CONFIG also needs to be changed accordingly. Uh-huh, that would force me to put #ifdef warts all over the code that accesses that variable. Modern compilers are really smart. Because this is static, the compiler is free to remove it if there are no users. All the users are behind if(IS_ENABLED()), meaning the compilers dead-code elimination will cull the lot, and this variable too: morse@eglon:~/kernel/mpam/build_x86_64/fs/resctrl$ nm -s monitor.o | grep closid_num_dirty morse@eglon:~/kernel/mpam/build_arm64/fs/resctrl$ nm -s monitor.o | grep closid_num_dirty 0000000000000000 b closid_num_dirty_rmid morse@eglon:~/kernel/mpam/build_arm64/fs/resctrl$ Using #ifdef is not only ugly - it prevents the compiler from seeing all the code, so the CI build systems get worse coverage. >> + >> /** >> * @rmid_limbo_count count of currently unused but (potentially) >> * dirty RMIDs. >> @@ -285,6 +292,17 @@ int resctrl_arch_rmid_read(struct rdt_resource *r, struct >> rdt_domain *d, >> return 0; >> } >> +static void limbo_release_entry(struct rmid_entry *entry) >> +{ >> + lockdep_assert_held(&rdtgroup_mutex); >> + >> + rmid_limbo_count--; >> + list_add_tail(&entry->list, &rmid_free_lru); >> + >> + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) >> + closid_num_dirty_rmid[entry->closid]--; > > > Maybe define some helpers (along with other similar ones) in resctrl.h like this: > > #ifdef CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID > static inline void closid_num_dirty_rmid_dec(struct rmid_entry *entry) > { > closid_num_dirty_rmid[entry->closid]--; > } > ... > #else > static inline void closid_num_dirty_rmid_dec(struct rmid_entry *unused) > { > } > ... > #endif > > Then directly call the helper here: > > + closid_num_dirty_rmid_dec(entry); > > On x86 this is noop without and the compiler knows this. > occupy any space Literally more lines of code. > and cleaner code. Maybe, this would hide the IS_ENABLED() check - but moving that out as a single use helper would required closid_num_dirty_rmid[] to be exported from this file - which would prevent it being optimised out. You'd get the result you were trying to avoid. >> +} >> + >> /* >> * Check the RMIDs that are marked as busy for this domain. If the >> * reported LLC occupancy is below the threshold clear the busy bit and >> @@ -321,10 +339,8 @@ void __check_limbo(struct rdt_domain *d, bool force_free) >> if (force_free || !rmid_dirty) { >> clear_bit(idx, d->rmid_busy_llc); >> - if (!--entry->busy) { >> - rmid_limbo_count--; >> - list_add_tail(&entry->list, &rmid_free_lru); >> - } >> + if (!--entry->busy) >> + limbo_release_entry(entry); >> } >> cur_idx = idx + 1; >> } >> @@ -391,6 +407,8 @@ static void add_rmid_to_limbo(struct rmid_entry *entry) >> u64 val = 0; >> u32 idx; >> + lockdep_assert_held(&rdtgroup_mutex); >> + >> idx = resctrl_arch_rmid_idx_encode(entry->closid, entry->rmid); >> entry->busy = 0; >> @@ -416,9 +434,11 @@ static void add_rmid_to_limbo(struct rmid_entry *entry) >> } >> put_cpu(); >> - if (entry->busy) >> + if (entry->busy) { >> rmid_limbo_count++; >> - else >> + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) >> + closid_num_dirty_rmid[entry->closid]++; > > Ditto. > >> + } else >> list_add_tail(&entry->list, &rmid_free_lru); >> } >> @@ -782,13 +802,28 @@ void mbm_setup_overflow_handler(struct rdt_domain *dom, unsigned >> long delay_ms) >> static int dom_data_init(struct rdt_resource *r) >> { >> u32 idx_limit = resctrl_arch_system_num_rmid_idx(); >> + u32 num_closid = resctrl_arch_get_num_closid(r); >> struct rmid_entry *entry = NULL; >> u32 idx; >> int i; >> + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) { >> + int *tmp; >> + >> + tmp = kcalloc(num_closid, sizeof(int), GFP_KERNEL); >> + if (!tmp) >> + return -ENOMEM; >> + >> + mutex_lock(&rdtgroup_mutex); > data_init() is called in __init. No need to lock here, right? __init code can still race with other callers - especially as there are CPUHP_AP_ONLINE_DYN cpuhp callbacks that are expected to sleep. This is about ensuring all accesses to those global variables are protected by the lock. This saves me a memory ordering headache. Thanks, James
diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c index de91ca781d9f..44addc0126fc 100644 --- a/arch/x86/kernel/cpu/resctrl/monitor.c +++ b/arch/x86/kernel/cpu/resctrl/monitor.c @@ -43,6 +43,13 @@ struct rmid_entry { */ static LIST_HEAD(rmid_free_lru); +/** + * @closid_num_dirty_rmid The number of dirty RMID each CLOSID has. + * Only allocated when CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID is defined. + * Indexed by CLOSID. Protected by rdtgroup_mutex. + */ +static int *closid_num_dirty_rmid; + /** * @rmid_limbo_count count of currently unused but (potentially) * dirty RMIDs. @@ -285,6 +292,17 @@ int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain *d, return 0; } +static void limbo_release_entry(struct rmid_entry *entry) +{ + lockdep_assert_held(&rdtgroup_mutex); + + rmid_limbo_count--; + list_add_tail(&entry->list, &rmid_free_lru); + + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) + closid_num_dirty_rmid[entry->closid]--; +} + /* * Check the RMIDs that are marked as busy for this domain. If the * reported LLC occupancy is below the threshold clear the busy bit and @@ -321,10 +339,8 @@ void __check_limbo(struct rdt_domain *d, bool force_free) if (force_free || !rmid_dirty) { clear_bit(idx, d->rmid_busy_llc); - if (!--entry->busy) { - rmid_limbo_count--; - list_add_tail(&entry->list, &rmid_free_lru); - } + if (!--entry->busy) + limbo_release_entry(entry); } cur_idx = idx + 1; } @@ -391,6 +407,8 @@ static void add_rmid_to_limbo(struct rmid_entry *entry) u64 val = 0; u32 idx; + lockdep_assert_held(&rdtgroup_mutex); + idx = resctrl_arch_rmid_idx_encode(entry->closid, entry->rmid); entry->busy = 0; @@ -416,9 +434,11 @@ static void add_rmid_to_limbo(struct rmid_entry *entry) } put_cpu(); - if (entry->busy) + if (entry->busy) { rmid_limbo_count++; - else + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) + closid_num_dirty_rmid[entry->closid]++; + } else list_add_tail(&entry->list, &rmid_free_lru); } @@ -782,13 +802,28 @@ void mbm_setup_overflow_handler(struct rdt_domain *dom, unsigned long delay_ms) static int dom_data_init(struct rdt_resource *r) { u32 idx_limit = resctrl_arch_system_num_rmid_idx(); + u32 num_closid = resctrl_arch_get_num_closid(r); struct rmid_entry *entry = NULL; u32 idx; int i; + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) { + int *tmp; + + tmp = kcalloc(num_closid, sizeof(int), GFP_KERNEL); + if (!tmp) + return -ENOMEM; + + mutex_lock(&rdtgroup_mutex); + closid_num_dirty_rmid = tmp; + mutex_unlock(&rdtgroup_mutex); + } + rmid_ptrs = kcalloc(idx_limit, sizeof(struct rmid_entry), GFP_KERNEL); - if (!rmid_ptrs) + if (!rmid_ptrs) { + kfree(closid_num_dirty_rmid); return -ENOMEM; + } for (i = 0; i < idx_limit; i++) { entry = &rmid_ptrs[i];