Re: [PATCH v6 14/40] arm_mpam: resctrl: Add boilerplate cpuhp and domain allocation

[James Morse]

Hi Ben, Gavin,

On 23/03/2026 10:13, Ben Horgan wrote:
> On 3/23/26 06:31, Gavin Shan wrote:
>> On 3/14/26 12:45 AM, Ben Horgan wrote:
>>> From: James Morse <james.morse@xxxxxxx>
>>>
>>> resctrl has its own data structures to describe its resources. We can't use
>>> these directly as we play tricks with the 'MBA' resource, picking the MPAM
>>> controls or monitors that best apply. We may export the same component as
>>> both L3 and MBA.
>>>
>>> Add mpam_resctrl_res[] as the array of class->resctrl mappings we are
>>> exporting, and add the cpuhp hooks that allocated and free the resctrl
>>> domain structures. Only the mpam control feature are considered here and
>>> monitor support will be added later.
>>>
>>> While we're here, plumb in a few other obvious things.
>>>
>>> CONFIG_ARM_CPU_RESCTRL is used to allow this code to be built even though
>>> it can't yet be linked against resctrl.


>> With the following two comments addressed. I don't think none of them are critical
>> given the fact that this series has been respinned to v6 and may be ready for Linux
>> v7.1. If there is still a chance for another respin, they may be worthy to be addressed.
>>
>> Reviewed-by: Gavin Shan <gshan@xxxxxxxxxx>

I've picked up the change from your second comment, I assume you're happy to keep this
tag. (otherwise shout!)


>>> diff --git a/drivers/resctrl/mpam_resctrl.c b/drivers/resctrl/mpam_resctrl.c
>>> new file mode 100644
>>> index 000000000000..e698b534e3db
>>> --- /dev/null
>>> +++ b/drivers/resctrl/mpam_resctrl.c

>>> +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_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 (r->alloc_capable) {
>>> +        dom->ctrl_comp = ctrl_comp;
>>> +
>>> +        ctrl_d = &dom->resctrl_ctrl_dom;
>>> +        mpam_resctrl_domain_hdr_init(cpu, ctrl_comp, r->rid, &ctrl_d->hdr);
>>> +        ctrl_d->hdr.type = RESCTRL_CTRL_DOMAIN;
>>> +        err = resctrl_online_ctrl_domain(r, ctrl_d);
>>> +        if (err)
>>> +            goto free_domain;
>>> +
>>> +        mpam_resctrl_domain_insert(&r->ctrl_domains, &ctrl_d->hdr);
>>> +    } else {
>>> +        pr_debug("Skipped control domain online - no controls\n");
>>> +    }
>>> +    return dom;

>> Even though we will never support "r->alloc_capable == false", it's worthy to maintain
>> the consistence in the code level here, meaning @dom needs to be released with a proper
>> error number returned.
>>
>>     if (r->alloc_capable) {
>>         :
>>     } else {
>>         pr_debug("Skipped control domain online - no controls\n");
>>         err = -EINVAL;
>>         goto free_domain;
>>     }
>>
>> Alternatively, the check can be done before locating the component from its calss.
>>
>>     
>>     lockdep_assert_held(&domain_list_lock);
>>
>>     if (!r->alloc_capable) {
>>         pr_debug("Skipped control domain online - no controls\n");
>>         return ERR_PTR(-EINVAL);
>>     }
>>
>>     ctrl_comp = NULL;
> 
> Once monitor support is added later in the series this is no longer an
> error path as monitor only platforms are valid. In order to avoid adding
> a change and then reverting it later in the series I would like to keep
> this as it is.

I've left this as it is - adding the monitoring support builds on top of this.


>>> +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;
>>> +        struct rdt_resource *r = &res->resctrl_res;
>>> +
>>> +        if (!res->class)
>>> +            continue;    // dummy_resource;
>>> +
>>> +        dom = mpam_resctrl_get_domain_from_cpu(cpu, res);
>>> +        if (!dom) {
>>> +            dom = mpam_resctrl_alloc_domain(cpu, res);
>>> +        } else {
>>> +            if (r->alloc_capable) {
>>> +                struct rdt_ctrl_domain *ctrl_d = &dom->resctrl_ctrl_dom;
>>> +
>>> +                mpam_resctrl_online_domain_hdr(cpu, &ctrl_d->hdr);
>>> +            }
>>> +        }
>>> +        if (IS_ERR(dom))
>>> +            return PTR_ERR(dom);
>>> +    }
>>> +
>>
>> I think the "if (IS_ERR(dom))" check can be moved after "dom = mpam_resctrl_alloc_domain(cpu, res)"
>> because it seems the only path where an erroneous domain can be returned.
>>
>>         dom = mpam_resctrl_get_domain_from_cpu(cpu, res);
>>         if (!dom) {
>>             dom = mpam_resctrl_alloc_domain(cpu, res);
>>             if (IS_ERR(dom))
>>                 return PTR_ERR(dom);
>>         } else {
>>             ...
>>         }
> 
> Yes, that would be clearer. I'll make this change if a respin becomes needed.

Applied locally.



Thanks!

James