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

[Drew Fustini <fustini@kernel.org>]

On Tue, Oct 20, 2026 at 11:37:52AM -0700, Reinette Chatre wrote:
> Hi Drew,

Hi, thanks for your comments and Ack.

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

Good point, I will remove that final sentence.

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

Thanks for pointing this out. I'll simplify it by calling
cbqri_init_domain_ctrlval() for each domain.

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

I'll change the wording to refer to the hardware configuration for each
type and not the staged config.

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

Good question. It does seem like an additional cpu would be beneficial
to a system, even if the qos functionality is not working as expected.
I will change the driver to warn but not fail the cpu online.

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

I'll remove the hooks if Tony's series lands first.

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

Thanks for your attention on this series!

Drew