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

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

Hi Drew,

On 9/21/26 11:57 PM, Drew Fustini wrote:
> On Thu, Sep 17, 2026 at 05:54:35PM -0700, Reinette Chatre wrote:
>> 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/20260916231320.14502-7-tony.luck@intel.com/
>>
>> I see this has impact on this driver that I comment more below.
> 
> Thanks for letting me know. cbqri_resctrl_control_init() already sets
> rdt_resource::alloc_capable, so I will drop exposed_alloc_capable. I
> will have cbqri_resctrl_teardown() clear rdt_resource::alloc_capable.
> 
> Should I wait to drop the hook until Tony's series is applied?

I do not think there is a choice here since resctrl fs will keep expecting
the architectural hook until that series lands. We'll have to coordinate
the inclusion of these two series around this change. If this series is
merged first then I expect Tony's series to include removal of RISC-V's
resctrl_arch_{alloc,mon}_capable(). This should be simplified thanks to
the clearing of rdt_resource::alloc_capable. 

...

>>> +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?
> 
> cbqri_resctrl_dom::hw_ctrl is needed when a cache level has more than
> one controller. Each cache instance has its own register block, so a
> domain has to reach its own controller.

ah - I missed this. Thank you. Your later explanation of my same misunderstanding in
cbqri_attach_cpu_to_all_ctrls() makes this clear.

...
 
>>> +/*
>>> + * 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.
> 
> The problem is that every controller that passed cbqri_cc_caps_agree()
> is forgotten except the first. I will change cbqri_resctrl_pick_caches()
> to keep every controller accepted for a level and have the online path
> look up the cpu's cache id in that set.

How controllers are associated to the resource and individual domains was not
clear to me. Thank you for explaining this.

Reinette