Re: [RFC PATCH] arm_mpam: resctrl: Separate MPAM domains

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

Hi Ben,

On 9/7/26 10:01 AM, Ben Horgan wrote:
> On 03/09/2026 16:29, Reinette Chatre wrote:
>> On 9/2/26 9:10 AM, Ben Horgan wrote:
>>> On 01/09/2026 00:54, Reinette Chatre wrote:

...

>>>
>>> 	/* class has no component for this CPU */
>>> 	if (WARN_ON_ONCE(!ctrl_comp))
>>> 		return ERR_PTR(-EINVAL);
>>>
>>>
>>> In mpam_resctrl_alloc_domain() this initial 'ctrl_comp' check ensures that there is for the class
>>> associated with the given resource, res, a component which includes the given cpu in its affinity
>>
>> Why does the monitoring require that there is a component associated with the resource as
>> opposed to only relying on the component associated with the the monitoring class, specifically
>> the mpam_resctrl_mon::class? (more below)
> 
> The class associated with a resource does double duty and is also used as an indication of whether
> the resource is in used at all, see also mpam_resctrl_online_cpu() and mpam_resctrl_offline_cpu().
> When a resource only supports monitoring then the class providing the monitors is used for
> res->class. As such, a check that there is a component for the given cpu, in the monitor only

Regarding "class providing the monitors": From what I can tell each monitor event is separately
mapped to a (potentially?) different MPAM class. With MPAM supporting two events, each associated
with a class, which of these classes are used for the class associated with the resource? 

> resource case, is just a way to error out early when the monitoring class has no cpu. We shouldn't
> hit this though as all monitoring classes are check that they have the same topology of the l3,
> cover all cpus and there components correspond to l3 instances.

It seems as though there is a requirement that all the monitoring classes as well as the control
class cover the same CPUs with the implementation flexible to support otherwise while assuming it does?

> 
> The resource supports monitoring but not controls case only occurs when there are no l3 controls and
> the condidtions for pretending the memory bandwidth counters at the memory are at the l3 are met.
> Those conditions for pretending occur when there is single l3, a single NUMA node and no
> intermediate caches.
> 
>>
>>
>>> mask. The class is all the L3 MSC or an equivalent of the same scope, see mpam_resctrl_monitor_init().
>>>
>>> 	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;
>>>
>>> If the resource is alloc capable this component is used as the domain 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");
>>> 	}
>>>
>>> 	if (r->mon_capable) {
>>> 		struct mpam_component *any_mon_comp = NULL;
>>> 		struct mpam_resctrl_mon *mon;
>>> 		enum resctrl_event_id eventid;
>>>
>>> 		/*
>>> 		 * Even if the monitor domain is backed by a different
>>> 		 * component, the L3 component IDs need to be used... only
>>> 		 * there may be no ctrl_comp for the L3.
>>> 		 * Search each event's class list for a component with
>>> 		 * overlapping CPUs and set up the dom->mon_comp array.
>>> 		 */
>>> The MSC at the L3 may only have monitors and so no control component.
>>
>> The code that follows is:
>>
>>         for_each_mpam_resctrl_mon(mon, eventid) {
>>                 struct mpam_component *mon_comp;
>>
>>                 if (!mon->class)
>>                         continue;       // dummy resource
>>
>>                 mon_comp = find_component(mon->class, cpu);
>>
>> Is this find_component() perhaps sufficient by itself (without the earlier "ctrl_comp" check)
>> to determine if there is a valid component associated with this CPU to support this
>> monitoring feature? 
> 
> Yes, I think is the "ctrl_comp" check is taken away but would have failed we will end up not finding
> any relevant monitoring components.
> 
> Although, as written it seems that it is ok for mon_comp to be NULL?

Yes. Are there scenarios under which it will be NULL? If it can be NULL, does it not
mean that the event that was already enabled during init cannot actually be supported in all
domains/components?

> 
> Isn't any_mon_comp the relevant thing?

(answer below)

> 
>>
>> It is not clear to me if a mon_comp of NULL is able to handle all scenarios since it looks
>> like mpam_resctrl_get_mon_domain_from_cpu() and mpam_resctrl_online_domain_hdr() does not
>> consider the component at all. Would that not cause monitoring features to depend on which
>> CPU of a domain comes online first?
>>
>> Could mon_comp perhaps be required to be !NULL here as a replacement for the earlier
>> "ctrl_comp" check to ensure there is a component with the CPU in its affinity mask?
> 
> Doesn't the !any_mon_comp check provide this?

This is the part that I do not understand since any_mon_comp seems to support the scenario
where a mon_comp may be NULL which is a scenario that I do not think resctrl can support.

At a high level there seems to be three affinity masks used by the monitoring code:
the CPU affinity of the component belonging to the control resource class, the CPU affinity
of each component supporting each monitoring event, while these are three separate masks with
code sometimes treating them as though they can be different they are actually required to be the same?

> 
>>
>>                 dom->mon_comp[eventid] = mon_comp;
>>                 if (mon_comp)
>>                         any_mon_comp = mon_comp;
>>         }
>>>
>>>
>>>
>>>
>>>
>>>
>>>> 	struct mpam_resctrl_mon_dom {
>>>> 		struct mpam_component		*mon_comp[QOS_NUM_EVENTS];
>>>> 		struct rdt_l3_mon_domain	resctrl_mon_dom;
>>>> 	}
>>>>
>>>> 	struct mpam_resctrl_ctrl_dom {
>>>> 		struct mpam_component		*ctrl_comp;
>>>> 		struct rdt_ctrl_domain		resctrl_ctrl_dom;
>>>> 	};
>>>
>>> What you have looks to work for me, with some local cmax, mbw_min, mbw_max additions but with the
>>> new layout also works. I gave it a go with this mechanical patch which uses the new layout.
>>>
>>> Thanks,
>>>
>>> Ben
>>>
>>> commit c67c624149474b96e07ccedda11a11ce968e5599
>>> Author: Ben Horgan <ben.horgan@arm.com>
>>> Date:   Tue Sep 1 17:36:29 2026 +0100
>>>
>>>     arm_mpam: resctrl: Separate monitor and control domain structure
>>>
>>> diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/mpam_internal.h
>>> index 3304ef64fcae..855f06657554 100644
>>> --- a/drivers/resctrl/mpam_internal.h
>>> +++ b/drivers/resctrl/mpam_internal.h
>>> @@ -393,18 +393,14 @@ struct mpam_resctrl_ctrl {
>>>  	struct resctrl_ctrl	r_ctrl;
>>>  };
>>>
>>> -struct mpam_resctrl_dom {
>>> -	struct mpam_component		*ctrl_comp;
>>> -
>>> -	/*
>>> -	 * There is no single mon_comp because different events may be backed
>>> -	 * by different class/components. mon_comp is indexed by the event
>>> -	 * number.
>>> -	 */
>>> +struct mpam_resctrl_mon_dom {
>>>  	struct mpam_component		*mon_comp[QOS_NUM_EVENTS];
>>> +	struct rdt_l3_mon_domain	resctrl_mon_dom;
>>> +};
>>>
>>> +struct mpam_resctrl_ctrl_dom {
>>> +	struct mpam_component		*ctrl_comp;
>>>  	struct rdt_ctrl_domain		resctrl_ctrl_dom;
>>> -	struct rdt_l3_mon_domain	resctrl_mon_dom;
>>>  };
>>>
>>
>> Thank you very much for trying this out. I find this layout better since the
>> architecture domain structure only contains those members related to the domain. 
>> I see your snippet is based on the PoC, would you prefer I incorporate it into a new
>> version of the PoC to get some more testing or to create a new version based on
>> current upstream so that we can work on its upstream inclusion for the multiple
>> controller support to build on?
> 
> In the spirit of getting the precursors for your PoC upstream I think it would make sense to work on
> this for upstream. Whatever you think is best for progressing multiple control support is ok with me
> though.

I think that it will support the multiple control work if this is done upstream first.
I will incorporate your changes in the next version ... I believe that would make you
a co-author?

Thank you very much.

Reinette