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

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Drew,

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.

> +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.

...

> +
> +/*
> + * 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!).

> +static void cbqri_resctrl_pick_caches(void)
> +{
> +	bool dropped[RDT_NUM_RESOURCES] = { };
> +	struct cbqri_controller *ctrl;
> +
> +	guard(mutex)(&cbqri_controllers_lock);
> +
> +	list_for_each_entry(ctrl, &cbqri_controllers, list) {
> +		struct cbqri_resctrl_res *cbqri_res;
> +		int rid;
> +
> +		if (ctrl->type != CBQRI_CONTROLLER_TYPE_CAPACITY)
> +			continue;
> +		if (!ctrl->alloc_capable)
> +			continue;
> +
> +		rid = cbqri_cache_level_to_rid(ctrl->cache.cache_level);
> +		if (rid < 0) {
> +			pr_info("skipping controller at unsupported cache level %u\n",
> +				ctrl->cache.cache_level);
> +			continue;
> +		}
> +
> +		if (dropped[rid])
> +			continue;
> +
> +		cbqri_res = &cbqri_resctrl_resources[rid];
> +		if (cbqri_res->ctrl) {
> +			/*
> +			 * CCs at the same cache level must agree on every cap
> +			 * resctrl exposes globally. On a mismatch, drop the
> +			 * whole level rather than expose one controller's caps
> +			 * while programming another's. The other level, and any
> +			 * other QoS feature, is left untouched.
> +			 */
> +			if (cbqri_res->ctrl->rcid_count != ctrl->rcid_count ||
> +			    cbqri_res->ctrl->cc.ncblks != ctrl->cc.ncblks ||
> +			    cbqri_res->ctrl->cc.supports_alloc_at_code !=
> +				    ctrl->cc.supports_alloc_at_code ||
> +			    cbqri_res->ctrl->alloc_capable != ctrl->alloc_capable) {
> +				pr_err("L%d controllers have mismatched capabilities, skipping this level\n",
> +				       ctrl->cache.cache_level);
> +				cbqri_res->ctrl = NULL;
> +				dropped[rid] = true;
> +			}
> +			continue;
> +		}
> +
> +		cbqri_res->ctrl = ctrl;
> +	}
> +}
> +
> +/*
> + * Fill the rdt_resource fields for one picked rid. An rid with no picked
> + * controller is left untouched so it stays out of resctrl_arch_get_resource().
> + */
> +static void cbqri_resctrl_control_init(struct cbqri_resctrl_res *cbqri_res)
> +{
> +	struct cbqri_controller *ctrl = cbqri_res->ctrl;
> +	struct rdt_resource *res = &cbqri_res->resctrl_res;
> +
> +	if (!ctrl)
> +		return;
> +
> +	switch (res->rid) {
> +	case RDT_RESOURCE_L2:
> +	case RDT_RESOURCE_L3:
> +		res->name = (res->rid == RDT_RESOURCE_L2) ? "L2" : "L3";
> +		res->schema_fmt = RESCTRL_SCHEMA_BITMAP;
> +		res->ctrl_scope = (res->rid == RDT_RESOURCE_L2) ?
> +				    RESCTRL_L2_CACHE : RESCTRL_L3_CACHE;
> +		res->cache.cbm_len = ctrl->cc.ncblks;
> +		res->cache.shareable_bits = 0;
> +		res->cache.min_cbm_bits = 1;
> +		res->cache.arch_has_sparse_bitmasks = false;
> +		res->cdp_capable = ctrl->cc.supports_alloc_at_code;
> +		res->alloc_capable = ctrl->alloc_capable;
> +		INIT_LIST_HEAD(&res->ctrl_domains);
> +		INIT_LIST_HEAD(&res->mon_domains);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +static void cbqri_resctrl_accumulate_caps(void)
> +{
> +	int rid;
> +
> +	for (rid = 0; rid < RDT_NUM_RESOURCES; rid++) {
> +		struct cbqri_resctrl_res *hw_res = &cbqri_resctrl_resources[rid];
> +
> +		if (!hw_res->ctrl)
> +			continue;
> +		if (hw_res->ctrl->alloc_capable)
> +			exposed_alloc_capable = true;
> +	}
> +}
> +
> +/*
> + * Create, list-insert, and online a fresh ctrl_domain backing ctrl on
> + * resource res, seeded with cpu and identified by dom_id. Caller must
> + * hold cbqri_domain_list_lock and must have already verified that no
> + * existing ctrl_domain on res carries this id.
> + */
> +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.

> +
> +	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.

> +
> +	return domain;
> +}
> +
> +static int cbqri_attach_cpu_to_cap_ctrl(struct cbqri_controller *ctrl,
> +					unsigned int cpu)
> +{
> +	struct cbqri_resctrl_res *hw_res;
> +	struct rdt_ctrl_domain *domain;
> +	struct rdt_resource *res;
> +	int dom_id;
> +	int rid;
> +
> +	rid = cbqri_cache_level_to_rid(ctrl->cache.cache_level);
> +	if (rid < 0)
> +		return 0;
> +	hw_res = &cbqri_resctrl_resources[rid];
> +
> +	if (!hw_res->ctrl)
> +		return 0;
> +
> +	res = &hw_res->resctrl_res;
> +	dom_id = ctrl->cache.cache_id;
> +
> +	domain = cbqri_find_ctrl_domain(&res->ctrl_domains, dom_id);
> +	if (domain) {
> +		cpumask_set_cpu(cpu, &domain->hdr.cpu_mask);
> +		return 0;
> +	}
> +
> +	domain = cbqri_create_ctrl_domain(ctrl, res, cpu, dom_id);
> +	if (IS_ERR(domain))
> +		return PTR_ERR(domain);
> +
> +	return 0;
> +}
> +
> +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)

> +			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.

> +
> +/*
> + * Remove a CPU from every domain it was attached to. The per-resource
> + * detach helpers act only when the CPU is set in a domain's mask, so this
> + * is idempotent and undoes a partial online attach as well as a full
> + * offline. Caller holds cbqri_domain_list_lock.
> + */
> +static void cbqri_detach_cpu_from_all_ctrls(unsigned int cpu)
> +{
> +	int rid;
> +
> +	lockdep_assert_held(&cbqri_domain_list_lock);
> +
> +	for (rid = 0; rid < RDT_NUM_RESOURCES; rid++) {
> +		struct cbqri_resctrl_res *hw_res = &cbqri_resctrl_resources[rid];
> +
> +		if (!hw_res->ctrl)
> +			continue;
> +		cbqri_detach_cpu_from_ctrl_domains(&hw_res->resctrl_res, cpu);
> +	}
> +}
> +
> +/*
> + * 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?

> +		if (!ctrl->alloc_capable)
> +			continue;
> +
> +		err = cbqri_attach_cpu_to_cap_ctrl(ctrl, cpu);
> +		if (err) {
> +			cbqri_detach_cpu_from_all_ctrls(cpu);
> +			return err;
> +		}
> +	}
> +
> +	return err;
> +}
> +
Reinette