Re: [PATCH RFC v4 13/18] riscv_cbqri: resctrl: Add MB_MIN bandwidth allocation via Rbwb

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Removing a resctrl group leaks the MB_MIN (Rbwb) bandwidth allocation, permanently preventing future allocations.
- [High] Secondary memory bandwidth controllers in the same NUMA node are silently ignored, leaving their hardware unconstrained.
- [High] Missing MRBWB capability validation causes integer underflow and assignment of 0 bandwidth, violating the hardware specification.
- [Low] The code comment incorrectly states that intermediate sums transiently exceed MRBWB during initialization.
--

commit e4a9c24653219ec74efc1b4f91aa0b08b1cfc5ac
Author: Drew Fustini <fustini@kernel.org>

riscv_cbqri: resctrl: Add MB_MIN bandwidth allocation via Rbwb

Add bandwidth allocation through Rbwb (reserved bandwidth blocks)
exposed as the MB_MIN resource. Rbwb's sum constraint does not fit MBA's
percentage cap, so MB_MIN lands as a new RDT_RESOURCE_* rather than
masquerading as MBA.

> diff --git a/drivers/resctrl/cbqri_resctrl.c b/drivers/resctrl/cbqri_resctrl.c
> index d8fd9b06703fa..bcd9367e3555c 100644
> --- a/drivers/resctrl/cbqri_resctrl.c
> +++ b/drivers/resctrl/cbqri_resctrl.c
> @@ -415,6 +415,9 @@ int resctrl_arch_update_one(struct rdt_resource *r, struct rdt_ctrl_domain *d,
>  	case RDT_RESOURCE_L2:
>  	case RDT_RESOURCE_L3:
>  		return cbqri_apply_cache_config_dom(dom, r, closid, t, cfg_val);
> +	case RDT_RESOURCE_MB_MIN:
> +		/* sum(Rbwb) <= MRBWB validation runs inside cbqri_apply_rbwb(). */
> +		return cbqri_apply_rbwb(dom->hw_ctrl, closid, cfg_val, true);

When a resctrl group is removed, does the framework invoke an architecture
hook to reset the hardware control values to a minimum default?

If the CLOSID is freed but the previously configured bandwidth value remains
pinned in the software cache (ctrl->rbwb_cache[closid]), will this abandoned
value falsely inflate the sum during subsequent cbqri_apply_rbwb() calls?

Could this cause new allocation attempts to fail with -EINVAL, effectively
creating a permanent denial of service for bandwidth allocations until the
leaked CLOSID is coincidentally re-allocated?

[ ... ]

> @@ -491,15 +503,42 @@ void resctrl_arch_reset_all_ctrls(struct rdt_resource *r)
>  		return;
>  
>  	list_for_each_entry(d, &r->ctrl_domains, hdr.list) {
> -		for (i = 0; i < hw_res->ctrl->rcid_count; i++) {
> -			for (t = 0; t < CDP_NUM_TYPES; t++) {
> +		dom = container_of(d, struct cbqri_resctrl_dom,
> +				   resctrl_ctrl_dom);
> +
> +		switch (r->rid) {
> +		case RDT_RESOURCE_MB_MIN:
> +			/*
> +			 * CBQRI section 4.5: Rbwb >= 1, sum(Rbwb) <= MRBWB.
> +			 * Walk N-1..1 first so the final sum lands at
> +			 * MRBWB. Use the unchecked helper since the
> +			 * intermediate sum may exceed MRBWB.
> +			 */

This isn't a bug, but is this comment accurate?

Since the loop strictly reduces the allocations for RCIDs N-1 down to 1
before finally increasing RCID 0 to the remaining budget, doesn't the
running hardware sum strictly decrease until the final step? 

Does the intermediate sum ever actually transiently exceed MRBWB?

> +			for (i = 0; i < hw_res->ctrl->rcid_count; i++) {
> +				u32 rcid = (i + 1) % hw_res->ctrl->rcid_count;
> +				u64 rbwb = (rcid == 0) ?
> +					dom->hw_ctrl->bc.mrbwb - (hw_res->ctrl->rcid_count - 1) : 1;

If the hardware capability reports mrbwb == rcid_count - 1, will this
calculation evaluate exactly to 0 for RCID 0?

If so, does cbqri_apply_rbwb() bypass the internal sum validation
(which checks if rbwb > 0) and blindly write 0 to the hardware, violating
the CBQRI section 4.5 requirement that Rbwb >= 1?

[ ... ]

> @@ -830,6 +954,37 @@ static int cbqri_attach_cpu_to_cap_ctrl(struct cbqri_controller *ctrl,
>  	return 0;
>  }
>  
> +static int cbqri_attach_cpu_to_one_bw_res(struct cbqri_controller *ctrl,
> +					  enum resctrl_res_level rid,
> +					  unsigned int cpu)
> +{
> +	struct cbqri_resctrl_res *hw_res = &cbqri_resctrl_resources[rid];
> +	struct rdt_resource *res = &hw_res->resctrl_res;
> +	struct rdt_ctrl_domain *domain;
> +	int dom_id = ctrl->mem.prox_dom;
> +
> +	if (!hw_res->ctrl)
> +		return 0;
> +
> +	domain = cbqri_find_ctrl_domain(&res->ctrl_domains, dom_id);
> +	if (domain) {

If a system has multiple memory bandwidth controllers within the same NUMA
node, will the second controller match the existing domain via
cbqri_find_ctrl_domain() here?

Since struct cbqri_resctrl_dom only stores a single hw_ctrl pointer,
does this mean the secondary controller is never tracked by the domain and
its hardware limits will be silently ignored during subsequent configurations?

> +		cpumask_set_cpu(cpu, &domain->hdr.cpu_mask);
> +		return 0;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260510-ssqosid-cbqri-rqsc-v7-0-v4-0-eb53831ef683@kernel.org?part=13