Message-ID: <818b03f6-effb-4e7e-9ee8-49917504a5dd@arm.com>
Date: Wed, 7 Jan 2026 14:21:37 +0000
From: Ben Horgan <ben.horgan@....com>
To: Jonathan Cameron <jonathan.cameron@...wei.com>
Cc: amitsinght@...vell.com, baisheng.gao@...soc.com,
 baolin.wang@...ux.alibaba.com, carl@...amperecomputing.com,
 dave.martin@....com, david@...nel.org, dfustini@...libre.com,
 fenghuay@...dia.com, gshan@...hat.com, james.morse@....com,
 kobak@...dia.com, lcherian@...vell.com,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 peternewman@...gle.com, punit.agrawal@....qualcomm.com,
 quic_jiles@...cinc.com, reinette.chatre@...el.com, rohit.mathew@....com,
 scott@...amperecomputing.com, sdonthineni@...dia.com,
 tan.shaopeng@...itsu.com, xhao@...ux.alibaba.com, catalin.marinas@....com,
 will@...nel.org, corbet@....net, maz@...nel.org, oupton@...nel.org,
 joey.gouly@....com, suzuki.poulose@....com, kvmarm@...ts.linux.dev,
 Zeng Heng <zengheng4@...wei.com>
Subject: Re: [PATCH v2 25/45] arm_mpam: resctrl: Add support for 'MB' resource

Hi Jonathan,

On 1/6/26 12:19, Jonathan Cameron wrote:
> On Fri, 19 Dec 2025 18:11:27 +0000
> Ben Horgan <ben.horgan@....com> wrote:
> 
>> From: James Morse <james.morse@....com>
>>
>> resctrl supports 'MB', as a percentage throttling of traffic somewhere
>> after the L3. This is the control that mba_sc uses, so ideally the class
>> chosen should be as close as possible to the counters used for mba_local.
>>
>> MB's percentage control should be backed either with the fixed point
> 
> either's "or" is missing.  (sentence should include the second choice)
> 
>> fraction MBW_MAX. The bandwidth portion bitmaps is not used as its tricky
>> to pick which bits to use to avoid contention, and may be possible to
>> expose this as something other than a percentage in the future.
>>
>> CC: Zeng Heng <zengheng4@...wei.com>
>> Co-developed-by: Dave Martin <Dave.Martin@....com>
>> Signed-off-by: Dave Martin <Dave.Martin@....com>
>> Signed-off-by: James Morse <james.morse@....com>>
>> Signed-off-by: Ben Horgan <ben.horgan@....com>
> 
> As mentioned in earlier review I'd like an overview doc of the heuristics
> used to map to the resctrl everything is a xeon view of the world.
> 
> At least some of our platforms are far enough from this view that the
> heuristics fail (others can provide more info on that than I can).
> That's fine as I'd rather not squash something inappropriate into a
> viewpoint that doesn't fit. Better to leave those for later solutions!

Yes, this is still in the pipeline.

> 
> 
> Otherwise, just minor comments inline.
> 
> Jonathan
> 
>> ---
>>  drivers/resctrl/mpam_resctrl.c | 212 ++++++++++++++++++++++++++++++++-
>>  1 file changed, 211 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/resctrl/mpam_resctrl.c b/drivers/resctrl/mpam_resctrl.c
>> index a20656e49edc..e376841c5596 100644
>> --- a/drivers/resctrl/mpam_resctrl.c
>> +++ b/drivers/resctrl/mpam_resctrl.c
> 
> ...
> 
>> +static u32 get_mba_min(struct mpam_props *cprops)
>> +{
>> +	u32 val = 0;
>> +
>> +	if (mba_class_use_mbw_max(cprops))
>> +		val = mbw_max_to_percent(val, cprops);
>> +	else
>> +		WARN_ON_ONCE(1);
>> +
>> +	return val;
> 
> I'd be tempted to handle the error case first to make
> clear this is really just a sanity checking wrapper around the max_to_percent()
> function.
> 
> 	if (!mba_class_use_mbw_max(cprops)) {
> 		WARN_ON_ONCE(1);
> 		return 0;
> 	}
> 
> 	return mbw_max_to_percent(val, cprops);

Sure, I've updated this.

> 
>> +}
> 
>> +/*
>> + * topology_matches_l3() - Is the provided class the same shape as L3
>> + * @victim:		The class we'd like to pretend is L3.
>> + *
>> + * resctrl expects all the world's a Xeon, and all counters are on the
>> + * L3. We play fast and loose with this, mapping counters on other
>> + * classes - provided the CPU->domain mapping is the same kind of shape.
>> + *
>> + * Using cacheinfo directly would make this work even if resctrl can't
>> + * use the L3 - but cacheinfo can't tell us anything about offline CPUs.
>> + * Using the L3 resctrl domain list also depends on CPUs being online.
>> + * Using the mpam_class we picked for L3 so we can use its domain list
>> + * assumes that there are MPAM controls on the L3.
>> + * Instead, this path eventually uses the mpam_get_cpumask_from_cache_id()
>> + * helper which can tell us about offline CPUs ... but getting the cache_id
>> + * to start with relies on at least one CPU per L3 cache being online at
>> + * boot.
> 
> So the usual mess of maxcpus=1 breaks it for anything other than first L3?
> I'm not entirely against that but it may be a little unexpected.

Yes but this doesn't really add any knew restriction as the mpam base
driver only enables mpam when all (known) msc have been probed and for
an msc on a cache we only ever access it from the cpus local to that cache.

> 
>> + *
>> + * Walk the victim component list and compare the affinity mask with the
>> + * corresponding L3. The topology matches if each victim:component's affinity
>> + * mask is the same as the CPU's corresponding L3's. These lists/masks are
>> + * computed from firmware tables so don't change at runtime.
>> + */
>> +static bool topology_matches_l3(struct mpam_class *victim)
> 
> 
> 
>> +
>>  static int mpam_resctrl_control_init(struct mpam_resctrl_res *res,
>>  				     enum resctrl_res_level type)
>>  {
>>  	struct mpam_class *class = res->class;
>> +	struct mpam_props *cprops = &class->props;
>>  	struct rdt_resource *r = &res->resctrl_res;
>>  
>>  	switch (r->rid) {
>> @@ -361,6 +531,20 @@ static int mpam_resctrl_control_init(struct mpam_resctrl_res *res,
>>  		 * 'all the bits' is the correct answer here.
>>  		 */
>>  		r->cache.shareable_bits = resctrl_get_default_ctrl(r);
>> +		break;
>> +	case RDT_RESOURCE_MBA:
>> +		r->alloc_capable = true;
>> +		r->schema_fmt = RESCTRL_SCHEMA_RANGE;
>> +		r->ctrl_scope = RESCTRL_L3_CACHE;
>> +
>> +		r->membw.delay_linear = true;
>> +		r->membw.throttle_mode = THREAD_THROTTLE_UNDEFINED;
>> +		r->membw.min_bw = get_mba_min(cprops);
>> +		r->membw.max_bw = MAX_MBA_BW;
>> +		r->membw.bw_gran = get_mba_granularity(cprops);
>> +
>> +		r->name = "MB";
>> +
> 
> Probably should be consistent with either a blank line before break or not.
> That might also make the diff a little nicer.

Dropped the blank line.

> 
>>  		break;
>>  	default:
>>  		break;
> 
> 

Thanks,

Ben

Powered by blists - more mailing lists