Re: [PATCH RFC v5 14/18] riscv_cbqri: resctrl: Add MB_WGHT bandwidth allocation via Mweight

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] If `cbqri_attach_cpu_to_bw_ctrl()` fails to attach the CPU to `RDT_RESOURCE_MB_WGHT`, the previously successful attachment to `RDT_RESOURCE_MB_MIN` is not rolled back, leading to a domain cpumask and memory leak.

Pre-existing issues:
- [High] If the controller iteration in `cbqri_resctrl_online_cpu` encounters an error midway, earlier successfully attached controllers are not detached, causing domain leaks on partial CPU bringup failure.
--

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

riscv_cbqri: resctrl: Add MB_WGHT bandwidth allocation via Mweight

This patch adds bandwidth allocation through Mweight, exposed as the MB_WGHT
resource. It configures the shared weight for unreserved bandwidth, handling
initialization and resource attachment.

> diff --git a/drivers/resctrl/cbqri_resctrl.c b/drivers/resctrl/cbqri_resctrl.c
> index 5d8389c58f4fe..ba764bc6ef176 100644
> --- a/drivers/resctrl/cbqri_resctrl.c
> +++ b/drivers/resctrl/cbqri_resctrl.c
[ ... ]
> @@ -1019,7 +1065,13 @@ static int cbqri_attach_cpu_to_one_bw_res(struct cbqri_controller *ctrl,
>  static int cbqri_attach_cpu_to_bw_ctrl(struct cbqri_controller *ctrl,
>  				       unsigned int cpu)
>  {
> -	return cbqri_attach_cpu_to_one_bw_res(ctrl, RDT_RESOURCE_MB_MIN, cpu);
> +	int err;
> +
> +	err = cbqri_attach_cpu_to_one_bw_res(ctrl, RDT_RESOURCE_MB_MIN, cpu);
> +	if (err)
> +		return err;
> +
> +	return cbqri_attach_cpu_to_one_bw_res(ctrl, RDT_RESOURCE_MB_WGHT, cpu);
>  }

[Severity: High]
If cbqri_attach_cpu_to_one_bw_res() fails for RDT_RESOURCE_MB_WGHT, does
this leave the CPU permanently attached to the RDT_RESOURCE_MB_MIN domain?

Since the CPU hotplug core expects a startup callback to either fully succeed
or completely roll back its progress on error, returning an error here skips
the teardown callback. It looks like this might leak the domain structure if
a new one was allocated, and leave the CPU in the domain's cpumask.

[Severity: High]
This is a pre-existing issue, but looking at the caller
cbqri_resctrl_online_cpu(), there appears to be a similar problem with the
controller iteration:

drivers/resctrl/cbqri_resctrl.c:cbqri_resctrl_online_cpu() {
	...
	list_for_each_entry(ctrl, &cbqri_controllers, list) {
		switch (ctrl->type) {
		...
		case CBQRI_CONTROLLER_TYPE_BANDWIDTH:
			...
			err = cbqri_attach_cpu_to_bw_ctrl(ctrl, cpu);
			break;
		}
		if (err)
			break;
	}

	mutex_unlock(&cbqri_domain_list_lock);
	return err;
}

If the loop terminates early due to an error, are the domains that were
successfully attached in earlier iterations left without being detached? Like
the issue above, the offline callback won't be called, potentially leaking
the domain allocations.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260524-ssqosid-cbqri-rqsc-v7-0-v5-0-78d3a7ba9dbe@kernel.org?part=14