[v2,06/18] x86/resctrl: Allow the allocator to check if a CLOSID can allocate clean RMID
Message ID | 20230113175459.14825-7-james.morse@arm.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:4e01:0:0:0:0:0 with SMTP id p1csp408716wrt; Fri, 13 Jan 2023 10:04:08 -0800 (PST) X-Google-Smtp-Source: AMrXdXvbVI92hrO/22RE2RZtF1+XO/M6r6vegmrRP90sm5NYgl0QkwESQN7BlEeAYLEUnPW2y/UG X-Received: by 2002:a17:902:be05:b0:192:a4e5:ac5f with SMTP id r5-20020a170902be0500b00192a4e5ac5fmr53663960pls.61.1673633048643; Fri, 13 Jan 2023 10:04:08 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1673633048; cv=none; d=google.com; s=arc-20160816; b=b/8AtRrg8nBG+/57DzMGKmyd1FtHcNTAYkJaEkMGU1DRf7kOdJzy3vOURE2qIxIzYp Hhvtrozb0NjLkQMk0eXTpwjPWrYoKWS6tAp0Uuym0TO3gBwk4qZOm81T0fsfKc3VJEkS pEtjGjK7HY+HB+2yIZpCbGu5gk99/kus+iXdr0NJdmjcis/kmQ7aB5KhYRD0MJf9Hpk0 LmR7mfO+FIexVrCsYNo75FHRjupVJYqx5uyJ+Ii6HcpsRiwsYlcwvT7yNK6JZgy39ElU b/huD2U9XojQ9YAmnLcC/UYg5F0xxSe9g6EEDFkONuL37RyqEZvqpTTaG2xgi6sOrK4w Szfg== 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=KGiIYknt6ZMXCTnvtqU1D9E0N7vSuqBBbWXS53nlgWM=; b=ISmBRO+iC9nHk4vh+Bf2kt5NbR1B/uxyf4Aa0jvDt3UsiOc2q9nHpiqKrKrwzimPzA liFITBHrzDvUKIVsQfbrSjWQfuDWEkCLWuxZdIoU3/QaiYl8KsAR/+o4g8vAKPw3u+1F vaaaum1VyjkDrHwitlCTW+PuSIt73BlFhYHlORzYbJKA6tWEXgns7p35NItLm3dHHFbF qHoXwyhXrMUg4RFQ38GMC5lRiBzeqAi76LpM7LZMzlRKvICu8yTUAgRMuLoNjc475Jpi piQMhr9++AmbR6sPK3pqG9YE+WwCc3Y2VQPL07JlQmPU3XB1w3uZqqM0Rqb8pCfwKaSv TMtg== 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 i1-20020a17090332c100b00189754b9d97si4783204plr.122.2023.01.13.10.03.51; Fri, 13 Jan 2023 10:04:08 -0800 (PST) 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 S229876AbjAMSCc (ORCPT <rfc822;callmefire3@gmail.com> + 99 others); Fri, 13 Jan 2023 13:02:32 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41342 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229841AbjAMSBa (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 13 Jan 2023 13:01:30 -0500 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 0E9B1E28 for <linux-kernel@vger.kernel.org>; Fri, 13 Jan 2023 09:56:00 -0800 (PST) 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 DF973FEC; Fri, 13 Jan 2023 09:56:41 -0800 (PST) Received: from merodach.members.linode.com (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 5464D3F67D; Fri, 13 Jan 2023 09:55:57 -0800 (PST) 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 Subject: [PATCH v2 06/18] x86/resctrl: Allow the allocator to check if a CLOSID can allocate clean RMID Date: Fri, 13 Jan 2023 17:54:47 +0000 Message-Id: <20230113175459.14825-7-james.morse@arm.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20230113175459.14825-1-james.morse@arm.com> References: <20230113175459.14825-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 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: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1754931447887340609?= X-GMAIL-MSGID: =?utf-8?q?1754931447887340609?= |
Series |
x86/resctrl: monitored closid+rmid together, separate arch/fs locking
|
|
Commit Message
James Morse
Jan. 13, 2023, 5:54 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. Add a helper to allow the CLOSID allocator to check if a CLOSID has dirty RMID values. This behaviour is enabled by a kconfig option selected by the architecture, which avoids a pointless search for x86. Tested-by: Shaopeng Tan <tan.shaopeng@fujitsu.com> Signed-off-by: James Morse <james.morse@arm.com> --- Changes since v1: * Removed superflous IS_ENABLED(). --- arch/x86/kernel/cpu/resctrl/internal.h | 1 + arch/x86/kernel/cpu/resctrl/monitor.c | 31 ++++++++++++++++++++++++++ arch/x86/kernel/cpu/resctrl/rdtgroup.c | 17 ++++++++------ 3 files changed, 42 insertions(+), 7 deletions(-)
Comments
Hi, James, > 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. > > Add a helper to allow the CLOSID allocator to check if a CLOSID has dirty RMID > values. This behaviour is enabled by a kconfig option selected by the > architecture, which avoids a pointless search for x86. > > Tested-by: Shaopeng Tan <tan.shaopeng@fujitsu.com> > Signed-off-by: James Morse <james.morse@arm.com> > > --- > Changes since v1: > * Removed superflous IS_ENABLED(). > --- > arch/x86/kernel/cpu/resctrl/internal.h | 1 + > arch/x86/kernel/cpu/resctrl/monitor.c | 31 ++++++++++++++++++++++++++ > arch/x86/kernel/cpu/resctrl/rdtgroup.c | 17 ++++++++------ > 3 files changed, 42 insertions(+), 7 deletions(-) > > diff --git a/arch/x86/kernel/cpu/resctrl/internal.h > b/arch/x86/kernel/cpu/resctrl/internal.h > index 013c8fc9fd28..adae6231324f 100644 > --- a/arch/x86/kernel/cpu/resctrl/internal.h > +++ b/arch/x86/kernel/cpu/resctrl/internal.h > @@ -509,6 +509,7 @@ int rdtgroup_pseudo_lock_create(struct rdtgroup > *rdtgrp); void rdtgroup_pseudo_lock_remove(struct rdtgroup *rdtgrp); struct > rdt_domain *get_domain_from_cpu(int cpu, struct rdt_resource *r); int > closids_supported(void); > +bool resctrl_closid_is_dirty(u32 closid); > void closid_free(int closid); > int alloc_rmid(u32 closid); > void free_rmid(u32 closid, u32 rmid); > diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c > b/arch/x86/kernel/cpu/resctrl/monitor.c > index 347be3767241..190ac183139e 100644 > --- a/arch/x86/kernel/cpu/resctrl/monitor.c > +++ b/arch/x86/kernel/cpu/resctrl/monitor.c > @@ -327,6 +327,37 @@ static struct rmid_entry *resctrl_find_free_rmid(u32 > closid) > return ERR_PTR(-ENOSPC); > } > > +/** > + * resctrl_closid_is_dirty - Determine if clean RMID can be allocate for this s/allocate/allocated/ > + * CLOSID. > + * @closid: The CLOSID that is being queried. > + * > + * MPAM's equivalent of RMID are per-CLOSID, meaning a freshly allocate s/allocate/allocated/ > +CLOSID > + * may not be able to allocate clean RMID. To avoid this the allocator > +will > + * only return clean CLOSID. > + */ > +bool resctrl_closid_is_dirty(u32 closid) { > + struct rmid_entry *entry; > + int i; > + > + lockdep_assert_held(&rdtgroup_mutex); It's better to move lockdep_asser_held() after if (!IS_ENABLE()). Then compiler might optimize this function to empty on X86. > + > + if (!IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) > + return false; > + > + for (i = 0; i < resctrl_arch_system_num_rmid_idx(); i++) { > + entry = &rmid_ptrs[i]; > + if (entry->closid != closid) > + continue; > + > + if (entry->busy) > + return true; > + } > + > + return false; > +} > + > /* > * As of now the RMIDs allocation is the same in each domain. > * However we keep track of which packages the RMIDs diff --git > a/arch/x86/kernel/cpu/resctrl/rdtgroup.c > b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > index ac88610a6946..e1f879e13823 100644 > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > @@ -93,7 +93,7 @@ void rdt_last_cmd_printf(const char *fmt, ...) > * - Our choices on how to configure each resource become progressively more > * limited as the number of resources grows. > */ > -static int closid_free_map; > +static unsigned long closid_free_map; > static int closid_free_map_len; > > int closids_supported(void) > @@ -119,14 +119,17 @@ static void closid_init(void) > > static int closid_alloc(void) > { > - u32 closid = ffs(closid_free_map); > + u32 closid; > > - if (closid == 0) > - return -ENOSPC; > - closid--; > - closid_free_map &= ~(1 << closid); > + for_each_set_bit(closid, &closid_free_map, closid_free_map_len) { > + if (resctrl_closid_is_dirty(closid)) > + continue; > > - return closid; > + clear_bit(closid, &closid_free_map); > + return closid; > + } > + > + return -ENOSPC; > } > > void closid_free(int closid) > -- > 2.30.2 Thanks. -Fenghua
Hi James, On 1/13/2023 9:54 AM, 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. > > Add a helper to allow the CLOSID allocator to check if a CLOSID has dirty > RMID values. This behaviour is enabled by a kconfig option selected by > the architecture, which avoids a pointless search for x86. > > Tested-by: Shaopeng Tan <tan.shaopeng@fujitsu.com> > Signed-off-by: James Morse <james.morse@arm.com> > > --- > Changes since v1: > * Removed superflous IS_ENABLED(). > --- > arch/x86/kernel/cpu/resctrl/internal.h | 1 + > arch/x86/kernel/cpu/resctrl/monitor.c | 31 ++++++++++++++++++++++++++ > arch/x86/kernel/cpu/resctrl/rdtgroup.c | 17 ++++++++------ > 3 files changed, 42 insertions(+), 7 deletions(-) > > diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h > index 013c8fc9fd28..adae6231324f 100644 > --- a/arch/x86/kernel/cpu/resctrl/internal.h > +++ b/arch/x86/kernel/cpu/resctrl/internal.h > @@ -509,6 +509,7 @@ int rdtgroup_pseudo_lock_create(struct rdtgroup *rdtgrp); > void rdtgroup_pseudo_lock_remove(struct rdtgroup *rdtgrp); > struct rdt_domain *get_domain_from_cpu(int cpu, struct rdt_resource *r); > int closids_supported(void); > +bool resctrl_closid_is_dirty(u32 closid); > void closid_free(int closid); > int alloc_rmid(u32 closid); > void free_rmid(u32 closid, u32 rmid); > diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c > index 347be3767241..190ac183139e 100644 > --- a/arch/x86/kernel/cpu/resctrl/monitor.c > +++ b/arch/x86/kernel/cpu/resctrl/monitor.c > @@ -327,6 +327,37 @@ static struct rmid_entry *resctrl_find_free_rmid(u32 closid) > return ERR_PTR(-ENOSPC); > } > > +/** > + * resctrl_closid_is_dirty - Determine if clean RMID can be allocate for this > + * CLOSID. > + * @closid: The CLOSID that is being queried. > + * > + * MPAM's equivalent of RMID are per-CLOSID, meaning a freshly allocate CLOSID > + * may not be able to allocate clean RMID. To avoid this the allocator will > + * only return clean CLOSID. > + */ > +bool resctrl_closid_is_dirty(u32 closid) > +{ > + struct rmid_entry *entry; > + int i; > + > + lockdep_assert_held(&rdtgroup_mutex); > + > + if (!IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) > + return false; Why is a config option chosen? Is this not something that can be set in the architecture specific code using a global in the form matching existing related items like "arch_has..." or "arch_needs..."? > + > + for (i = 0; i < resctrl_arch_system_num_rmid_idx(); i++) { > + entry = &rmid_ptrs[i]; > + if (entry->closid != closid) > + continue; > + > + if (entry->busy) > + return true; > + } > + > + return false; > +} If I understand this correctly resctrl_closid_is_dirty() will return true if _any_ RMID/PMG associated with a CLOSID is in use. That is, a CLOSID may be able to support 100s of PMG but if only one of them is busy then the CLOSID will be considered unusable ("dirty"). It sounds to me that there could be scenarios when CLOSID could be considered unavailable while there are indeed sufficient resources? The function comment states "Determine if clean RMID can be allocate for this CLOSID." - if I understand correctly it behaves more like "Determine if all RMID associated with CLOSID are available". Reinette
Hi Fenghua, On 17/01/2023 18:29, Yu, Fenghua 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. >> >> Add a helper to allow the CLOSID allocator to check if a CLOSID has dirty RMID >> values. This behaviour is enabled by a kconfig option selected by the >> architecture, which avoids a pointless search for x86. >> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c >> b/arch/x86/kernel/cpu/resctrl/monitor.c >> index 347be3767241..190ac183139e 100644 >> --- a/arch/x86/kernel/cpu/resctrl/monitor.c >> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c >> @@ -327,6 +327,37 @@ static struct rmid_entry *resctrl_find_free_rmid(u32 >> closid) >> return ERR_PTR(-ENOSPC); >> } >> >> +/** >> + * resctrl_closid_is_dirty - Determine if clean RMID can be allocate for this > > s/allocate/allocated/ > >> + * CLOSID. >> + * @closid: The CLOSID that is being queried. >> + * >> + * MPAM's equivalent of RMID are per-CLOSID, meaning a freshly allocate > > s/allocate/allocated/ (Both fixed, thanks) >> +CLOSID >> + * may not be able to allocate clean RMID. To avoid this the allocator >> +will >> + * only return clean CLOSID. >> + */ >> +bool resctrl_closid_is_dirty(u32 closid) { >> + struct rmid_entry *entry; >> + int i; >> + >> + lockdep_assert_held(&rdtgroup_mutex); > > It's better to move lockdep_asser_held() after if (!IS_ENABLE()). > Then compiler might optimize this function to empty on X86. If you compile without lockdep it will be empty! Is anyone worried about performance with lockdep enabled? The reason for it being here is documentation and for the runtime check if you run with lockdep. Having it here is so that new code that only runs on x86 (with lockdep) also checks this, even though it doesn't have CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID. I'd prefer to keep it so we can catch bugs early. Lockdep isn't on by default. >> + >> + if (!IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) >> + return false; >> + >> + for (i = 0; i < resctrl_arch_system_num_rmid_idx(); i++) { >> + entry = &rmid_ptrs[i]; >> + if (entry->closid != closid) >> + continue; >> + >> + if (entry->busy) >> + return true; >> + } >> + >> + return false; >> +} Thanks, James
Hi Reinette, On 02/02/2023 23:46, Reinette Chatre wrote: > On 1/13/2023 9:54 AM, 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. >> >> Add a helper to allow the CLOSID allocator to check if a CLOSID has dirty >> RMID values. This behaviour is enabled by a kconfig option selected by >> the architecture, which avoids a pointless search for x86. >> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c >> index 347be3767241..190ac183139e 100644 >> --- a/arch/x86/kernel/cpu/resctrl/monitor.c >> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c >> @@ -327,6 +327,37 @@ static struct rmid_entry *resctrl_find_free_rmid(u32 closid) >> return ERR_PTR(-ENOSPC); >> } >> >> +/** >> + * resctrl_closid_is_dirty - Determine if clean RMID can be allocate for this >> + * CLOSID. >> + * @closid: The CLOSID that is being queried. >> + * >> + * MPAM's equivalent of RMID are per-CLOSID, meaning a freshly allocate CLOSID >> + * may not be able to allocate clean RMID. To avoid this the allocator will >> + * only return clean CLOSID. >> + */ >> +bool resctrl_closid_is_dirty(u32 closid) >> +{ >> + struct rmid_entry *entry; >> + int i; >> + >> + lockdep_assert_held(&rdtgroup_mutex); >> + >> + if (!IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) >> + return false; > Why is a config option chosen? Is this not something that can be set in the > architecture specific code using a global in the form matching existing related > items like "arch_has..." or "arch_needs..."? It doesn't vary by platform, so making it a runtime variable would mean x86 has to carry this extra code around, even though it will never use it. Done like this, the compiler can dead-code eliminate the below checks and embed the constant return value in all the callers. > >> + >> + for (i = 0; i < resctrl_arch_system_num_rmid_idx(); i++) { >> + entry = &rmid_ptrs[i]; >> + if (entry->closid != closid) >> + continue; >> + >> + if (entry->busy) >> + return true; >> + } >> + >> + return false; >> +} > > If I understand this correctly resctrl_closid_is_dirty() will return true if > _any_ RMID/PMG associated with a CLOSID is in use. That is, a CLOSID may be > able to support 100s of PMG but if only one of them is busy then the CLOSID > will be considered unusable ("dirty"). It sounds to me that there could be scenarios > when CLOSID could be considered unavailable while there are indeed sufficient > resources? You are right this could happen. I guess the better approach would be to prefer the cleanest CLOSID that has a clean PMG=0. User-space may not be able to allocate all the monitor groups immediately, but that would be preferable to failing the control group creation. But as this code doesn't get built until the MPAM driver is merged, I'd like to keep it to an absolute minimum. This would be more than is needed for MPAM to have close to resctrl feature-parity, so I'd prefer to do this as an improvement once the MPAM driver is upstream. (also in this category is better use of MPAM's monitors and labelling traffic from the iommu) > The function comment states "Determine if clean RMID can be allocate for this > CLOSID." - if I understand correctly it behaves more like "Determine if all > RMID associated with CLOSID are available". Yes, I'll fix that. Thanks! James
Hi James, On 3/3/2023 10:36 AM, James Morse wrote: > Hi Reinette, > > On 02/02/2023 23:46, Reinette Chatre wrote: >> On 1/13/2023 9:54 AM, James Morse wrote: ... >>> +bool resctrl_closid_is_dirty(u32 closid) >>> +{ >>> + struct rmid_entry *entry; >>> + int i; >>> + >>> + lockdep_assert_held(&rdtgroup_mutex); >>> + >>> + if (!IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) >>> + return false; > >> Why is a config option chosen? Is this not something that can be set in the >> architecture specific code using a global in the form matching existing related >> items like "arch_has..." or "arch_needs..."? > > It doesn't vary by platform, so making it a runtime variable would mean x86 has to carry > this extra code around, even though it will never use it. Done like this, the compiler can > dead-code eliminate the below checks and embed the constant return value in all the callers. This is fair. I missed any other mention of this option in this series so I assume this will be a config that will be "select"ed automatically without users needing to think about whether it is needed? >>> + >>> + for (i = 0; i < resctrl_arch_system_num_rmid_idx(); i++) { >>> + entry = &rmid_ptrs[i]; >>> + if (entry->closid != closid) >>> + continue; >>> + >>> + if (entry->busy) >>> + return true; >>> + } >>> + >>> + return false; >>> +} >> >> If I understand this correctly resctrl_closid_is_dirty() will return true if >> _any_ RMID/PMG associated with a CLOSID is in use. That is, a CLOSID may be >> able to support 100s of PMG but if only one of them is busy then the CLOSID >> will be considered unusable ("dirty"). It sounds to me that there could be scenarios >> when CLOSID could be considered unavailable while there are indeed sufficient >> resources? > > You are right this could happen. I guess the better approach would be to prefer the > cleanest CLOSID that has a clean PMG=0. User-space may not be able to allocate all the > monitor groups immediately, but that would be preferable to failing the control group > creation. > > But as this code doesn't get built until the MPAM driver is merged, I'd like to keep it to > an absolute minimum. This would be more than is needed for MPAM to have close to resctrl > feature-parity, so I'd prefer to do this as an improvement once the MPAM driver is upstream. > > (also in this category is better use of MPAM's monitors and labelling traffic from the iommu) > > >> The function comment states "Determine if clean RMID can be allocate for this >> CLOSID." - if I understand correctly it behaves more like "Determine if all >> RMID associated with CLOSID are available". > > Yes, I'll fix that. I understand your reasoning for the solution chosen. Would you be ok to expand on the function comment more to document the intentions that you summarize above (eg. "This is the absolute minimum solution that will be/should be/could be improved ...")? Reinette
Hi Reinette, On 10/03/2023 19:59, Reinette Chatre wrote: > On 3/3/2023 10:36 AM, James Morse wrote: >> On 02/02/2023 23:46, Reinette Chatre wrote: >>> On 1/13/2023 9:54 AM, James Morse wrote: > > ... > >>>> +bool resctrl_closid_is_dirty(u32 closid) >>>> +{ >>>> + struct rmid_entry *entry; >>>> + int i; >>>> + >>>> + lockdep_assert_held(&rdtgroup_mutex); >>>> + >>>> + if (!IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) >>>> + return false; >> >>> Why is a config option chosen? Is this not something that can be set in the >>> architecture specific code using a global in the form matching existing related >>> items like "arch_has..." or "arch_needs..."? >> >> It doesn't vary by platform, so making it a runtime variable would mean x86 has to carry >> this extra code around, even though it will never use it. Done like this, the compiler can >> dead-code eliminate the below checks and embed the constant return value in all the callers. > This is fair. I missed any other mention of this option in this series so I > assume this will be a config that will be "select"ed automatically without > users needing to think about whether it is needed? Yes, MPAM platforms would unconditionally enable it, as it reflects how MPAM works. [0] Users would never need to know it exists. >>>> + for (i = 0; i < resctrl_arch_system_num_rmid_idx(); i++) { >>>> + entry = &rmid_ptrs[i]; >>>> + if (entry->closid != closid) >>>> + continue; >>>> + >>>> + if (entry->busy) >>>> + return true; >>>> + } >>>> + >>>> + return false; >>>> +} >>> >>> If I understand this correctly resctrl_closid_is_dirty() will return true if >>> _any_ RMID/PMG associated with a CLOSID is in use. That is, a CLOSID may be >>> able to support 100s of PMG but if only one of them is busy then the CLOSID >>> will be considered unusable ("dirty"). It sounds to me that there could be scenarios >>> when CLOSID could be considered unavailable while there are indeed sufficient >>> resources? >> >> You are right this could happen. I guess the better approach would be to prefer the >> cleanest CLOSID that has a clean PMG=0. User-space may not be able to allocate all the >> monitor groups immediately, but that would be preferable to failing the control group >> creation. >> >> But as this code doesn't get built until the MPAM driver is merged, I'd like to keep it to >> an absolute minimum. This would be more than is needed for MPAM to have close to resctrl >> feature-parity, so I'd prefer to do this as an improvement once the MPAM driver is upstream. >> >> (also in this category is better use of MPAM's monitors and labelling traffic from the iommu) >> >> >>> The function comment states "Determine if clean RMID can be allocate for this >>> CLOSID." - if I understand correctly it behaves more like "Determine if all >>> RMID associated with CLOSID are available". >> >> Yes, I'll fix that. > > I understand your reasoning for the solution chosen. Would you be ok to expand on > the function comment more to document the intentions that you summarize above (eg. "This > is the absolute minimum solution that will be/should be/could be improved ...")? Sure thing, Thanks, James [0] https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git/tree/drivers/platform/mpam/Kconfig?h=mpam/snapshot/v6.2&id=ef6b11256ba2cceaff846c96183e8eb6019642d7
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h index 013c8fc9fd28..adae6231324f 100644 --- a/arch/x86/kernel/cpu/resctrl/internal.h +++ b/arch/x86/kernel/cpu/resctrl/internal.h @@ -509,6 +509,7 @@ int rdtgroup_pseudo_lock_create(struct rdtgroup *rdtgrp); void rdtgroup_pseudo_lock_remove(struct rdtgroup *rdtgrp); struct rdt_domain *get_domain_from_cpu(int cpu, struct rdt_resource *r); int closids_supported(void); +bool resctrl_closid_is_dirty(u32 closid); void closid_free(int closid); int alloc_rmid(u32 closid); void free_rmid(u32 closid, u32 rmid); diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c index 347be3767241..190ac183139e 100644 --- a/arch/x86/kernel/cpu/resctrl/monitor.c +++ b/arch/x86/kernel/cpu/resctrl/monitor.c @@ -327,6 +327,37 @@ static struct rmid_entry *resctrl_find_free_rmid(u32 closid) return ERR_PTR(-ENOSPC); } +/** + * resctrl_closid_is_dirty - Determine if clean RMID can be allocate for this + * CLOSID. + * @closid: The CLOSID that is being queried. + * + * MPAM's equivalent of RMID are per-CLOSID, meaning a freshly allocate CLOSID + * may not be able to allocate clean RMID. To avoid this the allocator will + * only return clean CLOSID. + */ +bool resctrl_closid_is_dirty(u32 closid) +{ + struct rmid_entry *entry; + int i; + + lockdep_assert_held(&rdtgroup_mutex); + + if (!IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) + return false; + + for (i = 0; i < resctrl_arch_system_num_rmid_idx(); i++) { + entry = &rmid_ptrs[i]; + if (entry->closid != closid) + continue; + + if (entry->busy) + return true; + } + + return false; +} + /* * As of now the RMIDs allocation is the same in each domain. * However we keep track of which packages the RMIDs diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c index ac88610a6946..e1f879e13823 100644 --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c @@ -93,7 +93,7 @@ void rdt_last_cmd_printf(const char *fmt, ...) * - Our choices on how to configure each resource become progressively more * limited as the number of resources grows. */ -static int closid_free_map; +static unsigned long closid_free_map; static int closid_free_map_len; int closids_supported(void) @@ -119,14 +119,17 @@ static void closid_init(void) static int closid_alloc(void) { - u32 closid = ffs(closid_free_map); + u32 closid; - if (closid == 0) - return -ENOSPC; - closid--; - closid_free_map &= ~(1 << closid); + for_each_set_bit(closid, &closid_free_map, closid_free_map_len) { + if (resctrl_closid_is_dirty(closid)) + continue; - return closid; + clear_bit(closid, &closid_free_map); + return closid; + } + + return -ENOSPC; } void closid_free(int closid)