Re: [PATCH v3 14/47] arm_mpam: resctrl: Add boilerplate cpuhp and domain allocation

Reinette Chatre
2026-01-13T16:49:46Z

Hi Ben,

(Please note I am unfamiliar with this code so missing some context.)

On 1/12/26 8:58 AM, Ben Horgan wrote:
> +
> +static struct mpam_resctrl_dom *
> +mpam_resctrl_alloc_domain(unsigned int cpu, struct mpam_resctrl_res *res)
> +{
> + int err;
> + struct mpam_resctrl_dom *dom;
> + struct rdt_mon_domain *mon_d;
> + struct rdt_ctrl_domain *ctrl_d;
> + struct mpam_class *class = res->class;
> + struct mpam_component *comp_iter, *ctrl_comp;
> + struct rdt_resource *r = &res->resctrl_res;
> +
> + lockdep_assert_held(&domain_list_lock);
> +
> + ctrl_comp = NULL;
> + guard(srcu)(&mpam_srcu);
> + list_for_each_entry_srcu(comp_iter, &class->components, class_list,
> + srcu_read_lock_held(&mpam_srcu)) {
> + if (cpumask_test_cpu(cpu, &comp_iter->affinity)) {
> + ctrl_comp = comp_iter;
> + break;
> + }
> + }
> +
> + /* class has no component for this CPU */
> + if (WARN_ON_ONCE(!ctrl_comp))
> + return ERR_PTR(-EINVAL);
> +
> + dom = kzalloc_node(sizeof(*dom), GFP_KERNEL, cpu_to_node(cpu));
> + if (!dom)
> + return ERR_PTR(-ENOMEM);
> +
> + if (exposed_alloc_capable) {
> + dom->ctrl_comp = ctrl_comp;
> +
> + ctrl_d = &dom->resctrl_ctrl_dom;
> + mpam_resctrl_domain_hdr_init(cpu, ctrl_comp, &ctrl_d->hdr);
> + ctrl_d->hdr.type = RESCTRL_CTRL_DOMAIN;
> + /* TODO: this list should be sorted */
> + list_add_tail_rcu(&ctrl_d->hdr.list, &r->ctrl_domains);
> + err = resctrl_online_ctrl_domain(r, ctrl_d);
> + if (err) {
> + dom = ERR_PTR(err);
> + goto offline_ctrl_domain;

It should not be necessary to offline the control domain if attempt to
online it failed but removing it from the ctrl_domains list is necessary. What
happens to memory dom points to?

> + }
> + } else {
> + pr_debug("Skipped control domain online - no controls\n");
> + }
> +
> + if (exposed_mon_capable) {
> + mon_d = &dom->resctrl_mon_dom;
> + mpam_resctrl_domain_hdr_init(cpu, ctrl_comp, &mon_d->hdr);
> + mon_d->hdr.type = RESCTRL_MON_DOMAIN;
> + /* TODO: this list should be sorted */
> + list_add_tail_rcu(&mon_d->hdr.list, &r->mon_domains);
> + err = resctrl_online_mon_domain(r, mon_d);
> + if (err) {
> + dom = ERR_PTR(err);
> + goto offline_mon_hdr;
> + }
> + } else {
> + pr_debug("Skipped monitor domain online - no monitors\n");
> + }
> +
> + return dom;
> +
> +offline_mon_hdr:
> + mpam_resctrl_offline_domain_hdr(cpu, &mon_d->hdr);
> +offline_ctrl_domain:
> + resctrl_offline_ctrl_domain(r, ctrl_d);
> +
> + return dom;

This error path is unexpected to me. From what I can tell, if there is a problem
initializing the monitor domain this flow will undo both monitor and control domain,
even if initialization of control domain was successful. In this case:
- Flow jumps to error path from within the if (exposed_mon_capable) block and proceeds
to do control domain cleanup without considering whether control domain was initialized
or not. That is, does not take exposed_alloc_capable into account
- Control domain cleanup seems to be partial, for example, should it remove domain from ctrl_domains list?
- On failure there is dom = ERR_PTR(err) but I cannot see where this memory is freed in both
the monitor and control domain error paths.

> +int mpam_resctrl_online_cpu(unsigned int cpu)
> +{
> + struct mpam_resctrl_res *res;
> + enum resctrl_res_level rid;
> +
> + guard(mutex)(&domain_list_lock);
> + for_each_mpam_resctrl_control(res, rid) {
> + struct mpam_resctrl_dom *dom;
> +
> + if (!res->class)
> + continue; // dummy_resource;
> +
> + dom = mpam_resctrl_get_domain_from_cpu(cpu, res);

On success, should cpu be added to the respective headers' cpumask?

> + if (!dom)
> + dom = mpam_resctrl_alloc_domain(cpu, res);
> + if (IS_ERR(dom))
> + return PTR_ERR(dom);
> + }
> +
> + resctrl_online_cpu(cpu);
> +
> + return 0;
> +}

Reinette