From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 242A5271470 for ; Fri, 19 Dec 2025 12:02:46 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1766145772; cv=none; b=lDgffzIX/K5hoX+UDN7FouXjInoN79nMKWP7KKvS7kpvyJeG/QbpspkEIjCb9duKWygk6dzCcs3ZUUXevlX5uFVpto065S0kCQp9ZI91ORvnaJc25jzG7Y6fLFG5g6taDFM7UjFkC9seJC5LyDXjIoPabOKsfnv8n3PNNOOnPGI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1766145772; c=relaxed/simple; bh=TxbE0EL6QFacMCEpR+Mg/rbi91k/ZtaSIS9samgxDvw=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=naCKBvRhz81n7v+KGGbyzy7e0uNqUQkvn/k2nMP10nHAVnnxaphzkm9+wT0q6w+m6lj63+/vlTPbnPLeQcCaQSYuPWC/OUnoOKSY3WhgWJSCh3qpiGhGBV8d/2OWj9VUWXSUD8L/J+ueV5fYYaK7FOGjHDzVFUFCV6acJMpd9/U= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com 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 284D5FEC; Fri, 19 Dec 2025 04:02:39 -0800 (PST) Received: from [10.1.196.46] (e134344.arm.com [10.1.196.46]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 7FF953F5CA; Fri, 19 Dec 2025 04:02:42 -0800 (PST) Message-ID: <4495db3f-cfb8-4571-b83a-10a24f7b73a9@arm.com> Date: Fri, 19 Dec 2025 12:02:41 +0000 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [RFC PATCH 07/38] arm_mpam: resctrl: Add boilerplate cpuhp and domain allocation To: Jonathan Cameron , James Morse Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, D Scott Phillips OS , carl@os.amperecomputing.com, lcherian@marvell.com, bobo.shaobowang@huawei.com, tan.shaopeng@fujitsu.com, baolin.wang@linux.alibaba.com, Jamie Iles , Xin Hao , peternewman@google.com, dfustini@baylibre.com, amitsinght@marvell.com, David Hildenbrand , Dave Martin , Koba Ko , Shanker Donthineni , fenghuay@nvidia.com, baisheng.gao@unisoc.com, Gavin Shan , rohit.mathew@arm.com, reinette.chatre@intel.com, Punit Agrawal References: <20251205215901.17772-1-james.morse@arm.com> <20251205215901.17772-8-james.morse@arm.com> <20251218113014.00002691@huawei.com> From: Ben Horgan Content-Language: en-US In-Reply-To: <20251218113014.00002691@huawei.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Hi Jonathan, On 12/18/25 11:30, Jonathan Cameron wrote: > On Fri, 5 Dec 2025 21:58:30 +0000 > James Morse wrote: > >> resctrl has its own data structures to describe its resources. We >> can't use these directly as we play tricks with the 'MBA' resource, >> picking the MPAM controls or monitors that best apply. We may export >> the same component as both L3 and MBA. >> >> Add mpam_resctrl_exports[] as the array of class->resctrl mappings we >> are exporting, and add the cpuhp hooks that allocated and free the >> resctrl domain structures. >> >> While we're here, plumb in a few other obvious things. >> >> CONFIG_ARM_CPU_RESCTRL is used to allow this code to be built >> even though it can't yet be linked against resctrl. >> >> Signed-off-by: James Morse > Hi, > > A few code flow related comments. Fairly trivial stuff but I think > some parts of this can be made more readable / maintainable with > minor reorganization. > > Jonathan > > >> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c >> index 2996ad93fc3e..efaf7633bc35 100644 >> --- a/drivers/resctrl/mpam_devices.c >> +++ b/drivers/resctrl/mpam_devices.c > ... > >> @@ -2516,6 +2522,12 @@ static void mpam_enable_once(void) >> mutex_unlock(&mpam_list_lock); >> cpus_read_unlock(); >> >> + if (!err) { >> + err = mpam_resctrl_setup(); >> + if (err) >> + pr_err("Failed to initialise resctrl: %d\n", err); >> + } >> + >> if (err) { >> mpam_disable_reason = "Failed to enable."; >> schedule_work(&mpam_broken_work); > > I'd be tempted to move this to an error handling block via a goto > making this bit > if (err) > goto err_disable_mpam; > > err = mpam_resctrl_setup(); > if (err) { > pr_err(); > goto err_dsiable_mpam; > } > > Up to you though. Personally I like all my good paths as straight line > code with the errors handled in if (err) as that consistency really helps > readability. > >> diff --git a/drivers/resctrl/mpam_resctrl.c b/drivers/resctrl/mpam_resctrl.c >> new file mode 100644 >> index 000000000000..320cebbd37ce >> --- /dev/null >> +++ b/drivers/resctrl/mpam_resctrl.c >> @@ -0,0 +1,329 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +// Copyright (C) 2025 Arm Ltd. >> + >> +#define pr_fmt(fmt) "%s:%s: " fmt, KBUILD_MODNAME, __func__ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> + >> +#include "mpam_internal.h" > > >> +static struct mpam_resctrl_dom * >> +mpam_resctrl_alloc_domain(unsigned int cpu, struct mpam_resctrl_res *res) >> +{ >> + int err; >> + struct mpam_resctrl_dom *dom; >> + struct rdt_mon_domain *mon_d; >> + struct rdt_ctrl_domain *ctrl_d; >> + struct mpam_class *class = res->class; >> + struct mpam_component *comp_iter, *ctrl_comp; >> + struct rdt_resource *r = &res->resctrl_res; >> + >> + lockdep_assert_held(&domain_list_lock); >> + >> + ctrl_comp = NULL; >> + guard(srcu)(&mpam_srcu); >> + list_for_each_entry_srcu(comp_iter, &class->components, class_list, >> + srcu_read_lock_held(&mpam_srcu)) { >> + if (cpumask_test_cpu(cpu, &comp_iter->affinity)) { >> + ctrl_comp = comp_iter; >> + break; >> + } >> + } >> + >> + /* class has no component for this CPU */ >> + if (WARN_ON_ONCE(!ctrl_comp)) >> + return ERR_PTR(-EINVAL); >> + >> + dom = kzalloc_node(sizeof(*dom), GFP_KERNEL, cpu_to_node(cpu)); >> + if (!dom) >> + return ERR_PTR(-ENOMEM); >> + >> + if (exposed_alloc_capable) { >> + dom->ctrl_comp = ctrl_comp; >> + >> + ctrl_d = &dom->resctrl_ctrl_dom; >> + mpam_resctrl_domain_hdr_init(cpu, ctrl_comp, &ctrl_d->hdr); >> + ctrl_d->hdr.type = RESCTRL_CTRL_DOMAIN; >> + /* TODO: this list should be sorted */ >> + list_add_tail_rcu(&ctrl_d->hdr.list, &r->ctrl_domains); >> + err = resctrl_online_ctrl_domain(r, ctrl_d); >> + if (err) { >> + dom = ERR_PTR(err); >> + goto offline_ctrl_domain; >> + } >> + } else { >> + pr_debug("Skipped control domain online - no controls\n"); >> + } >> + >> + if (exposed_mon_capable) { >> + mon_d = &dom->resctrl_mon_dom; >> + mpam_resctrl_domain_hdr_init(cpu, ctrl_comp, &mon_d->hdr); >> + mon_d->hdr.type = RESCTRL_MON_DOMAIN; >> + /* TODO: this list should be sorted */ >> + list_add_tail_rcu(&mon_d->hdr.list, &r->mon_domains); >> + err = resctrl_online_mon_domain(r, mon_d); >> + if (err) { >> + dom = ERR_PTR(err); >> + goto offline_mon_hdr; >> + } >> + } else { >> + pr_debug("Skipped monitor domain online - no monitors\n"); >> + } >> + goto out; > > To keep flow simple, return here. I thought maybe there was more stuff > that was always done (added in later patches) but not seeing that. > If there were then it would be a fairly strong indicator that a different > code structure makes more sense - probably with some helper functions. Makes sense. > >> + >> +offline_mon_hdr: >> + mpam_resctrl_offline_domain_hdr(cpu, &mon_d->hdr); >> +offline_ctrl_domain: >> + resctrl_offline_ctrl_domain(r, ctrl_d); >> +out: >> + return dom; >> +} >> + >> +static struct mpam_resctrl_dom * >> +mpam_resctrl_get_domain_from_cpu(int cpu, struct mpam_resctrl_res *res) >> +{ >> + struct mpam_resctrl_dom *dom; >> + struct rdt_ctrl_domain *ctrl_d; >> + >> + lockdep_assert_cpus_held(); >> + >> + list_for_each_entry_rcu(ctrl_d, &res->resctrl_res.ctrl_domains, >> + hdr.list) { >> + dom = container_of(ctrl_d, struct mpam_resctrl_dom, >> + resctrl_ctrl_dom); > > I'm lazy so haven't checked for more code here in later patches, but > if not, why not iterate the list to access the domain directly rather > than jumping through the rdt_ctrl_domain? > > Something along lines of: > > list_for_each_entry_rcu(dom, &res->resctrl_res.ctrl_domains, > resctrl_ctrl_dom.hdr.list) { > } > Unless I've misunderstood I don't think this works because it's not what the fs/resctrl code expects. >> + >> + if (cpumask_test_cpu(cpu, &dom->ctrl_comp->affinity)) >> + return dom; >> + } >> + >> + return NULL; >> +} >> + >> +int mpam_resctrl_online_cpu(unsigned int cpu) >> +{ >> + int i; >> + struct mpam_resctrl_dom *dom; >> + struct mpam_resctrl_res *res; >> + >> + guard(mutex)(&domain_list_lock); >> + for (i = 0; i < RDT_NUM_RESOURCES; i++) { > > I'd narrow the scope for dom and res to inside the loop. > Maybe put the iterator in the for loop init (now considered > acceptable in kernel code) > > Similar applies in various other places. No that important > for functions that more or less just consist of a loop though. I've done a bit of scope reducing here and in some other places. > >> + res = &mpam_resctrl_controls[i]; >> + if (!res->class) >> + continue; // dummy_resource; >> + >> + dom = mpam_resctrl_get_domain_from_cpu(cpu, res); >> + if (!dom) >> + dom = mpam_resctrl_alloc_domain(cpu, res); >> + if (IS_ERR(dom)) >> + return PTR_ERR(dom); >> + } >> + >> + resctrl_online_cpu(cpu); >> + >> + return 0; >> +} > >> +int mpam_resctrl_setup(void) >> +{ >> + int err = 0; >> + enum resctrl_res_level i; >> + struct mpam_resctrl_res *res; >> + >> + cpus_read_lock(); >> + for (i = 0; i < RDT_NUM_RESOURCES; i++) { >> + res = &mpam_resctrl_controls[i]; >> + INIT_LIST_HEAD_RCU(&res->resctrl_res.ctrl_domains); >> + INIT_LIST_HEAD_RCU(&res->resctrl_res.mon_domains); >> + res->resctrl_res.rid = i; >> + } >> + >> + /* TODO: pick MPAM classes to map to resctrl resources */ >> + >> + /* Initialise the resctrl structures from the classes */ >> + for (i = 0; i < RDT_NUM_RESOURCES; i++) { >> + res = &mpam_resctrl_controls[i]; >> + if (!res->class) >> + continue; // dummy resource >> + >> + err = mpam_resctrl_control_init(res, i); >> + if (err) { >> + pr_debug("Failed to initialise rid %u\n", i); >> + break; >> + } >> + } >> + cpus_read_unlock(); >> + >> + if (err || (!exposed_alloc_capable && !exposed_mon_capable)) { >> + if (err) >> + pr_debug("Internal error %d - resctrl not supported\n", >> + err); >> + else >> + pr_debug("No alloc(%u) or monitor(%u) found - resctrl not supported\n", >> + exposed_alloc_capable, exposed_mon_capable); >> + err = -EOPNOTSUPP; > > return -EOPNOTSUPP; here to make the code flow simpler. > Mind you nice to avoid eating err if it is set and the sharing here doesn't seem > all that useful so perhaps just make this: > > if (err) { > pr_debug("Internal error %d - resctrl not supported\n", err); > return err; > } > > if (!exposed_alloc_capable && !exposed_mon_capable) { > pr_debug("No alloc(%u) or monitor(%u) found - resctrl not supported\n", > exposed_alloc_capable, exposed_mon_capable); > return -EOPNOTSUPP; > } I've gone for the second option. > > >> + } >> + >> + if (!err) { >> + if (!is_power_of_2(mpam_pmg_max + 1)) { >> + /* >> + * If not all the partid*pmg values are valid indexes, >> + * resctrl may allocate pmg that don't exist. This >> + * should cause an error interrupt. >> + */ >> + pr_warn("Number of PMG is not a power of 2! resctrl may misbehave"); >> + } >> + >> + /* TODO: call resctrl_init() */ >> + } >> + >> + return err; >> +} Thanks, Ben