Re: [PATCH v4 5/8] riscv_cbqri: resctrl: Add cache allocation via capacity block mask

[Drew Fustini <fustini@kernel.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