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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 57001C44501 for ; Sun, 12 Jul 2026 19:15:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=fGKOlbrD9qsZIVZxTXl7WA6Pwc0ZrT5uyvaV4BJ7k1s=; b=enjT2HWqy3Bet8 J8EzfL5Ftym65G3wGrpX99TshHMewxmSFAJ/vce4YNU8npD/cMbfi7RYp+LCyJ/xvR8bdburLK7jK IzAt26eL74jkMhpL7KBzkrL9pyLkHRO4CVWeKEyhHXdIZGe65ptk6yfqjOwDHfM2pubfF94FbSGv/ t6J0CT+ryx7Xh0wiZCTETGPmICXJ4HIVLKnqZo60AUQ3t+Thm2lPx8WcxHn/ULdBsEJMrPxignCPJ wK9PeMFk4F2ZaxSpw3QFsaqRIB8v1ORd8ZP6w6G0yxp5n1ooxyTN/8GXAjPCAoAazYdstkqt2JRbY SIFMCbqHjXJ5dvpInjWQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.99.1 #2 (Red Hat Linux)) id 1wizdq-00000007hGV-0bIo; Sun, 12 Jul 2026 19:15:06 +0000 Received: from sea.source.kernel.org ([2600:3c0a:e001:78e:0:1991:8:25]) by bombadil.infradead.org with esmtps (Exim 4.99.1 #2 (Red Hat Linux)) id 1wizdp-00000007hG9-0pTx for linux-riscv@lists.infradead.org; Sun, 12 Jul 2026 19:15:05 +0000 Received: from smtp.kernel.org (quasi.space.kernel.org [100.103.45.18]) by sea.source.kernel.org (Postfix) with ESMTP id 5FE8B41A90; Sun, 12 Jul 2026 19:15:04 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id B8B0A1F000E9; Sun, 12 Jul 2026 19:15:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1783883704; bh=73b4pL2XQtnec2/Fy5ja53Ba+xumzUzAkyQQn/NfjQc=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=A+1O9GP1ucy3KjYEern4zyQU/VnyDWPh8PtiWqJT8ty6GYQZqbKu1DkSIavsJncpD y3RZOJt5cI+dOZnoZefjzOrN+aUrcYds6YmyMBOdD8ssSPDV63Qs7o22SDqmlSGLjp umHQkoGwuBXkG2XG/EBnZ5+nkhIVtfYEXUtgGxWSatDlouhoNvsxnqqwrANc4Ppo4Q XmcRRIf8VIiN5HosFT/oTtM3/zjkNmI/IapPKXbFOfWY2+Dan3wVUw1AqY0v6nEmZt bGtHO4VTdUx30RXwrWDrLT7NgTHC47U+uf//4DgQpir0YXJ9Ho+XxNYaTEiUEu1h6b fwbdSyh1O4DQA== Date: Sun, 12 Jul 2026 12:15:02 -0700 From: Drew Fustini To: Reinette Chatre Cc: Adrien Ricciardi , Alexandre Ghiti , Atish Kumar Patra , Atish Patra , Babu Moger , Ben Horgan , Borislav Petkov , Chen Pei , Conor Dooley , Conor Dooley , Dave Hansen , Dave Martin , Fenghua Yu , Gong Shuai , Gong Shuai , guo.wenjia23@zte.com.cn, James Morse , Kornel =?utf-8?Q?Dul=C4=99ba?= , Krzysztof Kozlowski , liu.qingtao2@zte.com.cn, Liu Zhiwei , Palmer Dabbelt , Paul Walmsley , Peter Newman , Radim =?utf-8?B?S3LEjW3DocWZ?= , Rob Herring , Samuel Holland , Sebastian Andrzej Siewior , Tony Luck , Vasudevan Srinivasan , Ved Shanbhogue , Weiwei Li , yunhui cui , linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org, x86@kernel.org, devicetree@vger.kernel.org, linux-rt-devel@lists.linux.dev, linux-doc@vger.kernel.org Subject: Re: [PATCH v4 5/8] riscv_cbqri: resctrl: Add cache allocation via capacity block mask Message-ID: References: <20260706-dfustini-atl-sc-cbqri-dt-v4-0-e75c20201d64@kernel.org> <20260706-dfustini-atl-sc-cbqri-dt-v4-5-e75c20201d64@kernel.org> <3e7070fa-16f3-453e-9f0e-9c1a2ef38f9f@intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <3e7070fa-16f3-453e-9f0e-9c1a2ef38f9f@intel.com> X-BeenThere: linux-riscv@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-riscv" Errors-To: linux-riscv-bounces+linux-riscv=archiver.kernel.org@lists.infradead.org On Tue, Jul 07, 2026 at 11:17:14AM -0700, Reinette Chatre wrote: > Hi Drew, Thanks for reviewing and your suggestions. > On 7/6/26 4:06 PM, Drew Fustini wrote: > > ... > > > diff --git a/drivers/resctrl/cbqri_resctrl.c b/drivers/resctrl/cbqri_resctrl.c > > new file mode 100644 > > index 000000000000..cb12a868561b > > --- /dev/null > > +++ b/drivers/resctrl/cbqri_resctrl.c > > ... > > > + > > +/* > > + * fs/resctrl unconditionally references the symbols below before checking > > + * mon_capable. They are stubs for features CBQRI does not yet support. > > + */ > > resctrl should not access monitoring related arch functions if the arch > does not support monitoring. Could you please highlight which ones are causing > problems? From what I can tell, the first one below, resctrl_arch_is_evt_configurable(), > is indeed only called via resctrl_l3_mon_resource_init() if the L3 resource is > mon_capable. You are right, none of the monitoring stubs run for the allocation-only CBQRI implementation in this series. The stubs exist only so fs/resctrl resolves the symbols at link time. I'll drop the comment. > > +bool resctrl_arch_is_evt_configurable(enum resctrl_event_id evt) > > +{ > > + return false; > > +} > > + > > +void *resctrl_arch_mon_ctx_alloc(struct rdt_resource *r, > > + enum resctrl_event_id evtid) > > Similarly, this should only be called when reading monitoring data which should only be > possible if the resource is capable of monitoring. Agreed, same as above. > > +/* > > + * Walk cbqri_controllers and pick one capacity controller (CC) per cache > > + * level (L2/L3) to back the corresponding RDT_RESOURCE_L*. When more than > > + * one CC sits at the same level (e.g. one per socket), they must agree on > > + * rcid_count / ncblks / alloc_capable. A level whose controllers disagree > > + * is dropped, since resctrl exposes a single set of caps per rid, but the > > + * other level is still picked. The first matching controller wins. > > + */ > > It works but I find it to be a potentially confusing approach (just a personal opinion!). Good point. I'll restructure it to pick per cache level instead of per controller, which removes the separate dropped[] array and the overloaded NULL that made it confusing. > > +static struct rdt_ctrl_domain *cbqri_create_ctrl_domain(struct cbqri_controller *ctrl, > > + struct rdt_resource *res, > > + unsigned int cpu, int dom_id) > > +{ > > + struct rdt_ctrl_domain *domain; > > + struct list_head *pos = NULL; > > + int err; > > + > > + domain = cbqri_new_domain(ctrl); > > + if (!domain) > > + return ERR_PTR(-ENOMEM); > > + > > + cpumask_set_cpu(cpu, &domain->hdr.cpu_mask); > > + domain->hdr.id = dom_id; > > + domain->hdr.type = RESCTRL_CTRL_DOMAIN; > > Please also initialize domain->hdr.rid that is referenced by the often-used > domain_header_is_valid() helper. Okay, I will set 'domain->hdr.rid = res->rid'. > > + > > + err = cbqri_init_domain_ctrlval(res, domain); > > + if (err) { > > + kfree(container_of(domain, struct cbqri_resctrl_dom, > > + resctrl_ctrl_dom)); > > + return ERR_PTR(err); > > + } > > + > > + /* Insert sorted by id so user-visible ordering is deterministic. */ > > + resctrl_find_domain(&res->ctrl_domains, dom_id, &pos); > > + list_add_tail(&domain->hdr.list, pos); > > The domain list became an RCU list when resctrl started supporting MPAM. > commit fb700810d30b ("x86/resctrl: Separate arch and fs resctrl locks") contains > a great description of the motivation and the different accesses that the list > should support. Historically resctrl always accessed the list with CPU hotplug lock > held for which above is ok but there are some planned changes as part of a fix > that accesses the list via an RCU read-side critical section. Even with this addition > there is no immediate impact to this enabling since it is related to the > MBA software controller but I think it is best for archs and resctrl to agree on > how the domain list can be accessed safely. > > For reference to the upcoming resctrl usage see > https://lore.kernel.org/lkml/4c88e01e29df638d9ecad71b2ee3b411e24067bd.1783377598.git.reinette.chatre@intel.com/ > > > + > > + resctrl_online_ctrl_domain(res, domain); > > With the transition to RCU list the domain should only be added to the list after it is > fully initialized. Thanks for the explanation. I will adopt the RCU list convention. Fully initialize the domain, online it, and then publish it. > > +static void cbqri_detach_cpu_from_ctrl_domains(struct rdt_resource *res, > > + unsigned int cpu) > > +{ > > + struct rdt_ctrl_domain *domain, *tmp; > > + > > + list_for_each_entry_safe(domain, tmp, &res->ctrl_domains, hdr.list) { > > + if (!cpumask_test_cpu(cpu, &domain->hdr.cpu_mask)) > > + continue; > > + cpumask_clear_cpu(cpu, &domain->hdr.cpu_mask); > > + if (cpumask_empty(&domain->hdr.cpu_mask)) { > > + resctrl_offline_ctrl_domain(res, domain); > > + list_del(&domain->hdr.list); > > (also related to switch to RCU list, remove domain from list before > starting to offline it) Will do, thanks. > > + kfree(container_of(domain, struct cbqri_resctrl_dom, > > + resctrl_ctrl_dom)); > > + } > > + } > > +} > > How CPUs are associated with control domains are not clear to me. Above appears to > be consequence of how platform driver initializes so I will comment more there (patch #8). > Just for above, it is unexpected that a CPU needs to be tested against cpu_mask > of all control domains. Would a direct query that uses the CPU's cache ID not be > quicker? Also, above creates impression that a CPU may belong to multiple control > domains which should not happen. Thanks for the suggestion. I will change it to lookup the domain up directly by the cache id. > > +/* > > + * Attach a CPU to every controller that claims it. On failure, detach the > > + * CPU from everything attached so far: the cpuhp core does not run this > > + * state's offline teardown when its startup fails, so a partial attach > > + * would otherwise leak into the domain cpu_masks. Caller holds > > + * cbqri_domain_list_lock. > > + */ > > +static int cbqri_attach_cpu_to_all_ctrls(unsigned int cpu) > > +{ > > + struct cbqri_controller *ctrl; > > + int err = 0; > > + > > + lockdep_assert_held(&cbqri_domain_list_lock); > > + > > + /* > > + * Hold cbqri_controllers_lock across the walk so a controller > > + * registered after boot cannot corrupt it. The register path takes > > + * it as a leaf and never cbqri_domain_list_lock, so this nesting > > + * cannot invert. > > + */ > > + guard(mutex)(&cbqri_controllers_lock); > > + list_for_each_entry(ctrl, &cbqri_controllers, list) { > > + if (ctrl->type != CBQRI_CONTROLLER_TYPE_CAPACITY) > > + continue; > > + if (!cpumask_test_cpu(cpu, &ctrl->cache.cpu_mask)) > > + continue; > > What will happen if @cpu was offline when cbqri_capacity_probe() ran? From what > I can tell @cpu will not be in ctrl->cache.cpu_mask in this scenario? Yes, this is a problem. I will drop the probe-time cpu_mask and associate cpus with domains the way you suggest, from the cpu's own cache id at hotplug time. Thanks, Drew _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv