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

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

Hi Drew,

On 10/3/26 12:12 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..867db663d958
> --- /dev/null
> +++ b/arch/riscv/include/asm/resctrl.h

...

> +/**
> + * resctrl_arch_mon_ctx_alloc() - allocate per-monitor-event arch context
> + * @r:     resctrl resource being monitored
> + * @evtid: which monitor event needs context
> + *
> + * The CBQRI driver implements no monitoring events, so there is no per-event
> + * context to allocate and the stub returns NULL. fs/resctrl references it
> + * unconditionally before checking resctrl_arch_mon_capable().

The last sentence is unexpected. fs/resctrl only calls resctrl_arch_mon_ctx_alloc()
from mon_event_read(), __check_limbo() and mbm_update_one_event(), and none of these
can run on a system that does not support monitoring.


> + */
> +void *resctrl_arch_mon_ctx_alloc(struct rdt_resource *r, enum resctrl_event_id evtid);
> +
> +/**

...

> diff --git a/drivers/resctrl/cbqri_resctrl.c b/drivers/resctrl/cbqri_resctrl.c
> new file mode 100644
> index 000000000000..f10a24761b25
> --- /dev/null
> +++ b/drivers/resctrl/cbqri_resctrl.c

...

> +void resctrl_arch_reset_all_ctrls(struct rdt_resource *r)
> +{
> +	u32 num_closid = resctrl_arch_get_num_closid(r);
> +	struct rdt_ctrl_domain *d;
> +	enum resctrl_conf_type t;
> +	u32 default_ctrl;
> +	u32 i;
> +
> +	lockdep_assert_cpus_held();
> +
> +	if (!num_closid)
> +		return;
> +
> +	default_ctrl = resctrl_get_default_ctrl(r);
> +
> +	list_for_each_entry(d, &r->ctrl_domains, hdr.list) {
> +		for (i = 0; i < num_closid; i++) {
> +			for (t = 0; t < CDP_NUM_TYPES; t++) {
> +				int rerr;
> +
> +				rerr = resctrl_arch_update_one(r, d, i, t, default_ctrl);
> +				if (rerr)
> +					pr_err_ratelimited("rid=%d reset RCID %u type %u failed (%d)\n",
> +							   r->rid, i, t, rerr);
> +			}
> +		}


Above looks to be an open-code of cbqri_init_domain_ctrlval(). The loop
is unexpected here due to the duplicate writes impacting types not used by current
CDP mode that the comment within cbqri_init_domain_ctrlval() justifies.

> +	}
> +}
> +

...

> +
> +static int cbqri_init_domain_ctrlval(struct rdt_resource *r, struct rdt_ctrl_domain *d)
> +{
> +	u32 num_closid = resctrl_arch_get_num_closid(r);
> +	enum resctrl_conf_type t;
> +	int err = 0;
> +	u32 i;
> +
> +	for (i = 0; i < num_closid; i++) {
> +		/*
> +		 * Seed both DATA and CODE staged slots so a later mount

nit: This seems to refer to d->staged_config[] that is only populated by resctrl fs.

> +		 * with -o cdp does not see stale CODE values.
> +		 * On non-AT controllers cbqri_cc_alloc_op() masks AT to 0
> +		 * so all three iterations land on the same hardware state.
> +		 * The redundant writes are harmless.
> +		 */
> +		for (t = 0; t < CDP_NUM_TYPES; t++) {
> +			err = resctrl_arch_update_one(r, d, i, t,
> +						      resctrl_get_default_ctrl(r));
> +			if (err)
> +				return err;
> +		}
> +	}
> +	return 0;
> +}
> +

...

> +/*
> + * Attach a CPU to the picked 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_resctrl_res *hw_res;
> +	struct cbqri_controller *ctrl;
> +	struct rdt_ctrl_domain *d;
> +	struct list_head *pos;
> +	struct cacheinfo *ci;
> +	int i, rid;
> +
> +	lockdep_assert_held(&cbqri_domain_list_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->nr_ctrls)
> +			continue;
> +
> +		ctrl = cbqri_resctrl_find_ctrl(hw_res, ci->id);
> +		if (!ctrl) {
> +			pr_warn_once("no L%u controller for cache id %u, CPU %u is not controlled at this level\n",
> +				     levels[i], ci->id, cpu);
> +			continue;
> +		}
> +
> +		d = cbqri_find_ctrl_domain(&hw_res->resctrl_res.ctrl_domains,
> +					   ci->id, &pos);
> +		if (d) {
> +			cpumask_set_cpu(cpu, &d->hdr.cpu_mask);
> +			continue;
> +		}
> +
> +		d = cbqri_create_ctrl_domain(ctrl, &hw_res->resctrl_res, cpu,
> +					     ci->id, pos);
> +		if (IS_ERR(d)) {
> +			cbqri_detach_cpu_from_all_ctrls(cpu);
> +			return PTR_ERR(d);
> +		}

Should a failure to set up one domain prevent the CPU from coming online?

As I understand the driver already brings a CPU online without a domain (albeit
with a pr_warn_once()) at a level when its cache has no controller (the earlier
cbqri_resctrl_find_ctrl()), so I wonder if here (on domain creation failure) the
CPU could just be left without a domain at this level without failing the CPU
online entirely? It looks like cbqri_detach_cpu_from_all_ctrls() keeps the domains
consistent.

In the end this is a driver design question, so up to you.

I just have a few nits and a check-in on the design but all the resctrl filesystem 
interactions look good to me.

The one open is the coordination with Tony's series that moves the
resctrl_arch_alloc_capable() and resctrl_arch_mon_capable() work into
resctrl filesystem. Thanks for making the switch to use alloc_capable, that will
simplify the accompanying RISC-V changes.

Acked-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette