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

Hi Drew,

On 9/17/26 9:39 AM, Drew Fustini wrote:
> diff --git a/arch/riscv/include/asm/resctrl.h b/arch/riscv/include/asm/resctrl.h
> new file mode 100644
> index 000000000000..b08f4e12f7aa
> --- /dev/null
> +++ b/arch/riscv/include/asm/resctrl.h

...

> +/**
> + * resctrl_arch_alloc_capable() - any CBQRI controller exposes resctrl alloc
> + *
> + * Returns true once at least one CBQRI controller has successfully probed for
> + * a resctrl-exposed cache capacity allocation feature. Only meaningful after
> + * cbqri_resctrl_setup() runs at late_initcall.
> + */
> +bool resctrl_arch_alloc_capable(void);
> +
> +/**
> + * resctrl_arch_mon_capable() - any CBQRI controller exposes resctrl monitoring
> + *
> + * The CBQRI driver implements capacity allocation only and wires up no
> + * monitoring events, so this always returns false. fs/resctrl references it
> + * unconditionally, hence the stub.
> + */
> +bool resctrl_arch_mon_capable(void);
> +

fyi ... I aim to comment more details later in this patch but for now please note that
there are plans to remove the above two hooks since resctrl self has needed
information via the rdt_resource::alloc_capable and rdt_resource::mon_capable flags.

For reference:
	https://lore.kernel.org/lkml/[email protected]/

I see this has impact on this driver that I comment more below.

...

> diff --git a/drivers/resctrl/cbqri_resctrl.c b/drivers/resctrl/cbqri_resctrl.c
> new file mode 100644
> index 000000000000..0c4bfa2a7f43
> --- /dev/null
> +++ b/drivers/resctrl/cbqri_resctrl.c
> @@ -0,0 +1,785 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#define pr_fmt(fmt) "%s:%s: " fmt, KBUILD_MODNAME, __func__
> +
> +#include <linux/bitfield.h>
> +#include <linux/cacheinfo.h>
> +#include <linux/cleanup.h>
> +#include <linux/riscv_cbqri.h>

Just an observation (I have no intention to comment on the style):
The new c files in this series seem to make an effort to have the #include
files organized alphabetically, except when if comes to the file above?

> +#include <linux/cpu.h>
> +#include <linux/cpufeature.h>
> +#include <linux/cpuhotplug.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/rculist.h>
> +#include <linux/resctrl.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +#include <asm/csr.h>
> +#include <asm/qos.h>
> +
> +#include "cbqri_internal.h"
> +
> +struct cbqri_resctrl_res {
> +	struct cbqri_controller *ctrl;
> +	struct rdt_resource     resctrl_res;
> +	bool                    cdp_enabled;
> +};
> +
> +struct cbqri_resctrl_dom {
> +	struct rdt_ctrl_domain  resctrl_ctrl_dom;
> +	struct cbqri_controller *hw_ctrl;
> +};

Is cbqri_resctrl_dom::hw_ctrl necessary? From what I can tell it is
initialized from cbqri_resctrl_res::ctrl when a new domain is created
and thus identical in all domains that belong to a resource. 

It looks to me as though the resource is always available when the associated
controller information is needed so it looks like just cbqri_resctrl_res::ctrl
could do?

The way the data is organized results in potentially confusing code since, for
example, when initializing the control values of a single domain
(cbqri_init_domain_ctrlval()) the RCID count is obtained from the domain's
cbqri_resctrl_dom::hw_ctrl::rcid_count but when initializing the control values of
all domains (resctrl_arch_reset_all_ctrls()) the RCID count is obtained from the
resource's cbqri_resctrl_res::ctrl::rcid_count instead of each domain's domain
cbqri_resctrl_dom::hw_ctrl::rcid_count.

> +
> +static struct cbqri_resctrl_res cbqri_resctrl_resources[RDT_NUM_RESOURCES];
> +
> +static bool exposed_alloc_capable;
> +
> +/* Protects ctrl_domain list mutations across CPU hotplug. */
> +static DEFINE_MUTEX(cbqri_domain_list_lock);
> +
> +static struct rdt_ctrl_domain *
> +cbqri_find_ctrl_domain(struct list_head *h, int id)
> +{
> +	struct rdt_domain_hdr *hdr = resctrl_find_domain(h, id, NULL);
> +
> +	return hdr ? container_of(hdr, struct rdt_ctrl_domain, hdr) : NULL;
> +}
> +

...

> +
> +bool resctrl_arch_alloc_capable(void)
> +{
> +	return exposed_alloc_capable;
> +}
> +

...

> +
> +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;
> +	}
> +}

Since it captures whether any of the resources are alloc_capable it looks like
exposed_alloc_capable indeed reflects the same information as what resctrl will
use after resctrl_arch_alloc_capable() is dropped (see patch linked earlier).
Except that cbqri_resctrl_teardown() only resets exposed_alloc_capable but not
the rdt_resource::mon_capable and rdt_resource::alloc_capable flags that the new
helper will use to determine if a resource is capable of allocation or monitoring.

The new helper will thus change behavior. It looks like resctrl_exit() is only
called on error path during initialization so it seems unlikely that those flags
will be referenced by this driver.

If there are plans to call resctrl_exit() in other paths like MPAM driver does there
may be issues since the resctrl filesystem may still be mounted and when user space
unmounts it the rdt_resource::mon_capable and rdt_resource::alloc_capable will be
used and result in some arch callbacks called.

I wonder if it may simplify driver initialization to call resctrl_init() _after_
setting up the CPU online/offline handlers (this is how the x86 driver does it).

> +
> +/*
> + * 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;
> +	domain->hdr.rid = res->rid;
> +
> +	err = cbqri_init_domain_ctrlval(res, domain);
> +	if (err)
> +		goto free;
> +
> +	err = resctrl_online_ctrl_domain(res, domain);
> +	if (err)
> +		goto free;
> +
> +	/*
> +	 * Publish only after the domain is fully initialized and online, so a
> +	 * reader walking the RCU list never sees a half-built domain.
> +	 */
> +	resctrl_find_domain(&res->ctrl_domains, dom_id, &pos);

The caller loops over the domain list to determine whether it needs to create
a new domain or not and then this domain create code loops over the list again
to determine where to insert the new domain. Could this perhaps be simplified if
the first loop  cbqri_attach_cpu_to_all_ctrls()->cbqri_find_ctrl_domain() determines
the position at the same time as determining the presence and pass that to
this function to just do the insert without searching the list again?

> +	list_add_tail_rcu(&domain->hdr.list, pos);
> +
> +	return domain;
> +free:
> +	kfree(container_of(domain, struct cbqri_resctrl_dom, resctrl_ctrl_dom));
> +	return ERR_PTR(err);
> +}

...

> +/*
> + * Attach a CPU to the capacity controller at each cache level whose cache
> + * the CPU shares. 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)
> +{
> +	static const u32 levels[] = { 2, 3 };
> +	struct cbqri_controller *ctrl, *c;
> +	struct cbqri_resctrl_res *hw_res;
> +	struct rdt_ctrl_domain *d;
> +	struct cacheinfo *ci;
> +	int i, rid;
> +
> +	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);
> +
> +	for (i = 0; i < ARRAY_SIZE(levels); i++) {
> +		ci = get_cpu_cacheinfo_level(cpu, levels[i]);
> +		if (!ci)
> +			continue;
> +
> +		rid = cbqri_cache_level_to_rid(levels[i]);
> +		hw_res = &cbqri_resctrl_resources[rid];
> +		if (!hw_res->ctrl)
> +			continue;
> +
> +		/* The controller backing this CPU's cache at this level. */
> +		ctrl = NULL;
> +		list_for_each_entry(c, &cbqri_controllers, list) {
> +			if (c->type == CBQRI_CONTROLLER_TYPE_CAPACITY &&> +			    c->alloc_capable &&
> +			    c->cache.cache_level == levels[i] &&
> +			    c->cache.cache_id == ci->id) {
> +				ctrl = c;
> +				break;

Is it necessary to loop over cbqri_controllers and repeat these tests? Above seems to
duplicate the work done during initialization (cbqri_resctrl_pick_caches()) that resulted
in initialization of cbqri_resctrl_res::ctrl so it seems that after testing for existence
this function could just use cbqri_resctrl_res::ctrl without again referencing cbqri_controllers?

If I understand correctly it may be that new controllers appear in cbqri_controllers 
after this driver is initialized and the resources are initialized so the CPU online/offline
helpers may need to take care how any controllers in cbqri_controllers not seen by
cbqri_resctrl_setup() are handled.

> +			}
> +		}
> +		if (!ctrl)
> +			continue;
> +
> +		d = cbqri_find_ctrl_domain(&hw_res->resctrl_res.ctrl_domains,
> +					   ci->id);
> +		if (d) {
> +			cpumask_set_cpu(cpu, &d->hdr.cpu_mask);
> +			continue;
> +		}
> +
> +		d = cbqri_create_ctrl_domain(ctrl, &hw_res->resctrl_res, cpu,
> +					     ci->id);
> +		if (IS_ERR(d)) {
> +			cbqri_detach_cpu_from_all_ctrls(cpu);
> +			return PTR_ERR(d);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static bool cbqri_resctrl_inited;
> +
> +static void cbqri_resctrl_teardown(void)
> +{
> +	int rid;
> +
> +	if (!cbqri_resctrl_inited)
> +		return;
> +
> +	resctrl_exit();
> +
> +	for (rid = 0; rid < RDT_NUM_RESOURCES; rid++) {
> +		struct cbqri_resctrl_res *hw_res = &cbqri_resctrl_resources[rid];
> +
> +		hw_res->ctrl = NULL;
> +		hw_res->cdp_enabled = false;
> +	}
> +	exposed_alloc_capable = false;
> +	cbqri_resctrl_inited = false;
> +}
> +
> +static int cbqri_resctrl_setup(void)
> +{
> +	int rid;
> +	int err;
> +
> +	for (rid = 0; rid < RDT_NUM_RESOURCES; rid++)
> +		cbqri_resctrl_resources[rid].resctrl_res.rid = rid;
> +
> +	cbqri_resctrl_pick_caches();
> +
> +	for (rid = 0; rid < RDT_NUM_RESOURCES; rid++)
> +		cbqri_resctrl_control_init(&cbqri_resctrl_resources[rid]);
> +
> +	cbqri_resctrl_accumulate_caps();
> +
> +	if (!exposed_alloc_capable) {
> +		pr_debug("no resctrl-capable CBQRI controllers found\n");
> +		return -ENODEV;
> +	}
> +
> +	err = resctrl_init();
> +	if (err)
> +		return err;
> +
> +	cbqri_resctrl_inited = true;
> +	return 0;
> +}
> +
> +static int cbqri_resctrl_online_cpu(unsigned int cpu)
> +{
> +	int err;
> +
> +	mutex_lock(&cbqri_domain_list_lock);
> +	err = cbqri_attach_cpu_to_all_ctrls(cpu);
> +	mutex_unlock(&cbqri_domain_list_lock);
> +	if (err)
> +		return err;
> +
> +	/*
> +	 * Seed the per-CPU default RCID/MCID to the reserved (0, 0) pair and
> +	 * notify the resctrl core so it tracks this CPU in the default group.
> +	 */
> +	resctrl_arch_set_cpu_default_closid_rmid(cpu, 0, 0);
> +	resctrl_online_cpu(cpu);
> +	return 0;
> +}
> +
> +static int cbqri_resctrl_offline_cpu(unsigned int cpu)
> +{
> +	resctrl_offline_cpu(cpu);
> +
> +	mutex_lock(&cbqri_domain_list_lock);
> +	cbqri_detach_cpu_from_all_ctrls(cpu);
> +	mutex_unlock(&cbqri_domain_list_lock);
> +	return 0;
> +}
> +
> +static int __init cbqri_arch_late_init(void)
> +{
> +	int err;
> +
> +	if (!riscv_isa_extension_available(NULL, SSQOSID))
> +		return -ENODEV;
> +
> +	err = cbqri_resctrl_setup();
> +	if (err)
> +		return err;
> +
> +	err = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "cbqri:online",
> +				cbqri_resctrl_online_cpu,
> +				cbqri_resctrl_offline_cpu);
> +	if (err < 0) {
> +		cbqri_resctrl_teardown();

cbqri_resctrl_teardown() calls resctrl_exit() that will complain
via WARN_ON_ONCE() if any domains exist at that time. It is not clear
to me if this can be guaranteed here.

> +		return err;
> +	}
> +
> +	return 0;
> +}
> +late_initcall(cbqri_arch_late_init);
> 

Reinette